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

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

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.

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, and intel_fbc_calculate_cfb_size() sort of returns the
uncompressed fb size. 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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx