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?
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
>
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
