diff mbox

[13/29] drm/armada: Use a private mutex to protect priv->linear

Message ID 1448271183-20523-14-git-send-email-daniel.vetter@ffwll.ch
State New, archived
Headers show

Commit Message

Daniel Vetter Nov. 23, 2015, 9:32 a.m. UTC
Reusing the Big DRM Lock just leaks, and the few things left that
dev->struct_mutex protected are very well contained - it's just the
linear drm_mm manager.

With this armada is completely struct_mutex free!

Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/armada/armada_debugfs.c |  4 ++--
 drivers/gpu/drm/armada/armada_drm.h     |  3 ++-
 drivers/gpu/drm/armada/armada_drv.c     |  1 +
 drivers/gpu/drm/armada/armada_gem.c     | 10 +++++-----
 4 files changed, 10 insertions(+), 8 deletions(-)

Comments

Russell King - ARM Linux Nov. 23, 2015, 5:40 p.m. UTC | #1
On Mon, Nov 23, 2015 at 10:32:46AM +0100, Daniel Vetter wrote:
> diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
> index e3a86b96af2a..a43690ab18b9 100644
> --- a/drivers/gpu/drm/armada/armada_gem.c
> +++ b/drivers/gpu/drm/armada/armada_gem.c
> @@ -46,7 +46,7 @@ static size_t roundup_gem_size(size_t size)
>  	return roundup(size, PAGE_SIZE);
>  }
>  
> -/* dev->struct_mutex is held here */
> +/* dev_priv->linear_lock is held here */
>  void armada_gem_free_object(struct drm_gem_object *obj)

This is wrong (unless there's changes which I'm not aware of.)

This function is called from drm_gem_object_free(), which is called from
drm_gem_object_unreference(), both of which contain:

        WARN_ON(!mutex_is_locked(&dev->struct_mutex));

Now, unless you're changing the conditions on which
drm_gem_object_unreference() to be under dev_priv->linear_lock, the
above comment becomes misleading.

It'd also need drm_gem_object_unreference_unlocked() to become per-
driver, so it can take dev_priv->linear_lock, and I don't see anything
in the patches which I've received which does that.

So, I suspect the new comment is basically false.
Daniel Vetter Nov. 23, 2015, 8:17 p.m. UTC | #2
On Mon, Nov 23, 2015 at 6:40 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Nov 23, 2015 at 10:32:46AM +0100, Daniel Vetter wrote:
>> diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
>> index e3a86b96af2a..a43690ab18b9 100644
>> --- a/drivers/gpu/drm/armada/armada_gem.c
>> +++ b/drivers/gpu/drm/armada/armada_gem.c
>> @@ -46,7 +46,7 @@ static size_t roundup_gem_size(size_t size)
>>       return roundup(size, PAGE_SIZE);
>>  }
>>
>> -/* dev->struct_mutex is held here */
>> +/* dev_priv->linear_lock is held here */
>>  void armada_gem_free_object(struct drm_gem_object *obj)
>
> This is wrong (unless there's changes which I'm not aware of.)
>
> This function is called from drm_gem_object_free(), which is called from
> drm_gem_object_unreference(), both of which contain:
>
>         WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>
> Now, unless you're changing the conditions on which
> drm_gem_object_unreference() to be under dev_priv->linear_lock, the
> above comment becomes misleading.
>
> It'd also need drm_gem_object_unreference_unlocked() to become per-
> driver, so it can take dev_priv->linear_lock, and I don't see anything
> in the patches which I've received which does that.
>
> So, I suspect the new comment is basically false.

Well the patch is false. I should have grabbed the new
dev_priv->linear_lock around the cleanup functions in
armada_gem_free_object. Which is possible since armada never calls an
gem unref function while holding the new lock. Sorry for not
double-checking what I've done, I'll revise.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/armada/armada_debugfs.c b/drivers/gpu/drm/armada/armada_debugfs.c
index 471e45627f1e..d4f7ab0a30d4 100644
--- a/drivers/gpu/drm/armada/armada_debugfs.c
+++ b/drivers/gpu/drm/armada/armada_debugfs.c
@@ -21,9 +21,9 @@  static int armada_debugfs_gem_linear_show(struct seq_file *m, void *data)
 	struct armada_private *priv = dev->dev_private;
 	int ret;
 
-	mutex_lock(&dev->struct_mutex);
+	mutex_lock(&priv->linear_lock);
 	ret = drm_mm_dump_table(m, &priv->linear);
-	mutex_unlock(&dev->struct_mutex);
+	mutex_unlock(&priv->linear_lock);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/armada/armada_drm.h b/drivers/gpu/drm/armada/armada_drm.h
index 4df6f2af2b21..6d2d8be38297 100644
--- a/drivers/gpu/drm/armada/armada_drm.h
+++ b/drivers/gpu/drm/armada/armada_drm.h
@@ -57,7 +57,8 @@  struct armada_private {
 	DECLARE_KFIFO(fb_unref, struct drm_framebuffer *, 8);
 	struct drm_fb_helper	*fbdev;
 	struct armada_crtc	*dcrtc[2];
-	struct drm_mm		linear;
+	struct drm_mm		linear; /* protectec by linear_lock */
+	struct mutex		linear_lock;
 	struct drm_property	*csc_yuv_prop;
 	struct drm_property	*csc_rgb_prop;
 	struct drm_property	*colorkey_prop;
diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
index 77ab93d60125..3bd7e1cde99e 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -102,6 +102,7 @@  static int armada_drm_load(struct drm_device *dev, unsigned long flags)
 	dev->mode_config.preferred_depth = 24;
 	dev->mode_config.funcs = &armada_drm_mode_config_funcs;
 	drm_mm_init(&priv->linear, mem->start, resource_size(mem));
+	mutex_init(&priv->linear_lock);
 
 	ret = component_bind_all(dev->dev, dev);
 	if (ret)
diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
index e3a86b96af2a..a43690ab18b9 100644
--- a/drivers/gpu/drm/armada/armada_gem.c
+++ b/drivers/gpu/drm/armada/armada_gem.c
@@ -46,7 +46,7 @@  static size_t roundup_gem_size(size_t size)
 	return roundup(size, PAGE_SIZE);
 }
 
-/* dev->struct_mutex is held here */
+/* dev_priv->linear_lock is held here */
 void armada_gem_free_object(struct drm_gem_object *obj)
 {
 	struct armada_gem_object *dobj = drm_to_armada_gem(obj);
@@ -144,10 +144,10 @@  armada_gem_linear_back(struct drm_device *dev, struct armada_gem_object *obj)
 		if (!node)
 			return -ENOSPC;
 
-		mutex_lock(&dev->struct_mutex);
+		mutex_lock(&priv->linear_lock);
 		ret = drm_mm_insert_node(&priv->linear, node, size, align,
 					 DRM_MM_SEARCH_DEFAULT);
-		mutex_unlock(&dev->struct_mutex);
+		mutex_unlock(&priv->linear_lock);
 		if (ret) {
 			kfree(node);
 			return ret;
@@ -158,9 +158,9 @@  armada_gem_linear_back(struct drm_device *dev, struct armada_gem_object *obj)
 		/* Ensure that the memory we're returning is cleared. */
 		ptr = ioremap_wc(obj->linear->start, size);
 		if (!ptr) {
-			mutex_lock(&dev->struct_mutex);
+			mutex_lock(&priv->linear_lock);
 			drm_mm_remove_node(obj->linear);
-			mutex_unlock(&dev->struct_mutex);
+			mutex_unlock(&priv->linear_lock);
 			kfree(obj->linear);
 			obj->linear = NULL;
 			return -ENOMEM;