Re: [Intel-gfx] [PATCH v2] drm/i915/display: Ensure PSR gets disabled if no encoders in new state

["Souza, Jose" <jose.souza@intel.com>]

On Thu, 2022-07-14 at 07:22 +0000, Hogander, Jouni wrote:
> On Wed, 2022-07-13 at 21:04 +0000, Souza, Jose wrote:
> > On Wed, 2022-07-13 at 20:58 +0000, Souza, Jose wrote:
> > > On Mon, 2022-07-11 at 14:17 +0300, Jouni Högander wrote:
> > > > Currently PSR is left enabled when all planes are disabled if
> > > > there
> > > > is no attached encoder in new state. This seems to be causing
> > > > FIFO
> > > > underruns.
> > > 
> > > What is the case were there is no attached encoder and
> > > active_planes > 0?
> > 
> > Can you point to some test?
> 
> You just need to run kms_busy@basic and this triggers when dynamic
> subtest modeset is run.

Thanks, were able to reproduce the issue.

> 
> > I believe that a pipe to be enabled needs to have a enconder/port
> > attached, otherwise it will be disabled.
> 
> To my understanding pipe actually gets eventually disabled in this
> testcase as well. Before pipe is disabled we have state where planes
> are all disabled, but PSR is kept enabled. This was triggering FIFO
> underrun.

Yep that was happening but this is not the proper solution.
for_each_oldnew_intel_crtc_in_state() will iterate over all CRTCs in the drm_i915_private, not only the CRTC passed as parameter.

As this was already merged I have sent the fix: https://patchwork.freedesktop.org/series/106357/

> 
> > 
> > > > Fix this by checking if encoder exists in new crtc state and
> > > > disable
> > > > PSR if it doesn't.
> > > > 
> > > > v2: Unify disable logic with existing
> > > > 
> > > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > > Reported-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com
> > > > > 
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_psr.c | 53 ++++++++++++++--
> > > > --------
> > > >  1 file changed, 31 insertions(+), 22 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > index e6a870641cd2..90599dd1cb1b 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > @@ -1863,36 +1863,45 @@ void intel_psr_pre_plane_update(struct
> > > > intel_atomic_state *state,
> > > >  struct intel_crtc *crtc)
> > > >  {
> > > >  struct drm_i915_private *i915 = to_i915(state->base.dev);
> > > > -const struct intel_crtc_state *crtc_state =
> > > > -intel_atomic_get_new_crtc_state(state, crtc);
> > > > -struct intel_encoder *encoder;
> > > > +struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> > > > +int i;
> > > > 
> > > >  if (!HAS_PSR(i915))
> > > >  return;
> > > > 
> > > > -for_each_intel_encoder_mask_with_psr(state->base.dev, encoder,
> > > > -     crtc_state->uapi.encoder_mask) {
> > > > -struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > > -struct intel_psr *psr = &intel_dp->psr;
> > > > -bool needs_to_disable = false;
> > > > +for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > > +    new_crtc_state, i) {
> > > > +struct intel_encoder *encoder;
> > > > +u32 old_new_encoder_mask = old_crtc_state->uapi.encoder_mask |
> > > > +new_crtc_state->uapi.encoder_mask;
> > > > 
> > > > -mutex_lock(&psr->lock);
> > > > +for_each_intel_encoder_mask_with_psr(state->base.dev, encoder,
> > > > +     old_new_encoder_mask) {
> > > > +struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > > +struct intel_psr *psr = &intel_dp->psr;
> > > > +bool needs_to_disable = false;
> > > > 
> > > > -/*
> > > > - * Reasons to disable:
> > > > - * - PSR disabled in new state
> > > > - * - All planes will go inactive
> > > > - * - Changing between PSR versions
> > > > - */
> > > > -needs_to_disable |= intel_crtc_needs_modeset(crtc_state);
> > > > -needs_to_disable |= !crtc_state->has_psr;
> > > > -needs_to_disable |= !crtc_state->active_planes;
> > > > -needs_to_disable |= crtc_state->has_psr2 != psr->psr2_enabled;
> > > > +mutex_lock(&psr->lock);
> > > > 
> > > > -if (psr->enabled && needs_to_disable)
> > > > -intel_psr_disable_locked(intel_dp);
> > > > +/*
> > > > + * Reasons to disable:
> > > > + * - PSR disabled in new state
> > > > + * - All planes will go inactive
> > > > + * - Changing between PSR versions
> > > > + * - Encoder isn't present in new mask
> > > > + */
> > > > +needs_to_disable |= intel_crtc_needs_modeset(new_crtc_state);
> > > > +needs_to_disable |= !new_crtc_state->has_psr;
> > > > +needs_to_disable |= !new_crtc_state->active_planes;
> > > > +needs_to_disable |= new_crtc_state->has_psr2 != psr-
> > > > > psr2_enabled;
> > > > +needs_to_disable |= !(new_crtc_state->uapi.encoder_mask &
> > > > +      drm_encoder_mask(&(encoder)->base));
> > > > 
> > > > -mutex_unlock(&psr->lock);
> > > > +if (psr->enabled && needs_to_disable)
> > > > +intel_psr_disable_locked(intel_dp);
> > > > +
> > > > +mutex_unlock(&psr->lock);
> > > > +}
> > > >  }
> > > >  }
> > > > 
>