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@xxxxxxxxx> > > > > Reported-by: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx> > > > > Signed-off-by: Jouni Högander <jouni.hogander@xxxxxxxxx> > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx > > > > > > > > > --- > > > > 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); > > > > +} > > > > } > > > > } > > > > >
