| Message ID | 1419338145-1912-5-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 23, 2014 at 10:35:40AM -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Because that is probably not very a good idea: if we used the stolen > memory for more things, there could be a risk that someone would > "allocate" the memory that the HW is still using as the CFB while FBC > was still enabled. Not in the code as it currently is due to the struct mutex lock. You need to explain how and why you intend to remove that invariant. -Chris
2014-12-25 8:20 GMT-02:00 Chris Wilson <chris@chris-wilson.co.uk>: > On Tue, Dec 23, 2014 at 10:35:40AM -0200, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> Because that is probably not very a good idea: if we used the stolen >> memory for more things, there could be a risk that someone would >> "allocate" the memory that the HW is still using as the CFB while FBC >> was still enabled. > > Not in the code as it currently is due to the struct mutex lock. You > need to explain how and why you intend to remove that invariant. Ok, maybe this point is wrong, but I still wee we freeing and then reallocating the CFB while FBC is active, which is still a problem, even if we end up just reallocating the same chunk of memory later. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Fri, Dec 26, 2014 at 11:46:49AM -0200, Paulo Zanoni wrote: > 2014-12-25 8:20 GMT-02:00 Chris Wilson <chris@chris-wilson.co.uk>: > > On Tue, Dec 23, 2014 at 10:35:40AM -0200, Paulo Zanoni wrote: > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > >> Because that is probably not very a good idea: if we used the stolen > >> memory for more things, there could be a risk that someone would > >> "allocate" the memory that the HW is still using as the CFB while FBC > >> was still enabled. > > > > Not in the code as it currently is due to the struct mutex lock. You > > need to explain how and why you intend to remove that invariant. > > Ok, maybe this point is wrong, but I still wee we freeing and then > reallocating the CFB while FBC is active, which is still a problem, > even if we end up just reallocating the same chunk of memory later. No. If you read the current code it will disable FBC using the old location before it can be reallocated. That it may continue to use the old address for a frame after we have marked it free isn't clean, but it is not the bug you describe. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index d02c102..f84c5f3 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -198,6 +198,8 @@ static int i915_setup_compression(struct drm_device *dev, int size, int fb_cpp) struct drm_mm_node *uninitialized_var(compressed_llb); int ret; + WARN_ON(dev_priv->fbc.enabled); + ret = find_compression_threshold(dev, &dev_priv->fbc.compressed_fb, size, fb_cpp); if (!ret) @@ -268,6 +270,7 @@ void i915_gem_stolen_cleanup_compression(struct drm_device *dev) if (dev_priv->fbc.compressed_fb.allocated == 0) return; + WARN_ON(dev_priv->fbc.enabled); drm_mm_remove_node(&dev_priv->fbc.compressed_fb); if (dev_priv->fbc.compressed_llb) { diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index 1b10b06..83d3c8a 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -593,13 +593,6 @@ void intel_fbc_update(struct drm_device *dev) if (in_dbg_master()) goto out_disable; - if (i915_gem_stolen_setup_compression(dev, obj->base.size, - drm_format_plane_cpp(fb->pixel_format, 0))) { - if (set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL)) - DRM_DEBUG_KMS("framebuffer too large, disabling compression\n"); - goto out_disable; - } - /* If the scanout has not changed, don't modify the FBC settings. * Note that we make the fundamental assumption that the fb->obj * cannot be unpinned (and have its GTT offset and fence revoked) @@ -638,6 +631,13 @@ void intel_fbc_update(struct drm_device *dev) intel_fbc_disable(dev); } + if (i915_gem_stolen_setup_compression(dev, obj->base.size, + drm_format_plane_cpp(fb->pixel_format, 0))) { + if (set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL)) + DRM_DEBUG_KMS("framebuffer too large, disabling compression\n"); + return; + } + intel_fbc_enable(crtc); dev_priv->fbc.no_fbc_reason = FBC_OK; return;