[Intel-gfx] [PATCH v3 1/5] drm/i915/mtl: Fix Wa_16015201720 implementation

[Sripada, Radhakrishna]

Hi Andi,

> -----Original Message-----
> From: Andi Shyti <andi.shyti at linux.intel.com>
> Sent: Thursday, March 9, 2023 8:30 AM
> To: Sripada, Radhakrishna <radhakrishna.sripada at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; De Marchi, Lucas
> <lucas.demarchi at intel.com>
> Subject: Re: [Intel-gfx] [PATCH v3 1/5] drm/i915/mtl: Fix Wa_16015201720
> implementation
> 
> Hi Radhakrishna,
> 
> On Wed, Mar 01, 2023 at 12:10:49PM -0800, Radhakrishna Sripada wrote:
> > The commit 2357f2b271ad ("drm/i915/mtl: Initial display workarounds")
> > extended the workaround Wa_16015201720 to MTL. However the registers
> > that the original WA implemented moved for MTL.
> >
> > Implement the workaround with the correct register.
> >
> > v3: Skip clock gating for pipe C, D DMC's and fix the title
> >
> > Fixes: 2357f2b271ad ("drm/i915/mtl: Initial display workarounds")
> > Cc: Matt Atwood <matthew.s.atwood at intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dmc.c | 26 +++++++++++++++++++-----
> >  drivers/gpu/drm/i915/i915_reg.h          | 10 ++++++---
> >  2 files changed, 28 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c
> b/drivers/gpu/drm/i915/display/intel_dmc.c
> > index f70ada2357dc..b4283cf319f2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dmc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
> > @@ -389,15 +389,12 @@ static void disable_all_event_handlers(struct
> drm_i915_private *i915)
> >  	}
> >  }
> >
> > -static void pipedmc_clock_gating_wa(struct drm_i915_private *i915, bool
> enable)
> > +static void adlp_pipedmc_clock_gating_wa(struct drm_i915_private *i915,
> bool enable)
> >  {
> >  	enum pipe pipe;
> >
> > -	if (DISPLAY_VER(i915) < 13)
> > -		return;
> > -
> 
> Why is this not needed anyomore?
With the check below while calling the function the check here becomes redundant.

> 
> >  	/*
> > -	 * Wa_16015201720:adl-p,dg2, mtl
> > +	 * Wa_16015201720:adl-p,dg2
> >  	 * The WA requires clock gating to be disabled all the time
> >  	 * for pipe A and B.
> >  	 * For pipe C and D clock gating needs to be disabled only
> > @@ -413,6 +410,25 @@ static void pipedmc_clock_gating_wa(struct
> drm_i915_private *i915, bool enable)
> >  				     PIPEDMC_GATING_DIS, 0);
> >  }
> >
> > +static void mtl_pipedmc_clock_gating_wa(struct drm_i915_private *i915)
> > +{
> > +	/*
> > +	 * Wa_16015201720
> > +	 * The WA requires clock gating to be disabled all the time
> > +	 * for pipe A and B.
> > +	 */
> > +	intel_de_rmw(i915, GEN9_CLKGATE_DIS_0, 0,
> > +		     MTL_PIPEDMC_GATING_DIS_A |
> MTL_PIPEDMC_GATING_DIS_B);
> > +}
> > +
> > +static void pipedmc_clock_gating_wa(struct drm_i915_private *i915, bool
> enable)
> > +{
> > +	if (DISPLAY_VER(i915) >= 14 && enable)
> > +		return mtl_pipedmc_clock_gating_wa(i915);
> > +	else if (DISPLAY_VER(i915) == 13)
> > +		return adlp_pipedmc_clock_gating_wa(i915, enable);
> 
> don't you get an error here? Please don't return anything.
Addressed based on review comments from Matt Roper.
> 
> > +}
> > +
> >  void intel_dmc_enable_pipe(struct drm_i915_private *i915, enum pipe pipe)
> >  {
> >  	enum intel_dmc_id dmc_id = PIPE_TO_DMC_ID(pipe);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> > index c1efa655fb68..7c9ac5b43831 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1794,9 +1794,13 @@
> >   * GEN9 clock gating regs
> >   */
> >  #define GEN9_CLKGATE_DIS_0		_MMIO(0x46530)
> > -#define   DARBF_GATING_DIS		(1 << 27)
> > -#define   PWM2_GATING_DIS		(1 << 14)
> > -#define   PWM1_GATING_DIS		(1 << 13)
> > +#define   DARBF_GATING_DIS		REG_BIT(27)
> > +#define   MTL_PIPEDMC_GATING_DIS_A	REG_BIT(15)
> > +#define   MTL_PIPEDMC_GATING_DIS_B	REG_BIT(14)
> 
> you could eventually use a GENMASK here and it can be:
We may have to play with individual pipes here and b-spec defines them as
Individual bits. So leaving them as is.
> 
> #define   MTL_PIPEDMC_GATING_DIS	REG_GENMASK(15, 14)
> 
> > +#define   PWM2_GATING_DIS		REG_BIT(14)
> > +#define   MTL_PIPEDMC_GATING_DIS_C	REG_BIT(13)
> 
> Is this needed?
Below

> 
> > +#define   PWM1_GATING_DIS		REG_BIT(13)
> > +#define   MTL_PIPEDMC_GATING_DIS_D	REG_BIT(12)
> 
> Is this needed?
Removed MTL_PIPEDMC_GATING_DIS_D and MTL_PIPEDMC_GATING_DIS_C
Based on review feedback from MattR.


Since most of the comments aligned with Matt's suggestion pushed with Matt's r-b.

Thanks you for the review.

-Radhakrishna Sripada
> 
> Thanks,
> Andi
> 
> >  #define GEN9_CLKGATE_DIS_3		_MMIO(0x46538)
> >  #define   TGL_VRH_GATING_DIS		REG_BIT(31)
> > --
> > 2.34.1