[Intel-gfx] [PATCH 2/3] drm/i915/display/psr: Lock and unlock PSR around pipe updates

Souza, Jose jose.souza at intel.com
Mon Apr 4 13:43:33 UTC 2022


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 at intel.com>
> > Cc: Mika Kahola <mika.kahola at intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > ---
> >  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__ */
> 



More information about the Intel-gfx mailing list