Re: [PATCH 2/3] drm/i915: fix CFB size calculation

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

On Thu, Oct 01, 2015 at 07:55:57PM -0300, Paulo Zanoni wrote:
> We were considering the whole framebuffer height, but the spec says we
> should only consider the active display height size. There were still
> some unclear questions based on the spec, but the hardware guys
> clarified them for us. According to them:
> 
> - CFB size = CFB stride * Number of lines FBC writes to CFB
> - CFB stride = plane stride / compression limit
> - Number of lines FBC writes to CFB = MIN(plane source height, maximum
>   number of lines FBC writes to CFB)
> - Plane source height =
>   - pipe source height (PIPE_SRCSZ register) (before SKL)
>   - plane size register height (PLANE_SIZE register) (SKL+)
> - Maximum number of lines FBC writes to CFB =
>   - plane source height (before HSW)
>   - 2048 (HSW+)
> 
> For the plane source height, I could just have made our code do
> I915_READ() in order to be more future proof, but since it's not cool
> to do register reads I decided to just recalculate the values we use
> when we actually write to those registers.
> 
> With this patch, depending on your machine configuration, a lot of the
> kms_frontbuffer_tracking subtests that used to result in a SKIP due to
> not enough stolen memory still start resulting in a PASS.
> 
> v2: Use the clipped src size instead of pipe_src_h (Ville).
> v3: Use the appropriate information provided by the hardware guys.
> v4: Bikesheds: s/sizes/size/, s/fb_cpp/cpp/ (Ville).
> v5: - Don't use crtc->config->pipe_src_x for BDW- (Ville).
>     - Fix the register name written in the comment.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 54 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 49 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 1b2ebb2..18e228b 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -698,16 +698,61 @@ void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv)
>  	mutex_unlock(&dev_priv->fbc.lock);
>  }
>  
> -static int intel_fbc_setup_cfb(struct drm_i915_private *dev_priv, int size,
> -			       int fb_cpp)
> +/*
> + * For SKL+, the plane source size used by the hardware is based on the value we
> + * write to the PLANE_SIZE register. For BDW-, the hardware looks at the value
> + * we wrote to PIPESRC.
> + */
> +static void intel_fbc_get_plane_source_size(struct intel_crtc *crtc,
> +					    int *width, int *height)
>  {
> +	struct intel_plane_state *plane_state =
> +			to_intel_plane_state(crtc->base.primary->state);
> +	int w, h;
> +
> +	if (intel_rotation_90_or_270(plane_state->base.rotation)) {
> +		w = drm_rect_height(&plane_state->src) >> 16;
> +		h = drm_rect_width(&plane_state->src) >> 16;
> +	} else {
> +		w = drm_rect_width(&plane_state->src) >> 16;
> +		h = drm_rect_height(&plane_state->src) >> 16;
> +	}
> +
> +	if (width)
> +		*width = w;
> +	if (height)
> +		*height = h;
> +}

Yep, I like this much better. 

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
> +static int intel_fbc_calculate_cfb_size(struct intel_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> +	int lines;
> +
> +	intel_fbc_get_plane_source_size(crtc, NULL, &lines);
> +	if (INTEL_INFO(dev_priv)->gen >= 7)
> +		lines = min(lines, 2048);
> +
> +	return lines * fb->pitches[0];
> +}
> +
> +static int intel_fbc_setup_cfb(struct intel_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> +	int size, cpp;
> +
> +	size = intel_fbc_calculate_cfb_size(crtc);
> +	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> +
>  	if (size <= dev_priv->fbc.uncompressed_size)
>  		return 0;
>  
>  	/* Release any current block */
>  	__intel_fbc_cleanup_cfb(dev_priv);
>  
> -	return intel_fbc_alloc_cfb(dev_priv, size, fb_cpp);
> +	return intel_fbc_alloc_cfb(dev_priv, size, cpp);
>  }
>  
>  static bool stride_is_valid(struct drm_i915_private *dev_priv,
> @@ -897,8 +942,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>  		goto out_disable;
>  	}
>  
> -	if (intel_fbc_setup_cfb(dev_priv, obj->base.size,
> -				drm_format_plane_cpp(fb->pixel_format, 0))) {
> +	if (intel_fbc_setup_cfb(intel_crtc)) {
>  		set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL);
>  		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