Re: [PATCH] drm/i915: FBC needs vblank before enable / disable.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Apr 03, 2019 at 11:47:00AM +0530, kiran.s.kumar@intel.com wrote:
> From: Kiran Kumar S <kiran.s.kumar@intel.corp-partner.google.com>
> 
> As per the display workaround #1200, FBC needs wait for vblank before
> enabling and before disabling FBC.
> 
> In some cases, depending on whether FBC was compressing in that frame,
> several control signals in the compression engine also will fail to
> properly recognize the final segment of the frame as a result of the
> missing last pixel indication. As a result of this, we're seeing
> corrupted cache line/compression indicators after FBC re-enables
> which causes underruns or corruption when they're used to decompress.
> 
> WA sequence as below:
> 1) Display enables plane 1A
> 2) Wait for 1 vblank
> 3) FBC gets enabled
> 4) Wait for 1 VBLANK
> 5) Turn off FBC
> 
> In GLK Chrome OS, if FBC is enabled by default, few top lines on the screen
> got corrupted. With the above WA, issue was resolved.
> 
> Change-Id: I8a3baeda363b2d2ec4e9e8673e89013a341c646a
> Signed-off-by: Kiran Kumar S <kiran.s.kumar@intel.corp-partner.google.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8576a7f799f2..90360dfc674b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13207,8 +13207,12 @@ static void intel_update_crtc(struct drm_crtc *crtc,
>  
>  	if (pipe_config->update_pipe && !pipe_config->enable_fbc)
>  		intel_fbc_disable(intel_crtc);
> -	else if (new_plane_state)
> +	else if (new_plane_state) {
> +		/* Display WA #1200: GLK */

The w/a doesn't seem specific to glk.

> +		if (IS_GEMINILAKE(dev_priv))
> +			intel_wait_for_vblank(dev_priv, intel_crtc->pipe);

Doing this uncodintionally for any plane update is quite rude. We need
to limit it to cases where the plane is being enabled or disabled. And
since most planes can't even do FBC we should not have to pay the cost
for all planes.

>  		intel_fbc_enable(intel_crtc, pipe_config, new_plane_state);
> +	}
>  
>  	intel_begin_crtc_commit(crtc, old_crtc_state);
>  
> @@ -13419,6 +13423,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  
>  			dev_priv->display.crtc_disable(old_intel_crtc_state, state);
>  			intel_crtc->active = false;
> +			/* Display WA #1200: GLK */
> +			intel_wait_for_vblank(dev_priv, intel_crtc->pipe);

The disable case seems to be w/a 1198. Also not glk specific. Also, this
code is only used for the case where the whole pipe is getting disabled,
so this wouldn't cover the cases where just the plane is being
disabled.

Do we have any igts that excecise plane enable/disable vs. FBC?

>  			intel_fbc_disable(intel_crtc);
>  			intel_disable_shared_dpll(old_intel_crtc_state);
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx