[Intel-gfx] [PATCH v2] drm/i915/psr: Lockless version of psr_wait_for_idle

[Tarun Vyas]

On Fri, Jun 22, 2018 at 08:12:00AM +0100, Chris Wilson wrote:
> Quoting Tarun Vyas (2018-06-22 08:05:21)
> > This is a lockless version of the exisiting psr_wait_for_idle().
> > We want to wait for PSR to idle out inside intel_pipe_update_start.
> > At the time of a pipe update, we should never race with any psr
> > enable or disable code, which is a part of crtc enable/disable. So,
> > we can live w/o taking any psr locks at all.
> > The follow up patch will use this lockless wait inside pipe_update_
> > start to wait for PSR to idle out before checking for vblank evasion.
> > 
> > Even if psr is never enabled, psr2_enabled will be false and this
> > function will wait for PSR1 to idle out, which should just return
> > immediately, so a very short (~1-2 usec) wait for cases where PSR
> > is disabled.
> > 
> > v2: Add comment to explain the 25msec timeout (DK)
> > 
> > Signed-off-by: Tarun Vyas <tarun.vyas at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> >  drivers/gpu/drm/i915/intel_psr.c | 24 ++++++++++++++++++++++++
> >  2 files changed, 25 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 578346b8d7e2..a48aad0f99bf 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1920,6 +1920,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
> >                               struct intel_crtc_state *crtc_state);
> >  void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug);
> >  void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
> > +void psr_wait_for_idle_lockless(struct drm_i915_private *dev_priv);
> >  
> >  /* intel_runtime_pm.c */
> >  int intel_power_domains_init(struct drm_i915_private *);
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index aea81ace854b..8e69e6193063 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -757,6 +757,30 @@ void intel_psr_disable(struct intel_dp *intel_dp,
> >         cancel_work_sync(&dev_priv->psr.work);
> >  }
> >  
> > +void psr_wait_for_idle_lockless(struct drm_i915_private *dev_priv)
> 
> intel_psr_
> 
> No need for lockless here you don't export anything else. Rename one of
> the two to avoid the conflict. Probably __psr_wait_for_idle_locked,
> though you would expect some sort of reuse between very similarly named
> functions.
>
Thanks for the review Chris.

Yes, there is a lot duplication between the two, but the locking drop in psr_wait_for_idle(), before the wait_for_register is making it harder to reuse it.
Will rename the original wait function.  
> > +{
> > +       i915_reg_t reg;
> > +       u32 mask;
> > +       int err;
> > +
> > +       if (dev_priv->psr.psr2_enabled) {
> > +               reg = EDP_PSR2_STATUS;
> > +               mask = EDP_PSR2_STATUS_STATE_MASK;
> > +       } else {
> > +               reg = EDP_PSR_STATUS;
> > +               mask = EDP_PSR_STATUS_STATE_MASK;
> > +       }
> > +
> > +       /*
> > +        * The  25 msec timeout accounts for a frame @ 60Hz refresh rate,
> > +        * exit training an aux handshake time.
> > +        */
> > +       err = intel_wait_for_register(dev_priv, reg, mask,
> > +                                     EDP_PSR_STATUS_STATE_IDLE, 25);
> > +       if (err)
> > +               DRM_ERROR("Timed out waiting for PSR Idle for pipe update\n");
> 
> Propagate the error, let the caller decide if it's an error or not.
> -Chrisa
Will do, in the next version.