Re: [PATCH 3/3] drm/i915: fix FBC buffer size checks

Daniel Vetter
2015-10-09T07:33:15Z

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@linux.intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation