[Intel-gfx] [PATCH v6 1/2] drm/i915/psr: Lockless version of psr_wait_for_idle

[Chris Wilson]

Quoting Daniel Vetter (2018-06-26 09:26:57)
> On Mon, Jun 25, 2018 at 10:57:23PM -0700, Tarun Vyas wrote:
> > 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.
> 
> What's the upside of the lockless wait? The patch seems to be entirely
> missing the motivation for the change. "Make it lockless" isn't a good
> justification on itself, there needs to be data about overhead or stalls
> included if that's the reason for doing this change.
> 
> > 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)
> > 
> > v3: Rename psr_wait_for_idle to __psr_wait_for_idle_locked to avoid
> >     naming conflicts and propagate err (if any) to the caller (Chris)
> > 
> > v5: Form a series with the next patch
> > 
> > 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 | 25 +++++++++++++++++++++++--
> >  2 files changed, 24 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 578346b8d7e2..9cb2b8afdd3e 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);
> > +int intel_psr_wait_for_idle(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..41e6962923ae 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -757,7 +757,28 @@ void intel_psr_disable(struct intel_dp *intel_dp,
> >       cancel_work_sync(&dev_priv->psr.work);
> >  }
> >  
> > -static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > +int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > +{
> > +     i915_reg_t reg;
> > +     u32 mask;
> > +
> 
> I think a comment here explaining why the lockless access is correct is
> justified here.
> 
> > +     if (dev_priv->psr.psr2_enabled) {

In particular, it is this 'psr2_enabled' and which register we need that
is serialised in the locked version. The important question to answer is
why can we lift that here and not there.

In this case (and even in the other case), you could simply say "do both".
-Chris