[PATCH 1/4] drm/i915: Move compressed_fb to static allocation

[PATCH 1/4] drm/i915: Move compressed_fb to static allocation

[Ben Widawsky]

We are already using the size to determine whether or not to free the
object, so there is no functional change there. Almost everything else
has changed to static allocations of the drm_mm_node too.

Aside from bringing this inline with much of our other code, this makes
error paths slightly simpler, which benefits the look of an upcoming
patch.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h        |  2 +-
 drivers/gpu/drm/i915/i915_gem_stolen.c | 27 ++++++++++-----------------
 2 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0640071..0003206 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -605,7 +605,7 @@ struct i915_fbc {
 	enum plane plane;
 	int y;
 
-	struct drm_mm_node *compressed_fb;
+	struct drm_mm_node compressed_fb;
 	struct drm_mm_node *compressed_llb;
 
 	struct intel_fbc_work {
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 6441178..642fd36 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -106,27 +106,25 @@ static unsigned long i915_stolen_to_physical(struct drm_device *dev)
 static int i915_setup_compression(struct drm_device *dev, int size)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_mm_node *compressed_fb, *uninitialized_var(compressed_llb);
+	struct drm_mm_node *uninitialized_var(compressed_llb);
 	int ret;
 
-	compressed_fb = kzalloc(sizeof(*compressed_fb), GFP_KERNEL);
-	if (!compressed_fb)
-		goto err_llb;
-
 	/* Try to over-allocate to reduce reallocations and fragmentation */
-	ret = drm_mm_insert_node(&dev_priv->mm.stolen, compressed_fb,
+	ret = drm_mm_insert_node(&dev_priv->mm.stolen,
+				 &dev_priv->fbc.compressed_fb,
 				 size <<= 1, 4096, DRM_MM_SEARCH_DEFAULT);
 	if (ret)
-		ret = drm_mm_insert_node(&dev_priv->mm.stolen, compressed_fb,
+		ret = drm_mm_insert_node(&dev_priv->mm.stolen,
+					 &dev_priv->fbc.compressed_fb,
 					 size >>= 1, 4096,
 					 DRM_MM_SEARCH_DEFAULT);
 	if (ret)
 		goto err_llb;
 
 	if (HAS_PCH_SPLIT(dev))
-		I915_WRITE(ILK_DPFC_CB_BASE, compressed_fb->start);
+		I915_WRITE(ILK_DPFC_CB_BASE, dev_priv->fbc.compressed_fb.start);
 	else if (IS_GM45(dev)) {
-		I915_WRITE(DPFC_CB_BASE, compressed_fb->start);
+		I915_WRITE(DPFC_CB_BASE, dev_priv->fbc.compressed_fb.start);
 	} else {
 		compressed_llb = kzalloc(sizeof(*compressed_llb), GFP_KERNEL);
 		if (!compressed_llb)
@@ -140,12 +138,11 @@ static int i915_setup_compression(struct drm_device *dev, int size)
 		dev_priv->fbc.compressed_llb = compressed_llb;
 
 		I915_WRITE(FBC_CFB_BASE,
-			   dev_priv->mm.stolen_base + compressed_fb->start);
+			   dev_priv->mm.stolen_base + dev_priv->fbc.compressed_fb.start);
 		I915_WRITE(FBC_LL_BASE,
 			   dev_priv->mm.stolen_base + compressed_llb->start);
 	}
 
-	dev_priv->fbc.compressed_fb = compressed_fb;
 	dev_priv->fbc.size = size;
 
 	DRM_DEBUG_KMS("reserved %d bytes of contiguous stolen space for FBC\n",
@@ -155,9 +152,8 @@ static int i915_setup_compression(struct drm_device *dev, int size)
 
 err_fb:
 	kfree(compressed_llb);
-	drm_mm_remove_node(compressed_fb);
+	drm_mm_remove_node(&dev_priv->fbc.compressed_fb);
 err_llb:
-	kfree(compressed_fb);
 	pr_info_once("drm: not enough stolen space for compressed buffer (need %d more bytes), disabling. Hint: you may be able to increase stolen memory size in the BIOS to avoid this.\n", size);
 	return -ENOSPC;
 }
@@ -185,10 +181,7 @@ void i915_gem_stolen_cleanup_compression(struct drm_device *dev)
 	if (dev_priv->fbc.size == 0)
 		return;
 
-	if (dev_priv->fbc.compressed_fb) {
-		drm_mm_remove_node(dev_priv->fbc.compressed_fb);
-		kfree(dev_priv->fbc.compressed_fb);
-	}
+	drm_mm_remove_node(&dev_priv->fbc.compressed_fb);
 
 	if (dev_priv->fbc.compressed_llb) {
 		drm_mm_remove_node(dev_priv->fbc.compressed_llb);
-- 
2.0.0

[PATCH 2/4] drm/i915: Extract CFB threshold calculation

[Ben Widawsky]

Right now, there is no threshold (0 means fail, 1 means 1:1 compression
limit). This is to split the function/non-functional change of the next
patch.

The next patch will start to attempt to reduce the amount of CFB space
we need for dire situations. It will be contained within this function.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 642fd36..a86b331 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -103,22 +103,36 @@ static unsigned long i915_stolen_to_physical(struct drm_device *dev)
 	return base;
 }
 
-static int i915_setup_compression(struct drm_device *dev, int size)
+static int find_compression_threshold(struct drm_device *dev,
+				      struct drm_mm_node *node,
+				      int size)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_mm_node *uninitialized_var(compressed_llb);
+	const int compression_threshold = 1;
 	int ret;
 
 	/* Try to over-allocate to reduce reallocations and fragmentation */
-	ret = drm_mm_insert_node(&dev_priv->mm.stolen,
-				 &dev_priv->fbc.compressed_fb,
+	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,
-					 &dev_priv->fbc.compressed_fb,
+		ret = drm_mm_insert_node(&dev_priv->mm.stolen, node,
 					 size >>= 1, 4096,
 					 DRM_MM_SEARCH_DEFAULT);
 	if (ret)
+		return 0;
+	else
+		return compression_threshold;
+}
+
+static int i915_setup_compression(struct drm_device *dev, int size)
+{
+	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);
+	if (!ret)
 		goto err_llb;
 
 	if (HAS_PCH_SPLIT(dev))
-- 
2.0.0

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

[Ben Widawsky]

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

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

[PATCH 4/4] drm/i915: Reserve space for FBC (fbcon)

[Ben Widawsky]

This is one part in a few fixes needed to make FBC work with limited
stolen memory and large resolution displays. It is not the full
solution, but one (easy) step.

The patch is straight-forward, it attempts to check there will be room
for FBC before trying to "reclaim"

This modifies behavior originally introduced:
commit 0ffb0ff283cca16f72caf29c44496d83b0c291fb
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Nov 15 11:32:27 2012 +0000

    drm/i915: Allocate fbcon from stolen memory

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 27975c3..ca83af8 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -57,6 +57,36 @@ static struct fb_ops intelfb_ops = {
 	.fb_debug_leave = drm_fb_helper_debug_leave,
 };
 
+static bool intelfb_use_stolen(struct drm_device *dev, struct drm_mm *mm,
+			       int size)
+{
+	struct drm_mm_node *entry;
+	unsigned long adj_start;
+	unsigned long adj_end;
+
+	if (!drm_mm_initialized(mm))
+		return false;
+
+	if (!HAS_FBC(dev))
+		return true;
+
+	/* It is more desirable to use FBC (if enabled) than to allocate the
+	 * framebuffer from stolen. We can cheat this by rounding up the size by
+	 * 2 (and hope to get lucky with alignment). The other options are more
+	 * invasive, and arguably not any more effective.
+	 */
+	size *= 2;
+	__drm_mm_for_each_hole(entry, mm, adj_start, adj_end,
+			       DRM_MM_SEARCH_DEFAULT) {
+		if (adj_end - adj_start < size)
+			continue;
+
+		if (adj_end >= adj_start + size)
+			return true;
+	}
+
+	return false;
+}
 static int intelfb_alloc(struct drm_fb_helper *helper,
 			 struct drm_fb_helper_surface_size *sizes)
 {
@@ -64,8 +94,9 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 		container_of(helper, struct intel_fbdev, helper);
 	struct drm_framebuffer *fb;
 	struct drm_device *dev = helper->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_mode_fb_cmd2 mode_cmd = {};
-	struct drm_i915_gem_object *obj;
+	struct drm_i915_gem_object *obj = NULL;
 	int size, ret;
 
 	/* we don't do packed 24bpp */
@@ -82,7 +113,9 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 
 	size = mode_cmd.pitches[0] * mode_cmd.height;
 	size = ALIGN(size, PAGE_SIZE);
-	obj = i915_gem_object_create_stolen(dev, size);
+
+	if (intelfb_use_stolen(dev, &dev_priv->mm.stolen, size))
+		obj = i915_gem_object_create_stolen(dev, size);
 	if (obj == NULL)
 		obj = i915_gem_alloc_object(dev, size);
 	if (!obj) {
-- 
2.0.0

Re: [PATCH 4/4] drm/i915: Reserve space for FBC (fbcon)

[Chris Wilson]

On Thu, Jun 19, 2014 at 12:06:13PM -0700, Ben Widawsky wrote:
> This is one part in a few fixes needed to make FBC work with limited
> stolen memory and large resolution displays. It is not the full
> solution, but one (easy) step.
> 
> The patch is straight-forward, it attempts to check there will be room
> for FBC before trying to "reclaim"

But it special cases one particular allocation. Why don't you just
reserve stolen upfront for FBC? Compute the maximum buffer size the
hardware could support and try to claim it during stolen init.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/4] drm/i915: Reserve space for FBC (fbcon)

[Ben Widawsky]

On Thu, Jun 19, 2014 at 08:28:11PM +0100, Chris Wilson wrote:
> On Thu, Jun 19, 2014 at 12:06:13PM -0700, Ben Widawsky wrote:
> > This is one part in a few fixes needed to make FBC work with limited
> > stolen memory and large resolution displays. It is not the full
> > solution, but one (easy) step.
> > 
> > The patch is straight-forward, it attempts to check there will be room
> > for FBC before trying to "reclaim"
> 
> But it special cases one particular allocation. Why don't you just
> reserve stolen upfront for FBC? Compute the maximum buffer size the
> hardware could support and try to claim it during stolen init.
> -Chris
> 

This was my initial thought actually. I didn't want to have to rework
all of our initialization sequence, and verify I didn't break anything.
In particular I wasn't sure what happens when we try to recover the fb
from stolen (if it fails).

In particular, there is a logical conflict between fastboot and fbc -
and one needs to assign the preference over who gets stolen memory
first. Also, when I wrote this patch, I was unaware that the reclaimed
fb by itself was enough to not allow the full CFB to fit (though it will
with the 3 patches before this)

Anyway, consider this the demonstration of the problem, and [my only]
half-attempt to fix it. Hopefully you guys can fix it properly.

-- 
Ben Widawsky, Intel Open Source Technology Center

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

[Runyan, Arthur J]

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.

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

[Ben Widawsky]

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.

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH] drm/i915: Try harder to get FBC

[Rodrigo Vivi]

From: Ben Widawsky <benjamin.widawsky@intel.com>

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.

v2: Update message and including missing threshold case 3 (Spotted by Arthur).

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5b7aed2..9953ea8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -600,6 +600,7 @@ struct intel_context {
 
 struct i915_fbc {
 	unsigned long size;
+	unsigned threshold;
 	unsigned int fb_id;
 	enum plane plane;
 	int y;
@@ -2489,7 +2490,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..b695d18 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 less power savings than a non-reduced-size. 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 a90fdbd..4fcb2f7 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -229,9 +229,20 @@ 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:
+	case 3:
+		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 +296,21 @@ 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:
+	case 3:
+		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 +589,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;
-- 
1.9.1

Re: [PATCH 1/4] drm/i915: Move compressed_fb to static allocation

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 5199 bytes --]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


On Thu, Jun 19, 2014 at 12:06 PM, Ben Widawsky <benjamin.widawsky@intel.com>
wrote:

> We are already using the size to determine whether or not to free the
> object, so there is no functional change there. Almost everything else
> has changed to static allocations of the drm_mm_node too.
>
> Aside from bringing this inline with much of our other code, this makes
> error paths slightly simpler, which benefits the look of an upcoming
> patch.
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_drv.h        |  2 +-
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 27 ++++++++++-----------------
>  2 files changed, 11 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 0640071..0003206 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -605,7 +605,7 @@ struct i915_fbc {
>         enum plane plane;
>         int y;
>
> -       struct drm_mm_node *compressed_fb;
> +       struct drm_mm_node compressed_fb;
>         struct drm_mm_node *compressed_llb;
>
>         struct intel_fbc_work {
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
> b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 6441178..642fd36 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -106,27 +106,25 @@ static unsigned long i915_stolen_to_physical(struct
> drm_device *dev)
>  static int i915_setup_compression(struct drm_device *dev, int size)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       struct drm_mm_node *compressed_fb,
> *uninitialized_var(compressed_llb);
> +       struct drm_mm_node *uninitialized_var(compressed_llb);
>         int ret;
>
> -       compressed_fb = kzalloc(sizeof(*compressed_fb), GFP_KERNEL);
> -       if (!compressed_fb)
> -               goto err_llb;
> -
>         /* Try to over-allocate to reduce reallocations and fragmentation
> */
> -       ret = drm_mm_insert_node(&dev_priv->mm.stolen, compressed_fb,
> +       ret = drm_mm_insert_node(&dev_priv->mm.stolen,
> +                                &dev_priv->fbc.compressed_fb,
>                                  size <<= 1, 4096, DRM_MM_SEARCH_DEFAULT);
>         if (ret)
> -               ret = drm_mm_insert_node(&dev_priv->mm.stolen,
> compressed_fb,
> +               ret = drm_mm_insert_node(&dev_priv->mm.stolen,
> +                                        &dev_priv->fbc.compressed_fb,
>                                          size >>= 1, 4096,
>                                          DRM_MM_SEARCH_DEFAULT);
>         if (ret)
>                 goto err_llb;
>
>         if (HAS_PCH_SPLIT(dev))
> -               I915_WRITE(ILK_DPFC_CB_BASE, compressed_fb->start);
> +               I915_WRITE(ILK_DPFC_CB_BASE,
> dev_priv->fbc.compressed_fb.start);
>         else if (IS_GM45(dev)) {
> -               I915_WRITE(DPFC_CB_BASE, compressed_fb->start);
> +               I915_WRITE(DPFC_CB_BASE,
> dev_priv->fbc.compressed_fb.start);
>         } else {
>                 compressed_llb = kzalloc(sizeof(*compressed_llb),
> GFP_KERNEL);
>                 if (!compressed_llb)
> @@ -140,12 +138,11 @@ static int i915_setup_compression(struct drm_device
> *dev, int size)
>                 dev_priv->fbc.compressed_llb = compressed_llb;
>
>                 I915_WRITE(FBC_CFB_BASE,
> -                          dev_priv->mm.stolen_base +
> compressed_fb->start);
> +                          dev_priv->mm.stolen_base +
> dev_priv->fbc.compressed_fb.start);
>                 I915_WRITE(FBC_LL_BASE,
>                            dev_priv->mm.stolen_base +
> compressed_llb->start);
>         }
>
> -       dev_priv->fbc.compressed_fb = compressed_fb;
>         dev_priv->fbc.size = size;
>
>         DRM_DEBUG_KMS("reserved %d bytes of contiguous stolen space for
> FBC\n",
> @@ -155,9 +152,8 @@ static int i915_setup_compression(struct drm_device
> *dev, int size)
>
>  err_fb:
>         kfree(compressed_llb);
> -       drm_mm_remove_node(compressed_fb);
> +       drm_mm_remove_node(&dev_priv->fbc.compressed_fb);
>  err_llb:
> -       kfree(compressed_fb);
>         pr_info_once("drm: not enough stolen space for compressed buffer
> (need %d more bytes), disabling. Hint: you may be able to increase stolen
> memory size in the BIOS to avoid this.\n", size);
>         return -ENOSPC;
>  }
> @@ -185,10 +181,7 @@ void i915_gem_stolen_cleanup_compression(struct
> drm_device *dev)
>         if (dev_priv->fbc.size == 0)
>                 return;
>
> -       if (dev_priv->fbc.compressed_fb) {
> -               drm_mm_remove_node(dev_priv->fbc.compressed_fb);
> -               kfree(dev_priv->fbc.compressed_fb);
> -       }
> +       drm_mm_remove_node(&dev_priv->fbc.compressed_fb);
>
>         if (dev_priv->fbc.compressed_llb) {
>                 drm_mm_remove_node(dev_priv->fbc.compressed_llb);
> --
> 2.0.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 6770 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 2/4] drm/i915: Extract CFB threshold calculation

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 2927 bytes --]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


On Thu, Jun 19, 2014 at 12:06 PM, Ben Widawsky <benjamin.widawsky@intel.com>
wrote:

> Right now, there is no threshold (0 means fail, 1 means 1:1 compression
> limit). This is to split the function/non-functional change of the next
> patch.
>
> The next patch will start to attempt to reduce the amount of CFB space
> we need for dire situations. It will be contained within this function.
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
> b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 642fd36..a86b331 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -103,22 +103,36 @@ static unsigned long i915_stolen_to_physical(struct
> drm_device *dev)
>         return base;
>  }
>
> -static int i915_setup_compression(struct drm_device *dev, int size)
> +static int find_compression_threshold(struct drm_device *dev,
> +                                     struct drm_mm_node *node,
> +                                     int size)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       struct drm_mm_node *uninitialized_var(compressed_llb);
> +       const int compression_threshold = 1;
>         int ret;
>
>         /* Try to over-allocate to reduce reallocations and fragmentation
> */
> -       ret = drm_mm_insert_node(&dev_priv->mm.stolen,
> -                                &dev_priv->fbc.compressed_fb,
> +       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,
> -                                        &dev_priv->fbc.compressed_fb,
> +               ret = drm_mm_insert_node(&dev_priv->mm.stolen, node,
>                                          size >>= 1, 4096,
>                                          DRM_MM_SEARCH_DEFAULT);
>         if (ret)
> +               return 0;
> +       else
> +               return compression_threshold;
> +}
> +
> +static int i915_setup_compression(struct drm_device *dev, int size)
> +{
> +       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);
> +       if (!ret)
>                 goto err_llb;
>
>         if (HAS_PCH_SPLIT(dev))
> --
> 2.0.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 4161 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 4/4] drm/i915: Reserve space for FBC (fbcon)

[Ben Widawsky]

On Thu, Jun 19, 2014 at 08:28:11PM +0100, Chris Wilson wrote:
> On Thu, Jun 19, 2014 at 12:06:13PM -0700, Ben Widawsky wrote:
> > This is one part in a few fixes needed to make FBC work with limited
> > stolen memory and large resolution displays. It is not the full
> > solution, but one (easy) step.
> > 
> > The patch is straight-forward, it attempts to check there will be room
> > for FBC before trying to "reclaim"
> 
> But it special cases one particular allocation. Why don't you just
> reserve stolen upfront for FBC? Compute the maximum buffer size the
> hardware could support and try to claim it during stolen init.
> -Chris
> 

I agree this would be the best approach (and what I had planned to do).
For one, I didn't find the interfaces I wanted in the drm_mm to do what
I needed (though I didn't look very hard). I ended up getting stuck with
having to decide whether to reclaim the scanout (and fastboot), or FBC.
I believe this should be a decision left to the user, where user is the
distro packaging.

I'd like to just point out some math at this point too.
Common stolen size is 32M
3840 x 2160  x 4 = 31.64M

So we have a real problem if we want to reuse any of stolen memory,
which the first 3 patches address to some extent.

Anyway, I think I was pretty clear that the patch is incomplete, and
primarily meant to motivate the relevant parties to figure out how they
want to handle the stolen reclaim.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Try harder to get FBC

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 9315 bytes --]

Jani, please ignore the 4th patch on this series and merge the 3 I've
reviewed and tested already.

They are essential to allow FBC work on BDW without changing bios
configuration and allow PC7 residency.

Thanks,
Rodrigo.


On Mon, Jun 30, 2014 at 10:41 AM, Rodrigo Vivi <rodrigo.vivi@intel.com>
wrote:

> From: Ben Widawsky <benjamin.widawsky@intel.com>
>
> 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.
>
> v2: Update message and including missing threshold case 3 (Spotted by
> Arthur).
>
> Reviewedby: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Art Runyan <arthur.j.runyan@intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h        |  3 +-
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 54
> +++++++++++++++++++++++++---------
>  drivers/gpu/drm/i915/intel_pm.c        | 30 +++++++++++++++++--
>  3 files changed, 69 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 5b7aed2..9953ea8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -600,6 +600,7 @@ struct intel_context {
>
>  struct i915_fbc {
>         unsigned long size;
> +       unsigned threshold;
>         unsigned int fb_id;
>         enum plane plane;
>         int y;
> @@ -2489,7 +2490,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..b695d18 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 less power savings than a non-reduced-size. 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 a90fdbd..4fcb2f7 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -229,9 +229,20 @@ 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:
> +       case 3:
> +               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 +296,21 @@ 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:
> +       case 3:
> +               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 +589,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;
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 12115 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: Try harder to get FBC

[Jani Nikula]

On Tue, 01 Jul 2014, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> Jani, please ignore the 4th patch on this series and merge the 3 I've
> reviewed and tested already.

Patches 1-3 pushed to dinq. Thanks for the patches and review.

BR,
Jani.

>
> They are essential to allow FBC work on BDW without changing bios
> configuration and allow PC7 residency.
>
> Thanks,
> Rodrigo.
>
>
> On Mon, Jun 30, 2014 at 10:41 AM, Rodrigo Vivi <rodrigo.vivi@intel.com>
> wrote:
>
>> From: Ben Widawsky <benjamin.widawsky@intel.com>
>>
>> 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.
>>
>> v2: Update message and including missing threshold case 3 (Spotted by
>> Arthur).
>>
>> Reviewedby: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Art Runyan <arthur.j.runyan@intel.com>
>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h        |  3 +-
>>  drivers/gpu/drm/i915/i915_gem_stolen.c | 54
>> +++++++++++++++++++++++++---------
>>  drivers/gpu/drm/i915/intel_pm.c        | 30 +++++++++++++++++--
>>  3 files changed, 69 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 5b7aed2..9953ea8 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -600,6 +600,7 @@ struct intel_context {
>>
>>  struct i915_fbc {
>>         unsigned long size;
>> +       unsigned threshold;
>>         unsigned int fb_id;
>>         enum plane plane;
>>         int y;
>> @@ -2489,7 +2490,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..b695d18 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 less power savings than a non-reduced-size. 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 a90fdbd..4fcb2f7 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -229,9 +229,20 @@ 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:
>> +       case 3:
>> +               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 +296,21 @@ 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:
>> +       case 3:
>> +               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 +589,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;
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>
>
>
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

-- 
Jani Nikula, Intel Open Source Technology Center