| Message ID | 20220711061601.654694-1-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display: Ensure PSR gets disabled if no encoders in new state | expand |
On Mon, Jul 11, 2022 at 09:16:01AM +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. > > Fix this by checking if old and new crtc encoder masks are differing. > PSR is disabled for encoders not in new crtc state encoder mask. > > 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> > --- > drivers/gpu/drm/i915/display/intel_psr.c | 72 +++++++++++++++++------- > 1 file changed, 51 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > index e6a870641cd2..710cc31ace22 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.c > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > @@ -1863,36 +1863,66 @@ 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_crtc_state *new_crtc_state, *old_crtc_state; > struct intel_encoder *encoder; > + struct intel_dp *intel_dp; > + struct intel_psr *psr; > + 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; > - > - mutex_lock(&psr->lock); > - > + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > + new_crtc_state, i) { > /* > - * Reasons to disable: > - * - PSR disabled in new state > - * - All planes will go inactive > - * - Changing between PSR versions > + * Check if encoder exists in new state and disable > + * psr if it doesn't. Otherwise we will end update > + * having all planes disabled and psr enabled which > + * seems to be causing problems. > */ > - 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; > + if (old_crtc_state->uapi.encoder_mask != new_crtc_state->uapi.encoder_mask) { > + for_each_intel_encoder_mask_with_psr(state->base.dev, encoder, > + old_crtc_state->uapi.encoder_mask) { > + intel_dp = enc_to_intel_dp(encoder); > + psr = &intel_dp->psr; > > - if (psr->enabled && needs_to_disable) > - intel_psr_disable_locked(intel_dp); > + if (new_crtc_state->uapi.encoder_mask & > + drm_encoder_mask(&(encoder)->base)) > + continue; > > - mutex_unlock(&psr->lock); > + mutex_lock(&psr->lock); > + > + if (psr->enabled) > + intel_psr_disable_locked(intel_dp); > + > + mutex_unlock(&psr->lock); > + } > + } > + for_each_intel_encoder_mask_with_psr(state->base.dev, encoder, > + new_crtc_state->uapi.encoder_mask) { > + bool needs_to_disable = false; > + > + intel_dp = enc_to_intel_dp(encoder); > + psr = &intel_dp->psr; > + > + mutex_lock(&psr->lock); > + > + /* > + * Reasons to disable: > + * - PSR disabled in new state > + * - All planes will go inactive > + * - Changing between PSR versions > + */ > + 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; > + > + if (psr->enabled && needs_to_disable) > + intel_psr_disable_locked(intel_dp); > + > + mutex_unlock(&psr->lock); > + } > } > } Looks correct to me, the only thing that we could probable unite both for_each_intel_encoder_mask_with_psr cycles and also then get rid of if (old_crtc_state->uapi.encoder_mask != new_crtc_state->uapi.encoder_mask) check by doing something like: old_new_encoder_mask = old_crtc_state->uapi.encoder_mask | new_crtc_state->uapi.encoder_mask; for_each_intel_encoder_mask_with_psr(state->base.dev, encoder, old_new_encoder_mask) { bool needs_to_disable = false; intel_dp = enc_to_intel_dp(encoder); psr = &intel_dp->psr; mutex_lock(&psr->lock); 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; /* Check if this encoder isn't present in new mask */ needs_to_disable |= !(new_crtc_state->uapi.encoder_mask & drm_encoder_mask(&(encoder)->base)); if (psr->enabled && needs_to_disable) intel_psr_disable_locked(intel_dp); mutex_unlock(&psr_lock); } So basically we make non-presence of the encoder in new mask to be just one more check for needs_to_disable and have a single place where we turn it off.. Stan > > -- > 2.25.1 >
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c index e6a870641cd2..710cc31ace22 100644 --- a/drivers/gpu/drm/i915/display/intel_psr.c +++ b/drivers/gpu/drm/i915/display/intel_psr.c @@ -1863,36 +1863,66 @@ 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_crtc_state *new_crtc_state, *old_crtc_state; struct intel_encoder *encoder; + struct intel_dp *intel_dp; + struct intel_psr *psr; + 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; - - mutex_lock(&psr->lock); - + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, + new_crtc_state, i) { /* - * Reasons to disable: - * - PSR disabled in new state - * - All planes will go inactive - * - Changing between PSR versions + * Check if encoder exists in new state and disable + * psr if it doesn't. Otherwise we will end update + * having all planes disabled and psr enabled which + * seems to be causing problems. */ - 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; + if (old_crtc_state->uapi.encoder_mask != new_crtc_state->uapi.encoder_mask) { + for_each_intel_encoder_mask_with_psr(state->base.dev, encoder, + old_crtc_state->uapi.encoder_mask) { + intel_dp = enc_to_intel_dp(encoder); + psr = &intel_dp->psr; - if (psr->enabled && needs_to_disable) - intel_psr_disable_locked(intel_dp); + if (new_crtc_state->uapi.encoder_mask & + drm_encoder_mask(&(encoder)->base)) + continue; - mutex_unlock(&psr->lock); + mutex_lock(&psr->lock); + + if (psr->enabled) + intel_psr_disable_locked(intel_dp); + + mutex_unlock(&psr->lock); + } + } + for_each_intel_encoder_mask_with_psr(state->base.dev, encoder, + new_crtc_state->uapi.encoder_mask) { + bool needs_to_disable = false; + + intel_dp = enc_to_intel_dp(encoder); + psr = &intel_dp->psr; + + mutex_lock(&psr->lock); + + /* + * Reasons to disable: + * - PSR disabled in new state + * - All planes will go inactive + * - Changing between PSR versions + */ + 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; + + if (psr->enabled && needs_to_disable) + intel_psr_disable_locked(intel_dp); + + mutex_unlock(&psr->lock); + } } }
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. Fix this by checking if old and new crtc encoder masks are differing. PSR is disabled for encoders not in new crtc state encoder mask. 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> --- drivers/gpu/drm/i915/display/intel_psr.c | 72 +++++++++++++++++------- 1 file changed, 51 insertions(+), 21 deletions(-)