[Intel-gfx] [PATCH 3/3] drm/i915: fix FBC buffer size checks

[Daniel Vetter]

On Thu, Oct 01, 2015 at 07:57:12PM -0300, Paulo Zanoni wrote:
> According to my experiments (and later confirmation from the hardware
> developers), the maximum sizes mentioned in the specification delimit
> how far in the buffer the hardware tracking can go. And the hardware
> calculates the size based on the plane address we provide - and the
> provided plane address might not be the real x:0,y:0 point due to the
> compute_page_offset() function.
> 
> On platforms that do the x/y offset adjustment trick it will be really
> hard to reproduce a bug, but on the current SKL we can reproduce the
> bug with igt/kms_frontbuffer_tracking/fbc-farfromfence. With this
> patch, we'll go from "CRC assertion failure" to "FBC unexpectedly
> disabled", which is still a failure on the test suite but is not a
> perceived user bug - you will just not save as much power as you could
> if FBC is disabled.
> 
> v2, rewrite patch after clarification from the Hadware guys:
>   - Rename function so it's clear what the check is for.
>   - Use the new intel_fbc_get_plane_source_sizes() function in order
>     to get the proper sizes as seen by FBC.
> v3:
>   - Rebase after the s/sizes/size/ on the previous patch.
>   - Adjust comment wording (Ville).
>   - s/used_/effective_/ (Ville).
> 
> Testcase: igt/kms_frontbuffer_tracking/fbc-farfromfence (SKL)
> Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>

Merged these three patches to dinq, thanks.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 18e228b..cf47352 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -799,10 +799,16 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb)
>  	}
>  }
>  
> -static bool pipe_size_is_valid(struct intel_crtc *crtc)
> +/*
> + * For some reason, the hardware tracking starts looking at whatever we
> + * programmed as the display plane base address register. It does not look at
> + * the X and Y offset registers. That's why we look at the crtc->adjusted{x,y}
> + * variables instead of just looking at the pipe/plane size.
> + */
> +static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> -	unsigned int max_w, max_h;
> +	unsigned int effective_w, effective_h, max_w, max_h;
>  
>  	if (INTEL_INFO(dev_priv)->gen >= 8 || IS_HASWELL(dev_priv)) {
>  		max_w = 4096;
> @@ -815,8 +821,11 @@ static bool pipe_size_is_valid(struct intel_crtc *crtc)
>  		max_h = 1536;
>  	}
>  
> -	return crtc->config->pipe_src_w <= max_w &&
> -	       crtc->config->pipe_src_h <= max_h;
> +	intel_fbc_get_plane_source_size(crtc, &effective_w, &effective_h);
> +	effective_w += crtc->adjusted_x;
> +	effective_h += crtc->adjusted_y;
> +
> +	return effective_w <= max_w && effective_h <= max_h;
>  }
>  
>  /**
> @@ -893,7 +902,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>  		goto out_disable;
>  	}
>  
> -	if (!pipe_size_is_valid(intel_crtc)) {
> +	if (!intel_fbc_hw_tracking_covers_screen(intel_crtc)) {
>  		set_no_fbc_reason(dev_priv, FBC_MODE_TOO_LARGE);
>  		goto out_disable;
>  	}
> -- 
> 2.5.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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