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 31, 2026 at 01:03:42PM +0300, Luca Coelho wrote:
> On Tue, 2026-03-24 at 18:36 +0200, Ville Syrjälä wrote:
> > 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.
> 
> True, I'll skip passing it and just add the platform check in this
> function.
> 
> 
> > 
> > > +	}
> > > +}
> > > +
> > > +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?
> 
> It's because for BDW there is this one in between the two common calls:
> 
> 	/* WaSwitchSolVfFArbitrationPriority:bdw */
> 	intel_uncore_rmw(&i915->uncore, GAM_ECOCHK, 0, HSW_ECOCHK_ARB_PRIO_SOL);
> 
> ...this is probably independent from the other two and it may be okay
> to move it before the common code, but I wanted to avoid that.
> 
> Do you think it's safe to change the order from:
> 
> 1. CHICKEN_PIPESL_1
> 2. GAM_ECOCHK
> 3. CHICKEN_PAR1_1
> 
> ...to this?
> 
> 1. GAM_ECOCHK
> 2. CHICKEN_PIPESL_1
> 3. CHICKEN_PAR1_1
> 
> If that's the case, then it can be simplified.

There shouldn't be any ordering requirements here. But if you're
a it paranoid then you could do the reordering as a separate
step, just in case someone has to bisect it.

-- 
Ville Syrjälä
Intel