On Thu, Dec 01, 2022 at 03:27:25PM -0800, Lucas De Marchi wrote:
> On Thu, Dec 01, 2022 at 03:01:05PM -0800, Matt Roper wrote:
> > On Wed, Nov 30, 2022 at 03:17:08PM -0800, Matt Atwood wrote:
> > > From: Jouni Högander <jouni.hogander@xxxxxxxxx>
> > >
> > > This patch introduces initial workarounds for mtl platform
> > >
> > > Bspec: 66624
> > >
> > > Signed-off-by: Matt Atwood <matthew.s.atwood@xxxxxxxxx>
> > > Signed-off-by: Jouni Högander <jouni.hogander@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_fbc.c | 4 +++-
> > > drivers/gpu/drm/i915/display/intel_hdmi.c | 3 ++-
> > > drivers/gpu/drm/i915/display/intel_psr.c | 27 ++++++++++++++++-------
> > > drivers/gpu/drm/i915/i915_drv.h | 4 ++++
> > > 4 files changed, 28 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > index b5ee5ea0d010..8f269553d826 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > @@ -1095,7 +1095,9 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state,
> > > }
> > >
> > > /* Wa_14016291713 */
> > > - if (IS_DISPLAY_VER(i915, 12, 13) && crtc_state->has_psr) {
> > > + if ((IS_DISPLAY_VER(i915, 12, 13) ||
> > > + IS_MTL_DISPLAY_STEP(i915, STEP_A0, STEP_C0)) &&
> > > + crtc_state->has_psr) {
> > > plane_state->no_fbc_reason = "PSR1 enabled (Wa_14016291713)";
> > > return 0;
> > > }
> > > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > > index e82f8a07e2b0..efa2da080f62 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > > @@ -537,7 +537,8 @@ void hsw_write_infoframe(struct intel_encoder *encoder,
> > > 0);
> > >
> > > /* Wa_14013475917 */
> > > - if (DISPLAY_VER(dev_priv) == 13 && crtc_state->has_psr &&
> > > + if ((DISPLAY_VER(dev_priv) == 13 ||
> > > + IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0)) && crtc_state->has_psr &&
> > > type == DP_SDP_VSC)
> > > return;
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 5b678916e6db..7982157fb1ff 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -797,7 +797,7 @@ static bool psr2_granularity_check(struct intel_dp *intel_dp,
> > > return intel_dp->psr.su_y_granularity == 4;
> > >
> > > /*
> > > - * adl_p and display 14+ platforms has 1 line granularity.
> > > + * adl_p and mtl platforms has 1 line granularity.
> > > * For other platforms with SW tracking we can adjust the y coordinates
> > > * to match sink requirement if multiple of 4.
> > > */
> > > @@ -1170,11 +1170,14 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp,
> > > PSR2_ADD_VERTICAL_LINE_COUNT);
> > >
> > > /*
> > > - * Wa_16014451276:adlp
> > > + * Wa_16014451276:adlp,mtl[a0,b0]
> >
> > These days we don't really add steppings in these comments; at best
> > they're just reiterating information that can be easily seen from the
> > code below, at worst they get out of sync and cause confusion. I'd drop
> > the "[a0,b0]" part (which also isn't accurate anyway if you're using
> > range notation...it would be "[a0..b0)" in that case).
> >
> > Honestly even the list of platform names on workarounds is of
> > questionable value and I'm thinking about writing a patch that just
> > drops all of them throughout i915, leaving just the workaround lineage
> > number. Maybe I'd keep the platform names in the few cases where we
> > have multiple workaround numbers (with different sets of platforms) all
> > requesting the same programming of a register...
>
> I'd be happy to ack such patch :)
>
> >
> > > * All supported adlp panels have 1-based X granularity, this may
> > > * cause issues if non-supported panels are used.
> > > */
> > > - if (IS_ALDERLAKE_P(dev_priv))
> > > + if (IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0))
> > > + intel_de_rmw(dev_priv, MTL_CHICKEN_TRANS(cpu_transcoder), 0,
> > > + ADLP_1_BASED_X_GRANULARITY);
> > > + else if (IS_ALDERLAKE_P(dev_priv))
> > > intel_de_rmw(dev_priv, CHICKEN_TRANS(cpu_transcoder), 0,
> > > ADLP_1_BASED_X_GRANULARITY);
> > >
> > > @@ -1185,8 +1188,12 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp,
> > > TRANS_SET_CONTEXT_LATENCY_MASK,
> > > TRANS_SET_CONTEXT_LATENCY_VALUE(1));
> > >
> > > - /* Wa_16012604467:adlp */
> > > - if (IS_ALDERLAKE_P(dev_priv))
> > > + /* Wa_16012604467:adlp,mtl[a0,b0] */
> > > + if (IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0))
> > > + intel_de_rmw(dev_priv,
> > > + MTL_CLKGATE_DIS_TRANS(cpu_transcoder), 0,
> > > + MTL_CLKGATE_DIS_TRANS_DMASC_GATING_DIS);
> > > + else if (IS_ALDERLAKE_P(dev_priv))
> > > intel_de_rmw(dev_priv, CLKGATE_DIS_MISC, 0,
> > > CLKGATE_DIS_MISC_DMASC_GATING_DIS);
> > >
> > > @@ -1362,8 +1369,12 @@ static void intel_psr_disable_locked(struct intel_dp *intel_dp)
> > > TRANS_SET_CONTEXT_LATENCY(intel_dp->psr.transcoder),
> > > TRANS_SET_CONTEXT_LATENCY_MASK, 0);
> > >
> > > - /* Wa_16012604467:adlp */
> > > - if (IS_ALDERLAKE_P(dev_priv))
> > > + /* Wa_16012604467:adlp,mtl[a0,b0] */
> > > + if (IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0))
> > > + intel_de_rmw(dev_priv,
> > > + MTL_CLKGATE_DIS_TRANS(intel_dp->psr.transcoder),
> > > + MTL_CLKGATE_DIS_TRANS_DMASC_GATING_DIS, 0);
> > > + else if (IS_ALDERLAKE_P(dev_priv))
> > > intel_de_rmw(dev_priv, CLKGATE_DIS_MISC,
> > > CLKGATE_DIS_MISC_DMASC_GATING_DIS, 0);
> > >
> > > @@ -1625,7 +1636,7 @@ static void psr2_man_trk_ctl_calc(struct intel_crtc_state *crtc_state,
> > >
> > > if (full_update) {
> > > /*
> > > - * Not applying Wa_14014971508:adlp as we do not support the
> > > + * Not applying Wa_14014971508:adlp,mtl as we do not support the
> > > * feature that requires this workaround.
> > > */
> > > val |= man_trk_ctl_single_full_frame_bit_get(dev_priv);
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index a8a5bd426e78..ecb027626a21 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -727,6 +727,10 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
> > > (IS_SUBPLATFORM(__i915, INTEL_METEORLAKE, INTEL_SUBPLATFORM_##variant) && \
> > > IS_GRAPHICS_STEP(__i915, since, until))
> > >
> > > +#define IS_MTL_DISPLAY_STEP(__i915, since, until) \
> > > + (DISPLAY_VER(__i915) == 14 && \
> >
> > As Tvrtko noted, this could come back to bite us in the future if
> > another platform shows up with 14.10, 14.50, etc. MTL has exactly
> > version 14.0, so best to make this
> >
> > DISPLAY_VER_FULL(__i915) == IP_VER(14, 0)
> >
> > so that it won't automatically capture future platforms by accident.
>
> I think it's better to do a platform check as the other platforms are
> doing. See DG2 for example:
>
> #define IS_DG2_DISPLAY_STEP(__i915, since, until) \
> (IS_DG2(__i915) && \
> IS_DISPLAY_STEP(__i915, since, until))
I guess that's fine in the short term, but in the long term that's the
kind of thing we need to move away from. We're really not supposed to
be using platform checks (which are derived from PCI ID) anymore going
forward. At some point, even things like IS_MTL_DISPLAY_STEP() will get
replaced with something more along the lines of
IS_GMDID_DISPLAY_STEP(12, 70, STEP_A0, STEP_C0)
because the expectation is that none of this is actually tied to the
platform anymore, just to the IP versions of the various chiplets (which
can be mixed-and-matched and in theory could be re-used on other
platforms). But today MTL is the first/only hardware we have using this
design, so it doesn't matter too much; we don't have to clean this all
up immediately.
Matt
>
> Lucas De Marchi
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
