| Message ID | 1448271183-20523-14-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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 --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;
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(-)