Re: [PATCH 6/8] drm/i915/display: move HSW and BDW clock gating init to display

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Mar 24, 2026 at 04:29:55PM +0200, Luca Coelho wrote:
> Move the HSW and BDW display clock gating programming into the display
> code.  In this case we need two different helpers, because the common
> code between these two is split in the middle.
> 
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> ---
>  .../i915/display/intel_display_clock_gating.c | 33 +++++++++++++++++++
>  .../i915/display/intel_display_clock_gating.h |  6 ++++
>  drivers/gpu/drm/i915/intel_clock_gating.c     | 31 +++--------------
>  3 files changed, 43 insertions(+), 27 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 e3b7522b4101..0b2edf6acb79 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_clock_gating.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_clock_gating.c
> @@ -123,3 +123,36 @@ void intel_display_glk_init_clock_gating(struct intel_display *display)
>  		       intel_de_read(display, GEN9_CLKGATE_DIS_0) |
>  		       PWM1_GATING_DIS | PWM2_GATING_DIS);
>  }
> +
> +static void
> +intel_display_hsw_init_clock_gating_common(struct intel_display *display,
> +					   u32 unmask_vbl)

Passing that as a parameter feels a bit obfuscated.

> +{
> +	enum pipe pipe;
> +
> +	/* WaPsrDPAMaskVBlankInSRD:hsw */
> +	intel_de_rmw(display, CHICKEN_PAR1_1, 0, HSW_MASK_VBL_TO_PIPE_IN_SRD);
> +
> +	for_each_pipe(display, pipe) {
> +		/* WaPsrDPRSUnmaskVBlankInSRD:hsw,bdw */
> +		intel_de_rmw(display, CHICKEN_PIPESL_1(pipe), 0, unmask_vbl);

If we want to share the function then I'd probably just do
a platform check here.

> +	}
> +}
> +
> +void intel_display_bdw_hsw_init_clock_gating(struct intel_display *display)
> +{
> +	/* WaFbcAsynchFlipDisableFbcQueue:hsw,bdw */
> +	intel_de_rmw(display, CHICKEN_PIPESL_1(PIPE_A), 0, HSW_FBCQ_DIS);
> +}

Why do we have two different functions that shared by
both platforms?

> +
> +void intel_display_bdw_init_clock_gating(struct intel_display *display)
> +{
> +	intel_display_hsw_init_clock_gating_common(display,
> +						   BDW_UNMASK_VBL_TO_REGS_IN_SRD);
> +}
> +
> +void intel_display_hsw_init_clock_gating(struct intel_display *display)
> +{
> +	intel_display_hsw_init_clock_gating_common(display,
> +						   HSW_UNMASK_VBL_TO_REGS_IN_SRD);
> +}
> 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 4abd34fa5832..0eb240f2f69e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_clock_gating.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_clock_gating.h
> @@ -14,12 +14,18 @@ void intel_display_kbl_init_clock_gating(struct intel_display *display);
>  void intel_display_cfl_init_clock_gating(struct intel_display *display);
>  void intel_display_bxt_init_clock_gating(struct intel_display *display);
>  void intel_display_glk_init_clock_gating(struct intel_display *display);
> +void intel_display_bdw_hsw_init_clock_gating(struct intel_display *display);
> +void intel_display_bdw_init_clock_gating(struct intel_display *display);
> +void intel_display_hsw_init_clock_gating(struct intel_display *display);
>  #else
>  static inline void intel_display_skl_init_clock_gating(struct intel_display *display) {}
>  static inline void intel_display_kbl_init_clock_gating(struct intel_display *display) {}
>  static inline void intel_display_cfl_init_clock_gating(struct intel_display *display) {}
>  static inline void intel_display_bxt_init_clock_gating(struct intel_display *display) {}
>  static inline void intel_display_glk_init_clock_gating(struct intel_display *display) {}
> +static inline void intel_display_bdw_hsw_init_clock_gating(struct intel_display *display) {}
> +static inline void intel_display_bdw_init_clock_gating(struct intel_display *display) {}
> +static inline void intel_display_hsw_init_clock_gating(struct intel_display *display) {}
>  #endif
>  
>  #endif /* __INTEL_DISPLAY_CLOCK_GATING_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_clock_gating.c b/drivers/gpu/drm/i915/intel_clock_gating.c
> index 777314e0c75d..a8e3eb6f06c8 100644
> --- a/drivers/gpu/drm/i915/intel_clock_gating.c
> +++ b/drivers/gpu/drm/i915/intel_clock_gating.c
> @@ -284,23 +284,12 @@ static void skl_init_clock_gating(struct drm_i915_private *i915)
>  
>  static void bdw_init_clock_gating(struct drm_i915_private *i915)
>  {
> -	struct intel_display *display = i915->display;
> -	enum pipe pipe;
> -
> -	/* WaFbcAsynchFlipDisableFbcQueue:hsw,bdw */
> -	intel_uncore_rmw(&i915->uncore, CHICKEN_PIPESL_1(PIPE_A), 0, HSW_FBCQ_DIS);
> +	intel_display_bdw_hsw_init_clock_gating(i915->display);
>  
>  	/* WaSwitchSolVfFArbitrationPriority:bdw */
>  	intel_uncore_rmw(&i915->uncore, GAM_ECOCHK, 0, HSW_ECOCHK_ARB_PRIO_SOL);
>  
> -	/* WaPsrDPAMaskVBlankInSRD:bdw */
> -	intel_uncore_rmw(&i915->uncore, CHICKEN_PAR1_1, 0, HSW_MASK_VBL_TO_PIPE_IN_SRD);
> -
> -	for_each_pipe(display, pipe) {
> -		/* WaPsrDPRSUnmaskVBlankInSRD:bdw */
> -		intel_uncore_rmw(&i915->uncore, CHICKEN_PIPESL_1(pipe),
> -				 0, BDW_UNMASK_VBL_TO_REGS_IN_SRD);
> -	}
> +	intel_display_bdw_init_clock_gating(i915->display);
>  
>  	/* WaVSRefCountFullforceMissDisable:bdw */
>  	/* WaDSRefCountFullforceMissDisable:bdw */

...
WaKVMNotificationOnConfigChange:bdw somewhere in here is also a display
thing.

And given that I think sharing anything between the platforms in
this patch is a bit premature. I think it would be better to just
move things as is, and do the code sharing refactoring as a followup
once it's easier to see what's common and what isn't.

> @@ -332,20 +321,8 @@ static void bdw_init_clock_gating(struct drm_i915_private *i915)
>  
>  static void hsw_init_clock_gating(struct drm_i915_private *i915)
>  {
> -	struct intel_display *display = i915->display;
> -	enum pipe pipe;
> -
> -	/* WaFbcAsynchFlipDisableFbcQueue:hsw,bdw */
> -	intel_uncore_rmw(&i915->uncore, CHICKEN_PIPESL_1(PIPE_A), 0, HSW_FBCQ_DIS);
> -
> -	/* WaPsrDPAMaskVBlankInSRD:hsw */
> -	intel_uncore_rmw(&i915->uncore, CHICKEN_PAR1_1, 0, HSW_MASK_VBL_TO_PIPE_IN_SRD);
> -
> -	for_each_pipe(display, pipe) {
> -		/* WaPsrDPRSUnmaskVBlankInSRD:hsw */
> -		intel_uncore_rmw(&i915->uncore, CHICKEN_PIPESL_1(pipe),
> -				 0, HSW_UNMASK_VBL_TO_REGS_IN_SRD);
> -	}
> +	intel_display_bdw_hsw_init_clock_gating(i915->display);
> +	intel_display_hsw_init_clock_gating(i915->display);
>  
>  	/* This is required by WaCatErrorRejectionIssue:hsw */
>  	intel_uncore_rmw(&i915->uncore, GEN7_SQ_CHICKEN_MBCUNIT_CONFIG,
> -- 
> 2.53.0

-- 
Ville Syrjälä
Intel