diff mbox

[3/4] drm/i915: Try harder to get FBC

Message ID 1403204773-7112-3-git-send-email-benjamin.widawsky@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ben Widawsky June 19, 2014, 7:06 p.m. UTC
The GEN FBC unit provides the ability to set a low pass on frames it
attempts to compress. If a frame is less than a certain amount
compressibility (2:1, 4:1) it will not bother. This allows the driver to
reduce the size it requests out of stolen memory.

Unluckily, a few months ago, Ville actually began using this feature for
framebuffers that are 16bpp (not sure why not 8bpp). In those cases, we
are already using this mechanism for a different purpose, and so we can
only achieve one further level of compression (2:1 -> 4:1)

FBC GEN1, ie. pre-G45 is ignored.

The cleverness of the patch is Art's. The bugs are mine.

Cc: Art Runyan <arthur.j.runyan@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h        |  3 +-
 drivers/gpu/drm/i915/i915_gem_stolen.c | 54 +++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_pm.c        | 28 ++++++++++++++++--
 3 files changed, 67 insertions(+), 18 deletions(-)

Comments

Runyan, Arthur J June 20, 2014, 3:56 p.m. UTC | #1
You give me too much credit.  I just gave you an explanation of what the hardware does, then you ran with it.

On Thu, Jun 19, 2014 at 12:06:13PM -0700, Ben Widawsky wrote:
+		DRM_INFO("Reducing the compressed framebuffer size. This may lead to increased power. Try to increase stolen memory size if available in BIOS.\n");

I prefer "This may lead to less power savings than a non-reduced size." since FBC is still going to save power.

 	dpfc_ctl = DPFC_CTL_PLANE(intel_crtc->plane);
 	if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
+		dev_priv->fbc.threshold++;
+
+	switch (dev_priv->fbc.threshold) {
+	case 4:
+		dpfc_ctl |= DPFC_CTL_LIMIT_4X;
+		break;
+	case 2:
 		dpfc_ctl |= DPFC_CTL_LIMIT_2X;
-	else
+		break;
+	case 1:
 		dpfc_ctl |= DPFC_CTL_LIMIT_1X;
+		break;
+	}

I Am Not A Coder, but at a glance it looks like the ++ could lead to undefined case 3 when you want case 4.
Ben Widawsky June 20, 2014, 4:55 p.m. UTC | #2
Rodrigo, when you're ready, can you pull in Art's requests?

On Fri, Jun 20, 2014 at 03:56:08PM +0000, Runyan, Arthur J wrote:
> You give me too much credit.  I just gave you an explanation of what the hardware does, then you ran with it.
> 
> On Thu, Jun 19, 2014 at 12:06:13PM -0700, Ben Widawsky wrote:
> +		DRM_INFO("Reducing the compressed framebuffer size. This may lead to increased power. Try to increase stolen memory size if available in BIOS.\n");
> 
> I prefer "This may lead to less power savings than a non-reduced size." since FBC is still going to save power.

Got it.

> 
>  	dpfc_ctl = DPFC_CTL_PLANE(intel_crtc->plane);
>  	if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
> +		dev_priv->fbc.threshold++;
> +
> +	switch (dev_priv->fbc.threshold) {
> +	case 4:
> +		dpfc_ctl |= DPFC_CTL_LIMIT_4X;
> +		break;
> +	case 2:
>  		dpfc_ctl |= DPFC_CTL_LIMIT_2X;
> -	else
> +		break;
> +	case 1:
>  		dpfc_ctl |= DPFC_CTL_LIMIT_1X;
> +		break;
> +	}
> 
> I Am Not A Coder, but at a glance it looks like the ++ could lead to undefined case 3 when you want case 4.
> 
Thanks for your feedback. 3 is an invalid value, but a default case
should be added here. I'd do a BUG_ON, but that's strictly forbidden.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0003206..db9dac4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -601,6 +601,7 @@  struct intel_context {
 
 struct i915_fbc {
 	unsigned long size;
+	unsigned threshold;
 	unsigned int fb_id;
 	enum plane plane;
 	int y;
@@ -2446,7 +2447,7 @@  static inline void i915_gem_chipset_flush(struct drm_device *dev)
 
 /* i915_gem_stolen.c */
 int i915_gem_init_stolen(struct drm_device *dev);
-int i915_gem_stolen_setup_compression(struct drm_device *dev, int size);
+int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_cpp);
 void i915_gem_stolen_cleanup_compression(struct drm_device *dev);
 void i915_gem_cleanup_stolen(struct drm_device *dev);
 struct drm_i915_gem_object *
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index a86b331..4b9804e 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -105,35 +105,61 @@  static unsigned long i915_stolen_to_physical(struct drm_device *dev)
 
 static int find_compression_threshold(struct drm_device *dev,
 				      struct drm_mm_node *node,
-				      int size)
+				      int size,
+				      int fb_cpp)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	const int compression_threshold = 1;
+	int compression_threshold = 1;
 	int ret;
 
-	/* Try to over-allocate to reduce reallocations and fragmentation */
+	/* HACK: This code depends on what we will do in *_enable_fbc. If that
+	 * code changes, this code needs to change as well.
+	 *
+	 * The enable_fbc code will attempt to use one of our 2 compression
+	 * thresholds, therefore, in that case, we only have 1 resort.
+	 */
+
+	/* Try to over-allocate to reduce reallocations and fragmentation. */
 	ret = drm_mm_insert_node(&dev_priv->mm.stolen, node,
 				 size <<= 1, 4096, DRM_MM_SEARCH_DEFAULT);
-	if (ret)
-		ret = drm_mm_insert_node(&dev_priv->mm.stolen, node,
-					 size >>= 1, 4096,
-					 DRM_MM_SEARCH_DEFAULT);
-	if (ret)
+	if (ret == 0)
+		return compression_threshold;
+
+again:
+	/* HW's ability to limit the CFB is 1:4 */
+	if (compression_threshold > 4 ||
+	    (fb_cpp == 2 && compression_threshold == 2))
 		return 0;
-	else
+
+	ret = drm_mm_insert_node(&dev_priv->mm.stolen, node,
+				 size >>= 1, 4096,
+				 DRM_MM_SEARCH_DEFAULT);
+	if (ret && INTEL_INFO(dev)->gen <= 4) {
+		return 0;
+	} else if (ret) {
+		compression_threshold <<= 1;
+		goto again;
+	} else {
 		return compression_threshold;
+	}
 }
 
-static int i915_setup_compression(struct drm_device *dev, int size)
+static int i915_setup_compression(struct drm_device *dev, int size, int fb_cpp)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_mm_node *uninitialized_var(compressed_llb);
 	int ret;
 
 	ret = find_compression_threshold(dev, &dev_priv->fbc.compressed_fb,
-					 size);
+					 size, fb_cpp);
 	if (!ret)
 		goto err_llb;
+	else if (ret > 1) {
+		DRM_INFO("Reducing the compressed framebuffer size. This may lead to increased power. Try to increase stolen memory size if available in BIOS.\n");
+
+	}
+
+	dev_priv->fbc.threshold = ret;
 
 	if (HAS_PCH_SPLIT(dev))
 		I915_WRITE(ILK_DPFC_CB_BASE, dev_priv->fbc.compressed_fb.start);
@@ -157,7 +183,7 @@  static int i915_setup_compression(struct drm_device *dev, int size)
 			   dev_priv->mm.stolen_base + compressed_llb->start);
 	}
 
-	dev_priv->fbc.size = size;
+	dev_priv->fbc.size = size / dev_priv->fbc.threshold;
 
 	DRM_DEBUG_KMS("reserved %d bytes of contiguous stolen space for FBC\n",
 		      size);
@@ -172,7 +198,7 @@  err_llb:
 	return -ENOSPC;
 }
 
-int i915_gem_stolen_setup_compression(struct drm_device *dev, int size)
+int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_cpp)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -185,7 +211,7 @@  int i915_gem_stolen_setup_compression(struct drm_device *dev, int size)
 	/* Release any current block */
 	i915_gem_stolen_cleanup_compression(dev);
 
-	return i915_setup_compression(dev, size);
+	return i915_setup_compression(dev, size, fb_cpp);
 }
 
 void i915_gem_stolen_cleanup_compression(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2043c4b..340ca3a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -229,9 +229,19 @@  static void ironlake_enable_fbc(struct drm_crtc *crtc)
 
 	dpfc_ctl = DPFC_CTL_PLANE(intel_crtc->plane);
 	if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
+		dev_priv->fbc.threshold++;
+
+	switch (dev_priv->fbc.threshold) {
+	case 4:
+		dpfc_ctl |= DPFC_CTL_LIMIT_4X;
+		break;
+	case 2:
 		dpfc_ctl |= DPFC_CTL_LIMIT_2X;
-	else
+		break;
+	case 1:
 		dpfc_ctl |= DPFC_CTL_LIMIT_1X;
+		break;
+	}
 	dpfc_ctl |= DPFC_CTL_FENCE_EN;
 	if (IS_GEN5(dev))
 		dpfc_ctl |= obj->fence_reg;
@@ -285,9 +295,20 @@  static void gen7_enable_fbc(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++;
+
+	switch (dev_priv->fbc.threshold) {
+	case 4:
+		dpfc_ctl |= DPFC_CTL_LIMIT_4X;
+		break;
+	case 2:
 		dpfc_ctl |= DPFC_CTL_LIMIT_2X;
-	else
+		break;
+	case 1:
 		dpfc_ctl |= DPFC_CTL_LIMIT_1X;
+		break;
+	}
+
 	dpfc_ctl |= IVB_DPFC_CTL_FENCE_EN;
 
 	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
@@ -566,7 +587,8 @@  void intel_update_fbc(struct drm_device *dev)
 	if (in_dbg_master())
 		goto out_disable;
 
-	if (i915_gem_stolen_setup_compression(dev, intel_fb->obj->base.size)) {
+	if (i915_gem_stolen_setup_compression(dev, intel_fb->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;