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

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

On Thu, Oct 01, 2015 at 05:47:11PM +0000, Zanoni, Paulo R wrote:
> Em Qui, 2015-10-01 às 15:23 +0300, Ville Syrjälä escreveu:
> > 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.
> 
> This code is trying to recalculate the values we use to write to
> PIPESRC and PIPE_SIZE because that's what the HW is using. Since on the
> older platforms we use crtc->pipe_src_x for PIPESRC, I think the
> correct thing to do is to check those values.

The plane src size is still what matters. Older platforms just didn't
support windowing or scaling on the primary plane. And so there was no
need to have a dedicated register for plane size since PIPESRC would
anyway contain the same numbers.

I can't recall off the top of my head whether gen2/3 supported FBC on
the planes that had windowing (plane != A). Perhaps not.

> > > 
> > > 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.
> 
> But SKL FBC does support Y tiling (even though our code doesn't), so
> keeping the checks as they are would help preventing future problems.

I suppose. Although removing the dependency on hw tracking would be
required in any case for Y tiling.

> 
> > 
> > > 
> > > > +	} 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
> 
> I was just moving things around... 
> 
> > > same thing. Someone could also do a small search&replace to unify
> > > the
> > > whatever we have currently.
> 
> 
> Someone could patch the drm_format_plane_cpp() function so that it at
> least explains what CPP is supposed to mean in this context (Colors Per
> Pixel? Countofbytes Per Pixel?). Or rename the function to
> drm_format_pane_bpp() to make the name match the description.

"chars per pixel" I suppose. Or maybe "bpp" was already taken
and 'c' was just the next letter from 'b'. No one knows for sure
perhaps ;)

But in any case, still a fairly standard way to call it.

> 
> > > 
> > > > +
> > > >  	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