[PATCH 0/9] More simple FBC fixes

[PATCH 0/9] More simple FBC fixes

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Hi

Since the previous FBC series is already entirely reviewed, it's time to submit
a few more FBC patches to the mailing list. These patches just fix some simple
bugs and annoyances: nothing really major. I tested them on my BDW and they pass
the tests I have.

In theory we could even think about maybe enabling FBC on HSW+ since I can't
think of any bugs remaining on these platforms. The biggest reason to prevent
that is that I still didn't upstream the FBC tests I wrote since I still didn't
integrate them to kms_fbc_crc - which I'm also running.

This doesn't mean FBC's TODO list is empty: there are still a lot of small
improvements to do, and one of them is to _not_ disable FBC during page flips.
I also didn't really do any power or performance tests yet: the focus is
completely on bugs.

Regarding the previous platforms, I think there could be some hope to support
FBC on ILK+, but I'm really only focused on HSW+, so I don't know if we're
missing some checks and restrictions. For the older platforms, I think that,
given all the conditions, the risk of enabling FBC and breaking these platforms
even more is probably not worth it.

Thanks,
Paulo

Paulo Zanoni (9):
  drm/i915: don't reallocate the compressed FB at every frame
  drm/i915: fix the FBC CFB size tracking
  drm/i915: don't increment the FBC threshold at fbc_enable
  drm/i915: don't free the CFB while FBC is enabled
  drm/i915: don't set the FBC plane select bits on HSW+
  drm/i915: add the FBC mutex
  drm/i915: don't alloc/free fbc_work at every update
  drm/i915: print FBC compression status on debugfs
  drm/i915: FBC only supports 16bpp and 32bpp

 drivers/gpu/drm/i915/i915_debugfs.c    |   5 +
 drivers/gpu/drm/i915/i915_drv.h        |   5 +-
 drivers/gpu/drm/i915/i915_gem_stolen.c |  16 ++--
 drivers/gpu/drm/i915/i915_reg.h        |   3 +
 drivers/gpu/drm/i915/intel_fbc.c       | 161 +++++++++++++++++++++------------
 5 files changed, 123 insertions(+), 67 deletions(-)

-- 
2.1.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/9] drm/i915: don't reallocate the compressed FB at every frame

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

With the current code we just reallocate the compressed FB at every
FBC update: we have X in one frame, then in the other frame we need X
again, but we check "needed < have" instead of "needed <= have".

There are still other problems with this code: we don't take the
threshold into consideration and we also have cases where we
reallocate the CFB while FBC is enabled, but let's leave these for
later patches.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index a204584..4797138 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -253,7 +253,7 @@ 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 (size <= dev_priv->fbc.size)
 		return 0;
 
 	/* Release any current block */
-- 
2.1.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/9] drm/i915: fix the FBC CFB size tracking

[Paulo Zanoni]

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.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        |  1 -
 drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++--------
 drivers/gpu/drm/i915/intel_fbc.c       |  2 +-
 3 files changed, 6 insertions(+), 10 deletions(-)

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];
 
-- 
2.1.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/9] drm/i915: don't increment the FBC threshold at fbc_enable

[Paulo Zanoni]

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.

We're going to need this patch for a later rework.

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(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 5270dc4..1b10b06 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -187,14 +187,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;
@@ -258,14 +259,15 @@ 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;
 
 	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;
-- 
2.1.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/9] drm/i915: don't free the CFB while FBC is enabled

[Paulo Zanoni]

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.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c |  3 +++
 drivers/gpu/drm/i915/intel_fbc.c       | 14 +++++++-------
 2 files changed, 10 insertions(+), 7 deletions(-)

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;
-- 
2.1.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/9] drm/i915: don't set the FBC plane select bits on HSW+

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

This commit is just to make the intentions explicit: on HSW+ these
bits are MBZ, but since we only support plane A and the macro
evaluates to zero when plane A is the parameter, we're not fixing any
bug.

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 83d3c8a..c6e688c 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -258,12 +258,15 @@ static void gen7_fbc_enable(struct drm_crtc *crtc)
 	struct drm_framebuffer *fb = crtc->primary->fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	u32 dpfc_ctl;
+	u32 dpfc_ctl = 0;
 	int threshold = dev_priv->fbc.threshold;
 
 	dev_priv->fbc.enabled = true;
 
-	dpfc_ctl = IVB_DPFC_CTL_PLANE(intel_crtc->plane);
+
+	if (IS_IVYBRIDGE(dev))
+		dpfc_ctl |= IVB_DPFC_CTL_PLANE(intel_crtc->plane);
+
 	if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
 		threshold++;
 
-- 
2.1.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/9] drm/i915: add the FBC mutex

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Make sure we're not gonna have weird races in really weird cases where
a lot of different CRTCs are doing rendering and modesets at the same
time.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_fbc.c | 80 +++++++++++++++++++++++++++++++---------
 2 files changed, 64 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f0419c8..18fcce4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -734,6 +734,7 @@ enum fb_op_origin {
 };
 
 struct i915_fbc {
+	struct mutex lock;
 	unsigned threshold;
 	unsigned int fb_id;
 	unsigned int possible_framebuffer_bits;
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index c6e688c..6611266 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -335,6 +335,7 @@ static void intel_fbc_work_fn(struct work_struct *__work)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	mutex_lock(&dev->struct_mutex);
+	mutex_lock(&dev_priv->fbc.lock);
 	if (work == dev_priv->fbc.fbc_work) {
 		/* Double check that we haven't switched fb without cancelling
 		 * the prior work.
@@ -349,6 +350,7 @@ static void intel_fbc_work_fn(struct work_struct *__work)
 
 		dev_priv->fbc.fbc_work = NULL;
 	}
+	mutex_unlock(&dev_priv->fbc.lock);
 	mutex_unlock(&dev->struct_mutex);
 
 	kfree(work);
@@ -356,6 +358,8 @@ static void intel_fbc_work_fn(struct work_struct *__work)
 
 static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv)
 {
+	lockdep_assert_held(&dev_priv->fbc.lock);
+
 	if (dev_priv->fbc.fbc_work == NULL)
 		return;
 
@@ -383,6 +387,8 @@ static void intel_fbc_enable(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	lockdep_assert_held(&dev_priv->fbc.lock);
+
 	if (!dev_priv->display.enable_fbc)
 		return;
 
@@ -417,16 +423,12 @@ static void intel_fbc_enable(struct drm_crtc *crtc)
 	schedule_delayed_work(&work->work, msecs_to_jiffies(50));
 }
 
-/**
- * intel_fbc_disable - disable FBC
- * @dev: the drm_device
- *
- * This function disables FBC.
- */
-void intel_fbc_disable(struct drm_device *dev)
+static void __intel_fbc_disable(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	lockdep_assert_held(&dev_priv->fbc.lock);
+
 	intel_fbc_cancel_work(dev_priv);
 
 	if (!dev_priv->display.disable_fbc)
@@ -436,6 +438,21 @@ void intel_fbc_disable(struct drm_device *dev)
 	dev_priv->fbc.crtc = NULL;
 }
 
+/**
+ * intel_fbc_disable - disable FBC
+ * @dev: the drm_device
+ *
+ * This function disables FBC.
+ */
+void intel_fbc_disable(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev_priv->fbc.lock);
+	__intel_fbc_disable(dev);
+	mutex_unlock(&dev_priv->fbc.lock);
+}
+
 static bool set_no_fbc_reason(struct drm_i915_private *dev_priv,
 			      enum no_fbc_reason reason)
 {
@@ -484,7 +501,7 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
 }
 
 /**
- * intel_fbc_update - enable/disable FBC as needed
+ * __intel_fbc_update - enable/disable FBC as needed, unlocked
  * @dev: the drm_device
  *
  * Set up the framebuffer compression hardware at mode set time.  We
@@ -502,7 +519,7 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
  *
  * We need to enable/disable FBC on a global basis.
  */
-void intel_fbc_update(struct drm_device *dev)
+static void __intel_fbc_update(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = NULL;
@@ -512,6 +529,8 @@ void intel_fbc_update(struct drm_device *dev)
 	const struct drm_display_mode *adjusted_mode;
 	unsigned int max_width, max_height;
 
+	lockdep_assert_held(&dev_priv->fbc.lock);
+
 	if (!HAS_FBC(dev))
 		return;
 
@@ -631,7 +650,7 @@ void intel_fbc_update(struct drm_device *dev)
 		 * some point. And we wait before enabling FBC anyway.
 		 */
 		DRM_DEBUG_KMS("disabling active FBC for update\n");
-		intel_fbc_disable(dev);
+		__intel_fbc_disable(dev);
 	}
 
 	if (i915_gem_stolen_setup_compression(dev, obj->base.size,
@@ -649,11 +668,26 @@ out_disable:
 	/* Multiple disables should be harmless */
 	if (intel_fbc_enabled(dev)) {
 		DRM_DEBUG_KMS("unsupported config, disabling FBC\n");
-		intel_fbc_disable(dev);
+		__intel_fbc_disable(dev);
 	}
 	i915_gem_stolen_cleanup_compression(dev);
 }
 
+/*
+ * intel_fbc_update - enable/disable FBC as needed
+ * @dev: the drm_device
+ *
+ * This function reevaluates the overall state and enables or disables FBC.
+ */
+void intel_fbc_update(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev_priv->fbc.lock);
+	__intel_fbc_update(dev);
+	mutex_unlock(&dev_priv->fbc.lock);
+}
+
 void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 			  unsigned int frontbuffer_bits,
 			  enum fb_op_origin origin)
@@ -661,8 +695,10 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 	struct drm_device *dev = dev_priv->dev;
 	unsigned int fbc_bits;
 
+	mutex_lock(&dev_priv->fbc.lock);
+
 	if (origin == ORIGIN_GTT)
-		return;
+		goto out;
 
 	if (dev_priv->fbc.enabled)
 		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
@@ -673,9 +709,12 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 		fbc_bits = dev_priv->fbc.possible_framebuffer_bits;
 
 	if ((fbc_bits & frontbuffer_bits) == 0)
-		return;
+		goto out;
 
-	intel_fbc_disable(dev);
+	__intel_fbc_disable(dev);
+
+out:
+	mutex_unlock(&dev_priv->fbc.lock);
 }
 
 void intel_fbc_flush(struct drm_i915_private *dev_priv,
@@ -686,6 +725,8 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 	unsigned int fbc_bits = 0;
 	bool fbc_enabled;
 
+	mutex_lock(&dev_priv->fbc.lock);
+
 	if (dev_priv->fbc.enabled)
 		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
 	else
@@ -693,7 +734,7 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 
 	/* These are not the planes you are looking for. */
 	if ((frontbuffer_bits & fbc_bits) == 0)
-		return;
+		goto out;
 
 	fbc_enabled = intel_fbc_enabled(dev);
 
@@ -703,12 +744,15 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 			if (origin == ORIGIN_CS || origin == ORIGIN_CPU)
 				intel_fbc_nuke(dev_priv);
 		} else {
-			intel_fbc_update(dev);
+			__intel_fbc_update(dev);
 		}
 	} else {
 		if (fbc_enabled)
-			intel_fbc_disable(dev);
+			__intel_fbc_disable(dev);
 	}
+
+out:
+	mutex_unlock(&dev_priv->fbc.lock);
 }
 
 /**
@@ -721,6 +765,8 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 {
 	enum pipe pipe;
 
+	mutex_init(&dev_priv->fbc.lock);
+
 	if (!HAS_FBC(dev_priv)) {
 		dev_priv->fbc.enabled = false;
 		dev_priv->fbc.no_fbc_reason = FBC_UNSUPPORTED;
-- 
2.1.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 7/9] drm/i915: don't alloc/free fbc_work at every update

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Because there's no need for it. Just use a static structure with a
bool field to tell if it's in use or not. The big advantage here is
not saving kzalloc/kfree calls, it's cutting the ugly "failed to
allocate FBC work structure" code path: in this path we call
enable_fbc() directly but we don't update fbc.crtc, fbc.fb_id and
fbc.y - they are updated in intel_fbc_work_fn(), which we're not
calling. And since testing out-of-memory cases like this is really
hard, getting rid of the code path is a major relief.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  3 ++-
 drivers/gpu/drm/i915/intel_fbc.c | 41 +++++++++++++++-------------------------
 2 files changed, 17 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 18fcce4..40bc864 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -751,10 +751,11 @@ struct i915_fbc {
 	bool enabled;
 
 	struct intel_fbc_work {
+		bool scheduled;
 		struct delayed_work work;
 		struct drm_crtc *crtc;
 		struct drm_framebuffer *fb;
-	} *fbc_work;
+	} work;
 
 	enum no_fbc_reason {
 		FBC_OK, /* FBC is enabled */
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 6611266..80bdbde 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -336,7 +336,7 @@ static void intel_fbc_work_fn(struct work_struct *__work)
 
 	mutex_lock(&dev->struct_mutex);
 	mutex_lock(&dev_priv->fbc.lock);
-	if (work == dev_priv->fbc.fbc_work) {
+	if (dev_priv->fbc.work.scheduled) {
 		/* Double check that we haven't switched fb without cancelling
 		 * the prior work.
 		 */
@@ -348,42 +348,37 @@ static void intel_fbc_work_fn(struct work_struct *__work)
 			dev_priv->fbc.y = work->crtc->y;
 		}
 
-		dev_priv->fbc.fbc_work = NULL;
+		dev_priv->fbc.work.scheduled = false;
 	}
 	mutex_unlock(&dev_priv->fbc.lock);
 	mutex_unlock(&dev->struct_mutex);
-
-	kfree(work);
 }
 
 static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv)
 {
 	lockdep_assert_held(&dev_priv->fbc.lock);
 
-	if (dev_priv->fbc.fbc_work == NULL)
+	if (!dev_priv->fbc.work.scheduled)
 		return;
 
 	DRM_DEBUG_KMS("cancelling pending FBC enable\n");
 
-	/* Synchronisation is provided by struct_mutex and checking of
-	 * dev_priv->fbc.fbc_work, so we can perform the cancellation
+	/* Synchronisation is provided by fbc.lock and checking of
+	 * dev_priv->fbc.work.scheduled, so we can perform the cancellation
 	 * entirely asynchronously.
 	 */
-	if (cancel_delayed_work(&dev_priv->fbc.fbc_work->work))
-		/* tasklet was killed before being run, clean up */
-		kfree(dev_priv->fbc.fbc_work);
+	cancel_delayed_work(&dev_priv->fbc.work.work);
 
 	/* Mark the work as no longer wanted so that if it does
 	 * wake-up (because the work was already running and waiting
 	 * for our mutex), it will discover that is no longer
 	 * necessary to run.
 	 */
-	dev_priv->fbc.fbc_work = NULL;
+	dev_priv->fbc.work.scheduled = false;
 }
 
 static void intel_fbc_enable(struct drm_crtc *crtc)
 {
-	struct intel_fbc_work *work;
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -394,18 +389,10 @@ static void intel_fbc_enable(struct drm_crtc *crtc)
 
 	intel_fbc_cancel_work(dev_priv);
 
-	work = kzalloc(sizeof(*work), GFP_KERNEL);
-	if (work == NULL) {
-		DRM_ERROR("Failed to allocate FBC work structure\n");
-		dev_priv->display.enable_fbc(crtc);
-		return;
-	}
+	dev_priv->fbc.work.crtc = crtc;
+	dev_priv->fbc.work.fb = crtc->primary->fb;
 
-	work->crtc = crtc;
-	work->fb = crtc->primary->fb;
-	INIT_DELAYED_WORK(&work->work, intel_fbc_work_fn);
-
-	dev_priv->fbc.fbc_work = work;
+	dev_priv->fbc.work.scheduled = true;
 
 	/* Delay the actual enabling to let pageflipping cease and the
 	 * display to settle before starting the compression. Note that
@@ -420,7 +407,7 @@ static void intel_fbc_enable(struct drm_crtc *crtc)
 	 *
 	 * WaFbcWaitForVBlankBeforeEnable:ilk,snb
 	 */
-	schedule_delayed_work(&work->work, msecs_to_jiffies(50));
+	schedule_delayed_work(&dev_priv->fbc.work.work, msecs_to_jiffies(50));
 }
 
 static void __intel_fbc_disable(struct drm_device *dev)
@@ -702,9 +689,9 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 
 	if (dev_priv->fbc.enabled)
 		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
-	else if (dev_priv->fbc.fbc_work)
+	else if (dev_priv->fbc.work.scheduled)
 		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(
-			to_intel_crtc(dev_priv->fbc.fbc_work->crtc)->pipe);
+			to_intel_crtc(dev_priv->fbc.work.crtc)->pipe);
 	else
 		fbc_bits = dev_priv->fbc.possible_framebuffer_bits;
 
@@ -773,6 +760,8 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 		return;
 	}
 
+	INIT_DELAYED_WORK(&dev_priv->fbc.work.work, intel_fbc_work_fn);
+
 	for_each_pipe(dev_priv, pipe) {
 		dev_priv->fbc.possible_framebuffer_bits |=
 				INTEL_FRONTBUFFER_PRIMARY(pipe);
-- 
2.1.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 8/9] drm/i915: print FBC compression status on debugfs

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We already had a few bugs in the past where FBC was compressing
nothing when it was enabled, which makes the feature quite useless.
Add this information on debugfs so the test suites can check for
regressions in this piece of the code.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 5 +++++
 drivers/gpu/drm/i915/i915_reg.h     | 3 +++
 2 files changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index e515aad..e34e021 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1510,6 +1510,11 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
 		seq_putc(m, '\n');
 	}
 
+	if (INTEL_INFO(dev_priv)->gen >= 7 && !IS_VALLEYVIEW(dev_priv))
+		seq_printf(m, "Compressing: %s\n",
+			   yesno(I915_READ(FBC_STATUS2) &
+				 FBC_COMPRESSION_MASK));
+
 	intel_runtime_pm_put(dev_priv);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 40ca873..b6db966 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1600,6 +1600,9 @@ enum punit_power_well {
 #define FBC_FENCE_OFF		0x03218 /* BSpec typo has 321Bh */
 #define FBC_TAG			0x03300
 
+#define FBC_STATUS2		0x43214
+#define  FBC_COMPRESSION_MASK	0x7ff
+
 #define FBC_LL_SIZE		(1536)
 
 /* Framebuffer compression for GM45+ */
-- 
2.1.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 9/9] drm/i915: FBC only supports 16bpp and 32bpp

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

So check for this.

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 80bdbde..8c6fb1d 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -515,6 +515,7 @@ static void __intel_fbc_update(struct drm_device *dev)
 	struct drm_i915_gem_object *obj;
 	const struct drm_display_mode *adjusted_mode;
 	unsigned int max_width, max_height;
+	int fb_cpp;
 
 	lockdep_assert_held(&dev_priv->fbc.lock);
 
@@ -598,6 +599,13 @@ static void __intel_fbc_update(struct drm_device *dev)
 		goto out_disable;
 	}
 
+	fb_cpp = drm_format_plane_cpp(fb->pixel_format, 0);
+	if (!(fb_cpp == 2 || fb_cpp == 4)) {
+		if (set_no_fbc_reason(dev_priv, FBC_UNSUPPORTED_MODE));
+			DRM_DEBUG_KMS("FBC only supports 16bpp and 32bpp\n");
+		goto out_disable;
+	}
+
 	/* If the kernel debugger is active, always disable compression */
 	if (in_dbg_master())
 		goto out_disable;
@@ -640,8 +648,7 @@ static 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 (i915_gem_stolen_setup_compression(dev, obj->base.size, fb_cpp)) {
 		if (set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL))
 			DRM_DEBUG_KMS("framebuffer too large, disabling compression\n");
 		return;
-- 
2.1.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/9] drm/i915: fix the FBC CFB size tracking

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/9] drm/i915: don't free the CFB while FBC is enabled

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/9] drm/i915: add the FBC mutex

[Chris Wilson]

On Tue, Dec 23, 2014 at 10:35:42AM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Make sure we're not gonna have weird races in really weird cases where
> a lot of different CRTCs are doing rendering and modesets at the same
> time.

You are not actually reducing the partial coverage from other locks,
particularly struct_mutex, so this doesn't look like the full improvement
it should be.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/9] drm/i915: don't alloc/free fbc_work at every update

[Chris Wilson]

On Tue, Dec 23, 2014 at 10:35:43AM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Because there's no need for it. Just use a static structure with a
> bool field to tell if it's in use or not. The big advantage here is
> not saving kzalloc/kfree calls, it's cutting the ugly "failed to
> allocate FBC work structure" code path: in this path we call
> enable_fbc() directly but we don't update fbc.crtc, fbc.fb_id and
> fbc.y - they are updated in intel_fbc_work_fn(), which we're not
> calling. And since testing out-of-memory cases like this is really
> hard, getting rid of the code path is a major relief.

No, it is not that hard to test.

The complaint here should be addressed by a function to call
dev_priv->display.enable_fbc, which would do the common task of setting
dev_priv->fbc.crtc, .fb_id, .y and *.enabled*.

That would remove duplicated code first. And then you can argue about
the merits of replacing the kmalloc by growing our global state.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/9] drm/i915: fix the FBC CFB size tracking

[Paulo Zanoni]

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



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/9] drm/i915: don't free the CFB while FBC is enabled

[Paulo Zanoni]

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



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/9] drm/i915: add the FBC mutex

[Paulo Zanoni]

2014-12-25 8:25 GMT-02:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Tue, Dec 23, 2014 at 10:35:42AM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Make sure we're not gonna have weird races in really weird cases where
>> a lot of different CRTCs are doing rendering and modesets at the same
>> time.
>
> You are not actually reducing the partial coverage from other locks,
> particularly struct_mutex, so this doesn't look like the full improvement
> it should be.

Good point. I'll take a look at that.

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/9] drm/i915: don't alloc/free fbc_work at every update

[Paulo Zanoni]

2014-12-25 8:33 GMT-02:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Tue, Dec 23, 2014 at 10:35:43AM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Because there's no need for it. Just use a static structure with a
>> bool field to tell if it's in use or not. The big advantage here is
>> not saving kzalloc/kfree calls, it's cutting the ugly "failed to
>> allocate FBC work structure" code path: in this path we call
>> enable_fbc() directly but we don't update fbc.crtc, fbc.fb_id and
>> fbc.y - they are updated in intel_fbc_work_fn(), which we're not
>> calling. And since testing out-of-memory cases like this is really
>> hard, getting rid of the code path is a major relief.
>
> No, it is not that hard to test.

Yeah, I know we have tons of checks for out-of-memory stuff, but it
takes time to test and I think we just don't have the FBC-specific
test.

>
> The complaint here should be addressed by a function to call
> dev_priv->display.enable_fbc, which would do the common task of setting
> dev_priv->fbc.crtc, .fb_id, .y and *.enabled*.

Ok, I'll do that. I thought that by keeping a single code path we'd be
able to avoid it :)

>
> That would remove duplicated code first. And then you can argue about
> the merits of replacing the kmalloc by growing our global state.

And what is your opinion on this? Do you think it's an improvement to
the codebase?

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/9] drm/i915: fix the FBC CFB size tracking

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/9] drm/i915: don't free the CFB while FBC is enabled

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/9] drm/i915: don't set the FBC plane select bits on HSW+

[Daniel Vetter]

On Tue, Dec 23, 2014 at 10:35:41AM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This commit is just to make the intentions explicit: on HSW+ these
> bits are MBZ, but since we only support plane A and the macro
> evaluates to zero when plane A is the parameter, we're not fixing any
> bug.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 83d3c8a..c6e688c 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -258,12 +258,15 @@ static void gen7_fbc_enable(struct drm_crtc *crtc)
>  	struct drm_framebuffer *fb = crtc->primary->fb;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	u32 dpfc_ctl;
> +	u32 dpfc_ctl = 0;
>  	int threshold = dev_priv->fbc.threshold;
>  
>  	dev_priv->fbc.enabled = true;
>  
> -	dpfc_ctl = IVB_DPFC_CTL_PLANE(intel_crtc->plane);
> +
> +	if (IS_IVYBRIDGE(dev))

What about baytrail?
-Daniel

> +		dpfc_ctl |= IVB_DPFC_CTL_PLANE(intel_crtc->plane);
> +
>  	if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
>  		threshold++;
>  
> -- 
> 2.1.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx