Re: [PATCH v6 1/2] drm/i915/psr: Lockless version of psr_wait_for_idle

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jun 26, 2018 at 01:20:58PM -0700, Tarun Vyas wrote:
> On Tue, Jun 26, 2018 at 12:43:42PM -0700, Dhinakaran Pandiyan wrote:
> > On Tue, 2018-06-26 at 10:26 +0200, Daniel Vetter wrote:
> > > 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.
> > > 
> > Acquiring the PSR mutex means potential stalls due to PSR work having
> > already acquired it. The idea was to keep PSR changes in
> > pipe_update_start() less invasive latency wise.
> > 
> > But yeah, the commit has to add the explanation.
> > 
> > 
> >
> Yea, will explain it better in the commit message. 

Have we measured these stalls? Is it actually faster?

Directly poking hw registers because our own software tracking is a bit
funny still feels like a rather bad hack. And without gathering data I'd
assume that the mutex_lock is contended only when the there's a state
transition going on in psr, and in that case the register wait_for will
also take quite a while (equally long really, until the psr hw settles).
-Daniel

> > > > 
> > > > 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@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) {
> > > > +		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.
> > > > +	 */
> > > So this goes boom if the panel is running at e.g. 50Hz? Please either
> > > calculate this from the current mode (but that's a bit tricky, due to
> > > DRRS), or go with a more defensive timeout. Also small typo,
> > > s/an/and/.
> > > 
> > > Would also be good to have numbers for the exit training/aux
> > > handshake
> > > time.
> > 
> > bspec says exit should be compelete in  "one full frame time (1/refresh
> > rate), plus SRD exit training time (max of 6ms), plus SRD aux channel
> > handshake (max of 1.5ms)."
> > 
> > 
> > 
> So should we use 50 Hz as the lower limit for the refresh rate to calc our max timeout here. Can eDP go down to 30 Hz ?
> > > -Daniel
> > > 
> > > > 
> > > > +	return intel_wait_for_register(dev_priv, reg, mask,
> > > > +				       EDP_PSR_STATUS_STATE_IDLE,
> > > > 25);
> > > > +}
> > > > +
> > > > +static bool __psr_wait_for_idle_locked(struct drm_i915_private
> > > > *dev_priv)
> > > >  {
> > > >  	struct intel_dp *intel_dp;
> > > >  	i915_reg_t reg;
> > > > @@ -803,7 +824,7 @@ static void intel_psr_work(struct work_struct
> > > > *work)
> > > >  	 * PSR might take some time to get fully disabled
> > > >  	 * and be ready for re-enable.
> > > >  	 */
> > > > -	if (!psr_wait_for_idle(dev_priv))
> > > > +	if (!__psr_wait_for_idle_locked(dev_priv))
> > > >  		goto unlock;
> > > >  
> > > >  	/*
> > > > -- 
> > > > 2.13.5
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx