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@xxxxxxxxx> > >>> --- > >>> 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@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
