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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
