diff mbox

[v2,3/4] drm/i915: pre-alloc instead of drm_mm search/get_block

Message ID 1374925122-2127-1-git-send-email-dh.herrmann@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

David Herrmann July 27, 2013, 11:38 a.m. UTC 
i915 is the last user of the weird search+get_block drm_mm API. Convert it
to an explicit kmalloc()+insert_node(). This drops the last user of the
node-cache in drm_mm. We can remove it now in a follow-up patch.

v2:
 - simplify error path in i915_setup_compression()

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 75 +++++++++++++++++++++-------------
 1 file changed, 46 insertions(+), 29 deletions(-)

Comments

Chris Wilson July 27, 2013, 1:06 p.m. UTC | #1 
On Sat, Jul 27, 2013 at 01:38:42PM +0200, David Herrmann wrote:
> i915 is the last user of the weird search+get_block drm_mm API. Convert it
> to an explicit kmalloc()+insert_node(). This drops the last user of the
> node-cache in drm_mm. We can remove it now in a follow-up patch.
> 
> v2:
>  - simplify error path in i915_setup_compression()

You only applied it to the err path, I was expecting err_fb to do the
kfree(compressed_llb) as well. Feel free to rename those to err_fb and
err_llb respectively if you think that helps.
-Chris
David Herrmann July 27, 2013, 1:09 p.m. UTC | #2 
Hi

On Sat, Jul 27, 2013 at 3:06 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Sat, Jul 27, 2013 at 01:38:42PM +0200, David Herrmann wrote:
>> i915 is the last user of the weird search+get_block drm_mm API. Convert it
>> to an explicit kmalloc()+insert_node(). This drops the last user of the
>> node-cache in drm_mm. We can remove it now in a follow-up patch.
>>
>> v2:
>>  - simplify error path in i915_setup_compression()
>
> You only applied it to the err path, I was expecting err_fb to do the
> kfree(compressed_llb) as well. Feel free to rename those to err_fb and
> err_llb respectively if you think that helps.

I wasn't sure about that second error-path. Some kernel subsystems
tend to inline error-paths that are only taken once (which is the case
for compressed_llb). But if that's not the case for DRM, I will surely
fix that, too.

Thanks
David
Chris Wilson July 27, 2013, 1:15 p.m. UTC | #3 
On Sat, Jul 27, 2013 at 03:09:48PM +0200, David Herrmann wrote:
> Hi
> 
> On Sat, Jul 27, 2013 at 3:06 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Sat, Jul 27, 2013 at 01:38:42PM +0200, David Herrmann wrote:
> >> i915 is the last user of the weird search+get_block drm_mm API. Convert it
> >> to an explicit kmalloc()+insert_node(). This drops the last user of the
> >> node-cache in drm_mm. We can remove it now in a follow-up patch.
> >>
> >> v2:
> >>  - simplify error path in i915_setup_compression()
> >
> > You only applied it to the err path, I was expecting err_fb to do the
> > kfree(compressed_llb) as well. Feel free to rename those to err_fb and
> > err_llb respectively if you think that helps.
> 
> I wasn't sure about that second error-path. Some kernel subsystems
> tend to inline error-paths that are only taken once (which is the case
> for compressed_llb). But if that's not the case for DRM, I will surely
> fix that, too.

My concern in this case is to keep the two path similar; we use the same
allocation style so we should use the same cleanup. Otherwise, the next
time I look at the code, I wil wonder why they are different. :)
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index e355170..0044e65 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -112,34 +112,38 @@  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);
+	int ret;
 
-	/* Try to over-allocate to reduce reallocations and fragmentation */
-	compressed_fb = drm_mm_search_free(&dev_priv->mm.stolen,
-					   size <<= 1, 4096,
-					   DRM_MM_SEARCH_DEFAULT);
-	if (!compressed_fb)
-		compressed_fb = drm_mm_search_free(&dev_priv->mm.stolen,
-						   size >>= 1, 4096,
-						   DRM_MM_SEARCH_DEFAULT);
-	if (compressed_fb)
-		compressed_fb = drm_mm_get_block(compressed_fb, size, 4096);
+	compressed_fb = kzalloc(sizeof(*compressed_fb), GFP_KERNEL);
 	if (!compressed_fb)
 		goto err;
 
+	/* Try to over-allocate to reduce reallocations and fragmentation */
+	ret = drm_mm_insert_node(&dev_priv->mm.stolen, compressed_fb,
+				 size <<= 1, 4096, DRM_MM_SEARCH_DEFAULT);
+	if (ret)
+		ret = drm_mm_insert_node(&dev_priv->mm.stolen, compressed_fb,
+					 size >>= 1, 4096,
+					 DRM_MM_SEARCH_DEFAULT);
+	if (ret)
+		goto err;
+
 	if (HAS_PCH_SPLIT(dev))
 		I915_WRITE(ILK_DPFC_CB_BASE, compressed_fb->start);
 	else if (IS_GM45(dev)) {
 		I915_WRITE(DPFC_CB_BASE, compressed_fb->start);
 	} else {
-		compressed_llb = drm_mm_search_free(&dev_priv->mm.stolen,
-						    4096, 4096,
-						    DRM_MM_SEARCH_DEFAULT);
-		if (compressed_llb)
-			compressed_llb = drm_mm_get_block(compressed_llb,
-							  4096, 4096);
+		compressed_llb = kzalloc(sizeof(*compressed_llb), GFP_KERNEL);
 		if (!compressed_llb)
 			goto err_fb;
 
+		ret = drm_mm_insert_node(&dev_priv->mm.stolen, compressed_llb,
+					 4096, 4096, DRM_MM_SEARCH_DEFAULT);
+		if (ret) {
+			kfree(compressed_llb);
+			goto err_fb;
+		}
+
 		dev_priv->fbc.compressed_llb = compressed_llb;
 
 		I915_WRITE(FBC_CFB_BASE,
@@ -157,8 +161,9 @@  static int i915_setup_compression(struct drm_device *dev, int size)
 	return 0;
 
 err_fb:
-	drm_mm_put_block(compressed_fb);
+	drm_mm_remove_node(compressed_fb);
 err:
+	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;
 }
@@ -186,11 +191,15 @@  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_put_block(dev_priv->fbc.compressed_fb);
+	if (dev_priv->fbc.compressed_fb) {
+		drm_mm_remove_node(dev_priv->fbc.compressed_fb);
+		kfree(dev_priv->fbc.compressed_fb);
+	}
 
-	if (dev_priv->fbc.compressed_llb)
-		drm_mm_put_block(dev_priv->fbc.compressed_llb);
+	if (dev_priv->fbc.compressed_llb) {
+		drm_mm_remove_node(dev_priv->fbc.compressed_llb);
+		kfree(dev_priv->fbc.compressed_llb);
+	}
 
 	dev_priv->fbc.size = 0;
 }
@@ -323,6 +332,7 @@  i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj;
 	struct drm_mm_node *stolen;
+	int ret;
 
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
 		return NULL;
@@ -331,18 +341,23 @@  i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
 	if (size == 0)
 		return NULL;
 
-	stolen = drm_mm_search_free(&dev_priv->mm.stolen, size, 4096,
-				    DRM_MM_SEARCH_DEFAULT);
-	if (stolen)
-		stolen = drm_mm_get_block(stolen, size, 4096);
-	if (stolen == NULL)
+	stolen = kzalloc(sizeof(*stolen), GFP_KERNEL);
+	if (!stolen)
+		return NULL;
+
+	ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size,
+				 4096, DRM_MM_SEARCH_DEFAULT);
+	if (ret) {
+		kfree(stolen);
 		return NULL;
+	}
 
 	obj = _i915_gem_object_create_stolen(dev, stolen);
 	if (obj)
 		return obj;
 
-	drm_mm_put_block(stolen);
+	drm_mm_remove_node(stolen);
+	kfree(stolen);
 	return NULL;
 }
 
@@ -386,7 +401,8 @@  i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 	obj = _i915_gem_object_create_stolen(dev, stolen);
 	if (obj == NULL) {
 		DRM_DEBUG_KMS("failed to allocate stolen object\n");
-		drm_mm_put_block(stolen);
+		drm_mm_remove_node(stolen);
+		kfree(stolen);
 		return NULL;
 	}
 
@@ -426,7 +442,8 @@  void
 i915_gem_object_release_stolen(struct drm_i915_gem_object *obj)
 {
 	if (obj->stolen) {
-		drm_mm_put_block(obj->stolen);
+		drm_mm_remove_node(obj->stolen);
+		kfree(obj->stolen);
 		obj->stolen = NULL;
 	}
 }