diff mbox

[1/8] drm/i915: don't increment the FBC threshold at fbc_enable

Message ID 1435672392-7329-2-git-send-email-przanoni@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paulo Zanoni June 30, 2015, 1:53 p.m. UTC 
From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We first set the threshold value when we're allocating the CFB, and
then later at {ilk,gen7}_fbc_enable() we increment it in case we're
using 16bpp. While that is correct, it is dangerous: if we rework the
code a little bit in a way that allows us to call intel_fbc_enable()
without necessarily calling i915_gem_stolen_setup_compression() first,
we might end up incrementing threshold more than once. To prevent
that, increment a temporary variable instead.

v2: Rebase.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Chris Wilson June 30, 2015, 2:22 p.m. UTC | #1 
On Tue, Jun 30, 2015 at 10:53:05AM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We first set the threshold value when we're allocating the CFB, and
> then later at {ilk,gen7}_fbc_enable() we increment it in case we're
> using 16bpp. While that is correct, it is dangerous: if we rework the
> code a little bit in a way that allows us to call intel_fbc_enable()
> without necessarily calling i915_gem_stolen_setup_compression() first,
> we might end up incrementing threshold more than once. To prevent
> that, increment a temporary variable instead.
> 
> v2: Rebase.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

You can move all the fbc out of i915_gem_stolen.c now that we have
intel_fbc.c. I don't even think we need to export a function to provide
drm_mm_insert_node(&dev_priv->mm.stolen), but we probably should for
ease of future maintenance.
-Chris
Daniel Vetter July 1, 2015, 1:52 p.m. UTC | #2 
On Tue, Jun 30, 2015 at 03:22:51PM +0100, Chris Wilson wrote:
> On Tue, Jun 30, 2015 at 10:53:05AM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > We first set the threshold value when we're allocating the CFB, and
> > then later at {ilk,gen7}_fbc_enable() we increment it in case we're
> > using 16bpp. While that is correct, it is dangerous: if we rework the
> > code a little bit in a way that allows us to call intel_fbc_enable()
> > without necessarily calling i915_gem_stolen_setup_compression() first,
> > we might end up incrementing threshold more than once. To prevent
> > that, increment a temporary variable instead.
> > 
> > v2: Rebase.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Queued for -next, thanks for the patch.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 50ed333..9e55b9b 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -188,14 +188,15 @@  static void ilk_fbc_enable(struct drm_crtc *crtc)
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	u32 dpfc_ctl;
+	int threshold = dev_priv->fbc.threshold;
 
 	dev_priv->fbc.enabled = true;
 
 	dpfc_ctl = DPFC_CTL_PLANE(intel_crtc->plane);
 	if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
-		dev_priv->fbc.threshold++;
+		threshold++;
 
-	switch (dev_priv->fbc.threshold) {
+	switch (threshold) {
 	case 4:
 	case 3:
 		dpfc_ctl |= DPFC_CTL_LIMIT_4X;
@@ -259,6 +260,7 @@  static void gen7_fbc_enable(struct drm_crtc *crtc)
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	u32 dpfc_ctl;
+	int threshold = dev_priv->fbc.threshold;
 
 	dev_priv->fbc.enabled = true;
 
@@ -267,9 +269,9 @@  static void gen7_fbc_enable(struct drm_crtc *crtc)
 		dpfc_ctl |= IVB_DPFC_CTL_PLANE(intel_crtc->plane);
 
 	if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
-		dev_priv->fbc.threshold++;
+		threshold++;
 
-	switch (dev_priv->fbc.threshold) {
+	switch (threshold) {
 	case 4:
 	case 3:
 		dpfc_ctl |= DPFC_CTL_LIMIT_4X;