Hi Andi,
> -----Original Message-----
> From: Andi Shyti <andi.sh...@linux.intel.com>
> Sent: Thursday, March 9, 2023 8:30 AM
> To: Sripada, Radhakrishna <radhakrishna.srip...@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; De Marchi, Lucas
> <lucas.demar...@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.atw...@intel.com>
> > Cc: Lucas De Marchi <lucas.demar...@intel.com>
> > Signed-off-by: Radhakrishna Sripada <radhakrishna.srip...@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
