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

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

On Wed, 10 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.
>
> v2: Mark the interface optional and warn if
>     someone calls it when not provided (Jani)
>     Include the header to make sure the extern
>     declaration matches the definition (Jani)
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/Makefile                 |  1 +
>  .../drm/i915/display/intel_display_power.c    |  8 ++---
>  drivers/gpu/drm/i915/display/intel_parent.c   | 17 ++++++++++
>  drivers/gpu/drm/i915/display/intel_parent.h   |  3 ++
>  drivers/gpu/drm/i915/i915_display_pc8.c       | 31 +++++++++++++++++++
>  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, 75 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..ceb8ceea4ba4 100644
> --- a/drivers/gpu/drm/i915/display/intel_parent.c
> +++ b/drivers/gpu/drm/i915/display/intel_parent.c
> @@ -17,6 +17,7 @@
>   * function pointer interface.
>   */
>  
> +#include <drm/drm_print.h>
>  #include <drm/intel/display_parent_interface.h>
>  
>  #include "intel_display_core.h"
> @@ -56,6 +57,22 @@ void intel_parent_irq_synchronize(struct intel_display *display)
>  	display->parent->irq->synchronize(display->drm);
>  }
>  
> +void intel_parent_pc8_block(struct intel_display *display)
> +{
> +	if (drm_WARN_ON_ONCE(display->drm, !display->parent->pc8))
> +		return;
> +
> +	display->parent->pc8->block(display->drm);
> +}
> +
> +void intel_parent_pc8_unblock(struct intel_display *display)
> +{
> +	if (drm_WARN_ON_ONCE(display->drm, !display->parent->pc8))
> +		return;
> +
> +	display->parent->pc8->unblock(display->drm);
> +}
> +
>  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..2af5dbab20ef
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_display_pc8.c
> @@ -0,0 +1,31 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright 2025, Intel Corporation.
> + */
> +
> +#include <drm/drm_print.h>
> +#include <drm/intel/display_parent_interface.h>
> +
> +#include "i915_display_pc8.h"
> +#include "i915_drv.h"
> +#include "intel_uncore.h"
> +
> +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..f07d265e90e5 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. Optional. */
> +	const struct intel_display_pc8_interface *pc8;
> +
>  	/** @rpm: RPS interface. Optional. */
>  	const struct intel_display_rps_interface *rps;

-- 
Jani Nikula, Intel