Re: [PATCH v4 1/2] drm/i915/display: add a way to restore only display/pch registers in clock_gating

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

On Tue, 08 Sep 2026, Luca Coelho <luciano.coelho@intel.com> wrote:
> Currently, hsw_disable_pc8() calls intel_clock_gating_init() to restore
> clock gating after PC8+ exit.  This makes display call into core i915
> initialization, which in turn calls back into display code.
>
> Add intel_display_restore_clock_gating() to restore display and PCH
> registers directly.  Split out the HSW/BDW GT/uncore programming and
> restore it through the display parent interface, preserving the
> existing workarounds without calling core clock gating initialization
> from display.
>
> Assisted-by: Copilot:claude-sonnet-5
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> ---
>  .../i915/display/intel_display_clock_gating.c |  7 ++++
>  .../i915/display/intel_display_clock_gating.h |  1 +
>  .../drm/i915/display/intel_display_power.c    |  7 +++-
>  drivers/gpu/drm/i915/display/intel_parent.c   |  9 +++++
>  drivers/gpu/drm/i915/display/intel_parent.h   |  3 ++
>  drivers/gpu/drm/i915/i915_driver.c            |  1 +
>  drivers/gpu/drm/i915/intel_clock_gating.c     | 37 ++++++++++++++++---
>  drivers/gpu/drm/i915/intel_clock_gating.h     |  2 +
>  drivers/gpu/drm/xe/Makefile                   |  1 +
>  include/drm/intel/display_parent_interface.h  |  7 ++++
>  10 files changed, 67 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_clock_gating.c b/drivers/gpu/drm/i915/display/intel_display_clock_gating.c
> index ef1ee72494df..6716c377ef93 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_clock_gating.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_clock_gating.c
> @@ -11,6 +11,7 @@
>  #include "intel_display_clock_gating.h"
>  #include "intel_display_core.h"
>  #include "intel_display_regs.h"
> +#include "intel_pch.h"
>  
>  static void intel_display_gen9_init_clock_gating(struct intel_display *display)
>  {
> @@ -305,3 +306,9 @@ void intel_display_init_clock_gating(struct intel_display *display)
>  	else if (display->platform.i965gm)
>  		intel_display_i965gm_init_clock_gating(display);
>  }
> +
> +void intel_display_restore_clock_gating(struct intel_display *display)
> +{
> +	intel_display_init_clock_gating(display);
> +	intel_pch_init_clock_gating(display);
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_display_clock_gating.h b/drivers/gpu/drm/i915/display/intel_display_clock_gating.h
> index dbfa5892cffe..074708a22436 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_clock_gating.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_clock_gating.h
> @@ -9,5 +9,6 @@
>  struct intel_display;
>  
>  void intel_display_init_clock_gating(struct intel_display *display);
> +void intel_display_restore_clock_gating(struct intel_display *display);
>  
>  #endif /* __INTEL_DISPLAY_CLOCK_GATING_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 0ebec6e0c240..6e38cabc5581 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -12,7 +12,7 @@
>  
>  #include "intel_backlight_regs.h"
>  #include "intel_cdclk.h"
> -#include "intel_clock_gating.h"
> +#include "intel_display_clock_gating.h"
>  #include "intel_combo_phy.h"
>  #include "intel_crtc.h"
>  #include "intel_de.h"
> @@ -1588,7 +1588,10 @@ static void hsw_disable_pc8(struct intel_display *display)
>  	intel_init_pch_refclk(display);
>  
>  	/* Many display registers don't survive PC8+ */
> -	intel_clock_gating_init(display->drm);
> +	intel_display_restore_clock_gating(display);
> +
> +	/* Some GT/uncore clock gating workarounds don't survive PC8+ either */
> +	intel_parent_clock_gating_restore_gt(display);

Yeah, but this should not be the display driver's responsibility.

Main goal, separate the clock gating paths between display and the rest
at the *high* level. Display clock gating shouldn't call core driver
stuff, core driver clock gating shouldn't call display stuff. There
should be zero cross calls in the end.

We only ever end up in hsw_disable_pc8() through:

intel_display_driver_pm_resume_early()
  -> intel_display_power_resume_early()
     -> hsw_disable_pc8()

intel_display_driver_pm_runtime_resume_early()
  -> intel_display_power_runtime_resume()
     -> hsw_disable_pc8().

And (apart from the error paths) those have the intel_gt_resume_early(),
intel_uncore_runtime_resume(), or intel_gt_runtime_resume() calls. Those
are the ones that should be responsible for handling the GT/uncore
stuff, not display.


BR,
Jani.

>  }
>  
>  static void intel_pch_reset_handshake(struct intel_display *display,
> diff --git a/drivers/gpu/drm/i915/display/intel_parent.c b/drivers/gpu/drm/i915/display/intel_parent.c
> index 99cebb17763a..d8c38f138396 100644
> --- a/drivers/gpu/drm/i915/display/intel_parent.c
> +++ b/drivers/gpu/drm/i915/display/intel_parent.c
> @@ -24,6 +24,15 @@
>  #include "intel_display_core.h"
>  #include "intel_parent.h"
>  
> +/* clock_gating */
> +void intel_parent_clock_gating_restore_gt(struct intel_display *display)
> +{
> +	if (drm_WARN_ON_ONCE(display->drm, !display->parent->clock_gating))
> +		return;
> +
> +	display->parent->clock_gating->restore_gt(display->drm);
> +}
> +
>  /* dpt */
>  struct intel_dpt *intel_parent_dpt_create(struct intel_display *display,
>  					  struct drm_gem_object *obj, size_t size)
> diff --git a/drivers/gpu/drm/i915/display/intel_parent.h b/drivers/gpu/drm/i915/display/intel_parent.h
> index cc4a58f63166..db2272ad9be3 100644
> --- a/drivers/gpu/drm/i915/display/intel_parent.h
> +++ b/drivers/gpu/drm/i915/display/intel_parent.h
> @@ -22,6 +22,9 @@ struct intel_panic;
>  struct intel_stolen_node;
>  struct iosys_map;
>  
> +/* clock_gating */
> +void intel_parent_clock_gating_restore_gt(struct intel_display *display);
> +
>  /* dpt */
>  struct intel_dpt *intel_parent_dpt_create(struct intel_display *display,
>  					  struct drm_gem_object *obj, size_t size);
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index ce6d20958320..8da72cef63c9 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -745,6 +745,7 @@ static bool vgpu_active(struct drm_device *drm)
>  
>  static const struct intel_display_parent_interface parent = {
>  	.bo = &i915_display_bo_interface,
> +	.clock_gating = &i915_display_clock_gating_interface,
>  	.dpt = &i915_display_dpt_interface,
>  	.dsb = &i915_display_dsb_interface,
>  	.fb_pin = &i915_display_fb_pin_interface,
> diff --git a/drivers/gpu/drm/i915/intel_clock_gating.c b/drivers/gpu/drm/i915/intel_clock_gating.c
> index c5c4441f3a61..180cb5862016 100644
> --- a/drivers/gpu/drm/i915/intel_clock_gating.c
> +++ b/drivers/gpu/drm/i915/intel_clock_gating.c
> @@ -26,6 +26,7 @@
>   */
>  
>  #include <drm/drm_print.h>
> +#include <drm/intel/display_parent_interface.h>
>  #include <drm/intel/intel_gmd_interrupt_regs.h>
>  #include <drm/intel/intel_gmd_misc_regs.h>
>  #include <drm/intel/mchbar_regs.h>
> @@ -203,10 +204,8 @@ static void skl_init_clock_gating(struct drm_i915_private *i915)
>  	intel_display_init_clock_gating(i915->display);
>  }
>  
> -static void bdw_init_clock_gating(struct drm_i915_private *i915)
> +static void bdw_restore_gt_clock_gating(struct drm_i915_private *i915)
>  {
> -	intel_display_init_clock_gating(i915->display);
> -
>  	/* WaSwitchSolVfFArbitrationPriority:bdw */
>  	intel_uncore_rmw(&i915->uncore, GAM_ECOCHK, 0, HSW_ECOCHK_ARB_PRIO_SOL);
>  
> @@ -224,8 +223,6 @@ static void bdw_init_clock_gating(struct drm_i915_private *i915)
>  	/* WaProgramL3SqcReg1Default:bdw */
>  	gen8_set_l3sqc_credits(i915, 30, 2);
>  
> -	intel_pch_init_clock_gating(i915->display);
> -
>  	/* WaDisableDopClockGating:bdw
>  	 *
>  	 * Also see the CHICKEN2 write in bdw_init_workarounds() to disable DOP
> @@ -234,16 +231,30 @@ static void bdw_init_clock_gating(struct drm_i915_private *i915)
>  	intel_uncore_rmw(&i915->uncore, GEN6_UCGCTL1, 0, GEN6_EU_TCUNIT_CLOCK_GATE_DISABLE);
>  }
>  
> -static void hsw_init_clock_gating(struct drm_i915_private *i915)
> +static void bdw_init_clock_gating(struct drm_i915_private *i915)
>  {
>  	intel_display_init_clock_gating(i915->display);
>  
> +	bdw_restore_gt_clock_gating(i915);
> +
> +	intel_pch_init_clock_gating(i915->display);
> +}
> +
> +static void hsw_restore_gt_clock_gating(struct drm_i915_private *i915)
> +{
>  	/* This is required by WaCatErrorRejectionIssue:hsw */
>  	intel_uncore_rmw(&i915->uncore, GEN7_SQ_CHICKEN_MBCUNIT_CONFIG,
>  			 0, GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB);
>  
>  	/* WaSwitchSolVfFArbitrationPriority:hsw */
>  	intel_uncore_rmw(&i915->uncore, GAM_ECOCHK, 0, HSW_ECOCHK_ARB_PRIO_SOL);
> +}
> +
> +static void hsw_init_clock_gating(struct drm_i915_private *i915)
> +{
> +	intel_display_init_clock_gating(i915->display);
> +
> +	hsw_restore_gt_clock_gating(i915);
>  
>  	intel_pch_init_clock_gating(i915->display);
>  }
> @@ -448,6 +459,20 @@ void intel_clock_gating_init(struct drm_device *drm)
>  	i915->clock_gating_funcs->init_clock_gating(i915);
>  }
>  
> +static void intel_clock_gating_restore_gt(struct drm_device *drm)
> +{
> +	struct drm_i915_private *i915 = to_i915(drm);
> +
> +	if (IS_BROADWELL(i915))
> +		bdw_restore_gt_clock_gating(i915);
> +	else if (IS_HASWELL(i915))
> +		hsw_restore_gt_clock_gating(i915);
> +}
> +
> +const struct intel_display_clock_gating_interface i915_display_clock_gating_interface = {
> +	.restore_gt = intel_clock_gating_restore_gt,
> +};
> +
>  static void nop_init_clock_gating(struct drm_i915_private *i915)
>  {
>  	drm_dbg_kms(&i915->drm,
> diff --git a/drivers/gpu/drm/i915/intel_clock_gating.h b/drivers/gpu/drm/i915/intel_clock_gating.h
> index 3a4b443d9b8b..5c0f7c91c4f7 100644
> --- a/drivers/gpu/drm/i915/intel_clock_gating.h
> +++ b/drivers/gpu/drm/i915/intel_clock_gating.h
> @@ -11,4 +11,6 @@ struct drm_device;
>  void intel_clock_gating_init(struct drm_device *drm);
>  void intel_clock_gating_hooks_init(struct drm_device *drm);
>  
> +extern const struct intel_display_clock_gating_interface i915_display_clock_gating_interface;
> +
>  #endif /* __INTEL_CLOCK_GATING_H__ */
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index 67b8b5477639..7737fb27b259 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -260,6 +260,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
>  	i915-display/intel_ddi_buf_trans.o \
>  	i915-display/intel_de.o \
>  	i915-display/intel_display.o \
> +	i915-display/intel_display_clock_gating.o \
>  	i915-display/intel_display_conversion.o \
>  	i915-display/intel_display_device.o \
>  	i915-display/intel_display_driver.o \
> diff --git a/include/drm/intel/display_parent_interface.h b/include/drm/intel/display_parent_interface.h
> index 5e44c022d1ae..c6638c108009 100644
> --- a/include/drm/intel/display_parent_interface.h
> +++ b/include/drm/intel/display_parent_interface.h
> @@ -65,6 +65,10 @@ struct intel_display_bo_interface {
>  #endif
>  };
>  
> +struct intel_display_clock_gating_interface {
> +	void (*restore_gt)(struct drm_device *drm);
> +};
> +
>  struct intel_display_dpt_interface {
>  	struct intel_dpt *(*create)(struct drm_gem_object *obj, size_t size);
>  	void (*destroy)(struct intel_dpt *dpt);
> @@ -250,6 +254,9 @@ struct intel_display_parent_interface {
>  	/** @bo: BO interface */
>  	const struct intel_display_bo_interface *bo;
>  
> +	/** @clock_gating: Clock gating interface. Optional. */
> +	const struct intel_display_clock_gating_interface *clock_gating;
> +
>  	/** @dpt: DPT interface. Optional. */
>  	const struct intel_display_dpt_interface *dpt;

-- 
Jani Nikula, Intel