[Intel-gfx] [PATCH] drm/i915/psr: Chase psr.enabled only under the psr.lock

[Souza, Jose]

On Thu, 2018-04-05 at 12:49 +0100, Chris Wilson wrote:
> Inside the psr work function, we want to wait for PSR to idle first
> and
> wish to do so without blocking the normal modeset path, so we do so
> without holding the PSR lock. However, we first have to find which
> pipe
> PSR was enabled on, which requires chasing into the PSR struct and
> requires locking to prevent intel_psr_disable() from concurrently
> setting our pointer to NULL.
> 
> Fixes: 995d30477496 ("drm/i915: VLV/CHV PSR Software timer mode")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Durgadoss R <durgadoss.r at intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: <stable at vger.kernel.org> # v4.0+

Feel free to add:
Reviewed-by: Jose Roberto de Souza <jose.souza at intel.com>

> ---
>  drivers/gpu/drm/i915/intel_psr.c | 82 +++++++++++++++++++++---------
> ----------
>  1 file changed, 44 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 2d53f7398a6d..69a5b276f4d8 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -775,53 +775,59 @@ void intel_psr_disable(struct intel_dp
> *intel_dp,
>  	cancel_delayed_work_sync(&dev_priv->psr.work);
>  }
>  
> -static void intel_psr_work(struct work_struct *work)
> +static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
>  {
> -	struct drm_i915_private *dev_priv =
> -		container_of(work, typeof(*dev_priv),
> psr.work.work);
> -	struct intel_dp *intel_dp = dev_priv->psr.enabled;
> -	struct drm_crtc *crtc = dp_to_dig_port(intel_dp)-
> >base.base.crtc;
> -	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> +	struct intel_dp *intel_dp;

nitpick: Why not already set it?
struct intel_dp *intel_dp = dev_priv->psr.enabled;


> +	i915_reg_t reg;
> +	u32 mask;
> +	int err;
> +
> +	intel_dp = dev_priv->psr.enabled;
> +	if (!intel_dp)
> +		return false;
>  
> -	/* We have to make sure PSR is ready for re-enable
> -	 * otherwise it keeps disabled until next full
> enable/disable cycle.
> -	 * PSR might take some time to get fully disabled
> -	 * and be ready for re-enable.
> -	 */
>  	if (HAS_DDI(dev_priv)) {


nitpick: While on that you could replace this for:

if (!(IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))) {

>  		if (dev_priv->psr.psr2_enabled) {
> -			if (intel_wait_for_register(dev_priv,
> -						    EDP_PSR2_STATUS,
> -						    EDP_PSR2_STATUS_
> STATE_MASK,
> -						    0,
> -						    50)) {
> -				DRM_ERROR("Timed out waiting for
> PSR2 Idle for re-enable\n");
> -				return;
> -			}
> +			reg = EDP_PSR2_STATUS;
> +			mask = EDP_PSR2_STATUS_STATE_MASK;
>  		} else {
> -			if (intel_wait_for_register(dev_priv,
> -						    EDP_PSR_STATUS,
> -						    EDP_PSR_STATUS_S
> TATE_MASK,
> -						    0,
> -						    50)) {
> -				DRM_ERROR("Timed out waiting for PSR
> Idle for re-enable\n");
> -				return;
> -			}
> +			reg = EDP_PSR_STATUS;
> +			mask = EDP_PSR_STATUS_STATE_MASK;
>  		}
>  	} else {
> -		if (intel_wait_for_register(dev_priv,
> -					    VLV_PSRSTAT(pipe),
> -					    VLV_EDP_PSR_IN_TRANS,
> -					    0,
> -					    1)) {
> -			DRM_ERROR("Timed out waiting for PSR Idle
> for re-enable\n");
> -			return;
> -		}
> +		struct drm_crtc *crtc =
> +			dp_to_dig_port(intel_dp)->base.base.crtc;
> +		enum pipe pipe = to_intel_crtc(crtc)->pipe;
> +
> +		reg = VLV_PSRSTAT(pipe);
> +		mask = VLV_EDP_PSR_IN_TRANS;
>  	}
> +
> +	mutex_unlock(&dev_priv->psr.lock);
> +
> +	err = intel_wait_for_register(dev_priv, reg, mask, 0, 50);
> +	if (err)
> +		DRM_ERROR("Timed out waiting for PSR Idle for re-
> enable\n");
> +
> +	/* After the unlocked wait, verify that PSR is still wanted!
> */
>  	mutex_lock(&dev_priv->psr.lock);
> -	intel_dp = dev_priv->psr.enabled;
> +	return err == 0 && dev_priv->psr.enabled;
> +}
>  
> -	if (!intel_dp)
> +static void intel_psr_work(struct work_struct *work)
> +{
> +	struct drm_i915_private *dev_priv =
> +		container_of(work, typeof(*dev_priv),
> psr.work.work);
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +
> +	/*
> +	 * We have to make sure PSR is ready for re-enable
> +	 * otherwise it keeps disabled until next full
> enable/disable cycle.
> +	 * PSR might take some time to get fully disabled
> +	 * and be ready for re-enable.
> +	 */
> +	if (!psr_wait_for_idle(dev_priv))
>  		goto unlock;
>  
>  	/*
> @@ -832,7 +838,7 @@ static void intel_psr_work(struct work_struct
> *work)
>  	if (dev_priv->psr.busy_frontbuffer_bits)
>  		goto unlock;
>  
> -	intel_psr_activate(intel_dp);
> +	intel_psr_activate(dev_priv->psr.enabled);
>  unlock:
>  	mutex_unlock(&dev_priv->psr.lock);
>  }