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

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

On Thu, Oct 01, 2015 at 03:14:13PM +0300, Ville Syrjälä wrote:
> On Wed, Sep 30, 2015 at 05:05:44PM -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.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_fbc.c | 58 +++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 54 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> > index 1b2ebb2..d53f73f 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -698,9 +698,60 @@ 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 PIPE_SIZE register. For BDW-, the hardware looks at the value we
> > + * wrote to PIPESRC.
> > + */
> > +static void intel_fbc_get_plane_source_sizes(struct intel_crtc *crtc,
> > +					     int *width, int *height)
> 
> size in my mind already includes width and height, so plural _sizes
> doesn't make much sense to me.
> 
> >  {
> > +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> > +	struct intel_plane_state *plane_state =
> > +			to_intel_plane_state(crtc->base.primary->state);
> > +	int w, h;
> > +
> > +	if (INTEL_INFO(dev_priv)->gen >= 9) {
> > +		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;
> > +		}
> 
> You can just use this same code for all platforms.
> 
> Actually I'm not sure what we should do wrt. rotation. Do we support
> FBC with 90/270 degree rotation? The scanout happens in a rotated
> fashion, so swapping the dimensions like you do would seem like the
> right thing. But not sure.

While writing my reply tothe GTT tracking offset patch, I realized that
90/270 degree rotation requires Y-tiling, so since we're currently
limiting FBC to X-tiling we can never get here with 90/270 degree
rotation.

> 
> > +	} else {
> > +		w = crtc->config->pipe_src_w;
> > +		h = crtc->config->pipe_src_h;
> > +	}
> > +
> > +	if (width)
> > +		*width = w;
> > +	if (height)
> > +		*height = h;
> > +}
> > +
> > +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_sizes(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, fb_cpp;
> > +
> > +	size = intel_fbc_calculate_cfb_size(crtc);
> > +	fb_cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> 
> Can we just all it 'cpp' please. We already have too many names for the
> same thing. Someone could also do a small search&replace to unify the
> whatever we have currently.
> 
> > +
> >  	if (size <= dev_priv->fbc.uncompressed_size)
> >  		return 0;
> >  
> > @@ -897,8 +948,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
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> 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