It's less than last time around, but still some stragglers. And some fix bugs reported in RH bugzilla even. Either way I'll throw them all into drm-misc if there's no nacks within a week, assuming that this means an implicit ack.
Cheers, Daniel
Daniel Vetter (13): drm/nouveau: Use unlocked gem unreferencing drm/omapdrm: Use unlocked gem unreferencing drm/qxl: Use unlocked gem unreferencing drm/udl: Use unlocked gem unreferencing drm/nouveau: Drop dev->struct_mutex from fbdev init drm/exynos: Drop dev->struct_mutex from mmap offset function drm/exynos: drop struct_mutex from exynos_gem_map_sgt_with_dma drm/exynos: drop struct_mutex from exynos_drm_gem_get_ioctl drm/exynos: drop struct_mutex from fbdev setup drm/vgem: Simplify dum_map drm/vgem: Move get_pages to gem_create drm/vgem: Drop dev->struct_mutex drm/vma_manage: Drop has_offset
drivers/gpu/drm/drm_gem.c | 17 ++++++++++++++ drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 22 +++++++----------- drivers/gpu/drm/exynos/exynos_drm_gem.c | 19 +++------------- drivers/gpu/drm/i915/i915_gem.c | 3 --- drivers/gpu/drm/nouveau/nouveau_display.c | 2 +- drivers/gpu/drm/nouveau/nouveau_fbcon.c | 5 ----- drivers/gpu/drm/omapdrm/omap_fbdev.c | 2 +- drivers/gpu/drm/qxl/qxl_fb.c | 4 ++-- drivers/gpu/drm/udl/udl_fb.c | 2 +- drivers/gpu/drm/udl/udl_gem.c | 2 +- drivers/gpu/drm/vgem/vgem_drv.c | 37 +++++++++---------------------- include/drm/drm_vma_manager.h | 15 +------------ 12 files changed, 46 insertions(+), 84 deletions(-)
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: Ben Skeggs bskeggs@redhat.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/nouveau/nouveau_display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c index 7ce7fa5cb5e6..816342645f42 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.c +++ b/drivers/gpu/drm/nouveau/nouveau_display.c @@ -296,7 +296,7 @@ nouveau_user_framebuffer_create(struct drm_device *dev, err: kfree(nouveau_fb); err_unref: - drm_gem_object_unreference(gem); + drm_gem_object_unreference_unlocked(gem); return ERR_PTR(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: 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 3cb16f0cf381..89da41ac64d2 100644 --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c @@ -153,7 +153,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; }
On 30/03/16 12:40, Daniel Vetter wrote:
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_unreferenceCc: 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 3cb16f0cf381..89da41ac64d2 100644 --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c @@ -153,7 +153,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);
If this is already queued somewhere:
Acked-by: Tomi Valkeinen tomi.valkeinen@ti.com
If not, I can pick this up.
Tomi
On Mon, Apr 18, 2016 at 07:15:04PM +0300, Tomi Valkeinen wrote:
On 30/03/16 12:40, Daniel Vetter wrote:
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_unreferenceCc: 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 3cb16f0cf381..89da41ac64d2 100644 --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c @@ -153,7 +153,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);If this is already queued somewhere:
Acked-by: Tomi Valkeinen tomi.valkeinen@ti.com
If not, I can pick this up.
I've piled all the remaining ones into a branch and will send a pull request to Dave later this week, with this one included.
Thanks, Daniel
Hi Daniel,
Thank you for the patch.
On Wednesday 30 Mar 2016 11:40:41 Daniel Vetter wrote:
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_unreferenceCc: Tomi Valkeinen tomi.valkeinen@ti.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.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 3cb16f0cf381..89da41ac64d2 100644 --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c @@ -153,7 +153,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);
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 33239a2b264a..fd1eb9d03f0b 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -536,7 +536,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; }
On 30.03.2016 11:40, Daniel Vetter wrote:
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_unreferenceCc: 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 33239a2b264a..fd1eb9d03f0b 100644 --- a/drivers/gpu/drm/udl/udl_fb.c +++ b/drivers/gpu/drm/udl/udl_fb.c @@ -536,7 +536,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;
}
Reviewed-by: poma pomidorabelisima@gmail.com
Doesn't protect anything at all.
With this patch nouveau is completely dev->struct_mutex free!
Cc: Ben Skeggs bskeggs@redhat.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/nouveau/nouveau_fbcon.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c index 59f27e774acb..3bae706126bd 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c @@ -386,8 +386,6 @@ nouveau_fbcon_create(struct drm_fb_helper *helper, } }
- mutex_lock(&dev->struct_mutex); - info = drm_fb_helper_alloc_fbi(helper); if (IS_ERR(info)) { ret = PTR_ERR(info); @@ -426,8 +424,6 @@ nouveau_fbcon_create(struct drm_fb_helper *helper,
/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
- mutex_unlock(&dev->struct_mutex); - if (chan) nouveau_fbcon_accel_init(dev); nouveau_fbcon_zfill(dev, fbcon); @@ -441,7 +437,6 @@ nouveau_fbcon_create(struct drm_fb_helper *helper, return 0;
out_unlock: - mutex_unlock(&dev->struct_mutex); if (chan) nouveau_bo_vma_del(nvbo, &fbcon->nouveau_fb.vma); nouveau_bo_unmap(nvbo);
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 2914d62d0d80..3b7209335df0 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -458,8 +458,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 @@ -469,16 +467,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 3b7209335df0..60b9975bb0b1 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -388,16 +388,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(to_dma_dev(drm_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; }
On 30 March 2016 at 10:40, Daniel Vetter daniel.vetter@ffwll.ch wrote:
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 3b7209335df0..60b9975bb0b1 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -388,16 +388,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(to_dma_dev(drm_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;
Either my coffee hasn't kicked in or we have a preexisting bug. Namely - we are returning 0, regardless if we hit the above error ? If that's intentional shouldn't there be a comment explaining why ?
-Emil
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 60b9975bb0b1..6fb98f4c3544 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -362,12 +362,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; }
@@ -376,8 +373,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 4ae860c44f1d..4656cd6e7083 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 c503a840fd88..f3ee1c41da1e 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);
On 30 March 2016 at 10:40, Daniel Vetter daniel.vetter@ffwll.ch wrote:
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
With s/dum_map/dump_map/ in the title this is
Reviewed-by: Emil Velikov emil.l.velikov@gmail.com
-Emil
On 30 March 2016 at 11:29, Emil Velikov emil.l.velikov@gmail.com wrote:
On 30 March 2016 at 10:40, Daniel Vetter daniel.vetter@ffwll.ch wrote:
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
With s/dum_map/dump_map/ in the title this is
Or dumb_map. :)
Cheers, Daniel
vgem doesn't have a shrinker or anything like that and drops backing storage only at object_free time. There's no use in trying to be clever and allocating backing storage delayed, it only causes trouble by requiring locking.
Instead grab pages when we allocate the object right away.
v2: Fix compiling.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/vgem/vgem_drv.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c index f3ee1c41da1e..75f18987411a 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -154,6 +154,10 @@ static struct drm_gem_object *vgem_gem_create(struct drm_device *dev, if (err) goto out;
+ err = vgem_gem_get_pages(obj); + if (err) + goto out; + err = drm_gem_handle_create(file, gem_object, handle); if (err) goto handle_out; @@ -216,16 +220,8 @@ int vgem_gem_dumb_map(struct drm_file *file, struct drm_device *dev,
obj->filp->private_data = obj;
- ret = vgem_gem_get_pages(to_vgem_bo(obj)); - if (ret) - goto fail_get_pages; - *offset = drm_vma_node_offset_addr(&obj->vma_node);
- goto unref; - -fail_get_pages: - drm_gem_free_mmap_offset(obj); unref: drm_gem_object_unreference(obj); unlock:
With the previous two changes it doesn't protect anything any more.
v2: Use _unlocked unreference variant.
v3: Appease gcc noise.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/vgem/vgem_drv.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c index 75f18987411a..ae4de36d1d83 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -89,7 +89,6 @@ int vgem_gem_get_pages(struct drm_vgem_gem_object *obj) static int vgem_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf) { struct drm_vgem_gem_object *obj = vma->vm_private_data; - struct drm_device *dev = obj->base.dev; loff_t num_pages; pgoff_t page_offset; int ret; @@ -103,12 +102,8 @@ static int vgem_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf) if (page_offset > num_pages) return VM_FAULT_SIGBUS;
- mutex_lock(&dev->struct_mutex); - ret = vm_insert_page(vma, (unsigned long)vmf->virtual_address, obj->pages[page_offset]); - - mutex_unlock(&dev->struct_mutex); switch (ret) { case 0: return VM_FAULT_NOPAGE; @@ -205,12 +200,9 @@ int vgem_gem_dumb_map(struct drm_file *file, struct drm_device *dev, int ret = 0; struct drm_gem_object *obj;
- mutex_lock(&dev->struct_mutex); obj = drm_gem_object_lookup(dev, file, handle); - if (!obj) { - ret = -ENOENT; - goto unlock; - } + if (!obj) + return -ENOENT;
ret = drm_gem_create_mmap_offset(obj); if (ret) @@ -223,9 +215,8 @@ int vgem_gem_dumb_map(struct drm_file *file, struct drm_device *dev, *offset = drm_vma_node_offset_addr(&obj->vma_node);
unref: - drm_gem_object_unreference(obj); -unlock: - mutex_unlock(&dev->struct_mutex); + drm_gem_object_unreference_unlocked(obj); + return ret; }
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 2e8c77e71e1f..74867edb8739 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -422,6 +422,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) @@ -445,6 +449,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) @@ -466,6 +473,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) { @@ -759,6 +769,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 c7a997aeb33f..3bc56166dd0d 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2031,9 +2031,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);
dri-devel@lists.freedesktop.org
-
Daniel Stone -
Daniel Vetter -
Daniel Vetter -
Emil Velikov -
Laurent Pinchart -
poma -
Tomi Valkeinen