Re: [PATCH 10/13] drm/i915: fix the CFB size check

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

Em Qua, 2015-11-11 às 16:27 +0200, Ville Syrjälä escreveu:
> On Tue, Nov 10, 2015 at 02:04:48PM +0100, Maarten Lankhorst wrote:
> > Op 10-11-15 om 13:20 schreef Zanoni, Paulo R:
> > > Em Ter, 2015-11-10 às 11:22 +0100, Maarten Lankhorst escreveu:
> > > > Op 04-11-15 om 20:10 schreef Paulo Zanoni:
> > > > > In function find_compression_threshold() we try to over-
> > > > > allocate
> > > > > CFB
> > > > > space in order to reudce reallocations and fragmentation, and
> > > > > we're
> > > > > not considering that at the CFB size check. Consider it.
> > > > > 
> > > > > There is also a longer-term plan to kill
> > > > > dev_priv->fbc.uncompressed_size, but this will come later.
> > > > > 
> > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_fbc.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > index dee99c9..e99aacc 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > @@ -719,7 +719,8 @@ static int intel_fbc_setup_cfb(struct
> > > > > intel_crtc *crtc)
> > > > >  	size = intel_fbc_calculate_cfb_size(crtc);
> > > > >  	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> > > > >  
> > > > > -	if (size <= dev_priv->fbc.uncompressed_size)
> > > > > +	if (dev_priv->fbc.compressed_fb.allocated &&
> > > > > +	    size <= dev_priv->fbc.compressed_fb.size *
> > > > > dev_priv-
> > > > > > fbc.threshold)
> > > > >  		return 0;
> > > > >  
> > > > >  	/* Release any current block */
> > > > Should i8xx_fbc_enable be changed too then?
> > > As far as I understand, no. We're just reserving a bigger buffer
> > > in
> > > case we need it later, but the size used by the hardware is still
> > > the
> > > same. But I'm not 100% sure the i8xx code is actually correct
> > > since I
> > > didn't dig deep into the ancient scrolls. By not touching i8xx
> > > we're
> > > also avoiding a possible new regression.
> > > 
> > The original check was for size <= uncompressed_size,
> > new is threshold * compressed.
> > 
> > I think i8xx_fbc_enable might have to do the same when calculating
> > cfb_pitch
> > for this patch to work as intended.
> 
> I think the FBC1 code is fairly confused already.
> 
> It does the right thing by capping CFB_PITCH to the fb pitch. The way
> the hardware works is that any line that compresses to >CFB_PITCH is
> discarded and marked as uncompressed. So we definitely don't want to 
> allow CFB_PITCH to exceed the original pitch as that would just
> increase
> the amount of data getting scanned out. The nasty thing is that it'll
> try to recompress any line tagged as uncompressed every time it tries
> to recompress anything, so we should be rather careful not to set
> CFB_PITCH too low for FBC1, and if we have poorly compressing data we
> might just want to disable FBC entirely to avoid trying to recompress
> everything all the time.
> 
> What doesn't really make sense to me is the
> 'cfb_pitch = dev_priv->fbc.uncompressed_size / FBC_LL_SIZE' part. 
> That basically assumes we always have a maximum height plane (1536
> lines) in use. I'm not entirely sure if the hardware fully skips the
> unused lines, or if we would have to allocate some pixel run sets
> (swords) even for the unused lines (maybe 1 per line). But I wouldn't
> think that we would need to allocate them to cover the entire worst
> case length.

I really didn't stop to audit the i8xx code, and I was not planning to.
I'm just trying to not add any new regressions. Maybe we should open a
bug report or Jira task somewhere for your observation.



> What makes things more confusing I think is the naming of the
> variables
> and functions. uncompressed_size is actually the size we've allocated
> for the cfb

Except for the cases where threshold != 1, and for the cases where we
over-allocate the CFB.


> , and intel_fbc_calculate_cfb_size() sort of returns the
> uncompressed fb size

Yes, it's redundant. See below.


> . Well since it has the 2k line limit applied, I
> suppose you can actually think of it as the max cfb size we would
> want
> to have for the particular plane configuration.

Depends on how you see it. It's the maximum CFB size since we can
increase the threshold. It's also the minimum viable CFB size for
threshold=1. Since it's the base number used for the hardware
calculations, I decided to call it just "the cfb size". But I'm always
open to suggestions for better names.

By the way, I have a patch that completely kills uncompressed_size and
makes the i8xx code just call intel_fbc_calculate_cfb_size() directly.
IMHO, having less variables to track makes things simpler. The patch
will be on the list soon.

> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx