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@xxxxxxxxx>
> > Cc: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> >
> > Signed-off-by: Mika Kahola <mika.kahola@xxxxxxxxx>
> > Signed-off-by: Jouni Högander <jouni.hogander@xxxxxxxxx>
> > ---
> > 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
> >
>
