On Mon, 2022-04-04 at 13:43 +0000, Souza, Jose wrote:
> On Mon, 2022-04-04 at 07:41 +0000, Hogander, Jouni wrote:
> > Hello,
> >
> > Couple of questions below.
> > On Fri, 2022-04-01 at 15:29 -0700, José Roberto de Souza wrote:
> > > Frontbuffer rendering and page flips can race with each other
> > > and this can potentialy cause issues with PSR2 selective fetch.
> > >
> > > And because pipe/crtc updates are time sentive we can't grab the
> > > PSR lock after intel_pipe_update_start() and before
> > > intel_pipe_update_end().
> > >
> > > So here adding the lock and unlock functions and calls, the
> > > proper PSR2 selective fetch handling will come in a separated
> > > patch.
> > >
> >
> > Have you ensured that there is no case where pipe_update_end is not
> > called?
>
> Yep.
>
> > Why did you choose to add new hooks instead of using existing
> > intel_psr_pre_plane_update and intel_psr_post_plane_update?
>
> The lock would be held for too much time we those functions were
> used.
> In a modeset case it would take the lock before disable CRTC and only
> would release a way after pipes are enabled.
>
> If in the mean time something needs the PSR lock it would be blocked
> for a few milliseconds while here would be very small window of time.
Reviewed-by: Jouni Högander <jouni.hogander@xxxxxxxxx>
>
> > > Cc: Jouni Högander <jouni.hogander@xxxxxxxxx>
> > > Cc: Mika Kahola <mika.kahola@xxxxxxxxx>
> > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_crtc.c | 6 +-
> > > drivers/gpu/drm/i915/display/intel_psr.c | 69
> > > ++++++++++++++++++++-
> > > --
> > > drivers/gpu/drm/i915/display/intel_psr.h | 5 +-
> > > 3 files changed, 70 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c
> > > b/drivers/gpu/drm/i915/display/intel_crtc.c
> > > index f655c16228776..a5439182d5ae4 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> > > @@ -507,6 +507,8 @@ void intel_pipe_update_start(struct
> > > intel_crtc_state *new_crtc_state)
> > > VBLANK_EVASIO
> > > N_TI
> > > ME_US);
> > > max = vblank_start - 1;
> > >
> > > + intel_psr_lock(new_crtc_state);
> > > +
> > > if (min <= 0 || max <= 0)
> > > goto irq_disable;
> > >
> > > @@ -518,7 +520,7 @@ void intel_pipe_update_start(struct
> > > intel_crtc_state *new_crtc_state)
> > > * VBL interrupts will start the PSR exit and prevent a PSR
> > > * re-entry as well.
> > > */
> > > - intel_psr_wait_for_idle(new_crtc_state);
> > > + intel_psr_wait_for_idle_locked(new_crtc_state);
> > >
> > > local_irq_disable();
> > >
> > > @@ -683,6 +685,8 @@ void intel_pipe_update_end(struct
> > > intel_crtc_state *new_crtc_state)
> > >
> > > local_irq_enable();
> > >
> > > + intel_psr_unlock(new_crtc_state);
> > > +
> > > if (intel_vgpu_active(dev_priv))
> > > return;
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 2da2468f555ec..58597480054eb 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -1548,10 +1548,19 @@ void
> > > intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> > > void intel_psr2_program_trans_man_trk_ctl(const struct
> > > intel_crtc_state *crtc_state)
> > > {
> > > struct drm_i915_private *dev_priv = to_i915(crtc_state-
> > > > uapi.crtc->dev);
> > > + struct intel_encoder *encoder;
> > >
> > > if (!crtc_state->enable_psr2_sel_fetch)
> > > return;
> > >
> > > + for_each_intel_encoder_mask_with_psr(&dev_priv->drm,
> > > encoder,
> > > + crtc_state-
> > > > uapi.encoder_mask) {
> > > + struct intel_dp *intel_dp =
> > > enc_to_intel_dp(encoder);
> > > +
> > > + lockdep_assert_held(&intel_dp->psr.lock);
> > > + break;
> > > + }
> > > +
> > > intel_de_write(dev_priv, PSR2_MAN_TRK_CTL(crtc_state-
> > > > cpu_transcoder),
> > > crtc_state->psr2_man_track_ctl);
> > > }
> > > @@ -1919,13 +1928,13 @@ static int
> > > _psr1_ready_for_pipe_update_locked(struct intel_dp *intel_dp)
> > > }
> > >
> > > /**
> > > - * intel_psr_wait_for_idle - wait for PSR be ready for a pipe
> > > update
> > > + * intel_psr_wait_for_idle_locked - wait for PSR be ready for a
> > > pipe
> > > update
> > > * @new_crtc_state: new CRTC state
> > > *
> > > * This function is expected to be called from
> > > pipe_update_start()
> > > where it is
> > > * not expected to race with PSR enable or disable.
> > > */
> > > -void intel_psr_wait_for_idle(const struct intel_crtc_state
> > > *new_crtc_state)
> > > +void intel_psr_wait_for_idle_locked(const struct
> > > intel_crtc_state
> > > *new_crtc_state)
> > > {
> > > struct drm_i915_private *dev_priv = to_i915(new_crtc_state-
> > > > uapi.crtc->dev);
> > > struct intel_encoder *encoder;
> > > @@ -1938,12 +1947,10 @@ void intel_psr_wait_for_idle(const struct
> > > intel_crtc_state *new_crtc_state)
> > > struct intel_dp *intel_dp =
> > > enc_to_intel_dp(encoder);
> > > int ret;
> > >
> > > - mutex_lock(&intel_dp->psr.lock);
> > > + lockdep_assert_held(&intel_dp->psr.lock);
> > >
> > > - if (!intel_dp->psr.enabled) {
> > > - mutex_unlock(&intel_dp->psr.lock);
> > > + if (!intel_dp->psr.enabled)
> > > continue;
> > > - }
> > >
> > > if (intel_dp->psr.psr2_enabled)
> > > ret =
> > > _psr2_ready_for_pipe_update_locked(intel_dp);
> > > @@ -1952,8 +1959,6 @@ void intel_psr_wait_for_idle(const struct
> > > intel_crtc_state *new_crtc_state)
> > >
> > > if (ret)
> > > drm_err(&dev_priv->drm, "PSR wait timed
> > > out,
> > > atomic update may fail\n");
> > > -
> > > - mutex_unlock(&intel_dp->psr.lock);
> > > }
> > > }
> > >
> > > @@ -2444,3 +2449,51 @@ bool intel_psr_enabled(struct intel_dp
> > > *intel_dp)
> > >
> > > return ret;
> > > }
> > > +
> > > +/**
> > > + * intel_psr_lock - grab psr.lock mutex
> > > + * @crtc_state: the crtc state
> > > + *
> > > + * This is initially meant to be used by around CRTC update,
> > > when
> > > + * vblank sensitive registers are updated and we need grab the
> > > lock
> > > + * before it to avoid vblank evasion.
> > > + */
> > > +void intel_psr_lock(const struct intel_crtc_state *crtc_state)
> > > +{
> > > + struct drm_i915_private *i915 = to_i915(crtc_state-
> > > >uapi.crtc-
> > > > dev);
> > > + struct intel_encoder *encoder;
> > > +
> > > + if (!crtc_state->has_psr)
> > > + return;
> > > +
> > > + for_each_intel_encoder_mask_with_psr(&i915->drm, encoder,
> > > + crtc_state-
> > > > uapi.encoder_mask) {
> > > + struct intel_dp *intel_dp =
> > > enc_to_intel_dp(encoder);
> > > +
> > > + mutex_lock(&intel_dp->psr.lock);
> > > + break;
> > > + }
> > > +}
> > > +
> > > +/**
> > > + * intel_psr_lock - grab psr.lock mutex
> > > + * @crtc_state: the crtc state
> > > + *
> > > + * Release the PSR lock that was held during pipe update.
> > > + */
> > > +void intel_psr_unlock(const struct intel_crtc_state *crtc_state)
> > > +{
> > > + struct drm_i915_private *i915 = to_i915(crtc_state-
> > > >uapi.crtc-
> > > > dev);
> > > + struct intel_encoder *encoder;
> > > +
> > > + if (!crtc_state->has_psr)
> > > + return;
> > > +
> > > + for_each_intel_encoder_mask_with_psr(&i915->drm, encoder,
> > > + crtc_state-
> > > > uapi.encoder_mask) {
> > > + struct intel_dp *intel_dp =
> > > enc_to_intel_dp(encoder);
> > > +
> > > + mutex_unlock(&intel_dp->psr.lock);
> > > + break;
> > > + }
> > > +}
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> > > b/drivers/gpu/drm/i915/display/intel_psr.h
> > > index f6526d9ccfdc6..2ac3a46cccc50 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> > > @@ -41,7 +41,7 @@ void intel_psr_get_config(struct intel_encoder
> > > *encoder,
> > > struct intel_crtc_state *pipe_config);
> > > void intel_psr_irq_handler(struct intel_dp *intel_dp, u32
> > > psr_iir);
> > > void intel_psr_short_pulse(struct intel_dp *intel_dp);
> > > -void intel_psr_wait_for_idle(const struct intel_crtc_state
> > > *new_crtc_state);
> > > +void intel_psr_wait_for_idle_locked(const struct
> > > intel_crtc_state
> > > *new_crtc_state);
> > > bool intel_psr_enabled(struct intel_dp *intel_dp);
> > > int intel_psr2_sel_fetch_update(struct intel_atomic_state
> > > *state,
> > > struct intel_crtc *crtc);
> > > @@ -55,4 +55,7 @@ void intel_psr2_disable_plane_sel_fetch(struct
> > > intel_plane *plane,
> > > void intel_psr_pause(struct intel_dp *intel_dp);
> > > void intel_psr_resume(struct intel_dp *intel_dp);
> > >
> > > +void intel_psr_lock(const struct intel_crtc_state *crtc_state);
> > > +void intel_psr_unlock(const struct intel_crtc_state
> > > *crtc_state);
> > > +
> > > #endif /* __INTEL_PSR_H__ */
