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.
>
> > 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_EVASION_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__ */
>
