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

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

On Wed, Sep 30, 2015 at 05:05:45PM -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.
> 
> Testcase: igt/kms_frontbuffer_tracking/fbc-farfromfence (SKL)
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  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 d53f73f..313ef91 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -805,10 +805,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 size.

"plane size" or something

> + */
> +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 used_w, used_h, max_w, max_h;
>  
>  	if (INTEL_INFO(dev_priv)->gen >= 8 || IS_HASWELL(dev_priv)) {
>  		max_w = 4096;
> @@ -821,8 +827,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_sizes(crtc, &used_w, &used_h);
> +	used_w += crtc->adjusted_x;
> +	used_h += crtc->adjusted_y;
> +
> +	return used_w <= max_w && used_h <= max_h;

Makes sense. Not sure used_ is the best prefix. Maybe effective_ ? But I
don't mind if you think used_ is better.

So with the comment fixed a bit this is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

You know, we could avoid these issues if we stopped using hw gtt
tracking too. And then we could use FBC without a fence, and hence
wouldn't need X-tiling. IIRC that's now allowed on SKL+.

>  }
>  
>  /**
> @@ -899,7 +908,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

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