Re: [Intel-gfx] [PATCH] drm/i915/display: Implement Wa_14015648006

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Tue, 2023-01-03 at 16:40 -0800, Matt Roper wrote:
> On Wed, Dec 21, 2022 at 03:21:18PM +0200, Jouni Högander wrote:
> > Add 4th pipe and extend TGL Wa_16013835468 to support ADLP, MTL and
> > DG2 and all TGL steppings.
> > 
> > BSpec: 54369, 55378, 66624
> > 
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > 
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_psr.c | 20 ++++++++++++++------
> >  drivers/gpu/drm/i915/i915_reg.h          |  1 +
> >  2 files changed, 15 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 9820e5fdd087..0d01b8a7a75d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -1112,6 +1112,8 @@ static u32 wa_16013835468_bit_get(struct
> > intel_dp *intel_dp)
> >                 return LATENCY_REPORTING_REMOVED_PIPE_B;
> >         case PIPE_C:
> >                 return LATENCY_REPORTING_REMOVED_PIPE_C;
> > +       case PIPE_D:
> > +               return LATENCY_REPORTING_REMOVED_PIPE_D;
> >         default:
> >                 MISSING_CASE(intel_dp->psr.pipe);
> >                 return 0;
> > @@ -1197,9 +1199,12 @@ static void intel_psr_enable_source(struct
> > intel_dp *intel_dp,
> >                         intel_de_rmw(dev_priv, CLKGATE_DIS_MISC, 0,
> >                                     
> > CLKGATE_DIS_MISC_DMASC_GATING_DIS);
> >  
> > -               /* Wa_16013835468:tgl[b0+], dg1 */
> > -               if (IS_TGL_DISPLAY_STEP(dev_priv, STEP_B0,
> > STEP_FOREVER) ||
> > -                   IS_DG1(dev_priv)) {
> > +               /*
> > +                * Wa_16013835468:tgl[b0+], dg1,
> > +                * Wa_14015648006:adlp[a0+], mtl[a0], dg2, tgl[a0+]
> > +                */
> > +               if (IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0)
> > ||
> > +                   IS_DISPLAY_VER(dev_priv, 12, 13)) {
> 
> There's another thread where we're discussing possibly dropping all
> of
> the platform/stepping information from workaround comments, but this
> is
> a great supporting example for why the detailed comments are causing
> more confusion than they're worth.  The code condition includes RKL
> and
> ADL-S, whereas the comment does not mention them.  In this case the
> code
> is correct and the comment is incomplete.
> 
> If we move forward with Lucas' patch, this should just turn into
> 
>         /*
>          * Wa_16013835468
>          * Wa_14015648006
>          */
> 
> and let the code speak for itself about which platform(s) it covers.
> 
> 
> As for the workaround itself here, the existing implementation of
> Wa_16013835468 is in a 'if (intel_dp->psr.psr2_enabled)' but it looks
> like the description of the new Wa_14015648006 is also supposed to
> apply
> to PSR1 as well.  Do we need to lift these out of that conditional
> block?

You are right. It should be applied for PSR1 as well.

Thank you for all your comments. Addressed all of them in new version.
Please check.

> 
> 
> Matt
> 
> >                         u16 vtotal, vblank;
> >  
> >                         vtotal = crtc_state-
> > >uapi.adjusted_mode.crtc_vtotal -
> > @@ -1378,9 +1383,12 @@ static void intel_psr_disable_locked(struct
> > intel_dp *intel_dp)
> >                         intel_de_rmw(dev_priv, CLKGATE_DIS_MISC,
> >                                     
> > CLKGATE_DIS_MISC_DMASC_GATING_DIS, 0);
> >  
> > -               /* Wa_16013835468:tgl[b0+], dg1 */
> > -               if (IS_TGL_DISPLAY_STEP(dev_priv, STEP_B0,
> > STEP_FOREVER) ||
> > -                   IS_DG1(dev_priv))
> > +               /*
> > +                * Wa_16013835468:tgl[b0+], dg1,
> > +                * Wa_14015648006:adlp[a0+], mtl[a0], dg2, tgl[a0+]
> > +                */
> > +               if (IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0)
> > ||
> > +                   IS_DISPLAY_VER(dev_priv, 12, 13))
> >                         intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1,
> >                                     
> > wa_16013835468_bit_get(intel_dp), 0);
> >         }
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index cef9418beec0..a70a1b6e6a15 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5737,6 +5737,7 @@
> >  #define  RESET_PCH_HANDSHAKE_ENABLE    REG_BIT(4)
> >  
> >  #define GEN8_CHICKEN_DCPR_1                    _MMIO(0x46430)
> > +#define   LATENCY_REPORTING_REMOVED_PIPE_D     REG_BIT(31)
> >  #define   SKL_SELECT_ALTERNATE_DC_EXIT         REG_BIT(30)
> >  #define   LATENCY_REPORTING_REMOVED_PIPE_C     REG_BIT(25)
> >  #define   LATENCY_REPORTING_REMOVED_PIPE_B     REG_BIT(24)
> > -- 
> > 2.34.1
> > 
>