For drm_gem_object_unreference callers are required to hold dev->struct_mutex, which these paths don't. Enforcing this requirement has become a bit more strict with

commit ef4c6270bf2867e2f8032e9614d1a8cfc6c71663 Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Thu Oct 15 09:36:25 2015 +0200

drm/gem: Check locking in drm_gem_object_unreference

Cc: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/armada/armada_crtc.c | 2 +- drivers/gpu/drm/armada/armada_gem.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c index cebcab560626..7ea35bee7cd5 100644 --- a/drivers/gpu/drm/armada/armada_crtc.c +++ b/drivers/gpu/drm/armada/armada_crtc.c @@ -972,7 +972,7 @@ static void armada_drm_crtc_destroy(struct drm_crtc *crtc) struct armada_private *priv = crtc->dev->dev_private;

if (dcrtc->cursor_obj) - drm_gem_object_unreference(&dcrtc->cursor_obj->obj); + drm_gem_object_unreference_unlocked(&dcrtc->cursor_obj->obj);

priv->dcrtc[dcrtc->num] = NULL; drm_crtc_cleanup(&dcrtc->crtc); diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c index 60a688ef81c7..aaf88641bfc5 100644 --- a/drivers/gpu/drm/armada/armada_gem.c +++ b/drivers/gpu/drm/armada/armada_gem.c @@ -352,13 +352,13 @@ int armada_gem_mmap_ioctl(struct drm_device *dev, void *data, return -ENOENT;

if (!dobj->obj.filp) { - drm_gem_object_unreference(&dobj->obj); + drm_gem_object_unreference_unlocked(&dobj->obj); return -EINVAL; }

addr = vm_mmap(dobj->obj.filp, 0, args->size, PROT_READ | PROT_WRITE, MAP_SHARED, args->offset); - drm_gem_object_unreference(&dobj->obj); + drm_gem_object_unreference_unlocked(&dobj->obj); if (IS_ERR_VALUE(addr)) return addr;

For drm_gem_object_unreference callers are required to hold dev->struct_mutex, which these paths don't. Enforcing this requirement has become a bit more strict with

commit ef4c6270bf2867e2f8032e9614d1a8cfc6c71663 Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Thu Oct 15 09:36:25 2015 +0200

drm/gem: Check locking in drm_gem_object_unreference

Cc: Tomi Valkeinen tomi.valkeinen@ti.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/omapdrm/omap_fbdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c index b8e4cdec28c3..c59090e8d23f 100644 --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c @@ -156,7 +156,7 @@ static int omap_fbdev_create(struct drm_fb_helper *helper, /* note: if fb creation failed, we can't rely on fb destroy * to unref the bo: */ - drm_gem_object_unreference(fbdev->bo); + drm_gem_object_unreference_unlocked(fbdev->bo); ret = PTR_ERR(fb); goto fail; }

For drm_gem_object_unreference callers are required to hold dev->struct_mutex, which these paths don't. Enforcing this requirement has become a bit more strict with

commit ef4c6270bf2867e2f8032e9614d1a8cfc6c71663 Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Thu Oct 15 09:36:25 2015 +0200

drm/gem: Check locking in drm_gem_object_unreference

Cc: Dave Airlie airlied@redhat.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/qxl/qxl_fb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c index 7136e521e6db..bb7ce07b788b 100644 --- a/drivers/gpu/drm/qxl/qxl_fb.c +++ b/drivers/gpu/drm/qxl/qxl_fb.c @@ -443,11 +443,11 @@ out_unref: } } if (fb && ret) { - drm_gem_object_unreference(gobj); + drm_gem_object_unreference_unlocked(gobj); drm_framebuffer_cleanup(fb); kfree(fb); } - drm_gem_object_unreference(gobj); + drm_gem_object_unreference_unlocked(gobj); return ret; }

For drm_gem_object_unreference callers are required to hold dev->struct_mutex, which these paths don't. Enforcing this requirement has become a bit more strict with

commit ef4c6270bf2867e2f8032e9614d1a8cfc6c71663 Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Thu Oct 15 09:36:25 2015 +0200

drm/gem: Check locking in drm_gem_object_unreference

Cc: Dave Airlie airlied@redhat.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/udl/udl_fb.c | 2 +- drivers/gpu/drm/udl/udl_gem.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c index 200419d4d43c..18a2acbccb7d 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -538,7 +538,7 @@ static int udlfb_create(struct drm_fb_helper *helper, out_destroy_fbi: drm_fb_helper_release_fbi(helper); out_gfree: - drm_gem_object_unreference(&ufbdev->ufb.obj->base); + drm_gem_object_unreference_unlocked(&ufbdev->ufb.obj->base); out: return ret; } diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c index 2a0a784ab6ee..d7528e0d8442 100644 --- a/drivers/gpu/drm/udl/udl_gem.c +++ b/drivers/gpu/drm/udl/udl_gem.c @@ -52,7 +52,7 @@ udl_gem_create(struct drm_file *file, return ret; }

- drm_gem_object_unreference(&obj->base); + drm_gem_object_unreference_unlocked(&obj->base); *handle_p = handle; return 0; }

Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).

v2: Split out the leak fix in dump_map_offset into a separate patch as requested by Russell. Also align labels the same way as before to stick with local coding style.

Cc: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/armada/armada_gem.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c index 2a3ef7938f30..e3a86b96af2a 100644 --- a/drivers/gpu/drm/armada/armada_gem.c +++ b/drivers/gpu/drm/armada/armada_gem.c @@ -274,12 +274,10 @@ int armada_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, struct armada_gem_object *obj; int ret = 0;

- mutex_lock(&dev->struct_mutex); obj = armada_gem_object_lookup(dev, file, handle); if (!obj) { DRM_ERROR("failed to lookup gem object\n"); - ret = -EINVAL; - goto err_unlock; + return -EINVAL; }

/* Don't allow imported objects to be mapped */ @@ -295,9 +293,7 @@ int armada_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, }

err_unref: - drm_gem_object_unreference(&obj->obj); - err_unlock: - mutex_unlock(&dev->struct_mutex); + drm_gem_object_unreference_unlocked(&obj->obj);

return ret; }

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

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;

Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).

v2: Finally get rid of the copypasta from another commit in this commit message. And convert to _unlocked like we need to (Patrik).

Cc: Patrik Jakobsson patrik.r.jakobsson@gmail.com Cc: Thierry Reding thierry.reding@gmail.com Acked-by: Thierry Reding thierry.reding@gmail.com Reviewed-by: Patrik Jakobsson patrik.r.jakobsson@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/tegra/gem.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c index 01e16e146bfe..fb712316c522 100644 --- a/drivers/gpu/drm/tegra/gem.c +++ b/drivers/gpu/drm/tegra/gem.c @@ -408,12 +408,9 @@ int tegra_bo_dumb_map_offset(struct drm_file *file, struct drm_device *drm, struct drm_gem_object *gem; struct tegra_bo *bo;

- mutex_lock(&drm->struct_mutex); - gem = drm_gem_object_lookup(drm, file, handle); if (!gem) { dev_err(drm->dev, "failed to lookup GEM object\n"); - mutex_unlock(&drm->struct_mutex); return -EINVAL; }

@@ -421,9 +418,7 @@ int tegra_bo_dumb_map_offset(struct drm_file *file, struct drm_device *drm,

*offset = drm_vma_node_offset_addr(&bo->gem.vma_node);

- drm_gem_object_unreference(gem); - - mutex_unlock(&drm->struct_mutex); + drm_gem_object_unreference_unlocked(gem);

return 0; }

This only grabs the mutex when really needed, but still has a might-acquire lockdep check to make sure that's always possible. With this patch tegra is officially struct_mutex free, yay!

v2: refernce_unlocked doesn't exist as kbuild spotted.

Cc: Thierry Reding thierry.reding@gmail.com Acked-by: Thierry Reding thierry.reding@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/tegra/drm.c | 4 +--- drivers/gpu/drm/tegra/gem.c | 6 +----- 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index 3479c57151af..34d067b677bd 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -277,9 +277,7 @@ host1x_bo_lookup(struct drm_device *drm, struct drm_file *file, u32 handle) if (!gem) return NULL;

- mutex_lock(&drm->struct_mutex); - drm_gem_object_unreference(gem); - mutex_unlock(&drm->struct_mutex); + drm_gem_object_unreference_unlocked(gem);

bo = to_tegra_bo(gem); return &bo->base; diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c index fb712316c522..fbec29516bcb 100644 --- a/drivers/gpu/drm/tegra/gem.c +++ b/drivers/gpu/drm/tegra/gem.c @@ -30,9 +30,7 @@ static void tegra_bo_put(struct host1x_bo *bo) struct tegra_bo *obj = host1x_to_tegra_bo(bo); struct drm_device *drm = obj->gem.dev;

- mutex_lock(&drm->struct_mutex); - drm_gem_object_unreference(&obj->gem); - mutex_unlock(&drm->struct_mutex); + drm_gem_object_unreference_unlocked(&obj->gem); }

static dma_addr_t tegra_bo_pin(struct host1x_bo *bo, struct sg_table **sgt) @@ -74,9 +72,7 @@ static struct host1x_bo *tegra_bo_get(struct host1x_bo *bo) struct tegra_bo *obj = host1x_to_tegra_bo(bo); struct drm_device *drm = obj->gem.dev;

- mutex_lock(&drm->struct_mutex); drm_gem_object_reference(&obj->gem); - mutex_unlock(&drm->struct_mutex);

return bo; }

This is called without dev->struct_mutex held, we need to use the _unlocked variant.

Never caught in the wild since you'd need an evil userspace which races a gem_close ioctl call with the in-progress open.

Cc: Patrik Jakobsson patrik.r.jakobsson@gmail.com Acked-by: Patrik Jakobsson patrik.r.jakobsson@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/gma500/gem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index c707fa6fca85..e3bdc8b1c32c 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -130,7 +130,7 @@ int psb_gem_create(struct drm_file *file, struct drm_device *dev, u64 size, return ret; } /* We have the initial and handle reference but need only one now */ - drm_gem_object_unreference(&r->gem); + drm_gem_object_unreference_unlocked(&r->gem); *handlep = handle; return 0; }

This is init/teardown code, locking is just to appease locking checks. And since gem create/free doesn't need this any more there's really no reason for grabbing dev->struct_mutex.

Again important to switch obj_unref to _unlocked variants.

Cc: Patrik Jakobsson patrik.r.jakobsson@gmail.com Acked-by: Patrik Jakobsson patrik.r.jakobsson@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/gma500/framebuffer.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c index dc0508dca1d4..ee95c03a8c54 100644 --- a/drivers/gpu/drm/gma500/framebuffer.c +++ b/drivers/gpu/drm/gma500/framebuffer.c @@ -406,8 +406,6 @@ static int psbfb_create(struct psb_fbdev *fbdev,

memset(dev_priv->vram_addr + backing->offset, 0, size);

- mutex_lock(&dev->struct_mutex); - info = drm_fb_helper_alloc_fbi(&fbdev->psb_fb_helper); if (IS_ERR(info)) { ret = PTR_ERR(info); @@ -463,17 +461,15 @@ static int psbfb_create(struct psb_fbdev *fbdev, dev_dbg(dev->dev, "allocated %dx%d fb\n", psbfb->base.width, psbfb->base.height);

- mutex_unlock(&dev->struct_mutex); return 0; out_unref: if (backing->stolen) psb_gtt_free_range(dev, backing); else - drm_gem_object_unreference(&backing->gem); + drm_gem_object_unreference_unlocked(&backing->gem);

drm_fb_helper_release_fbi(&fbdev->psb_fb_helper); out_err1: - mutex_unlock(&dev->struct_mutex); psb_gtt_free_range(dev, backing); return ret; } @@ -569,7 +565,7 @@ static int psb_fbdev_destroy(struct drm_device *dev, struct psb_fbdev *fbdev) drm_framebuffer_cleanup(&psbfb->base);

if (psbfb->gtt) - drm_gem_object_unreference(&psbfb->gtt->gem); + drm_gem_object_unreference_unlocked(&psbfb->gtt->gem); return 0; }

@@ -784,12 +780,8 @@ void psb_modeset_cleanup(struct drm_device *dev) { struct drm_psb_private *dev_priv = dev->dev_private; if (dev_priv->modeset) { - mutex_lock(&dev->struct_mutex); - drm_kms_helper_poll_fini(dev); psb_fbdev_fini(dev); drm_mode_config_cleanup(dev); - - mutex_unlock(&dev->struct_mutex); } }

There's currently two places where the gma500 fault handler relies upon dev->struct_mutex: - To protect r->mappping - To make sure vm_insert_pfn isn't called concurrently (in which case the 2nd thread would get an error code).

Everything else (specifically psb_gtt_pin) is already protected by some other locks. Hence just create a new driver-private mmap_mutex just for this function.

With this gma500 is complete dev->struct_mutex free!

Cc: Patrik Jakobsson patrik.r.jakobsson@gmail.com Acked-by: Patrik Jakobsson patrik.r.jakobsson@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/gma500/gem.c | 4 ++-- drivers/gpu/drm/gma500/gtt.c | 1 + drivers/gpu/drm/gma500/psb_drv.h | 2 ++ 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index f0357f525f56..506224b3a0ad 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -182,7 +182,7 @@ int psb_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)

/* Make sure we don't parallel update on a fault, nor move or remove something from beneath our feet */ - mutex_lock(&dev->struct_mutex); + mutex_lock(&dev_priv->mmap_mutex);

/* For now the mmap pins the object and it stays pinned. As things stand that will do us no harm */ @@ -208,7 +208,7 @@ int psb_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf) ret = vm_insert_pfn(vma, (unsigned long)vmf->virtual_address, pfn);

fail: - mutex_unlock(&dev->struct_mutex); + mutex_unlock(&dev_priv->mmap_mutex); switch (ret) { case 0: case -ERESTARTSYS: diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c index ce015db59dc6..8f69225ce2b4 100644 --- a/drivers/gpu/drm/gma500/gtt.c +++ b/drivers/gpu/drm/gma500/gtt.c @@ -425,6 +425,7 @@ int psb_gtt_init(struct drm_device *dev, int resume)

if (!resume) { mutex_init(&dev_priv->gtt_mutex); + mutex_init(&dev_priv->mmap_mutex); psb_gtt_alloc(dev); }

diff --git a/drivers/gpu/drm/gma500/psb_drv.h b/drivers/gpu/drm/gma500/psb_drv.h index e21726ecac32..3bd2c726dd61 100644 --- a/drivers/gpu/drm/gma500/psb_drv.h +++ b/drivers/gpu/drm/gma500/psb_drv.h @@ -465,6 +465,8 @@ struct drm_psb_private { struct mutex gtt_mutex; struct resource *gtt_mem; /* Our PCI resource */

+ struct mutex mmap_mutex; + struct psb_mmu_driver *mmu; struct psb_mmu_pd *pf_pd;

Simply forgotten about this when I was doing my general cleansing of simple gem mmap offset functions. There's nothing but core functions called here, and they all have their own protection already.

Aside: DRM_ERROR for userspace controlled input isn't great, but that's for another patch.

v2: Use _unlocked unreference (Daniel Stone).

Cc: Daniel Stone daniel@fooishbar.org Cc: Inki Dae inki.dae@samsung.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/exynos/exynos_drm_gem.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c index 252eb301470c..44e710ab84de 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -448,8 +448,6 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv, struct drm_gem_object *obj; int ret = 0;

- mutex_lock(&dev->struct_mutex); - /* * get offset of memory allocated for drm framebuffer. * - this callback would be called by user application @@ -459,16 +457,13 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv, obj = drm_gem_object_lookup(dev, file_priv, handle); if (!obj) { DRM_ERROR("failed to lookup gem object.\n"); - ret = -EINVAL; - goto unlock; + return -EINVAL; }

*offset = drm_vma_node_offset_addr(&obj->vma_node); DRM_DEBUG_KMS("offset = 0x%lx\n", (unsigned long)*offset);

- drm_gem_object_unreference(obj); -unlock: - mutex_unlock(&dev->struct_mutex); + drm_gem_object_unreference_unlocked(obj); return ret; }

The sg table isn't refcounted, there's no corresponding locking for unmapping and drm_map_sg is ok with being called concurrently.

So drop the locking since it doesn't protect anything.

Cc: Inki Dae inki.dae@samsung.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/exynos/exynos_drm_gem.c | 4 ---- 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c index 44e710ab84de..b198fa560106 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -378,16 +378,12 @@ int exynos_gem_map_sgt_with_dma(struct drm_device *drm_dev, { int nents;

- mutex_lock(&drm_dev->struct_mutex); - nents = dma_map_sg(drm_dev->dev, sgt->sgl, sgt->nents, dir); if (!nents) { DRM_ERROR("failed to map sgl with dma.\n"); - mutex_unlock(&drm_dev->struct_mutex); return nents; }

- mutex_unlock(&drm_dev->struct_mutex); return 0; }

The only things this protects is reading ->flags and ->size, both of which are invariant over the lifetime of an exynos gem bo. So no locking needed at all (besides that, nothing protects the writers anyway).

Aside: exynos_gem_obj->size is redundant with exynos_gem_obj->base.size and probably should be removed.

v2: Use _unlocked unreference (Daniel Stone).

Cc: Daniel Stone daniel@fooishbar.org Cc: Inki Dae inki.dae@samsung.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/exynos/exynos_drm_gem.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c index b198fa560106..e735827e0c6d 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -352,12 +352,9 @@ int exynos_drm_gem_get_ioctl(struct drm_device *dev, void *data, struct drm_exynos_gem_info *args = data; struct drm_gem_object *obj;

- mutex_lock(&dev->struct_mutex); - obj = drm_gem_object_lookup(dev, file_priv, args->handle); if (!obj) { DRM_ERROR("failed to lookup gem object.\n"); - mutex_unlock(&dev->struct_mutex); return -EINVAL; }

@@ -366,8 +363,7 @@ int exynos_drm_gem_get_ioctl(struct drm_device *dev, void *data, args->flags = exynos_gem->flags; args->size = exynos_gem->size;

- drm_gem_object_unreference(obj); - mutex_unlock(&dev->struct_mutex); + drm_gem_object_unreference_unlocked(obj);

return 0; }

Doesn't protect anything at all, and probably just here because a long time ago dev->struct_mutex was required to allocate gem objects.

With this patch exynos is completely struct_mutex free!

Cc: Inki Dae inki.dae@samsung.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c index f6118baa8e3e..e1f7abaa61df 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c @@ -138,8 +138,6 @@ static int exynos_drm_fbdev_create(struct drm_fb_helper *helper, mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth);

- mutex_lock(&dev->struct_mutex); - size = mode_cmd.pitches[0] * mode_cmd.height;

exynos_gem = exynos_drm_gem_create(dev, EXYNOS_BO_CONTIG, size); @@ -154,10 +152,8 @@ static int exynos_drm_fbdev_create(struct drm_fb_helper *helper, size); }

- if (IS_ERR(exynos_gem)) { - ret = PTR_ERR(exynos_gem); - goto out; - } + if (IS_ERR(exynos_gem)) + return PTR_ERR(exynos_gem);

exynos_fbdev->exynos_gem = exynos_gem;

@@ -173,7 +169,6 @@ static int exynos_drm_fbdev_create(struct drm_fb_helper *helper, if (ret < 0) goto err_destroy_framebuffer;

- mutex_unlock(&dev->struct_mutex); return ret;

err_destroy_framebuffer: @@ -181,13 +176,12 @@ err_destroy_framebuffer: err_destroy_gem: exynos_drm_gem_destroy(exynos_gem);

-/* - * if failed, all resources allocated above would be released by - * drm_mode_config_cleanup() when drm_load() had been called prior - * to any specific driver such as fimd or hdmi driver. - */ -out: - mutex_unlock(&dev->struct_mutex); + /* + * if failed, all resources allocated above would be released by + * drm_mode_config_cleanup() when drm_load() had been called prior + * to any specific driver such as fimd or hdmi driver. + */ + return ret; }

The offset manager already checks for existing offsets internally, while holding suitable locks. We can drop this check.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/vgem/vgem_drv.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c index 02ae60c395b7..63026d4324ad 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -208,11 +208,9 @@ int vgem_gem_dumb_map(struct drm_file *file, struct drm_device *dev, goto unlock; }

- if (!drm_vma_node_has_offset(&obj->vma_node)) { - ret = drm_gem_create_mmap_offset(obj); - if (ret) - goto unref; - } + ret = drm_gem_create_mmap_offset(obj); + if (ret) + goto unref;

BUG_ON(!obj->filp);

It's racy, creating mmap offsets is a slowpath, so better to remove it to avoid drivers doing broken things.

The only user is i915, and it's ok there because everything (well almost) is protected by dev->struct_mutex in i915-gem.

While at it add a note in the create_mmap_offset kerneldoc that drivers must release it again. And then I also noticed that drm_gem_object_release entirely lacks kerneldoc.

Cc: David Herrmann dh.herrmann@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_gem.c | 17 +++++++++++++++++ drivers/gpu/drm/i915/i915_gem.c | 3 --- include/drm/drm_vma_manager.h | 15 +-------------- 3 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 2e10bba4468b..5c5001171dd3 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -392,6 +392,10 @@ EXPORT_SYMBOL(drm_gem_handle_create); * @obj: obj in question * * This routine frees fake offsets allocated by drm_gem_create_mmap_offset(). + * + * Note that drm_gem_object_release() already calls this function, so drivers + * don't have to take care of releasing the mmap offset themselves when freeing + * the GEM object. */ void drm_gem_free_mmap_offset(struct drm_gem_object *obj) @@ -415,6 +419,9 @@ EXPORT_SYMBOL(drm_gem_free_mmap_offset); * This routine allocates and attaches a fake offset for @obj, in cases where * the virtual size differs from the physical size (ie. obj->size). Otherwise * just use drm_gem_create_mmap_offset(). + * + * This function is idempotent and handles an already allocated mmap offset + * transparently. Drivers do not need to check for this case. */ int drm_gem_create_mmap_offset_size(struct drm_gem_object *obj, size_t size) @@ -436,6 +443,9 @@ EXPORT_SYMBOL(drm_gem_create_mmap_offset_size); * structures. * * This routine allocates and attaches a fake offset for @obj. + * + * Drivers can call drm_gem_free_mmap_offset() before freeing @obj to release + * the fake offset again. */ int drm_gem_create_mmap_offset(struct drm_gem_object *obj) { @@ -754,6 +764,13 @@ drm_gem_release(struct drm_device *dev, struct drm_file *file_private) idr_destroy(&file_private->object_idr); }

+/** + * drm_gem_object_release - release GEM buffer object resources + * @obj: GEM buffer object + * + * This releases any structures and resources used by @obj and is the invers of + * drm_gem_object_init(). + */ void drm_gem_object_release(struct drm_gem_object *obj) { diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 33adc8f8ab20..f10a5d57377e 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1972,9 +1972,6 @@ static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj) struct drm_i915_private *dev_priv = obj->base.dev->dev_private; int ret;

- if (drm_vma_node_has_offset(&obj->base.vma_node)) - return 0; - dev_priv->mm.shrinker_no_lock_stealing = true;

ret = drm_gem_create_mmap_offset(&obj->base); diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h index 2f63dd5e05eb..06ea8e077ec2 100644 --- a/include/drm/drm_vma_manager.h +++ b/include/drm/drm_vma_manager.h @@ -176,19 +176,6 @@ static inline unsigned long drm_vma_node_size(struct drm_vma_offset_node *node) }

/** - * drm_vma_node_has_offset() - Check whether node is added to offset manager - * @node: Node to be checked - * - * RETURNS: - * true iff the node was previously allocated an offset and added to - * an vma offset manager. - */ -static inline bool drm_vma_node_has_offset(struct drm_vma_offset_node *node) -{ - return drm_mm_node_allocated(&node->vm_node); -} - -/** * drm_vma_node_offset_addr() - Return sanitized offset for user-space mmaps * @node: Linked offset node * @@ -220,7 +207,7 @@ static inline __u64 drm_vma_node_offset_addr(struct drm_vma_offset_node *node) static inline void drm_vma_node_unmap(struct drm_vma_offset_node *node, struct address_space *file_mapping) { - if (drm_vma_node_has_offset(node)) + if (drm_mm_node_allocated(&node->vm_node)) unmap_mapping_range(file_mapping, drm_vma_node_offset_addr(node), drm_vma_node_size(node) << PAGE_SHIFT, 1);