[Intel-gfx] [PATCH] drm/i915: wait PSR state back to idle when turn PSR off

[Souza, Jose]

On Wed, 2020-10-21 at 22:24 +0800, Lee Shawn C wrote:
> Driver should refer to commit 'b2fc2252ce41 ("drm/i915/psr:
> Always wait for idle state when disabling PSR")' to wait for
> idle state when turn PSR off. But it did not follow
> previous method. Driver just call intel_psr_exit() in
> intel_psr_invalidate() and psr_force_hw_tracking_exit().
> Then leave the function right away.
> 
> After PSR disabled, we found some user space applications
> would enabled PSR again immediately. That caused particular
> TCON to get into incorrect state machine and can't recognize
> video data from source properly.

How? I don't see how this is possible this change is only adding delay between userspace calls.

Take a look at intel_psr_work(), PSR will only be enabled again when idle.

> 
> Add this change to wait PSR idle state in intel_psr_invalidate()
> and psr_force_hw_tracking_exit(). This symptom is not able
> to replicate anymore.
> 
> Fixes: b2fc2252ce41 (drm/i915/psr: Always wait for idle state
> when disabling PSR).
> 
> Cc: Manasi Navare <manasi.d.navare at intel.com>
> Cc: Jani Nikula <jani.nikula at linux.intel.com>
> Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> Cc: José Roberto de Souza <jose.souza at intel.com>
> Cc: Cooper Chiou <cooper.chiou at intel.com>
> Cc: Khaled Almahallawy <khaled.almahallawy at intel.com>
> Signed-off-by: Lee Shawn C <shawn.c.lee at intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_psr.c | 43 ++++++++++++++----------
>  1 file changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index a591a475f148..83b642a5567e 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1036,6 +1036,25 @@ void intel_psr_enable(struct intel_dp *intel_dp,
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
>  
> 
> 
> 
> +static void intel_psr_wait_idle(struct drm_i915_private *dev_priv)
> +{
> +	i915_reg_t psr_status;
> +	u32 psr_status_mask;
> +
> +	if (dev_priv->psr.psr2_enabled) {
> +		psr_status = EDP_PSR2_STATUS(dev_priv->psr.transcoder);
> +		psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> +	} else {
> +		psr_status = EDP_PSR_STATUS(dev_priv->psr.transcoder);
> +		psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
> +	}
> +
> +	/* Wait till PSR is idle */
> +	if (intel_de_wait_for_clear(dev_priv, psr_status,
> +				    psr_status_mask, 2000))
> +		drm_err(&dev_priv->drm, "Timed out waiting PSR idle state\n");
> +}
> +
>  static void intel_psr_exit(struct drm_i915_private *dev_priv)
>  {
>  	u32 val;
> @@ -1076,8 +1095,6 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
>  static void intel_psr_disable_locked(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> -	i915_reg_t psr_status;
> -	u32 psr_status_mask;
>  
> 
> 
> 
>  	lockdep_assert_held(&dev_priv->psr.lock);
>  
> 
> 
> 
> @@ -1088,19 +1105,7 @@ static void intel_psr_disable_locked(struct intel_dp *intel_dp)
>  		    dev_priv->psr.psr2_enabled ? "2" : "1");
>  
> 
> 
> 
>  	intel_psr_exit(dev_priv);
> -
> -	if (dev_priv->psr.psr2_enabled) {
> -		psr_status = EDP_PSR2_STATUS(dev_priv->psr.transcoder);
> -		psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> -	} else {
> -		psr_status = EDP_PSR_STATUS(dev_priv->psr.transcoder);
> -		psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
> -	}
> -
> -	/* Wait till PSR is idle */
> -	if (intel_de_wait_for_clear(dev_priv, psr_status,
> -				    psr_status_mask, 2000))
> -		drm_err(&dev_priv->drm, "Timed out waiting PSR idle state\n");
> +	intel_psr_wait_idle(dev_priv);
>  
> 
> 
> 
>  	/* WA 1408330847 */
>  	if (dev_priv->psr.psr2_sel_fetch_enabled &&
> @@ -1158,12 +1163,14 @@ static void psr_force_hw_tracking_exit(struct drm_i915_private *dev_priv)
>  		 * pipe.
>  		 */
>  		intel_de_write(dev_priv, CURSURFLIVE(dev_priv->psr.pipe), 0);
> -	else
> +	else {
>  		/*
>  		 * A write to CURSURFLIVE do not cause HW tracking to exit PSR
>  		 * on older gens so doing the manual exit instead.
>  		 */
>  		intel_psr_exit(dev_priv);
> +		intel_psr_wait_idle(dev_priv);
> +	}
>  }
>  
> 
> 
> 
>  void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> @@ -1593,8 +1600,10 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv,
>  	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(dev_priv->psr.pipe);
>  	dev_priv->psr.busy_frontbuffer_bits |= frontbuffer_bits;
>  
> 
> 
> 
> -	if (frontbuffer_bits)
> +	if (frontbuffer_bits) {
>  		intel_psr_exit(dev_priv);
> +		intel_psr_wait_idle(dev_priv);
> +	}
>  
> 
> 
> 
>  	mutex_unlock(&dev_priv->psr.lock);
>  }