| Message ID | 20220714150755.154985-2-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] Revert "drm/i915/display: Ensure PSR gets disabled if no encoders in new state" | expand |
On Thu, 2022-07-14 at 08:07 -0700, José Roberto de Souza wrote: > The issue here was on for_each_intel_encoder_mask_with_psr() over the > new_crtc_state encoder mask, so if the CRTC was being disabled mask > would be zero and it would not have any chance to disable PSR. > > So here doing for_each_intel_encoder_mask_with_psr() over the > old_crtc_state encoder mask and then using the new_crtc_state to > check if PSR needs to be disabled. > > Cc: Jouni Högander <jouni.hogander@intel.com> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/display/intel_psr.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > b/drivers/gpu/drm/i915/display/intel_psr.c > index e6a870641cd25..98c3c8015a5c4 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.c > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > @@ -1863,7 +1863,9 @@ 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 = > + const struct intel_crtc_state *old_crtc_state = > + intel_atomic_get_old_crtc_state(state, crtc); > + const struct intel_crtc_state *new_crtc_state = > intel_atomic_get_new_crtc_state(state, crtc); > struct intel_encoder *encoder; > > @@ -1871,7 +1873,7 @@ void intel_psr_pre_plane_update(struct > intel_atomic_state *state, > return; > > for_each_intel_encoder_mask_with_psr(state->base.dev, encoder, > - crtc_state- > >uapi.encoder_mask) { > + old_crtc_state- > >uapi.encoder_mask) { I would add comment here explaining why using encoder mask from old_crtc_state, but using new_crtc_state inside the loop. It's up to you: Reviewed-by: Jouni Högander <jouni.hogander@intel.com> > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > struct intel_psr *psr = &intel_dp->psr; > bool needs_to_disable = false; > @@ -1884,10 +1886,10 @@ void intel_psr_pre_plane_update(struct > intel_atomic_state *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; > + 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);
On Fri, Jul 15, 2022 at 08:33:43AM +0300, Hogander, Jouni wrote: > On Thu, 2022-07-14 at 08:07 -0700, José Roberto de Souza wrote: > > The issue here was on for_each_intel_encoder_mask_with_psr() over the > > new_crtc_state encoder mask, so if the CRTC was being disabled mask > > would be zero and it would not have any chance to disable PSR. > > > > So here doing for_each_intel_encoder_mask_with_psr() over the > > old_crtc_state encoder mask and then using the new_crtc_state to > > check if PSR needs to be disabled. Are we sure that using old_crtc_state mask is safe here? Because currently we would be basically mixing a usage of encoder from old_crtc_state mask with new_crtc_state. If you mention a specific scenario, when this happens(i.e crtc is being disabled and new mask is 0) should we add a specific check instructing us to use old_crtc_state mask only? Because currently you might be touching some other scenarios as well, that is what I'm concerned about. Stan > > > > Cc: Jouni Högander <jouni.hogander@intel.com> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_psr.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > b/drivers/gpu/drm/i915/display/intel_psr.c > > index e6a870641cd25..98c3c8015a5c4 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > @@ -1863,7 +1863,9 @@ 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 = > > + const struct intel_crtc_state *old_crtc_state = > > + intel_atomic_get_old_crtc_state(state, crtc); > > + const struct intel_crtc_state *new_crtc_state = > > intel_atomic_get_new_crtc_state(state, crtc); > > struct intel_encoder *encoder; > > > > @@ -1871,7 +1873,7 @@ void intel_psr_pre_plane_update(struct > > intel_atomic_state *state, > > return; > > > > for_each_intel_encoder_mask_with_psr(state->base.dev, encoder, > > - crtc_state- > > >uapi.encoder_mask) { > > + old_crtc_state- > > >uapi.encoder_mask) { > > I would add comment here explaining why using encoder mask from > old_crtc_state, but using new_crtc_state inside the loop. It's up to > you: > > Reviewed-by: Jouni Högander <jouni.hogander@intel.com> > > > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > struct intel_psr *psr = &intel_dp->psr; > > bool needs_to_disable = false; > > @@ -1884,10 +1886,10 @@ void intel_psr_pre_plane_update(struct > > intel_atomic_state *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; > > + 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); >
On Fri, 2022-07-15 at 11:39 +0300, Lisovskiy, Stanislav wrote: > On Fri, Jul 15, 2022 at 08:33:43AM +0300, Hogander, Jouni wrote: > > On Thu, 2022-07-14 at 08:07 -0700, José Roberto de Souza wrote: > > > The issue here was on for_each_intel_encoder_mask_with_psr() over the > > > new_crtc_state encoder mask, so if the CRTC was being disabled mask > > > would be zero and it would not have any chance to disable PSR. > > > > > > So here doing for_each_intel_encoder_mask_with_psr() over the > > > old_crtc_state encoder mask and then using the new_crtc_state to > > > check if PSR needs to be disabled. > > Are we sure that using old_crtc_state mask is safe here? > Because currently we would be basically mixing a usage of > encoder from old_crtc_state mask with new_crtc_state. > > If you mention a specific scenario, when this happens(i.e crtc > is being disabled and new mask is 0) should we add a specific check > instructing us to use old_crtc_state mask only? > This scenario were new_crtc_state is being disabled(ie encoder_mask = 0) is handled by intel_crtc_needs_modeset() check. This same check will handle more complicated cases like pipe A(with PSR) now will drive a different port... > Because currently you might be touching some other scenarios as > well, that is what I'm concerned about. > > > Stan > > > > > > > Cc: Jouni Högander <jouni.hogander@intel.com> > > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_psr.c | 14 ++++++++------ > > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > > b/drivers/gpu/drm/i915/display/intel_psr.c > > > index e6a870641cd25..98c3c8015a5c4 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > @@ -1863,7 +1863,9 @@ 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 = > > > + const struct intel_crtc_state *old_crtc_state = > > > + intel_atomic_get_old_crtc_state(state, crtc); > > > + const struct intel_crtc_state *new_crtc_state = > > > intel_atomic_get_new_crtc_state(state, crtc); > > > struct intel_encoder *encoder; > > > > > > @@ -1871,7 +1873,7 @@ void intel_psr_pre_plane_update(struct > > > intel_atomic_state *state, > > > return; > > > > > > for_each_intel_encoder_mask_with_psr(state->base.dev, encoder, > > > - crtc_state- > > > > uapi.encoder_mask) { > > > + old_crtc_state- > > > > uapi.encoder_mask) { > > > > I would add comment here explaining why using encoder mask from > > old_crtc_state, but using new_crtc_state inside the loop. It's up to > > you: > > > > Reviewed-by: Jouni Högander <jouni.hogander@intel.com> > > > > > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > > struct intel_psr *psr = &intel_dp->psr; > > > bool needs_to_disable = false; > > > @@ -1884,10 +1886,10 @@ void intel_psr_pre_plane_update(struct > > > intel_atomic_state *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; > > > + 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); > >
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c index e6a870641cd25..98c3c8015a5c4 100644 --- a/drivers/gpu/drm/i915/display/intel_psr.c +++ b/drivers/gpu/drm/i915/display/intel_psr.c @@ -1863,7 +1863,9 @@ 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 = + const struct intel_crtc_state *old_crtc_state = + intel_atomic_get_old_crtc_state(state, crtc); + const struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); struct intel_encoder *encoder; @@ -1871,7 +1873,7 @@ void intel_psr_pre_plane_update(struct intel_atomic_state *state, return; for_each_intel_encoder_mask_with_psr(state->base.dev, encoder, - crtc_state->uapi.encoder_mask) { + old_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; @@ -1884,10 +1886,10 @@ void intel_psr_pre_plane_update(struct intel_atomic_state *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; + 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);
The issue here was on for_each_intel_encoder_mask_with_psr() over the new_crtc_state encoder mask, so if the CRTC was being disabled mask would be zero and it would not have any chance to disable PSR. So here doing for_each_intel_encoder_mask_with_psr() over the old_crtc_state encoder mask and then using the new_crtc_state to check if PSR needs to be disabled. Cc: Jouni Högander <jouni.hogander@intel.com> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/display/intel_psr.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)