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

[Daniel Vetter]

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) {
> +		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.
-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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch