On Tue, 09 Dec 2025, Jani Nikula <[email protected]> wrote:
> On Tue, 09 Dec 2025, Ville Syrjala <[email protected]> wrote:
>> From: Ville Syrjälä <[email protected]>
>>
>> We use forcewake to prevent the SoC from actually entering
>> PC8 while performing the PC8 disable sequence. Hide that
>> behind a new parent interface to eliminate the naked
>> forcewake/uncore usage from the display power code.
>>
>> Signed-off-by: Ville Syrjälä <[email protected]>
>> ---
>> drivers/gpu/drm/i915/Makefile | 1 +
>> .../drm/i915/display/intel_display_power.c | 8 ++---
>> drivers/gpu/drm/i915/display/intel_parent.c | 10 +++++++
>> drivers/gpu/drm/i915/display/intel_parent.h | 3 ++
>> drivers/gpu/drm/i915/i915_display_pc8.c | 30 +++++++++++++++++++
>> drivers/gpu/drm/i915/i915_display_pc8.h | 9 ++++++
>> drivers/gpu/drm/i915/i915_driver.c | 2 ++
>> include/drm/intel/display_parent_interface.h | 8 +++++
>> 8 files changed, 67 insertions(+), 4 deletions(-)
>> create mode 100644 drivers/gpu/drm/i915/i915_display_pc8.c
>> create mode 100644 drivers/gpu/drm/i915/i915_display_pc8.h
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index 175bd99e1d0d..b57e51d626b1 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -76,6 +76,7 @@ i915-$(CONFIG_PERF_EVENTS) += \
>>
>> # core display adaptation
>> i915-y += \
>> + i915_display_pc8.o \
>> i915_hdcp_gsc.o
>>
>> # "Graphics Technology" (aka we talk to the gpu)
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
>> b/drivers/gpu/drm/i915/display/intel_display_power.c
>> index 9f323c39d798..47042a4c3a30 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
>> @@ -1339,10 +1339,10 @@ static void hsw_restore_lcpll(struct intel_display
>> *display)
>> return;
>>
>> /*
>> - * Make sure we're not on PC8 state before disabling PC8, otherwise
>> - * we'll hang the machine. To prevent PC8 state, just enable force_wake.
>> + * Make sure we're not on PC8 state before disabling
>> + * PC8, otherwise we'll hang the machine.
>> */
>> - intel_uncore_forcewake_get(&dev_priv->uncore, FORCEWAKE_ALL);
>> + intel_parent_pc8_block(display);
>>
>> if (val & LCPLL_POWER_DOWN_ALLOW) {
>> val &= ~LCPLL_POWER_DOWN_ALLOW;
>> @@ -1372,7 +1372,7 @@ static void hsw_restore_lcpll(struct intel_display
>> *display)
>> "Switching back to LCPLL failed\n");
>> }
>>
>> - intel_uncore_forcewake_put(&dev_priv->uncore, FORCEWAKE_ALL);
>> + intel_parent_pc8_unblock(display);
>>
>> intel_update_cdclk(display);
>> intel_cdclk_dump_config(display, &display->cdclk.hw, "Current CDCLK");
>> diff --git a/drivers/gpu/drm/i915/display/intel_parent.c
>> b/drivers/gpu/drm/i915/display/intel_parent.c
>> index 2ea310cc3509..9201d506c851 100644
>> --- a/drivers/gpu/drm/i915/display/intel_parent.c
>> +++ b/drivers/gpu/drm/i915/display/intel_parent.c
>> @@ -56,6 +56,16 @@ void intel_parent_irq_synchronize(struct intel_display
>> *display)
>> display->parent->irq->synchronize(display->drm);
>> }
>>
>> +void intel_parent_pc8_block(struct intel_display *display)
>> +{
>> + display->parent->pc8->block(display->drm);
>> +}
>> +
>> +void intel_parent_pc8_unblock(struct intel_display *display)
>> +{
>> + display->parent->pc8->unblock(display->drm);
>> +}
>
> I think I'd like either:
>
> - A substruct is mandatory, always initialized by parent, will never be
> NULL, and there are no checks here.
>
> - A substruct is optional, may be initialized by parent, may be NULL,
> and there's a NULL check here.
>
> I think it makes the interface easier to reason about. Even if I
> understand that this particular interface will only be called for
> platforms supported by i915.
>
> So this should have the display->parent->pc8 != NULL check, and
> "optional" mentioned in display_parent_interface.h.
Oh, I also used this in [1]:
int intel_parent_stolen_insert_node(struct intel_display *display, struct
intel_stolen_node *node, u64 size,
unsigned int align)
{
+ if (drm_WARN_ON_ONCE(display->drm,
!display->parent->stolen->insert_node))
+ return -ENODEV;
+
return display->parent->stolen->insert_node(node, size, align);
}
It's optional, but using it when it's not there gives a warning splat.
BR,
Jani.
[1]
https://lore.kernel.org/r/0dbb460e8bd1df29df98862d08fcdfda03912673.1764930576.git.jani.nik...@intel.com
>
>> +
>> bool intel_parent_rps_available(struct intel_display *display)
>> {
>> return display->parent->rps;
>> diff --git a/drivers/gpu/drm/i915/display/intel_parent.h
>> b/drivers/gpu/drm/i915/display/intel_parent.h
>> index 8f91a6f75c53..974a016ab3be 100644
>> --- a/drivers/gpu/drm/i915/display/intel_parent.h
>> +++ b/drivers/gpu/drm/i915/display/intel_parent.h
>> @@ -22,6 +22,9 @@ void intel_parent_hdcp_gsc_context_free(struct
>> intel_display *display,
>> bool intel_parent_irq_enabled(struct intel_display *display);
>> void intel_parent_irq_synchronize(struct intel_display *display);
>>
>> +void intel_parent_pc8_block(struct intel_display *display);
>> +void intel_parent_pc8_unblock(struct intel_display *display);
>> +
>> bool intel_parent_rps_available(struct intel_display *display);
>> void intel_parent_rps_boost_if_not_started(struct intel_display *display,
>> struct dma_fence *fence);
>> void intel_parent_rps_mark_interactive(struct intel_display *display, bool
>> interactive);
>> diff --git a/drivers/gpu/drm/i915/i915_display_pc8.c
>> b/drivers/gpu/drm/i915/i915_display_pc8.c
>> new file mode 100644
>> index 000000000000..443935d282e3
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/i915_display_pc8.c
>> @@ -0,0 +1,30 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright 2025, Intel Corporation.
>> + */
>> +
>> +#include <drm/drm_print.h>
>> +#include <drm/intel/display_parent_interface.h>
>> +
>> +#include "i915_drv.h"
>> +#include "intel_uncore.h"
>
> For completeness, I think this should include i915_display_pc8.h. I'm a
> bit surprised the compilers only warn about non-static functions without
> declarations, not about non-static variables.
>
>> +
>> +static void i915_display_pc8_block(struct drm_device *drm)
>> +{
>> + struct intel_uncore *uncore = &to_i915(drm)->uncore;
>> +
>> + /* to prevent PC8 state, just enable force_wake */
>> + intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
>> +}
>> +
>> +static void i915_display_pc8_unblock(struct drm_device *drm)
>> +{
>> + struct intel_uncore *uncore = &to_i915(drm)->uncore;
>> +
>> + intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL);
>> +}
>> +
>> +const struct intel_display_pc8_interface i915_display_pc8_interface = {
>> + .block = i915_display_pc8_block,
>> + .unblock = i915_display_pc8_unblock,
>> +};
>> diff --git a/drivers/gpu/drm/i915/i915_display_pc8.h
>> b/drivers/gpu/drm/i915/i915_display_pc8.h
>> new file mode 100644
>> index 000000000000..717f313d2a21
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/i915_display_pc8.h
>> @@ -0,0 +1,9 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/* Copyright © 2025 Intel Corporation */
>> +
>> +#ifndef __I915_DISPLAY_PC8_H__
>> +#define __I915_DISPLAY_PC8_H__
>> +
>> +extern const struct intel_display_pc8_interface i915_display_pc8_interface;
>> +
>> +#endif /* __I915_DISPLAY_PC8_H__ */
>> diff --git a/drivers/gpu/drm/i915/i915_driver.c
>> b/drivers/gpu/drm/i915/i915_driver.c
>> index d98839427ef9..723cb424b2ba 100644
>> --- a/drivers/gpu/drm/i915/i915_driver.c
>> +++ b/drivers/gpu/drm/i915/i915_driver.c
>> @@ -89,6 +89,7 @@
>> #include "pxp/intel_pxp_pm.h"
>>
>> #include "i915_debugfs.h"
>> +#include "i915_display_pc8.h"
>> #include "i915_driver.h"
>> #include "i915_drm_client.h"
>> #include "i915_drv.h"
>> @@ -761,6 +762,7 @@ static const struct intel_display_parent_interface
>> parent = {
>> .hdcp = &i915_display_hdcp_interface,
>> .rpm = &i915_display_rpm_interface,
>> .irq = &i915_display_irq_interface,
>> + .pc8 = &i915_display_pc8_interface,
>> .rps = &i915_display_rps_interface,
>> .vgpu_active = vgpu_active,
>> .has_fenced_regions = has_fenced_regions,
>> diff --git a/include/drm/intel/display_parent_interface.h
>> b/include/drm/intel/display_parent_interface.h
>> index 61d1b22adc83..af43b213eafa 100644
>> --- a/include/drm/intel/display_parent_interface.h
>> +++ b/include/drm/intel/display_parent_interface.h
>> @@ -41,6 +41,11 @@ struct intel_display_irq_interface {
>> void (*synchronize)(struct drm_device *drm);
>> };
>>
>> +struct intel_display_pc8_interface {
>> + void (*block)(struct drm_device *drm);
>> + void (*unblock)(struct drm_device *drm);
>> +};
>> +
>> struct intel_display_rps_interface {
>> void (*boost_if_not_started)(struct dma_fence *fence);
>> void (*mark_interactive)(struct drm_device *drm, bool interactive);
>> @@ -69,6 +74,9 @@ struct intel_display_parent_interface {
>> /** @irq: IRQ interface */
>> const struct intel_display_irq_interface *irq;
>>
>> + /** @pc8: PC8 interface */
>
> I think this should have "Optional" in there.
>
>> + const struct intel_display_pc8_interface *pc8;
>> +
>> /** @rpm: RPS interface. Optional. */
>> const struct intel_display_rps_interface *rps;
--
Jani Nikula, Intel
