Re: [PATCH] drm/i915/pc8: Add parent interface for PC8 forcewake tricks

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 09 Dec 2025, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Tue, 09 Dec 2025, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> 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ä <ville.syrjala@linux.intel.com>
>> ---
>>  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.nikula@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