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?
Why did you choose to add new hooks instead of using existing
intel_psr_pre_plane_update and intel_psr_post_plane_update?
> 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__ */
