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

[Tarun Vyas]

On Tue, Jun 26, 2018 at 09:42:11AM +0100, Chris Wilson wrote:
> 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
With the locked case, the concern was that intel_psr_work can race with psr_enable/disable paths but we have no such concern here, as we are already too far in the commit path. psr2_enabled should be stable by the time we do pipe_update(s), so we can do a fast check in pipe_update w/o grabbing and subsequently dropping locks.

Also, another difference here is that, "intel_dp = dev_priv->psr.enabled;" has been dropped in the lockless version as we don't really care if psr is enabled or disabled. If it is disabled, then the intel_wait_for_register should be close to a noop, ~1usec hit as per the preliminary measurements.