[Intel-gfx] [PATCH 1/2] drm/i915/psr: Fix Wa_16013835468 and Wa_14015648006

[Hogander, Jouni]

On Fri, 2023-03-17 at 14:33 +0200, Ville Syrjälä wrote:
> On Fri, Mar 17, 2023 at 06:54:02AM +0000, Hogander, Jouni wrote:
> > On Thu, 2023-03-16 at 20:21 +0200, Ville Syrjälä wrote:
> > > On Tue, Mar 14, 2023 at 11:01:41AM +0200, Jouni Högander wrote:
> > > > PSR WM optimization should be disabled based on any wm level
> > > > being
> > > > disabled. Currently it's disabled always when using delayed
> > > > vblank.
> > > > 
> > > > Bspec: 71580
> > > > 
> > > > Signed-off-by: Jouni Högander <jouni.hogander at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_display_types.h |  1 +
> > > >  drivers/gpu/drm/i915/display/intel_psr.c           | 12 +++++-
> > > > ----
> > > > --
> > > >  drivers/gpu/drm/i915/display/skl_watermark.c       |  7 +++++-
> > > > -
> > > >  3 files changed, 11 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > index c32bfba06ca1..60504c390408 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > @@ -1152,6 +1152,7 @@ struct intel_crtc_state {
> > > >         bool has_psr2;
> > > >         bool enable_psr2_sel_fetch;
> > > >         bool req_psr2_sdp_prior_scanline;
> > > > +       bool wm_level_disabled;
> > > >         u32 dc3co_exitline;
> > > >         u16 su_y_granularity;
> > > >         struct drm_dp_vsc_sdp psr_vsc;
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > index 44610b20cd29..a6edd65b8edb 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > @@ -1177,13 +1177,11 @@ static void
> > > > intel_psr_enable_source(struct
> > > > intel_dp *intel_dp,
> > > >          * Wa_16013835468
> > > >          * Wa_14015648006
> > > >          */
> > > > -       if (IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) ||
> > > > -           IS_DISPLAY_VER(dev_priv, 12, 13)) {
> > > > -               if (crtc_state-
> > > > >hw.adjusted_mode.crtc_vblank_start
> > > > !=
> > > > -                   crtc_state->hw.adjusted_mode.crtc_vdisplay)
> > > 
> > > That looks like something we want to keep. The delayed vblank w/a
> > > is
> > > still supposedly necessary.
> > 
> > 16013835468 and 14015648006 are specifically mentioning "low power
> > watermark (level 1 and up) is disabled" I haven't seen or couldn't
> > find
> > any older Wa speaking generally about delayed vblank.
> 
> 14015648006:
> "High refresh rate panels with small vblank size (either because of
> the
>  panel vblank size or the internal delayed vblank) must have some
>  watermark levels disabled..."
> -> that's the w/a you're now implementing, so the comment is
>    lying to us by claiming it was already implemented
> 
> 16013835468:
> "Display underrun when using delayed vblank with PSR2..."
> -> that's the one we actually already had implemented
> 
> > 
> > > 
> > > > -                       intel_de_rmw(dev_priv,
> > > > GEN8_CHICKEN_DCPR_1,
> > > > 0,
> > > > -                                   
> > > > wa_16013835468_bit_get(intel_dp));
> > > > -       }
> > > > +       if ((IS_MTL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0) ||
> > > > +            IS_DISPLAY_VER(dev_priv, 12, 13)) &&
> > > 
> > > I think we need this for all KBL+.
> > 
> > Do you have Wa lineage for a reference?
> 
> I think it was part of the w/a 1136. You see it only lists
> skl/bxt/kbl a-b to need the full psr disable, leaving kbl c+
> to do something different. And the latency reporting chicken
> bits were added exactly for kbl c+.
> 
> But yeah, the way this is now documented in bpsec is very poor.
> And sadly the older hsds have now disappread into bit heaven
> so we can't double check the full details there. But I'm 99%
> sure I read through those hsds in the past and came to this
> same conclusion back then.

I just sent new version of the set. I.e. disabling PSR for < 12 and
using chicken bit for >= 12 if any wm level is disabled. That is at
least safe bet... Can you please check the set if that is ok to you? :

https://patchwork.freedesktop.org/series/115109/

> 
> > 
> > > 
> > > > +           crtc_state->wm_level_disabled)
> > > > +               intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1, 0,
> > > > +                            wa_16013835468_bit_get(intel_dp));
> > > >  
> > > >         if (intel_dp->psr.psr2_enabled) {
> > > >                 if (DISPLAY_VER(dev_priv) == 9)
> > > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > index 50a9a6adbe32..afb751c024ba 100644
> > > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > @@ -2273,9 +2273,12 @@ static int skl_wm_check_vblank(struct
> > > > intel_crtc_state *crtc_state)
> > > >                 return level;
> > > >  
> > > >         /*
> > > > -        * FIXME PSR needs to toggle
> > > > LATENCY_REPORTING_REMOVED_PIPE_*
> > > > +        * PSR needs to toggle LATENCY_REPORTING_REMOVED_PIPE_*
> > > >          * based on whether we're limited by the vblank
> > > > duration.
> > > > -        *
> > > > +        */
> > > > +       crtc_state->wm_level_disabled = level < i915-
> > > > > display.wm.num_levels - 1;
> > > > +
> > > > +       /*
> > > >          * FIXME also related to skl+ w/a 1136 (also
> > > > unimplemented
> > > > as of
> > > >          * now) perhaps?
> > > >          */
> > > 
> > > And that is more or less corresponding w/a for SKL/BXT that did
> > > not
> > > yet have
> > > these chicken bits.
> > 
> > Ok, I will check this as well.
> > 
> > > 
> > > > -- 
> > > > 2.34.1
> > > 
> > 
>