[Intel-gfx] [PATCH v5 8/9] drm/i915: Force PSR exit when getting pipe CRC

[Dhinakaran Pandiyan]

On Tue, 2019-03-05 at 22:47 -0800, José Roberto de Souza wrote:
> If PSR is active when pipe CRC is enabled the CRC calculations will
> be inhibit by the transition to low power states that PSR brings.
The MMIO write to enable CRCs should bring the hardware out from PSR,
but I can imagine some initial CRCs  are going to be corrupt or
unavailable.

> So lets for a PSR exit and as soon as pipe CRC is enabled it will
There is a missing word.

> block PSR activation avoid CRC timeouts when running IGT tests.

This surely has fdo bugs, please include them in the commit message.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 36 ++++++++++++++++++++--------
> ----
>  1 file changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index d3e3996551c6..5d66e7313c75 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -452,6 +452,7 @@ static void hsw_activate_psr1(struct intel_dp
> *intel_dp)
>  	 * frames, we'll go with 9 frames for now
>  	 */
>  	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency
> + 1);
> +
>  	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
>  
>  	val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
> @@ -851,6 +852,20 @@ void intel_psr_disable(struct intel_dp
> *intel_dp,
>  	cancel_work_sync(&dev_priv->psr.work);
>  }
>  
> +static void psr_force_hw_tracking_exit(struct drm_i915_private
> *dev_priv)
> +{
> +	/*
> +	 * Display WA #0884: all
> +	 * This documented WA for bxt can be safely applied
> +	 * broadly so we can force HW tracking to exit PSR
> +	 * instead of disabling and re-enabling.
> +	 * Workaround tells us to write 0 to CUR_SURFLIVE_A,
> +	 * but it makes more sense write to the current active
> +	 * pipe.
> +	 */
> +	I915_WRITE(CURSURFLIVE(dev_priv->psr.pipe), 0);
> +}
> +
>  /**
>   * intel_psr_update - Update PSR state
>   * @intel_dp: Intel DP
> @@ -875,8 +890,13 @@ void intel_psr_update(struct intel_dp *intel_dp,
>  	enable = crtc_state->has_psr && psr_global_enabled(psr->debug);
>  	psr2_enable = intel_psr2_enabled(dev_priv, crtc_state);
>  
> -	if (enable == psr->enabled && psr2_enable == psr->psr2_enabled)
> +	if (enable == psr->enabled && psr2_enable == psr->psr2_enabled) 
> {

PSR2 is enabled, then user requests CRCs, the mode_changed commit
switches to PSR1. The above condition isn't true in that case.

Also, since the CRC workaround is done before enabling pipe CRC, isn't
there a possibility that the idle frame counter times out and activates
PSR1 before CRC is enabled?

> +		/* Force a PSR exit when enabling CRC to avoid CRC
> timeouts */
> +		if (crtc_state->crc_enabled && psr->enabled)
> +			psr_force_hw_tracking_exit(dev_priv);
The patch fixes a PSR1 issue, why isn't there any reference to PSR1
anywhere?


> +
>  		goto unlock;
> +	}
>  
>  	if (psr->enabled)
>  		intel_psr_disable_locked(intel_dp);
> @@ -1146,18 +1166,8 @@ void intel_psr_flush(struct drm_i915_private
> *dev_priv,
>  	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
>  
>  	/* By definition flush = invalidate + flush */
> -	if (frontbuffer_bits) {
> -		/*
> -		 * Display WA #0884: all
> -		 * This documented WA for bxt can be safely applied
> -		 * broadly so we can force HW tracking to exit PSR
> -		 * instead of disabling and re-enabling.
> -		 * Workaround tells us to write 0 to CUR_SURFLIVE_A,
> -		 * but it makes more sense write to the current active
> -		 * pipe.
> -		 */
> -		I915_WRITE(CURSURFLIVE(dev_priv->psr.pipe), 0);
> -	}
> +	if (frontbuffer_bits)
> +		psr_force_hw_tracking_exit(dev_priv);
>  
>  	if (!dev_priv->psr.active && !dev_priv-
> >psr.busy_frontbuffer_bits)
>  		schedule_work(&dev_priv->psr.work);