| Message ID | 20221221132118.1822697-1-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display: Implement Wa_14015648006 | expand |
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? 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 >
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 > > >
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)) { 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)