diff mbox

[2/7] drm/i915: move FBC code out of i915_gem_stolen.c

Message ID 1435781726-7282-3-git-send-email-przanoni@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paulo Zanoni July 1, 2015, 8:15 p.m. UTC
From: Paulo Zanoni <paulo.r.zanoni@intel.com>

With the abstractions created by the last patch, we can move this code
and the only thing inside intel_fbc.c that knows about dev_priv->mm is
the code that reads stolen_base.

We also had to move a call to i915_gem_stolen_cleanup_compression()
- now called intel_fbc_cleanup_cfb() - outside i915_gem_stolen.c.

Requested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c        |   1 +
 drivers/gpu/drm/i915/i915_drv.h        |   2 -
 drivers/gpu/drm/i915/i915_gem_stolen.c | 126 --------------------------------
 drivers/gpu/drm/i915/intel_drv.h       |   1 +
 drivers/gpu/drm/i915/intel_fbc.c       | 128 ++++++++++++++++++++++++++++++++-
 5 files changed, 127 insertions(+), 131 deletions(-)

Comments

Chris Wilson July 1, 2015, 8:44 p.m. UTC | #1
On Wed, Jul 01, 2015 at 05:15:21PM -0300, Paulo Zanoni wrote:

Looks much cleaner with the split.

> +void intel_fbc_cleanup_cfb(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (dev_priv->fbc.uncompressed_size == 0)
> +		return;
> +
> +	i915_gem_stolen_remove_node(&dev_priv->fbc.compressed_fb);
> +
> +	if (dev_priv->fbc.compressed_llb) {
> +		i915_gem_stolen_remove_node(dev_priv->fbc.compressed_llb);
> +		kfree(dev_priv->fbc.compressed_llb);
> +	}

Any reason why one node is embedded and the other allocated? Just feels
a little inconsistent, so lacks an explanation. Just that one is always
used, and the other on rare gen would probably suffice.
-Chris
Paulo Zanoni July 2, 2015, 1:39 p.m. UTC | #2
2015-07-01 17:44 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Wed, Jul 01, 2015 at 05:15:21PM -0300, Paulo Zanoni wrote:
>
> Looks much cleaner with the split.
>
>> +void intel_fbc_cleanup_cfb(struct drm_device *dev)
>> +{
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +     if (dev_priv->fbc.uncompressed_size == 0)
>> +             return;
>> +
>> +     i915_gem_stolen_remove_node(&dev_priv->fbc.compressed_fb);
>> +
>> +     if (dev_priv->fbc.compressed_llb) {
>> +             i915_gem_stolen_remove_node(dev_priv->fbc.compressed_llb);
>> +             kfree(dev_priv->fbc.compressed_llb);
>> +     }
>
> Any reason why one node is embedded and the other allocated? Just feels
> a little inconsistent, so lacks an explanation. Just that one is always
> used, and the other on rare gen would probably suffice.

I really didn't stop to pay attention to the ancient FBC pieces. IMHO
reasoning/explanation/change about this should be on a separate patch,
since this one is just moving the code around.

I only think about the gen2-4 FBC code when I remember it has the
"disable FBC when more than one pipe is visible" restriction which I
can't even find on the documentation I have. I wish we could either
remove it or just remove the whole gen2-4 FBC support (will we ever
have the courage to enable it by default?).

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
Chris Wilson July 2, 2015, 1:45 p.m. UTC | #3
On Thu, Jul 02, 2015 at 10:39:05AM -0300, Paulo Zanoni wrote:
> 2015-07-01 17:44 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Wed, Jul 01, 2015 at 05:15:21PM -0300, Paulo Zanoni wrote:
> >
> > Looks much cleaner with the split.
> >
> >> +void intel_fbc_cleanup_cfb(struct drm_device *dev)
> >> +{
> >> +     struct drm_i915_private *dev_priv = dev->dev_private;
> >> +
> >> +     if (dev_priv->fbc.uncompressed_size == 0)
> >> +             return;
> >> +
> >> +     i915_gem_stolen_remove_node(&dev_priv->fbc.compressed_fb);
> >> +
> >> +     if (dev_priv->fbc.compressed_llb) {
> >> +             i915_gem_stolen_remove_node(dev_priv->fbc.compressed_llb);
> >> +             kfree(dev_priv->fbc.compressed_llb);
> >> +     }
> >
> > Any reason why one node is embedded and the other allocated? Just feels
> > a little inconsistent, so lacks an explanation. Just that one is always
> > used, and the other on rare gen would probably suffice.
> 
> I really didn't stop to pay attention to the ancient FBC pieces. IMHO
> reasoning/explanation/change about this should be on a separate patch,
> since this one is just moving the code around.
> 
> I only think about the gen2-4 FBC code when I remember it has the
> "disable FBC when more than one pipe is visible" restriction which I
> can't even find on the documentation I have. I wish we could either
> remove it or just remove the whole gen2-4 FBC support (will we ever
> have the courage to enable it by default?).

Oh, no worries, that is how the code was is no problem. It just stood
out as inconsistent and hence made me stop and think when reviewing.

Might as well make them the same whilst we are here so I don't ask
stupid questions.
-Chris
Daniel Vetter July 6, 2015, 8:44 a.m. UTC | #4
On Thu, Jul 02, 2015 at 10:39:05AM -0300, Paulo Zanoni wrote:
> 2015-07-01 17:44 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Wed, Jul 01, 2015 at 05:15:21PM -0300, Paulo Zanoni wrote:
> >
> > Looks much cleaner with the split.
> >
> >> +void intel_fbc_cleanup_cfb(struct drm_device *dev)
> >> +{
> >> +     struct drm_i915_private *dev_priv = dev->dev_private;
> >> +
> >> +     if (dev_priv->fbc.uncompressed_size == 0)
> >> +             return;
> >> +
> >> +     i915_gem_stolen_remove_node(&dev_priv->fbc.compressed_fb);
> >> +
> >> +     if (dev_priv->fbc.compressed_llb) {
> >> +             i915_gem_stolen_remove_node(dev_priv->fbc.compressed_llb);
> >> +             kfree(dev_priv->fbc.compressed_llb);
> >> +     }
> >
> > Any reason why one node is embedded and the other allocated? Just feels
> > a little inconsistent, so lacks an explanation. Just that one is always
> > used, and the other on rare gen would probably suffice.
> 
> I really didn't stop to pay attention to the ancient FBC pieces. IMHO
> reasoning/explanation/change about this should be on a separate patch,
> since this one is just moving the code around.
> 
> I only think about the gen2-4 FBC code when I remember it has the
> "disable FBC when more than one pipe is visible" restriction which I
> can't even find on the documentation I have. I wish we could either
> remove it or just remove the whole gen2-4 FBC support (will we ever
> have the courage to enable it by default?).

Yeah I think killing gen2-4 fbc would be ok. Same for g4x fbc (hw too
broken) and ilk fbc (same really according to Art). Then we'd only need to
deal with fbc on gen6+, which is reasonably sane and consistent.

But we can rip the code out whenever you want, no hurry.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index c5349fa..1ae9e0b 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1120,6 +1120,7 @@  int i915_driver_unload(struct drm_device *dev)
 	i915_gem_cleanup_ringbuffer(dev);
 	i915_gem_context_fini(dev);
 	mutex_unlock(&dev->struct_mutex);
+	intel_fbc_cleanup_cfb(dev);
 	i915_gem_cleanup_stolen(dev);
 
 	intel_csr_ucode_fini(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b9de374..c955037 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3114,8 +3114,6 @@  int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv,
 				unsigned alignment);
 void i915_gem_stolen_remove_node(struct drm_mm_node *node);
 int i915_gem_init_stolen(struct drm_device *dev);
-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 *
 i915_gem_object_create_stolen(struct drm_device *dev, u32 size);
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 6b43234..0619786 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -167,131 +167,6 @@  static unsigned long i915_stolen_to_physical(struct drm_device *dev)
 	return base;
 }
 
-static int find_compression_threshold(struct drm_device *dev,
-				      struct drm_mm_node *node,
-				      int size,
-				      int fb_cpp)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	int compression_threshold = 1;
-	int ret;
-
-	/* 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 = i915_gem_stolen_insert_node(dev_priv, node, size <<= 1, 4096);
-	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;
-
-	ret = i915_gem_stolen_insert_node(dev_priv, node, size >>= 1, 4096);
-	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, 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, 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 (INTEL_INFO(dev_priv)->gen >= 5)
-		I915_WRITE(ILK_DPFC_CB_BASE, dev_priv->fbc.compressed_fb.start);
-	else if (IS_GM45(dev)) {
-		I915_WRITE(DPFC_CB_BASE, dev_priv->fbc.compressed_fb.start);
-	} else {
-		compressed_llb = kzalloc(sizeof(*compressed_llb), GFP_KERNEL);
-		if (!compressed_llb)
-			goto err_fb;
-
-		ret = i915_gem_stolen_insert_node(dev_priv, compressed_llb,
-						  4096, 4096);
-		if (ret)
-			goto err_fb;
-
-		dev_priv->fbc.compressed_llb = compressed_llb;
-
-		I915_WRITE(FBC_CFB_BASE,
-			   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.uncompressed_size = size;
-
-	DRM_DEBUG_KMS("reserved %d bytes of contiguous stolen space for FBC\n",
-		      size);
-
-	return 0;
-
-err_fb:
-	kfree(compressed_llb);
-	i915_gem_stolen_remove_node(&dev_priv->fbc.compressed_fb);
-err_llb:
-	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;
-}
-
-int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_cpp)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	if (!drm_mm_initialized(&dev_priv->mm.stolen))
-		return -ENODEV;
-
-	if (size <= dev_priv->fbc.uncompressed_size)
-		return 0;
-
-	/* Release any current block */
-	i915_gem_stolen_cleanup_compression(dev);
-
-	return i915_setup_compression(dev, size, fb_cpp);
-}
-
-void i915_gem_stolen_cleanup_compression(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	if (dev_priv->fbc.uncompressed_size == 0)
-		return;
-
-	i915_gem_stolen_remove_node(&dev_priv->fbc.compressed_fb);
-
-	if (dev_priv->fbc.compressed_llb) {
-		i915_gem_stolen_remove_node(dev_priv->fbc.compressed_llb);
-		kfree(dev_priv->fbc.compressed_llb);
-	}
-
-	dev_priv->fbc.uncompressed_size = 0;
-}
-
 void i915_gem_cleanup_stolen(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -299,7 +174,6 @@  void i915_gem_cleanup_stolen(struct drm_device *dev)
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
 		return;
 
-	i915_gem_stolen_cleanup_compression(dev);
 	drm_mm_takedown(&dev_priv->mm.stolen);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3f0a890..82abbfa 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1258,6 +1258,7 @@  void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 void intel_fbc_flush(struct drm_i915_private *dev_priv,
 		     unsigned int frontbuffer_bits);
 const char *intel_no_fbc_reason_str(enum no_fbc_reason reason);
+void intel_fbc_cleanup_cfb(struct drm_device *dev);
 
 /* intel_hdmi.c */
 void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port);
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 9e55b9b..a91bf82 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -515,6 +515,128 @@  static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
 	return crtc;
 }
 
+static int find_compression_threshold(struct drm_device *dev,
+				      struct drm_mm_node *node,
+				      int size,
+				      int fb_cpp)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int compression_threshold = 1;
+	int ret;
+
+	/* 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 = i915_gem_stolen_insert_node(dev_priv, node, size <<= 1, 4096);
+	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;
+
+	ret = i915_gem_stolen_insert_node(dev_priv, node, size >>= 1, 4096);
+	if (ret && INTEL_INFO(dev)->gen <= 4) {
+		return 0;
+	} else if (ret) {
+		compression_threshold <<= 1;
+		goto again;
+	} else {
+		return compression_threshold;
+	}
+}
+
+static int intel_fbc_alloc_cfb(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, 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 (INTEL_INFO(dev_priv)->gen >= 5)
+		I915_WRITE(ILK_DPFC_CB_BASE, dev_priv->fbc.compressed_fb.start);
+	else if (IS_GM45(dev)) {
+		I915_WRITE(DPFC_CB_BASE, dev_priv->fbc.compressed_fb.start);
+	} else {
+		compressed_llb = kzalloc(sizeof(*compressed_llb), GFP_KERNEL);
+		if (!compressed_llb)
+			goto err_fb;
+
+		ret = i915_gem_stolen_insert_node(dev_priv, compressed_llb,
+						  4096, 4096);
+		if (ret)
+			goto err_fb;
+
+		dev_priv->fbc.compressed_llb = compressed_llb;
+
+		I915_WRITE(FBC_CFB_BASE,
+			   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.uncompressed_size = size;
+
+	DRM_DEBUG_KMS("reserved %d bytes of contiguous stolen space for FBC\n",
+		      size);
+
+	return 0;
+
+err_fb:
+	kfree(compressed_llb);
+	i915_gem_stolen_remove_node(&dev_priv->fbc.compressed_fb);
+err_llb:
+	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;
+}
+
+void intel_fbc_cleanup_cfb(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (dev_priv->fbc.uncompressed_size == 0)
+		return;
+
+	i915_gem_stolen_remove_node(&dev_priv->fbc.compressed_fb);
+
+	if (dev_priv->fbc.compressed_llb) {
+		i915_gem_stolen_remove_node(dev_priv->fbc.compressed_llb);
+		kfree(dev_priv->fbc.compressed_llb);
+	}
+
+	dev_priv->fbc.uncompressed_size = 0;
+}
+
+static int intel_fbc_setup_cfb(struct drm_device *dev, int size, int fb_cpp)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (size <= dev_priv->fbc.uncompressed_size)
+		return 0;
+
+	/* Release any current block */
+	intel_fbc_cleanup_cfb(dev);
+
+	return intel_fbc_alloc_cfb(dev, size, fb_cpp);
+}
+
 /**
  * intel_fbc_update - enable/disable FBC as needed
  * @dev: the drm_device
@@ -624,8 +746,8 @@  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 (intel_fbc_setup_cfb(dev, obj->base.size,
+				drm_format_plane_cpp(fb->pixel_format, 0))) {
 		set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL);
 		goto out_disable;
 	}
@@ -678,7 +800,7 @@  out_disable:
 		DRM_DEBUG_KMS("unsupported config, disabling FBC\n");
 		intel_fbc_disable(dev);
 	}
-	i915_gem_stolen_cleanup_compression(dev);
+	intel_fbc_cleanup_cfb(dev);
 }
 
 void intel_fbc_invalidate(struct drm_i915_private *dev_priv,