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@xxxxxxxxx>
> > > > ---
> > > > 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
> > >
> >
>
