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?
I believe that a pipe to be enabled needs to have a enconder/port attached,
otherwise it will be disabled.
>
> >
> > 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.kah...@intel.com>
> > Reported-by: Stanislav Lisovskiy <stanislav.lisovs...@intel.com>
> > Signed-off-by: Jouni Högander <jouni.hogan...@intel.com>
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovs...@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);
> > + }
> > }
> > }
> >
>
