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

["Zanoni, Paulo R" <paulo.r.zanoni@intel.com>]

Em Qui, 2015-10-01 às 21:11 +0300, Ville Syrjälä escreveu:
> 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 still think the fact that we we use pipe_src_x to set PIPESRC is the
most relevant argument. Anyway, since you're sure changing the value
won't cause problems, I'll do it: we have way too many plane size
fields in our structs and I'm always confused by their undocumented
differences.

> 
> 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
> > > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx