| Message ID | 1419338145-1912-3-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 23, 2014 at 10:35:38AM -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > We have dev_priv->fbc.size which is supposed to contain the compressed > FB size, but it is not: at find_compression_threshold() we try to > overallocate the CFB, but we don't consider this when we assign a > value to dev_priv->fbc.size. Since the correct CFB size should already > be stored at dev_priv->fbc.compressed_fb.size, just kill > dev_priv->fbc.size and use the correct value isntead. They should not be equivalent though. We actually want fbc.size to compensate for the compression in the allocation so that the simple check for enough space succeeds even if we are compressing. -Chris
2014-12-25 8:16 GMT-02:00 Chris Wilson <chris@chris-wilson.co.uk>: > On Tue, Dec 23, 2014 at 10:35:38AM -0200, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> We have dev_priv->fbc.size which is supposed to contain the compressed >> FB size, but it is not: at find_compression_threshold() we try to >> overallocate the CFB, but we don't consider this when we assign a >> value to dev_priv->fbc.size. Since the correct CFB size should already >> be stored at dev_priv->fbc.compressed_fb.size, just kill >> dev_priv->fbc.size and use the correct value isntead. > > They should not be equivalent though. We actually want fbc.size to > compensate for the compression in the allocation so that the simple > check for enough space succeeds even if we are compressing. Can you please elaborate more on that? Are you talking about the threshold? As far as I can see, we're failing to properly take it into consideration both before and after this patch, so it wouldn't be a valid reason. I was planning to fix the "take threshold into consideration when checking the size" problem later: I wanted to think on a way that would guarantee that we'd always get the best possible threshold to prevent cases where we'd keep reusing 1:4 compression even while we could just free+realloc to have 1:1 compression back. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Fri, Dec 26, 2014 at 11:46:34AM -0200, Paulo Zanoni wrote: > 2014-12-25 8:16 GMT-02:00 Chris Wilson <chris@chris-wilson.co.uk>: > > On Tue, Dec 23, 2014 at 10:35:38AM -0200, Paulo Zanoni wrote: > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > >> We have dev_priv->fbc.size which is supposed to contain the compressed > >> FB size, but it is not: at find_compression_threshold() we try to > >> overallocate the CFB, but we don't consider this when we assign a > >> value to dev_priv->fbc.size. Since the correct CFB size should already > >> be stored at dev_priv->fbc.compressed_fb.size, just kill > >> dev_priv->fbc.size and use the correct value isntead. > > > > They should not be equivalent though. We actually want fbc.size to > > compensate for the compression in the allocation so that the simple > > check for enough space succeeds even if we are compressing. > > Can you please elaborate more on that? Are you talking about the > threshold? As far as I can see, we're failing to properly take it into > consideration both before and after this patch, so it wouldn't be a > valid reason. We have both identified the problem, and the fix to the current code looks rather easy as it seems a simple bug in commit 5e59f7175f96550ede91f58d267d2b551cb6fbba Author: Ben Widawsky <benjamin.widawsky@intel.com> Date: Mon Jun 30 10:41:24 2014 -0700 drm/i915: Try harder to get FBC that fluffs the compare of uncompressed request sizes. Your subject line claims to be a fix, so fix something. -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 3752040..f0419c8 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -734,7 +734,6 @@ enum fb_op_origin { }; struct i915_fbc { - unsigned long size; unsigned threshold; unsigned int fb_id; unsigned int possible_framebuffer_bits; diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index 4797138..d02c102 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -231,10 +231,8 @@ static int i915_setup_compression(struct drm_device *dev, int size, int fb_cpp) dev_priv->mm.stolen_base + compressed_llb->start); } - dev_priv->fbc.size = size / dev_priv->fbc.threshold; - - DRM_DEBUG_KMS("reserved %d bytes of contiguous stolen space for FBC\n", - size); + DRM_DEBUG_KMS("reserved %lu bytes of contiguous stolen space for FBC\n", + dev_priv->fbc.compressed_fb.size); return 0; @@ -253,7 +251,8 @@ int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_c if (!drm_mm_initialized(&dev_priv->mm.stolen)) return -ENODEV; - if (size <= dev_priv->fbc.size) + if (dev_priv->fbc.compressed_fb.allocated && + size <= dev_priv->fbc.compressed_fb.size) return 0; /* Release any current block */ @@ -266,7 +265,7 @@ void i915_gem_stolen_cleanup_compression(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; - if (dev_priv->fbc.size == 0) + if (dev_priv->fbc.compressed_fb.allocated == 0) return; drm_mm_remove_node(&dev_priv->fbc.compressed_fb); @@ -275,8 +274,6 @@ void i915_gem_stolen_cleanup_compression(struct drm_device *dev) drm_mm_remove_node(dev_priv->fbc.compressed_llb); kfree(dev_priv->fbc.compressed_llb); } - - dev_priv->fbc.size = 0; } void i915_gem_cleanup_stolen(struct drm_device *dev) diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index d3ff2c1..5270dc4 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -78,7 +78,7 @@ static void i8xx_fbc_enable(struct drm_crtc *crtc) dev_priv->fbc.enabled = true; - cfb_pitch = dev_priv->fbc.size / FBC_LL_SIZE; + cfb_pitch = dev_priv->fbc.compressed_fb.size / FBC_LL_SIZE; if (fb->pitches[0] < cfb_pitch) cfb_pitch = fb->pitches[0];