Hi all,
This patch series is my 2nd real stab at fixing up the locking issues around our two buffer sharing mechanisms in gem: flink and prime.
I think the approach taken here is much better than my first stab, and it also seems to no longer leak buffers ;-) There some assorted cleanup and prep work (and one i915 fix) thrown into the mix, it's all stuff I've stumbled over while digging through the code.
Open issues left in prime-land after these patches: - exynos probably wants a similar patch to "drm/i915: explicit store base gem object in dma_buf->priv". The current code should be correct, but it's a bit tricky. I've opted not to do that since last time around I've touched exynos a bit it broke horribly ;-) - The prime core should now no longer depend upon obj->import_attach being set by drivers in their prime_import callback. This should allos us to fix udl which really doesn't need (nor want, it confuses swiotlb among other things) a device attachment. Didn't write that patch since my displaylink seems to have died. - There's still the issue Inki's team pointed out where if you import a foreign object on different fds you'll get different gem objects. So we need some form of a per-device import cache (on top of the per-file-priv dma-buf cache we already have). Didn't do this yet since I want to have good test coverage (already started a bit), it looks like a bit more work and I'm not sure about the exact design of the code yet.
Review and testing highly welcome.
Cheers, Daniel
Daniel Vetter (20): drm: use common drm_gem_dmabuf_release in i915/exynos drivers drm/i915: unpin backing storage in dmabuf_unmap drm/i915: explicit store base gem object in dma_buf->priv drm/prime: add a bit of documentation about gem_obj->import_attach drm/gem: remove drm_gem_object_handle_unreference drm/gem: inline drm_gem_object_handle_reference drm/gem: move drm_gem_object_handle_unreference_unlocked into drm_gem.c drm/gem: remove bogus NULL check from drm_gem_object_handle_unreference_unlocked drm/gem: WARN about unbalanced handle refcounts drm/gem: fix up flink name create race drm/prime: fix error path in drm_gem_prime_fd_to_handle drm/gem: make drm_gem_object_handle_unreference_unlocked static drm/gem: create drm_gem_dumb_destroy drm/prime: use proper pointer in drm_gem_prime_handle_to_fd drm/prime: shrink critical section protected by prime lock drm/prime: clarify logic a bit in drm_gem_prime_fd_to_handle drm/gem: switch dev->object_name_lock to a mutex drm/gem: completely close gem_open vs. gem_close races drm/prime: proper locking+refcounting for obj->dma_buf link drm/prime: Simplify drm_gem_remove_prime_handles
drivers/gpu/drm/ast/ast_drv.c | 2 +- drivers/gpu/drm/ast/ast_drv.h | 3 - drivers/gpu/drm/ast/ast_main.c | 7 -- drivers/gpu/drm/cirrus/cirrus_drv.c | 2 +- drivers/gpu/drm/cirrus/cirrus_drv.h | 3 - drivers/gpu/drm/cirrus/cirrus_main.c | 7 -- drivers/gpu/drm/drm_fops.c | 1 + drivers/gpu/drm/drm_gem.c | 192 ++++++++++++++++++++--------- drivers/gpu/drm/drm_gem_cma_helper.c | 10 -- drivers/gpu/drm/drm_info.c | 2 +- drivers/gpu/drm/drm_prime.c | 96 ++++++++++----- drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 23 +--- drivers/gpu/drm/exynos/exynos_drm_drv.c | 2 +- drivers/gpu/drm/exynos/exynos_drm_gem.c | 22 +--- drivers/gpu/drm/exynos/exynos_drm_gem.h | 9 -- drivers/gpu/drm/gma500/gem.c | 17 --- drivers/gpu/drm/gma500/psb_drv.c | 2 +- drivers/gpu/drm/gma500/psb_drv.h | 2 - drivers/gpu/drm/i915/i915_drv.c | 2 +- drivers/gpu/drm/i915/i915_drv.h | 2 - drivers/gpu/drm/i915/i915_gem.c | 7 -- drivers/gpu/drm/i915/i915_gem_dmabuf.c | 34 +++-- drivers/gpu/drm/mgag200/mgag200_drv.c | 2 +- drivers/gpu/drm/mgag200/mgag200_drv.h | 3 - drivers/gpu/drm/mgag200/mgag200_main.c | 7 -- drivers/gpu/drm/nouveau/nouveau_display.c | 7 -- drivers/gpu/drm/nouveau/nouveau_display.h | 2 - drivers/gpu/drm/nouveau/nouveau_drm.c | 2 +- drivers/gpu/drm/omapdrm/omap_drv.c | 2 +- drivers/gpu/drm/omapdrm/omap_drv.h | 2 - drivers/gpu/drm/omapdrm/omap_gem.c | 15 --- drivers/gpu/drm/qxl/qxl_drv.c | 2 +- drivers/gpu/drm/qxl/qxl_drv.h | 3 - drivers/gpu/drm/qxl/qxl_dumb.c | 7 -- drivers/gpu/drm/radeon/radeon.h | 3 - drivers/gpu/drm/radeon/radeon_drv.c | 5 +- drivers/gpu/drm/radeon/radeon_gem.c | 7 -- drivers/gpu/drm/rcar-du/rcar_du_drv.c | 2 +- drivers/gpu/drm/shmobile/shmob_drm_drv.c | 2 +- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 +- drivers/gpu/drm/udl/udl_drv.c | 2 +- drivers/gpu/drm/udl/udl_drv.h | 2 - drivers/gpu/drm/udl/udl_gem.c | 6 - drivers/gpu/host1x/drm/drm.c | 2 +- drivers/gpu/host1x/drm/gem.c | 6 - drivers/gpu/host1x/drm/gem.h | 2 - drivers/staging/imx-drm/imx-drm-core.c | 2 +- include/drm/drmP.h | 94 +++++++------- include/drm/drm_gem_cma_helper.h | 8 -- 49 files changed, 279 insertions(+), 367 deletions(-)
Note that this is slightly tricky since both drivers store their native objects in dma_buf->priv. But both also embed the base drm_gem_object at the first position, so the implicit cast is ok.
To use the release helper we need to export it, too.
Cc: Inki Dae inki.dae@samsung.com Cc: Intel Graphics Development intel-gfx@lists.freedesktop.org Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_prime.c | 3 ++- drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 23 +---------------------- drivers/gpu/drm/i915/i915_gem_dmabuf.c | 13 +------------ include/drm/drmP.h | 1 + 4 files changed, 5 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 85e450e..a35f206 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -192,7 +192,7 @@ static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach, /* nothing to be done here */ }
-static void drm_gem_dmabuf_release(struct dma_buf *dma_buf) +void drm_gem_dmabuf_release(struct dma_buf *dma_buf) { struct drm_gem_object *obj = dma_buf->priv;
@@ -202,6 +202,7 @@ static void drm_gem_dmabuf_release(struct dma_buf *dma_buf) drm_gem_object_unreference_unlocked(obj); } } +EXPORT_SYMBOL(drm_gem_dmabuf_release);
static void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf) { diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c index a0f997e..3cd56e1 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c @@ -127,27 +127,6 @@ static void exynos_gem_unmap_dma_buf(struct dma_buf_attachment *attach, /* Nothing to do. */ }
-static void exynos_dmabuf_release(struct dma_buf *dmabuf) -{ - struct exynos_drm_gem_obj *exynos_gem_obj = dmabuf->priv; - - /* - * exynos_dmabuf_release() call means that file object's - * f_count is 0 and it calls drm_gem_object_handle_unreference() - * to drop the references that these values had been increased - * at drm_prime_handle_to_fd() - */ - if (exynos_gem_obj->base.export_dma_buf == dmabuf) { - exynos_gem_obj->base.export_dma_buf = NULL; - - /* - * drop this gem object refcount to release allocated buffer - * and resources. - */ - drm_gem_object_unreference_unlocked(&exynos_gem_obj->base); - } -} - static void *exynos_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf, unsigned long page_num) { @@ -193,7 +172,7 @@ static struct dma_buf_ops exynos_dmabuf_ops = { .kunmap = exynos_gem_dmabuf_kunmap, .kunmap_atomic = exynos_gem_dmabuf_kunmap_atomic, .mmap = exynos_gem_dmabuf_mmap, - .release = exynos_dmabuf_release, + .release = drm_gem_dmabuf_release, };
struct dma_buf *exynos_dmabuf_prime_export(struct drm_device *drm_dev, diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index dc53a52..2e82286 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -90,17 +90,6 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, kfree(sg); }
-static void i915_gem_dmabuf_release(struct dma_buf *dma_buf) -{ - struct drm_i915_gem_object *obj = dma_buf->priv; - - if (obj->base.export_dma_buf == dma_buf) { - /* drop the reference on the export fd holds */ - obj->base.export_dma_buf = NULL; - drm_gem_object_unreference_unlocked(&obj->base); - } -} - static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf) { struct drm_i915_gem_object *obj = dma_buf->priv; @@ -211,7 +200,7 @@ static int i915_gem_begin_cpu_access(struct dma_buf *dma_buf, size_t start, size static const struct dma_buf_ops i915_dmabuf_ops = { .map_dma_buf = i915_gem_map_dma_buf, .unmap_dma_buf = i915_gem_unmap_dma_buf, - .release = i915_gem_dmabuf_release, + .release = drm_gem_dmabuf_release, .kmap = i915_gem_dmabuf_kmap, .kmap_atomic = i915_gem_dmabuf_kmap_atomic, .kunmap = i915_gem_dmabuf_kunmap, diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 12083dc..cd57646 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1550,6 +1550,7 @@ extern struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev, struct dma_buf *dma_buf); extern int drm_gem_prime_fd_to_handle(struct drm_device *dev, struct drm_file *file_priv, int prime_fd, uint32_t *handle); +extern void drm_gem_dmabuf_release(struct dma_buf *dma_buf);
extern int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv);
This fixes a WARN in i915_gem_free_object when the obj->pages_pin_count isn't 0.
Reported-by: Maarten Lankhorst <maarten.lankhorst@canonical.com Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/i915_gem_dmabuf.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index 2e82286..5cec8b0 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -85,9 +85,13 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, struct sg_table *sg, enum dma_data_direction dir) { + struct drm_i915_gem_object *obj = attachment->dmabuf->priv; + dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir); sg_free_table(sg); kfree(sg); + + i915_gem_object_unpin_pages(obj); }
static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf)
Makes it more obviously correct what tricks we play by reusing the drm prime release helper.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/i915_gem_dmabuf.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index 5cec8b0..cd5ee4a 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -27,10 +27,15 @@ #include "i915_drv.h" #include <linux/dma-buf.h>
+static struct drm_i915_gem_object *dma_buf_to_obj(struct dma_buf *buf) +{ + return to_intel_bo(buf->priv); +} + static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachment, enum dma_data_direction dir) { - struct drm_i915_gem_object *obj = attachment->dmabuf->priv; + struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment->dmabuf); struct sg_table *st; struct scatterlist *src, *dst; int ret, i; @@ -85,7 +90,7 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, struct sg_table *sg, enum dma_data_direction dir) { - struct drm_i915_gem_object *obj = attachment->dmabuf->priv; + struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment->dmabuf);
dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir); sg_free_table(sg); @@ -96,7 +101,7 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment,
static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf) { - struct drm_i915_gem_object *obj = dma_buf->priv; + struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf); struct drm_device *dev = obj->base.dev; struct sg_page_iter sg_iter; struct page **pages; @@ -144,7 +149,7 @@ error:
static void i915_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr) { - struct drm_i915_gem_object *obj = dma_buf->priv; + struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf); struct drm_device *dev = obj->base.dev; int ret;
@@ -187,7 +192,7 @@ static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *
static int i915_gem_begin_cpu_access(struct dma_buf *dma_buf, size_t start, size_t length, enum dma_data_direction direction) { - struct drm_i915_gem_object *obj = dma_buf->priv; + struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf); struct drm_device *dev = obj->base.dev; int ret; bool write = (direction == DMA_BIDIRECTIONAL || direction == DMA_TO_DEVICE); @@ -218,9 +223,7 @@ static const struct dma_buf_ops i915_dmabuf_ops = { struct dma_buf *i915_gem_prime_export(struct drm_device *dev, struct drm_gem_object *gem_obj, int flags) { - struct drm_i915_gem_object *obj = to_intel_bo(gem_obj); - - return dma_buf_export(obj, &i915_dmabuf_ops, obj->base.size, flags); + return dma_buf_export(gem_obj, &i915_dmabuf_ops, gem_obj->size, flags); }
static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj) @@ -257,7 +260,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
/* is this one of own objects? */ if (dma_buf->ops == &i915_dmabuf_ops) { - obj = dma_buf->priv; + obj = dma_buf_to_obj(dma_buf); /* is it from our device? */ if (obj->base.dev == dev) { /*
Lifetime rules seem to be solid around ->import_attach. So this patch just properly documents them.
Note that pointing directly at the attachment might have issues for devices that have multiple struct device *dev parts constituting the logical gpu and so might need multiple attachment points. Similarly for drm devices which don't need a dma attachment at all (like udl).
But fixing that up is material for different patches.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- include/drm/drmP.h | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index cd57646..baf030c 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -681,7 +681,16 @@ struct drm_gem_object { /* dma buf exported from this GEM object */ struct dma_buf *export_dma_buf;
- /* dma buf attachment backing this object */ + /** + * import_attach - dma buf attachment backing this object + * + * Any foreign dma_buf imported as a gem object has this set to the + * attachment point for the device. This is invariant over the lifetime + * of a gem object. + * + * The driver's ->gem_free_object callback is responsible for cleaning + * up the dma_buf attachment and references acquired at import time. + */ struct dma_buf_attachment *import_attach; };
On Tue, Jul 16, 2013 at 3:11 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
yeah, with msm drm driver, that might become (eventually) an issue. (But not yet, because they seem to use iommu<->dev attachment in a.. slightly different way)
But fixing that up is material for different patches.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
Reviewed-by: Rob Clark robdclark@gmail.com
It's unused, everyone is using the _unlocked variant only.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- include/drm/drmP.h | 18 ------------------ 1 file changed, 18 deletions(-)
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index baf030c..f949cb2 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1671,24 +1671,6 @@ drm_gem_object_handle_reference(struct drm_gem_object *obj) }
static inline void -drm_gem_object_handle_unreference(struct drm_gem_object *obj) -{ - if (obj == NULL) - return; - - if (atomic_read(&obj->handle_count) == 0) - return; - /* - * Must bump handle count first as this may be the last - * ref, in which case the object would disappear before we - * checked for a name - */ - if (atomic_dec_and_test(&obj->handle_count)) - drm_gem_object_handle_free(obj); - drm_gem_object_unreference(obj); -} - -static inline void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) { if (obj == NULL)
On Tue, Jul 16, 2013 at 3:11 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
It's unused, everyone is using the _unlocked variant only.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
Reviewed-by: Rob Clark robdclark@gmail.com
Only one callsite and since ->handle_count is not a simple reference count (it can resurrect) it's imo better to be explicit about things than hide the refcount dance.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_gem.c | 3 ++- include/drm/drmP.h | 7 ------- 2 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 603f256..7bcd851 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -280,7 +280,8 @@ drm_gem_handle_create(struct drm_file *file_priv, return ret; *handlep = ret;
- drm_gem_object_handle_reference(obj); + drm_gem_object_reference(obj); + atomic_inc(&obj->handle_count);
if (dev->driver->gem_open_object) { ret = dev->driver->gem_open_object(obj, file_priv); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index f949cb2..114db57 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1664,13 +1664,6 @@ int drm_gem_handle_create(struct drm_file *file_priv, int drm_gem_handle_delete(struct drm_file *filp, u32 handle);
static inline void -drm_gem_object_handle_reference(struct drm_gem_object *obj) -{ - drm_gem_object_reference(obj); - atomic_inc(&obj->handle_count); -} - -static inline void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) { if (obj == NULL)
On Tue, Jul 16, 2013 at 3:11 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
I'm not really sure I like this one.. I guess it could be that I'm just used to the handle-ref stuff, so it doesn't seem odd not-inlined. And it does seem kinda odd / unsymmetric to have an unref w/out a ref. I guess I kinda like the bikeshed's current color in this case.
BR, -R
On Tue, Jul 23, 2013 at 2:07 PM, Rob Clark robdclark@gmail.com wrote:
I generally agree but in this case a follow-up patch ("drm/gem: fix up flink name create race") will change obj->handle_count from an atomic_t to a normal int protected by the dev->object_name_lock spinlock. To avoid races we need to hold that spinlock over a few different instructions, so with the refcounting dance inlined it's much more obvious that that obj->handle_count is always correctly protected imo. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Tue, Jul 23, 2013 at 8:31 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
hmm, ok.. then I'll reserve judgment until I get further through the series ;-)
if you do spin another version of the series, it could be worth mentioning this in the commit msg
BR, -R
On Tue, Jul 23, 2013 at 10:43 PM, Rob Clark robdclark@gmail.com wrote:
Just merge this into that series or patch,
deinlining it now it just noise.
Dave.
On Wed, Jul 24, 2013 at 2:00 AM, Dave Airlie airlied@gmail.com wrote:
Yeah, I'll do so. I've started with a few refactoring patches to get a clearer picture (since I've implemented&dismissed a few approaches to fix the flink race until I've settled on this one here), but now this patch here really looks a bit silly ;-) -Danie -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
We have three callers of this function now and it's neither performance critical nor really small. So an inline function feels like overkill and unecessarily separates the different parts of the code.
Since all callers of drm_gem_object_handle_free are now in drm_gem.c we can make that static (and remove the unused EXPORT_SYMBOL). To avoid a forward declaration move it (and drm_gem_object_free_bug) up a bit.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_gem.c | 89 ++++++++++++++++++++++++++++------------------- include/drm/drmP.h | 21 +---------- 2 files changed, 55 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 7bcd851..e53d1c9 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -210,6 +210,60 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp) } }
+static void drm_gem_object_ref_bug(struct kref *list_kref) +{ + BUG(); +} + +/** + * Called after the last handle to the object has been closed + * + * Removes any name for the object. Note that this must be + * called before drm_gem_object_free or we'll be touching + * freed memory + */ +static void drm_gem_object_handle_free(struct drm_gem_object *obj) +{ + struct drm_device *dev = obj->dev; + + /* Remove any name for this object */ + spin_lock(&dev->object_name_lock); + if (obj->name) { + idr_remove(&dev->object_name_idr, obj->name); + obj->name = 0; + spin_unlock(&dev->object_name_lock); + /* + * The object name held a reference to this object, drop + * that now. + * + * This cannot be the last reference, since the handle holds one too. + */ + kref_put(&obj->refcount, drm_gem_object_ref_bug); + } else + spin_unlock(&dev->object_name_lock); + +} + +void +drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) +{ + if (obj == NULL) + return; + + if (atomic_read(&obj->handle_count) == 0) + return; + + /* + * Must bump handle count first as this may be the last + * ref, in which case the object would disappear before we + * checked for a name + */ + + if (atomic_dec_and_test(&obj->handle_count)) + drm_gem_object_handle_free(obj); + drm_gem_object_unreference_unlocked(obj); +} + /** * Removes the mapping from handle to filp for this object. */ @@ -578,41 +632,6 @@ drm_gem_object_free(struct kref *kref) } EXPORT_SYMBOL(drm_gem_object_free);
-static void drm_gem_object_ref_bug(struct kref *list_kref) -{ - BUG(); -} - -/** - * Called after the last handle to the object has been closed - * - * Removes any name for the object. Note that this must be - * called before drm_gem_object_free or we'll be touching - * freed memory - */ -void drm_gem_object_handle_free(struct drm_gem_object *obj) -{ - struct drm_device *dev = obj->dev; - - /* Remove any name for this object */ - spin_lock(&dev->object_name_lock); - if (obj->name) { - idr_remove(&dev->object_name_idr, obj->name); - obj->name = 0; - spin_unlock(&dev->object_name_lock); - /* - * The object name held a reference to this object, drop - * that now. - * - * This cannot be the last reference, since the handle holds one too. - */ - kref_put(&obj->refcount, drm_gem_object_ref_bug); - } else - spin_unlock(&dev->object_name_lock); - -} -EXPORT_SYMBOL(drm_gem_object_handle_free); - void drm_gem_vm_open(struct vm_area_struct *vma) { struct drm_gem_object *obj = vma->vm_private_data; diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 114db57..2fb83b4 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1625,7 +1625,6 @@ int drm_gem_object_init(struct drm_device *dev, struct drm_gem_object *obj, size_t size); int drm_gem_private_object_init(struct drm_device *dev, struct drm_gem_object *obj, size_t size); -void drm_gem_object_handle_free(struct drm_gem_object *obj); void drm_gem_vm_open(struct vm_area_struct *vma); void drm_gem_vm_close(struct vm_area_struct *vma); int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, @@ -1663,25 +1662,7 @@ int drm_gem_handle_create(struct drm_file *file_priv, u32 *handlep); int drm_gem_handle_delete(struct drm_file *filp, u32 handle);
-static inline void -drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) -{ - if (obj == NULL) - return; - - if (atomic_read(&obj->handle_count) == 0) - return; - - /* - * Must bump handle count first as this may be the last - * ref, in which case the object would disappear before we - * checked for a name - */ - - if (atomic_dec_and_test(&obj->handle_count)) - drm_gem_object_handle_free(obj); - drm_gem_object_unreference_unlocked(obj); -} +void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj);
void drm_gem_free_mmap_offset(struct drm_gem_object *obj); int drm_gem_create_mmap_offset(struct drm_gem_object *obj);
Calling this function with a NULL object is simply a bug, so papering over a NULL object not a good idea.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_gem.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index e53d1c9..5d41af1 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -247,9 +247,6 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj) void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) { - if (obj == NULL) - return; - if (atomic_read(&obj->handle_count) == 0) return;
Trying to drop a reference we don't have is a pretty serious bug. Trying to paper over it is an even worse offense.
So scream into dmesg with a big WARN in case that ever happens.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_gem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 5d41af1..b07519e 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -247,7 +247,7 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj) void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) { - if (atomic_read(&obj->handle_count) == 0) + if (WARN_ON(atomic_read(&obj->handle_count) == 0)) return;
/*
This is the 2nd attempt, I've always been a bit dissatisified with the tricky nature of the first one:
http://lists.freedesktop.org/archives/dri-devel/2012-July/025451.html
The issue is that the flink ioctl can race with calling gem_close on the last gem handle. In that case we'll end up with a zero handle count, but an flink name (and it's corresponding reference). Which results in a neat space leak.
In my first attempt I've solved this by rechecking the handle count. But fundamentally the issue is that ->handle_count isn't your usual refcount - it can be resurrected from 0 among other things.
For those special beasts atomic_t often suggest way more ordering that it actually guarantees. To prevent being tricked by those hairy semantics take the easy way out and simply protect the handle with the existing dev->object_name_lock.
With that change implemented it's dead easy to fix the flink vs. gem close reace: When we try to create the name we simply have to check whether there's still officially a gem handle around and if not refuse to create the flink name. Since the handle count decrement and flink name destruction is now also protected by that lock the reace is gone and we can't ever leak the flink reference again.
Outside of the drm core only the exynos driver looks at the handle count, and tbh I have no idea why (it's just for debug dmesg output luckily).
I've considered inlining the drm_gem_object_handle_free, but I plan to add more name-like things (like the exported dma_buf) to this scheme, so it's clearer to leave the handle freeing in its own function.
v2: Fix up the error path handling in handle_create and make it more robust by simply calling object_handle_unreference.
v3: Fix up the handle_unreference logic bug - atomic_dec_and_test retursn 1 for 0. Oops.
Cc: Inki Dae inki.dae@samsung.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_gem.c | 34 ++++++++++++++++++++------------- drivers/gpu/drm/drm_info.c | 2 +- drivers/gpu/drm/exynos/exynos_drm_gem.c | 2 +- include/drm/drmP.h | 12 ++++++++++-- 4 files changed, 33 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index b07519e..14c70b5 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -140,7 +140,7 @@ int drm_gem_object_init(struct drm_device *dev, return PTR_ERR(obj->filp);
kref_init(&obj->refcount); - atomic_set(&obj->handle_count, 0); + obj->handle_count = 0; obj->size = size;
return 0; @@ -161,7 +161,7 @@ int drm_gem_private_object_init(struct drm_device *dev, obj->filp = NULL;
kref_init(&obj->refcount); - atomic_set(&obj->handle_count, 0); + obj->handle_count = 0; obj->size = size;
return 0; @@ -227,11 +227,9 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj) struct drm_device *dev = obj->dev;
/* Remove any name for this object */ - spin_lock(&dev->object_name_lock); if (obj->name) { idr_remove(&dev->object_name_idr, obj->name); obj->name = 0; - spin_unlock(&dev->object_name_lock); /* * The object name held a reference to this object, drop * that now. @@ -239,15 +237,13 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj) * This cannot be the last reference, since the handle holds one too. */ kref_put(&obj->refcount, drm_gem_object_ref_bug); - } else - spin_unlock(&dev->object_name_lock); - + } }
void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) { - if (WARN_ON(atomic_read(&obj->handle_count) == 0)) + if (WARN_ON(obj->handle_count == 0)) return;
/* @@ -256,8 +252,11 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) * checked for a name */
- if (atomic_dec_and_test(&obj->handle_count)) + spin_lock(&obj->dev->object_name_lock); + if (--obj->handle_count == 0) drm_gem_object_handle_free(obj); + spin_unlock(&obj->dev->object_name_lock); + drm_gem_object_unreference_unlocked(obj); }
@@ -321,18 +320,21 @@ drm_gem_handle_create(struct drm_file *file_priv, * allocation under our spinlock. */ idr_preload(GFP_KERNEL); + spin_lock(&dev->object_name_lock); spin_lock(&file_priv->table_lock);
ret = idr_alloc(&file_priv->object_idr, obj, 1, 0, GFP_NOWAIT); - + drm_gem_object_reference(obj); + obj->handle_count++; spin_unlock(&file_priv->table_lock); + spin_unlock(&dev->object_name_lock); idr_preload_end(); - if (ret < 0) + if (ret < 0) { + drm_gem_object_handle_unreference_unlocked(obj); return ret; + } *handlep = ret;
- drm_gem_object_reference(obj); - atomic_inc(&obj->handle_count);
if (dev->driver->gem_open_object) { ret = dev->driver->gem_open_object(obj, file_priv); @@ -499,6 +501,12 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data,
idr_preload(GFP_KERNEL); spin_lock(&dev->object_name_lock); + /* prevent races with concurrent gem_close. */ + if (obj->handle_count == 0) { + ret = -ENOENT; + goto err; + } + if (!obj->name) { ret = idr_alloc(&dev->object_name_idr, obj, 1, 0, GFP_NOWAIT); if (ret < 0) diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c index d4b20ce..f4b348c 100644 --- a/drivers/gpu/drm/drm_info.c +++ b/drivers/gpu/drm/drm_info.c @@ -207,7 +207,7 @@ static int drm_gem_one_name_info(int id, void *ptr, void *data)
seq_printf(m, "%6d %8zd %7d %8d\n", obj->name, obj->size, - atomic_read(&obj->handle_count), + obj->handle_count, atomic_read(&obj->refcount.refcount)); return 0; } diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c index 24c22a8..16963ca 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -135,7 +135,7 @@ void exynos_drm_gem_destroy(struct exynos_drm_gem_obj *exynos_gem_obj) obj = &exynos_gem_obj->base; buf = exynos_gem_obj->buffer;
- DRM_DEBUG_KMS("handle count = %d\n", atomic_read(&obj->handle_count)); + DRM_DEBUG_KMS("handle count = %d\n", obj->handle_count);
/* * do not release memory region from exporter. diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 2fb83b4..25da8e0 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -634,8 +634,16 @@ struct drm_gem_object { /** Reference count of this object */ struct kref refcount;
- /** Handle count of this object. Each handle also holds a reference */ - atomic_t handle_count; /* number of handles on this object */ + /** + * handle_count - gem file_priv handle count of this object + * + * Each handle also holds a reference. Note that when the handle_count + * drops to 0 any global names (e.g. the id in the flink namespace) will + * be cleared. + * + * Protected by dev->object_name_lock. + * */ + unsigned handle_count;
/** Related drm device */ struct drm_device *dev;
Hi
On Tue, Jul 16, 2013 at 9:12 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
If you inline this here, you can actually drop the huge comment for "caller still holds reference" as you call "object_unreference()" below, anyway. And with this patch, gem_object_handle_free() is pretty small, anyway.
I don't actually understand what you try to say in the commit message about new name-like stuff, but if you reuse it, it's fine.
The locking order isn't really documented. What's wrong with:
idr_preload(GFP_KERNEL); spin_lock(&file_priv->table_lock); ret = idr_alloc(&file_priv->object_idr, obj, 1, 0, GFP_NOWAIT); spin_unlock(&file_priv->table_lock); idr_preload_end();
if (ret < 0) return ret;
spin_lock(&dev->object_name_lock); obj->handle_count++; spin_unlock(&dev->object_name_lock); drm_gem_object_reference(obj);
This is safe against flink() as we don't care whether flink() fails if user-space isn't even aware of the handle, yet. And if user-space already has a handle, then "handle_count" is >0, anyway.
And gem_object_handle_unreference can only be called if another handle exists (thus, handle_count > 0).
Or am I missing something?
spin_unlock(&dev->object_name_lock); ?
Aside from the spin_unlock(), it's all a matter of taste, so: Reviewed-by: David Herrmann dh.herrmann@gmail.com
Cheers David
On Wed, Jul 17, 2013 at 06:38:35PM +0200, David Herrmann wrote:
Later patches will add a 2nd function call here to clean up dma-buf referenes (that's the name-like stuff), so I've figured inlining actually reduces code-readability in the end. Hence why I don't do this here.
At this spot a 2nd thread could sneak in with a gem_close (handle names are easily guessable) which drops the handle reference and removes the handle before we've fully set things up. End result is that we leak a reference (since we decrement from 0 to -1 so won't treat it as the last unref), and the handle_count++ later on here restores it to 0. But the reference won't ever get cleaned up again.
Hence we need to protect the entire section from concurrent gem_close calls.
See above, userspace can sneak in before we've actually incremented handle_count.
Oops, will fix.
This is the 2nd attempt, I've always been a bit dissatisified with the tricky nature of the first one:
http://lists.freedesktop.org/archives/dri-devel/2012-July/025451.html
The issue is that the flink ioctl can race with calling gem_close on the last gem handle. In that case we'll end up with a zero handle count, but an flink name (and it's corresponding reference). Which results in a neat space leak.
In my first attempt I've solved this by rechecking the handle count. But fundamentally the issue is that ->handle_count isn't your usual refcount - it can be resurrected from 0 among other things.
For those special beasts atomic_t often suggest way more ordering that it actually guarantees. To prevent being tricked by those hairy semantics take the easy way out and simply protect the handle with the existing dev->object_name_lock.
With that change implemented it's dead easy to fix the flink vs. gem close reace: When we try to create the name we simply have to check whether there's still officially a gem handle around and if not refuse to create the flink name. Since the handle count decrement and flink name destruction is now also protected by that lock the reace is gone and we can't ever leak the flink reference again.
Outside of the drm core only the exynos driver looks at the handle count, and tbh I have no idea why (it's just for debug dmesg output luckily).
I've considered inlining the drm_gem_object_handle_free, but I plan to add more name-like things (like the exported dma_buf) to this scheme, so it's clearer to leave the handle freeing in its own function.
This is exercised by the new gem_flink_race i-g-t testcase, which on my snb leaks gem objects at a rate of roughly 1k objects/s.
v2: Fix up the error path handling in handle_create and make it more robust by simply calling object_handle_unreference.
v3: Fix up the handle_unreference logic bug - atomic_dec_and_test retursn 1 for 0. Oops.
v4: Squash in inlining of drm_gem_object_handle_reference as suggested by Dave Airlie and add a note that we now have a testcase.
Cc: Dave Airlie airlied@gmail.com Cc: Inki Dae inki.dae@samsung.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_gem.c | 33 +++++++++++++++++++++------------ drivers/gpu/drm/drm_info.c | 2 +- drivers/gpu/drm/exynos/exynos_drm_gem.c | 2 +- include/drm/drmP.h | 19 ++++++++++--------- 4 files changed, 33 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index e7d2b7f..14c70b5 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -140,7 +140,7 @@ int drm_gem_object_init(struct drm_device *dev, return PTR_ERR(obj->filp);
kref_init(&obj->refcount); - atomic_set(&obj->handle_count, 0); + obj->handle_count = 0; obj->size = size;
return 0; @@ -161,7 +161,7 @@ int drm_gem_private_object_init(struct drm_device *dev, obj->filp = NULL;
kref_init(&obj->refcount); - atomic_set(&obj->handle_count, 0); + obj->handle_count = 0; obj->size = size;
return 0; @@ -227,11 +227,9 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj) struct drm_device *dev = obj->dev;
/* Remove any name for this object */ - spin_lock(&dev->object_name_lock); if (obj->name) { idr_remove(&dev->object_name_idr, obj->name); obj->name = 0; - spin_unlock(&dev->object_name_lock); /* * The object name held a reference to this object, drop * that now. @@ -239,15 +237,13 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj) * This cannot be the last reference, since the handle holds one too. */ kref_put(&obj->refcount, drm_gem_object_ref_bug); - } else - spin_unlock(&dev->object_name_lock); - + } }
void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) { - if (WARN_ON(atomic_read(&obj->handle_count) == 0)) + if (WARN_ON(obj->handle_count == 0)) return;
/* @@ -256,8 +252,11 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) * checked for a name */
- if (atomic_dec_and_test(&obj->handle_count)) + spin_lock(&obj->dev->object_name_lock); + if (--obj->handle_count == 0) drm_gem_object_handle_free(obj); + spin_unlock(&obj->dev->object_name_lock); + drm_gem_object_unreference_unlocked(obj); }
@@ -321,17 +320,21 @@ drm_gem_handle_create(struct drm_file *file_priv, * allocation under our spinlock. */ idr_preload(GFP_KERNEL); + spin_lock(&dev->object_name_lock); spin_lock(&file_priv->table_lock);
ret = idr_alloc(&file_priv->object_idr, obj, 1, 0, GFP_NOWAIT); - + drm_gem_object_reference(obj); + obj->handle_count++; spin_unlock(&file_priv->table_lock); + spin_unlock(&dev->object_name_lock); idr_preload_end(); - if (ret < 0) + if (ret < 0) { + drm_gem_object_handle_unreference_unlocked(obj); return ret; + } *handlep = ret;
- drm_gem_object_handle_reference(obj);
if (dev->driver->gem_open_object) { ret = dev->driver->gem_open_object(obj, file_priv); @@ -498,6 +501,12 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data,
idr_preload(GFP_KERNEL); spin_lock(&dev->object_name_lock); + /* prevent races with concurrent gem_close. */ + if (obj->handle_count == 0) { + ret = -ENOENT; + goto err; + } + if (!obj->name) { ret = idr_alloc(&dev->object_name_idr, obj, 1, 0, GFP_NOWAIT); if (ret < 0) diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c index d4b20ce..f4b348c 100644 --- a/drivers/gpu/drm/drm_info.c +++ b/drivers/gpu/drm/drm_info.c @@ -207,7 +207,7 @@ static int drm_gem_one_name_info(int id, void *ptr, void *data)
seq_printf(m, "%6d %8zd %7d %8d\n", obj->name, obj->size, - atomic_read(&obj->handle_count), + obj->handle_count, atomic_read(&obj->refcount.refcount)); return 0; } diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c index 24c22a8..16963ca 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -135,7 +135,7 @@ void exynos_drm_gem_destroy(struct exynos_drm_gem_obj *exynos_gem_obj) obj = &exynos_gem_obj->base; buf = exynos_gem_obj->buffer;
- DRM_DEBUG_KMS("handle count = %d\n", atomic_read(&obj->handle_count)); + DRM_DEBUG_KMS("handle count = %d\n", obj->handle_count);
/* * do not release memory region from exporter. diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 301a6d6..25da8e0 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -634,8 +634,16 @@ struct drm_gem_object { /** Reference count of this object */ struct kref refcount;
- /** Handle count of this object. Each handle also holds a reference */ - atomic_t handle_count; /* number of handles on this object */ + /** + * handle_count - gem file_priv handle count of this object + * + * Each handle also holds a reference. Note that when the handle_count + * drops to 0 any global names (e.g. the id in the flink namespace) will + * be cleared. + * + * Protected by dev->object_name_lock. + * */ + unsigned handle_count;
/** Related drm device */ struct drm_device *dev; @@ -1662,13 +1670,6 @@ int drm_gem_handle_create(struct drm_file *file_priv, u32 *handlep); int drm_gem_handle_delete(struct drm_file *filp, u32 handle);
-static inline void -drm_gem_object_handle_reference(struct drm_gem_object *obj) -{ - drm_gem_object_reference(obj); - atomic_inc(&obj->handle_count); -} - void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj);
void drm_gem_free_mmap_offset(struct drm_gem_object *obj);
On Wed, Jul 24, 2013 at 8:04 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
That's actually incorrect since the leak I've found is just a race in the drm/i915 object tracking. So I need to go back to the drawing board and figure out which are the ghosts and which the dragons here.
I've turned that testcase into an exercise for "drm/gem: completely close gem_open vs. gem_close races", but that race only results in userspace seeing different flink names for the same object. And that only happens if userspace is racy already.
For this patch here I still think there's an issue, but I seriously need to restart my brain first and flush out the bogons with some coffee before I try again ;-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Wed, Jul 24, 2013 at 11:02:02AM +0200, Daniel Vetter wrote:
Ok, I've written a second subtest now, and the race is indeed there and I've managed to leak a few objects. It's rather hard to hit though, I get a leak for roughly ever 1M attempts to provoke the race. But I'm rather convinced now that the leak is indeed there ;-)
So I think the patch as-is stands as correct and required to block off evil usespace. -Daniel
handle_unreference only clears up the obj->name and the reference, but would leave a dangling handle in the idr. The right thing to do is to call handle_delete.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_prime.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index a35f206..02619d4 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -479,7 +479,7 @@ fail: /* hmm, if driver attached, we are relying on the free-object path * to detach.. which seems ok.. */ - drm_gem_object_handle_unreference_unlocked(obj); + drm_gem_handle_delete(file_priv, *handle); out_put: dma_buf_put(dma_buf); mutex_unlock(&file_priv->prime.lock);
No one outside of drm should use this, the official interfaces are drm_gem_handle_create and drm_gem_handle_delete. The handle refcounting is purely an implementation detail of gem.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_gem.c | 2 +- include/drm/drmP.h | 1 - 2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 14c70b5..3b5305c 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -240,7 +240,7 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj) } }
-void +static void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) { if (WARN_ON(obj->handle_count == 0)) diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 25da8e0..1da1ca2 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1670,7 +1670,6 @@ int drm_gem_handle_create(struct drm_file *file_priv, u32 *handlep); int drm_gem_handle_delete(struct drm_file *filp, u32 handle);
-void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj);
void drm_gem_free_mmap_offset(struct drm_gem_object *obj); int drm_gem_create_mmap_offset(struct drm_gem_object *obj);
Hi
On Tue, Jul 16, 2013 at 9:12 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Yepp. Maybe we could even remove them as handle_reference() is only called by handle_create().
Anyway: Reviewed-by: David Herrmann dh.herrmann@gmail.com
Cheers David
On Wed, Jul 17, 2013 at 06:41:05PM +0200, David Herrmann wrote:
Handle reference is already inlined in a previous patch. -Daniel
All the gem based kms drivers really want the same function to destroy a dumb framebuffer backing storage object.
So give it to them and roll it out in all drivers.
This still leaves the option open for kms drivers which don't use GEM for backing storage, but it does decently simplify matters for gem drivers.
Cc: Inki Dae inki.dae@samsung.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Intel Graphics Development intel-gfx@lists.freedesktop.org Cc: Ben Skeggs skeggsb@gmail.com Cc: Rob Clark robdclark@gmail.com Cc: Alex Deucher alexdeucher@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/ast/ast_drv.c | 2 +- drivers/gpu/drm/ast/ast_drv.h | 3 --- drivers/gpu/drm/ast/ast_main.c | 7 ------- drivers/gpu/drm/cirrus/cirrus_drv.c | 2 +- drivers/gpu/drm/cirrus/cirrus_drv.h | 3 --- drivers/gpu/drm/cirrus/cirrus_main.c | 7 ------- drivers/gpu/drm/drm_gem.c | 14 ++++++++++++++ drivers/gpu/drm/drm_gem_cma_helper.c | 10 ---------- drivers/gpu/drm/exynos/exynos_drm_drv.c | 2 +- drivers/gpu/drm/exynos/exynos_drm_gem.c | 20 -------------------- drivers/gpu/drm/exynos/exynos_drm_gem.h | 9 --------- drivers/gpu/drm/gma500/gem.c | 17 ----------------- drivers/gpu/drm/gma500/psb_drv.c | 2 +- drivers/gpu/drm/gma500/psb_drv.h | 2 -- drivers/gpu/drm/i915/i915_drv.c | 2 +- drivers/gpu/drm/i915/i915_drv.h | 2 -- drivers/gpu/drm/i915/i915_gem.c | 7 ------- drivers/gpu/drm/mgag200/mgag200_drv.c | 2 +- drivers/gpu/drm/mgag200/mgag200_drv.h | 3 --- drivers/gpu/drm/mgag200/mgag200_main.c | 7 ------- drivers/gpu/drm/nouveau/nouveau_display.c | 7 ------- drivers/gpu/drm/nouveau/nouveau_display.h | 2 -- drivers/gpu/drm/nouveau/nouveau_drm.c | 2 +- drivers/gpu/drm/omapdrm/omap_drv.c | 2 +- drivers/gpu/drm/omapdrm/omap_drv.h | 2 -- drivers/gpu/drm/omapdrm/omap_gem.c | 15 --------------- drivers/gpu/drm/qxl/qxl_drv.c | 2 +- drivers/gpu/drm/qxl/qxl_drv.h | 3 --- drivers/gpu/drm/qxl/qxl_dumb.c | 7 ------- drivers/gpu/drm/radeon/radeon.h | 3 --- drivers/gpu/drm/radeon/radeon_drv.c | 5 +---- drivers/gpu/drm/radeon/radeon_gem.c | 7 ------- drivers/gpu/drm/rcar-du/rcar_du_drv.c | 2 +- drivers/gpu/drm/shmobile/shmob_drm_drv.c | 2 +- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 +- drivers/gpu/drm/udl/udl_drv.c | 2 +- drivers/gpu/drm/udl/udl_drv.h | 2 -- drivers/gpu/drm/udl/udl_gem.c | 6 ------ drivers/gpu/host1x/drm/drm.c | 2 +- drivers/gpu/host1x/drm/gem.c | 6 ------ drivers/gpu/host1x/drm/gem.h | 2 -- drivers/staging/imx-drm/imx-drm-core.c | 2 +- include/drm/drmP.h | 3 +++ include/drm/drm_gem_cma_helper.h | 8 -------- 44 files changed, 33 insertions(+), 186 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c index df0d0a0..a144fb0 100644 --- a/drivers/gpu/drm/ast/ast_drv.c +++ b/drivers/gpu/drm/ast/ast_drv.c @@ -216,7 +216,7 @@ static struct drm_driver driver = { .gem_free_object = ast_gem_free_object, .dumb_create = ast_dumb_create, .dumb_map_offset = ast_dumb_mmap_offset, - .dumb_destroy = ast_dumb_destroy, + .dumb_destroy = drm_gem_dumb_destroy,
};
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h index 622d4ae..796dbb2 100644 --- a/drivers/gpu/drm/ast/ast_drv.h +++ b/drivers/gpu/drm/ast/ast_drv.h @@ -322,9 +322,6 @@ ast_bo(struct ttm_buffer_object *bo) extern int ast_dumb_create(struct drm_file *file, struct drm_device *dev, struct drm_mode_create_dumb *args); -extern int ast_dumb_destroy(struct drm_file *file, - struct drm_device *dev, - uint32_t handle);
extern int ast_gem_init_object(struct drm_gem_object *obj); extern void ast_gem_free_object(struct drm_gem_object *obj); diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c index f60fd7b..0ef4228 100644 --- a/drivers/gpu/drm/ast/ast_main.c +++ b/drivers/gpu/drm/ast/ast_main.c @@ -449,13 +449,6 @@ int ast_dumb_create(struct drm_file *file, return 0; }
-int ast_dumb_destroy(struct drm_file *file, - struct drm_device *dev, - uint32_t handle) -{ - return drm_gem_handle_delete(file, handle); -} - int ast_gem_init_object(struct drm_gem_object *obj) { BUG(); diff --git a/drivers/gpu/drm/cirrus/cirrus_drv.c b/drivers/gpu/drm/cirrus/cirrus_drv.c index 8ecb601..d35d99c 100644 --- a/drivers/gpu/drm/cirrus/cirrus_drv.c +++ b/drivers/gpu/drm/cirrus/cirrus_drv.c @@ -102,7 +102,7 @@ static struct drm_driver driver = { .gem_free_object = cirrus_gem_free_object, .dumb_create = cirrus_dumb_create, .dumb_map_offset = cirrus_dumb_mmap_offset, - .dumb_destroy = cirrus_dumb_destroy, + .dumb_destroy = drm_gem_dumb_destroy, };
static struct pci_driver cirrus_pci_driver = { diff --git a/drivers/gpu/drm/cirrus/cirrus_drv.h b/drivers/gpu/drm/cirrus/cirrus_drv.h index bae5560..9b0bb91 100644 --- a/drivers/gpu/drm/cirrus/cirrus_drv.h +++ b/drivers/gpu/drm/cirrus/cirrus_drv.h @@ -203,9 +203,6 @@ int cirrus_gem_create(struct drm_device *dev, int cirrus_dumb_create(struct drm_file *file, struct drm_device *dev, struct drm_mode_create_dumb *args); -int cirrus_dumb_destroy(struct drm_file *file, - struct drm_device *dev, - uint32_t handle);
int cirrus_framebuffer_init(struct drm_device *dev, struct cirrus_framebuffer *gfb, diff --git a/drivers/gpu/drm/cirrus/cirrus_main.c b/drivers/gpu/drm/cirrus/cirrus_main.c index 35cbae8..c1ed9e1 100644 --- a/drivers/gpu/drm/cirrus/cirrus_main.c +++ b/drivers/gpu/drm/cirrus/cirrus_main.c @@ -255,13 +255,6 @@ int cirrus_dumb_create(struct drm_file *file, return 0; }
-int cirrus_dumb_destroy(struct drm_file *file, - struct drm_device *dev, - uint32_t handle) -{ - return drm_gem_handle_delete(file, handle); -} - int cirrus_gem_init_object(struct drm_gem_object *obj) { BUG(); diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 3b5305c..1609a3d 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -303,6 +303,20 @@ drm_gem_handle_delete(struct drm_file *filp, u32 handle) EXPORT_SYMBOL(drm_gem_handle_delete);
/** + * drm_gem_dumb_destroy - dumb fb callback helper for gem based drivers + * + * This implements the ->dumb_destroy kms driver callback for drivers which use + * gem to manage their backing storage. + */ +int drm_gem_dumb_destroy(struct drm_file *file, + struct drm_device *dev, + uint32_t handle) +{ + return drm_gem_handle_delete(file, handle); +} +EXPORT_SYMBOL(drm_gem_dumb_destroy); + +/** * Create a handle for this object. This adds a handle reference * to the object, which includes a regular reference count. Callers * will likely want to dereference the object afterwards. diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index ece72a8..2e4840d 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -286,16 +286,6 @@ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma) } EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
-/* - * drm_gem_cma_dumb_destroy - (struct drm_driver)->dumb_destroy callback function - */ -int drm_gem_cma_dumb_destroy(struct drm_file *file_priv, - struct drm_device *drm, unsigned int handle) -{ - return drm_gem_handle_delete(file_priv, handle); -} -EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_destroy); - #ifdef CONFIG_DEBUG_FS void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj, struct seq_file *m) { diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c index ca2729a..21fc28a 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c @@ -271,7 +271,7 @@ static struct drm_driver exynos_drm_driver = { .gem_vm_ops = &exynos_drm_gem_vm_ops, .dumb_create = exynos_drm_gem_dumb_create, .dumb_map_offset = exynos_drm_gem_dumb_map_offset, - .dumb_destroy = exynos_drm_gem_dumb_destroy, + .dumb_destroy = drm_gem_dumb_destroy, .prime_handle_to_fd = drm_gem_prime_handle_to_fd, .prime_fd_to_handle = drm_gem_prime_fd_to_handle, .gem_prime_export = exynos_dmabuf_prime_export, diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c index 16963ca..2ecd945 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -719,26 +719,6 @@ unlock: return ret; }
-int exynos_drm_gem_dumb_destroy(struct drm_file *file_priv, - struct drm_device *dev, - unsigned int handle) -{ - int ret; - - /* - * obj->refcount and obj->handle_count are decreased and - * if both them are 0 then exynos_drm_gem_free_object() - * would be called by callback to release resources. - */ - ret = drm_gem_handle_delete(file_priv, handle); - if (ret < 0) { - DRM_ERROR("failed to delete drm_gem_handle.\n"); - return ret; - } - - return 0; -} - int exynos_drm_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf) { struct drm_gem_object *obj = vma->vm_private_data; diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h b/drivers/gpu/drm/exynos/exynos_drm_gem.h index 468766b..09555af 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h @@ -151,15 +151,6 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv, struct drm_device *dev, uint32_t handle, uint64_t *offset);
-/* - * destroy memory region allocated. - * - a gem handle and physical memory region pointed by a gem object - * would be released by drm_gem_handle_delete(). - */ -int exynos_drm_gem_dumb_destroy(struct drm_file *file_priv, - struct drm_device *dev, - unsigned int handle); - /* page fault handler and mmap fault address(virtual) to physical memory. */ int exynos_drm_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf);
diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index eefd6cc..5ac9741 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -165,23 +165,6 @@ int psb_gem_dumb_create(struct drm_file *file, struct drm_device *dev, }
/** - * psb_gem_dumb_destroy - destroy a dumb buffer - * @file: client file - * @dev: our DRM device - * @handle: the object handle - * - * Destroy a handle that was created via psb_gem_dumb_create, at least - * we hope it was created that way. i915 seems to assume the caller - * does the checking but that might be worth review ! FIXME - */ -int psb_gem_dumb_destroy(struct drm_file *file, struct drm_device *dev, - uint32_t handle) -{ - /* No special work needed, drop the reference and see what falls out */ - return drm_gem_handle_delete(file, handle); -} - -/** * psb_gem_fault - pagefault handler for GEM objects * @vma: the VMA of the GEM object * @vmf: fault detail diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c index bddea58..ed06d5c 100644 --- a/drivers/gpu/drm/gma500/psb_drv.c +++ b/drivers/gpu/drm/gma500/psb_drv.c @@ -652,7 +652,7 @@ static struct drm_driver driver = { .gem_vm_ops = &psb_gem_vm_ops, .dumb_create = psb_gem_dumb_create, .dumb_map_offset = psb_gem_dumb_map_gtt, - .dumb_destroy = psb_gem_dumb_destroy, + .dumb_destroy = drm_gem_dumb_destroy, .fops = &psb_gem_fops, .name = DRIVER_NAME, .desc = DRIVER_DESC, diff --git a/drivers/gpu/drm/gma500/psb_drv.h b/drivers/gpu/drm/gma500/psb_drv.h index 6053b8a..984cacf 100644 --- a/drivers/gpu/drm/gma500/psb_drv.h +++ b/drivers/gpu/drm/gma500/psb_drv.h @@ -838,8 +838,6 @@ extern int psb_gem_get_aperture(struct drm_device *dev, void *data, struct drm_file *file); extern int psb_gem_dumb_create(struct drm_file *file, struct drm_device *dev, struct drm_mode_create_dumb *args); -extern int psb_gem_dumb_destroy(struct drm_file *file, struct drm_device *dev, - uint32_t handle); extern int psb_gem_dumb_map_gtt(struct drm_file *file, struct drm_device *dev, uint32_t handle, uint64_t *offset); extern int psb_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf); diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index b07362f..cca12db 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1163,7 +1163,7 @@ static struct drm_driver driver = {
.dumb_create = i915_gem_dumb_create, .dumb_map_offset = i915_gem_mmap_gtt, - .dumb_destroy = i915_gem_dumb_destroy, + .dumb_destroy = drm_gem_dumb_destroy, .ioctls = i915_ioctls, .fops = &i915_driver_fops, .name = DRIVER_NAME, diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index cef35d3..5cb3e4d 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1732,8 +1732,6 @@ int i915_gem_dumb_create(struct drm_file *file_priv, struct drm_mode_create_dumb *args); int i915_gem_mmap_gtt(struct drm_file *file_priv, struct drm_device *dev, uint32_t handle, uint64_t *offset); -int i915_gem_dumb_destroy(struct drm_file *file_priv, struct drm_device *dev, - uint32_t handle); /** * Returns true if seq1 is later than seq2. */ diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 46bf7e3..10b8c43 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -246,13 +246,6 @@ i915_gem_dumb_create(struct drm_file *file, args->size, &args->handle); }
-int i915_gem_dumb_destroy(struct drm_file *file, - struct drm_device *dev, - uint32_t handle) -{ - return drm_gem_handle_delete(file, handle); -} - /** * Creates a new mm object and returns a handle to it. */ diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c index 122b571..bd91964 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.c +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c @@ -104,7 +104,7 @@ static struct drm_driver driver = { .gem_free_object = mgag200_gem_free_object, .dumb_create = mgag200_dumb_create, .dumb_map_offset = mgag200_dumb_mmap_offset, - .dumb_destroy = mgag200_dumb_destroy, + .dumb_destroy = drm_gem_dumb_destroy, };
static struct pci_driver mgag200_pci_driver = { diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 12e2499..baaae19 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -264,9 +264,6 @@ int mgag200_gem_init_object(struct drm_gem_object *obj); int mgag200_dumb_create(struct drm_file *file, struct drm_device *dev, struct drm_mode_create_dumb *args); -int mgag200_dumb_destroy(struct drm_file *file, - struct drm_device *dev, - uint32_t handle); void mgag200_gem_free_object(struct drm_gem_object *obj); int mgag200_dumb_mmap_offset(struct drm_file *file, diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c index 9fa5685..8d46c22 100644 --- a/drivers/gpu/drm/mgag200/mgag200_main.c +++ b/drivers/gpu/drm/mgag200/mgag200_main.c @@ -310,13 +310,6 @@ int mgag200_dumb_create(struct drm_file *file, return 0; }
-int mgag200_dumb_destroy(struct drm_file *file, - struct drm_device *dev, - uint32_t handle) -{ - return drm_gem_handle_delete(file, handle); -} - int mgag200_gem_init_object(struct drm_gem_object *obj) { BUG(); diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c index 708b2d1..e6dfaaa 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.c +++ b/drivers/gpu/drm/nouveau/nouveau_display.c @@ -681,13 +681,6 @@ nouveau_display_dumb_create(struct drm_file *file_priv, struct drm_device *dev, }
int -nouveau_display_dumb_destroy(struct drm_file *file_priv, struct drm_device *dev, - uint32_t handle) -{ - return drm_gem_handle_delete(file_priv, handle); -} - -int nouveau_display_dumb_map_offset(struct drm_file *file_priv, struct drm_device *dev, uint32_t handle, uint64_t *poffset) diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h b/drivers/gpu/drm/nouveau/nouveau_display.h index 1ea3e47..185e741 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.h +++ b/drivers/gpu/drm/nouveau/nouveau_display.h @@ -68,8 +68,6 @@ int nouveau_display_dumb_create(struct drm_file *, struct drm_device *, struct drm_mode_create_dumb *args); int nouveau_display_dumb_map_offset(struct drm_file *, struct drm_device *, u32 handle, u64 *offset); -int nouveau_display_dumb_destroy(struct drm_file *, struct drm_device *, - u32 handle);
void nouveau_hdmi_mode_set(struct drm_encoder *, struct drm_display_mode *);
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 218a4b5..c683bae 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -715,7 +715,7 @@ driver = {
.dumb_create = nouveau_display_dumb_create, .dumb_map_offset = nouveau_display_dumb_map_offset, - .dumb_destroy = nouveau_display_dumb_destroy, + .dumb_destroy = drm_gem_dumb_destroy,
.name = DRIVER_NAME, .desc = DRIVER_DESC, diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 826586f..75886a3 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -618,7 +618,7 @@ static struct drm_driver omap_drm_driver = { .gem_vm_ops = &omap_gem_vm_ops, .dumb_create = omap_gem_dumb_create, .dumb_map_offset = omap_gem_dumb_map_offset, - .dumb_destroy = omap_gem_dumb_destroy, + .dumb_destroy = drm_gem_dumb_destroy, .ioctls = ioctls, .num_ioctls = DRM_OMAP_NUM_IOCTLS, .fops = &omapdriver_fops, diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index 215a20d..fd13601 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -224,8 +224,6 @@ int omap_gem_init_object(struct drm_gem_object *obj); void *omap_gem_vaddr(struct drm_gem_object *obj); int omap_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, uint32_t handle, uint64_t *offset); -int omap_gem_dumb_destroy(struct drm_file *file, struct drm_device *dev, - uint32_t handle); int omap_gem_dumb_create(struct drm_file *file, struct drm_device *dev, struct drm_mode_create_dumb *args); int omap_gem_mmap(struct file *filp, struct vm_area_struct *vma); diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index ebbdf41..9c30aaa 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -629,21 +629,6 @@ int omap_gem_dumb_create(struct drm_file *file, struct drm_device *dev, }
/** - * omap_gem_dumb_destroy - destroy a dumb buffer - * @file: client file - * @dev: our DRM device - * @handle: the object handle - * - * Destroy a handle that was created via omap_gem_dumb_create. - */ -int omap_gem_dumb_destroy(struct drm_file *file, struct drm_device *dev, - uint32_t handle) -{ - /* No special work needed, drop the reference and see what falls out */ - return drm_gem_handle_delete(file, handle); -} - -/** * omap_gem_dumb_map - buffer mapping for dumb interface * @file: our drm client file * @dev: drm device diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c index df0b577..48f2dfd 100644 --- a/drivers/gpu/drm/qxl/qxl_drv.c +++ b/drivers/gpu/drm/qxl/qxl_drv.c @@ -221,7 +221,7 @@ static struct drm_driver qxl_driver = {
.dumb_create = qxl_mode_dumb_create, .dumb_map_offset = qxl_mode_dumb_mmap, - .dumb_destroy = qxl_mode_dumb_destroy, + .dumb_destroy = drm_gem_dumb_destroy, #if defined(CONFIG_DEBUG_FS) .debugfs_init = qxl_debugfs_init, .debugfs_cleanup = qxl_debugfs_takedown, diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h index aacb791..57cb7a8 100644 --- a/drivers/gpu/drm/qxl/qxl_drv.h +++ b/drivers/gpu/drm/qxl/qxl_drv.h @@ -418,9 +418,6 @@ int qxl_bo_kmap(struct qxl_bo *bo, void **ptr); int qxl_mode_dumb_create(struct drm_file *file_priv, struct drm_device *dev, struct drm_mode_create_dumb *args); -int qxl_mode_dumb_destroy(struct drm_file *file_priv, - struct drm_device *dev, - uint32_t handle); int qxl_mode_dumb_mmap(struct drm_file *filp, struct drm_device *dev, uint32_t handle, uint64_t *offset_p); diff --git a/drivers/gpu/drm/qxl/qxl_dumb.c b/drivers/gpu/drm/qxl/qxl_dumb.c index 847c4ee..d34bb41 100644 --- a/drivers/gpu/drm/qxl/qxl_dumb.c +++ b/drivers/gpu/drm/qxl/qxl_dumb.c @@ -68,13 +68,6 @@ int qxl_mode_dumb_create(struct drm_file *file_priv, return 0; }
-int qxl_mode_dumb_destroy(struct drm_file *file_priv, - struct drm_device *dev, - uint32_t handle) -{ - return drm_gem_handle_delete(file_priv, handle); -} - int qxl_mode_dumb_mmap(struct drm_file *file_priv, struct drm_device *dev, uint32_t handle, uint64_t *offset_p) diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 9b7025d..39e5be8 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -490,9 +490,6 @@ int radeon_mode_dumb_create(struct drm_file *file_priv, int radeon_mode_dumb_mmap(struct drm_file *filp, struct drm_device *dev, uint32_t handle, uint64_t *offset_p); -int radeon_mode_dumb_destroy(struct drm_file *file_priv, - struct drm_device *dev, - uint32_t handle);
/* * Semaphores. diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c index e5419b3..f01a218 100644 --- a/drivers/gpu/drm/radeon/radeon_drv.c +++ b/drivers/gpu/drm/radeon/radeon_drv.c @@ -120,9 +120,6 @@ int radeon_mode_dumb_mmap(struct drm_file *filp, int radeon_mode_dumb_create(struct drm_file *file_priv, struct drm_device *dev, struct drm_mode_create_dumb *args); -int radeon_mode_dumb_destroy(struct drm_file *file_priv, - struct drm_device *dev, - uint32_t handle); struct sg_table *radeon_gem_prime_get_sg_table(struct drm_gem_object *obj); struct drm_gem_object *radeon_gem_prime_import_sg_table(struct drm_device *dev, size_t size, @@ -420,7 +417,7 @@ static struct drm_driver kms_driver = { .dma_ioctl = radeon_dma_ioctl_kms, .dumb_create = radeon_mode_dumb_create, .dumb_map_offset = radeon_mode_dumb_mmap, - .dumb_destroy = radeon_mode_dumb_destroy, + .dumb_destroy = drm_gem_dumb_destroy, .fops = &radeon_driver_kms_fops,
.prime_handle_to_fd = drm_gem_prime_handle_to_fd, diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c index aa79603..dce99c8 100644 --- a/drivers/gpu/drm/radeon/radeon_gem.c +++ b/drivers/gpu/drm/radeon/radeon_gem.c @@ -570,13 +570,6 @@ int radeon_mode_dumb_create(struct drm_file *file_priv, return 0; }
-int radeon_mode_dumb_destroy(struct drm_file *file_priv, - struct drm_device *dev, - uint32_t handle) -{ - return drm_gem_handle_delete(file_priv, handle); -} - #if defined(CONFIG_DEBUG_FS) static int radeon_debugfs_gem_info(struct seq_file *m, void *data) { diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index ff82877..3ebd419 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -253,7 +253,7 @@ static struct drm_driver rcar_du_driver = { .gem_prime_export = drm_gem_cma_dmabuf_export, .dumb_create = rcar_du_dumb_create, .dumb_map_offset = drm_gem_cma_dumb_map_offset, - .dumb_destroy = drm_gem_cma_dumb_destroy, + .dumb_destroy = drm_gem_dumb_destroy, .fops = &rcar_du_fops, .name = "rcar-du", .desc = "Renesas R-Car Display Unit", diff --git a/drivers/gpu/drm/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/shmobile/shmob_drm_drv.c index edc1018..7e1843f 100644 --- a/drivers/gpu/drm/shmobile/shmob_drm_drv.c +++ b/drivers/gpu/drm/shmobile/shmob_drm_drv.c @@ -280,7 +280,7 @@ static struct drm_driver shmob_drm_driver = { .gem_prime_export = drm_gem_cma_dmabuf_export, .dumb_create = drm_gem_cma_dumb_create, .dumb_map_offset = drm_gem_cma_dumb_map_offset, - .dumb_destroy = drm_gem_cma_dumb_destroy, + .dumb_destroy = drm_gem_dumb_destroy, .fops = &shmob_drm_fops, .name = "shmob-drm", .desc = "Renesas SH Mobile DRM", diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index 40b71da..14801c2 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c @@ -519,7 +519,7 @@ static struct drm_driver tilcdc_driver = { .gem_vm_ops = &drm_gem_cma_vm_ops, .dumb_create = drm_gem_cma_dumb_create, .dumb_map_offset = drm_gem_cma_dumb_map_offset, - .dumb_destroy = drm_gem_cma_dumb_destroy, + .dumb_destroy = drm_gem_dumb_destroy, #ifdef CONFIG_DEBUG_FS .debugfs_init = tilcdc_debugfs_init, .debugfs_cleanup = tilcdc_debugfs_cleanup, diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index c0770db..bb0af58 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -84,7 +84,7 @@ static struct drm_driver driver = {
.dumb_create = udl_dumb_create, .dumb_map_offset = udl_gem_mmap, - .dumb_destroy = udl_dumb_destroy, + .dumb_destroy = drm_gem_dumb_destroy, .fops = &udl_driver_fops,
.prime_fd_to_handle = drm_gem_prime_fd_to_handle, diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h index cc6d90f..56aec94 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -114,8 +114,6 @@ int udl_dumb_create(struct drm_file *file_priv, struct drm_mode_create_dumb *args); int udl_gem_mmap(struct drm_file *file_priv, struct drm_device *dev, uint32_t handle, uint64_t *offset); -int udl_dumb_destroy(struct drm_file *file_priv, struct drm_device *dev, - uint32_t handle);
int udl_gem_init_object(struct drm_gem_object *obj); void udl_gem_free_object(struct drm_gem_object *gem_obj); diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c index ef034fa..c82c84a 100644 --- a/drivers/gpu/drm/udl/udl_gem.c +++ b/drivers/gpu/drm/udl/udl_gem.c @@ -66,12 +66,6 @@ int udl_dumb_create(struct drm_file *file, args->size, &args->handle); }
-int udl_dumb_destroy(struct drm_file *file, struct drm_device *dev, - uint32_t handle) -{ - return drm_gem_handle_delete(file, handle); -} - int udl_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) { int ret; diff --git a/drivers/gpu/host1x/drm/drm.c b/drivers/gpu/host1x/drm/drm.c index e184b00..87aa09b 100644 --- a/drivers/gpu/host1x/drm/drm.c +++ b/drivers/gpu/host1x/drm/drm.c @@ -633,7 +633,7 @@ struct drm_driver tegra_drm_driver = { .gem_vm_ops = &tegra_bo_vm_ops, .dumb_create = tegra_bo_dumb_create, .dumb_map_offset = tegra_bo_dumb_map_offset, - .dumb_destroy = tegra_bo_dumb_destroy, + .dumb_destroy = drm_gem_dumb_destroy,
.ioctls = tegra_drm_ioctls, .num_ioctls = ARRAY_SIZE(tegra_drm_ioctls), diff --git a/drivers/gpu/host1x/drm/gem.c b/drivers/gpu/host1x/drm/gem.c index c5e9a9b..e4fbd78 100644 --- a/drivers/gpu/host1x/drm/gem.c +++ b/drivers/gpu/host1x/drm/gem.c @@ -262,9 +262,3 @@ int tegra_drm_mmap(struct file *file, struct vm_area_struct *vma)
return ret; } - -int tegra_bo_dumb_destroy(struct drm_file *file, struct drm_device *drm, - unsigned int handle) -{ - return drm_gem_handle_delete(file, handle); -} diff --git a/drivers/gpu/host1x/drm/gem.h b/drivers/gpu/host1x/drm/gem.h index 34de2b4..2e93b03 100644 --- a/drivers/gpu/host1x/drm/gem.h +++ b/drivers/gpu/host1x/drm/gem.h @@ -49,8 +49,6 @@ int tegra_bo_dumb_create(struct drm_file *file, struct drm_device *drm, struct drm_mode_create_dumb *args); int tegra_bo_dumb_map_offset(struct drm_file *file, struct drm_device *drm, uint32_t handle, uint64_t *offset); -int tegra_bo_dumb_destroy(struct drm_file *file, struct drm_device *drm, - unsigned int handle);
int tegra_drm_mmap(struct file *file, struct vm_area_struct *vma);
diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c index 6455305..9b16b56 100644 --- a/drivers/staging/imx-drm/imx-drm-core.c +++ b/drivers/staging/imx-drm/imx-drm-core.c @@ -800,7 +800,7 @@ static struct drm_driver imx_drm_driver = { .gem_vm_ops = &drm_gem_cma_vm_ops, .dumb_create = drm_gem_cma_dumb_create, .dumb_map_offset = drm_gem_cma_dumb_map_offset, - .dumb_destroy = drm_gem_cma_dumb_destroy, + .dumb_destroy = drm_gem_dumb_destroy,
.get_vblank_counter = drm_vblank_count, .enable_vblank = imx_drm_enable_vblank, diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 1da1ca2..4d7ea3d 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1670,6 +1670,9 @@ int drm_gem_handle_create(struct drm_file *file_priv, u32 *handlep); int drm_gem_handle_delete(struct drm_file *filp, u32 handle);
+int drm_gem_dumb_destroy(struct drm_file *file, + struct drm_device *dev, + uint32_t handle);
void drm_gem_free_mmap_offset(struct drm_gem_object *obj); int drm_gem_create_mmap_offset(struct drm_gem_object *obj); diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h index c34f27f..89b4d7d 100644 --- a/include/drm/drm_gem_cma_helper.h +++ b/include/drm/drm_gem_cma_helper.h @@ -30,14 +30,6 @@ int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv, /* set vm_flags and we can change the vm attribute to other one at here. */ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma);
-/* - * destroy memory region allocated. - * - a gem handle and physical memory region pointed by a gem object - * would be released by drm_gem_handle_delete(). - */ -int drm_gem_cma_dumb_destroy(struct drm_file *file_priv, - struct drm_device *drm, unsigned int handle); - /* allocate physical memory. */ struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm, unsigned int size);
On Tue, Jul 16, 2013 at 3:12 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Reviewed-by: Rob Clark robdclark@gmail.com
Hi Daniel,
Thanks for the patch.
On Tuesday 16 July 2013 09:12:04 Daniel Vetter wrote:
Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
On Tue, Jul 16, 2013 at 9:12 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Looks good Acked-by: Patrik Jakobsson patrik.r.jakobsson@gmail.com
Thanks Patrik
Part of the function uses the properly-typed dmabuf variable, the other an untyped void *buf. Kill the later.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_prime.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 02619d4..b76e694 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -306,7 +306,6 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, int *prime_fd) { struct drm_gem_object *obj; - void *buf; int ret = 0; struct dma_buf *dmabuf;
@@ -326,15 +325,15 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, goto out_have_obj; }
- buf = dev->driver->gem_prime_export(dev, obj, flags); - if (IS_ERR(buf)) { + dmabuf = dev->driver->gem_prime_export(dev, obj, flags); + if (IS_ERR(dmabuf)) { /* normally the created dma-buf takes ownership of the ref, * but if that fails then drop the ref */ - ret = PTR_ERR(buf); + ret = PTR_ERR(dmabuf); goto out; } - obj->export_dma_buf = buf; + obj->export_dma_buf = dmabuf;
/* if we've exported this buffer the cheat and add it to the import list * so we get the correct handle back @@ -344,7 +343,7 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, if (ret) goto fail_put_dmabuf;
- ret = dma_buf_fd(buf, flags); + ret = dma_buf_fd(dmabuf, flags); if (ret < 0) goto fail_rm_handle;
@@ -365,11 +364,12 @@ out_have_obj: goto out;
fail_rm_handle: - drm_prime_remove_buf_handle_locked(&file_priv->prime, buf); + drm_prime_remove_buf_handle_locked(&file_priv->prime, + dmabuf); fail_put_dmabuf: /* clear NOT to be checked when releasing dma_buf */ obj->export_dma_buf = NULL; - dma_buf_put(buf); + dma_buf_put(dmabuf); out: drm_gem_object_unreference_unlocked(obj); mutex_unlock(&file_priv->prime.lock);
When exporting a gem object as a dma-buf the critical section for the per-fd prime lock is just the adding (and in case of errors, removing) of the handle to the per-fd lookup cache.
So restrict the critical section to just that part of the function.
This simplifies later reordering.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_prime.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index b76e694..d8f4274 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -313,7 +313,6 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, if (!obj) return -ENOENT;
- mutex_lock(&file_priv->prime.lock); /* re-export the original imported object */ if (obj->import_attach) { dmabuf = obj->import_attach->dmabuf; @@ -335,6 +334,7 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, } obj->export_dma_buf = dmabuf;
+ mutex_lock(&file_priv->prime.lock); /* if we've exported this buffer the cheat and add it to the import list * so we get the correct handle back */ @@ -366,13 +366,13 @@ out_have_obj: fail_rm_handle: drm_prime_remove_buf_handle_locked(&file_priv->prime, dmabuf); + mutex_unlock(&file_priv->prime.lock); fail_put_dmabuf: /* clear NOT to be checked when releasing dma_buf */ obj->export_dma_buf = NULL; dma_buf_put(dmabuf); out: drm_gem_object_unreference_unlocked(obj); - mutex_unlock(&file_priv->prime.lock); return ret; } EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
if (!ret) implies that ret == 0, so no need to clear it again. And explicitly check for ret == 0 to indicate that we're checking an errno integer.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_prime.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index d8f4274..e4436c1 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -447,10 +447,8 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
ret = drm_prime_lookup_buf_handle(&file_priv->prime, dma_buf, handle); - if (!ret) { - ret = 0; + if (ret == 0) goto out_put; - }
/* never seen this one, need to import */ obj = dev->driver->gem_prime_import(dev, dma_buf);
I want to wrap the creation of a dma-buf from a gem object in it, so that the obj->export_dma_buf cache can be atomically filled in.
Instead of creating a new mutex just for that variable I've figured I can reuse the existing dev->object_name_lock, especially since the new semantics will exactly mirror the flink obj->name already protected by that lock.
v2: idr_preload/idr_preload_end is now an atomic section, so need to move the mutex locking outside.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_gem.c | 18 +++++++++--------- include/drm/drmP.h | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 1609a3d..2bcffb9 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -92,7 +92,7 @@ drm_gem_init(struct drm_device *dev) { struct drm_gem_mm *mm;
- spin_lock_init(&dev->object_name_lock); + mutex_init(&dev->object_name_lock); idr_init(&dev->object_name_idr);
mm = kzalloc(sizeof(struct drm_gem_mm), GFP_KERNEL); @@ -252,10 +252,10 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) * checked for a name */
- spin_lock(&obj->dev->object_name_lock); + mutex_lock(&obj->dev->object_name_lock); if (--obj->handle_count == 0) drm_gem_object_handle_free(obj); - spin_unlock(&obj->dev->object_name_lock); + mutex_unlock(&obj->dev->object_name_lock);
drm_gem_object_unreference_unlocked(obj); } @@ -333,16 +333,16 @@ drm_gem_handle_create(struct drm_file *file_priv, * Get the user-visible handle using idr. Preload and perform * allocation under our spinlock. */ + mutex_lock(&dev->object_name_lock); idr_preload(GFP_KERNEL); - spin_lock(&dev->object_name_lock); spin_lock(&file_priv->table_lock);
ret = idr_alloc(&file_priv->object_idr, obj, 1, 0, GFP_NOWAIT); drm_gem_object_reference(obj); obj->handle_count++; spin_unlock(&file_priv->table_lock); - spin_unlock(&dev->object_name_lock); idr_preload_end(); + mutex_unlock(&dev->object_name_lock); if (ret < 0) { drm_gem_object_handle_unreference_unlocked(obj); return ret; @@ -513,8 +513,8 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data, if (obj == NULL) return -ENOENT;
+ mutex_lock(&dev->object_name_lock); idr_preload(GFP_KERNEL); - spin_lock(&dev->object_name_lock); /* prevent races with concurrent gem_close. */ if (obj->handle_count == 0) { ret = -ENOENT; @@ -536,8 +536,8 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data, ret = 0;
err: - spin_unlock(&dev->object_name_lock); idr_preload_end(); + mutex_unlock(&dev->object_name_lock); drm_gem_object_unreference_unlocked(obj); return ret; } @@ -560,11 +560,11 @@ drm_gem_open_ioctl(struct drm_device *dev, void *data, if (!(dev->driver->driver_features & DRIVER_GEM)) return -ENODEV;
- spin_lock(&dev->object_name_lock); + mutex_lock(&dev->object_name_lock); obj = idr_find(&dev->object_name_idr, (int) args->name); if (obj) drm_gem_object_reference(obj); - spin_unlock(&dev->object_name_lock); + mutex_unlock(&dev->object_name_lock); if (!obj) return -ENOENT;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 4d7ea3d..59a1cf1 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1229,7 +1229,7 @@ struct drm_device {
/** \name GEM information */ /*@{ */ - spinlock_t object_name_lock; + struct mutex object_name_lock; struct idr object_name_idr; /*@} */ int switch_power_state;
The gem flink name holds a reference onto the object itself, and this self-reference would prevent an flink'ed object from every being freed. To break that loop we remove the flink name when the last userspace handle disappears, i.e. when obj->handle_count reaches 0.
Now in gem_open we drop the dev->object_name_lock between the flink name lookup and actually adding the handle. This means a concurrent gem_close of the last handle could result in the flink name getting reaped right inbetween, i.e.
Thread 1 Thread 2 gem_open gem_close
flink -> obj lookup handle_count drops to 0 remove flink name create_handle handle_count++
If someone now flinks this object again, we'll get a new flink name.
We can close this race by removing the lock dropping and making the entire lookup+handle_create sequence atomic. Unfortunately to still be able to share the handle_create logic this requires a handle_create_tail function which drops the lock - we can't hold the object_name_lock while calling into a driver's ->gem_open callback.
Note that for flink fixing this race isn't really important, since racing gem_open against gem_close is clearly a userspace bug. And no matter how the race ends, we won't leak any references.
But with dma-buf where the userspace dma-buf fd itself is refcounted this is a valid sequence and hence we should fix it. Therefore this patch here is just a warm-up exercise (and for consistency between flink buffer sharing and dma-buf buffer sharing with self-imports).
Also note that this extension of the critical section in gem_open protected by dev->object_name_lock only works because it's now a mutex: A spinlock would conflict with the potential memory allocation in idr_preload().
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_gem.c | 42 +++++++++++++++++++++++++++++++----------- include/drm/drmP.h | 3 +++ 2 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 2bcffb9..dd758c8 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -317,23 +317,26 @@ int drm_gem_dumb_destroy(struct drm_file *file, EXPORT_SYMBOL(drm_gem_dumb_destroy);
/** - * Create a handle for this object. This adds a handle reference - * to the object, which includes a regular reference count. Callers - * will likely want to dereference the object afterwards. + * drm_gem_handle_create_tail - internal functions to create a handle + * + * This expects the dev->object_name_lock to be held already and will drop it + * before returning. Used to avoid races in establishing new handles when + * importing an object from either an flink name or a dma-buf. */ int -drm_gem_handle_create(struct drm_file *file_priv, - struct drm_gem_object *obj, - u32 *handlep) +drm_gem_handle_create_tail(struct drm_file *file_priv, + struct drm_gem_object *obj, + u32 *handlep) { struct drm_device *dev = obj->dev; int ret;
+ WARN_ON(!mutex_is_locked(&dev->object_name_lock)); + /* * Get the user-visible handle using idr. Preload and perform * allocation under our spinlock. */ - mutex_lock(&dev->object_name_lock); idr_preload(GFP_KERNEL); spin_lock(&file_priv->table_lock);
@@ -360,6 +363,21 @@ drm_gem_handle_create(struct drm_file *file_priv,
return 0; } + +/** + * Create a handle for this object. This adds a handle reference + * to the object, which includes a regular reference count. Callers + * will likely want to dereference the object afterwards. + */ +int +drm_gem_handle_create(struct drm_file *file_priv, + struct drm_gem_object *obj, + u32 *handlep) +{ + mutex_lock(&obj->dev->object_name_lock); + + return drm_gem_handle_create_tail(file_priv, obj, handlep); +} EXPORT_SYMBOL(drm_gem_handle_create);
@@ -562,13 +580,15 @@ drm_gem_open_ioctl(struct drm_device *dev, void *data,
mutex_lock(&dev->object_name_lock); obj = idr_find(&dev->object_name_idr, (int) args->name); - if (obj) + if (obj) { drm_gem_object_reference(obj); - mutex_unlock(&dev->object_name_lock); - if (!obj) + } else { + mutex_unlock(&dev->object_name_lock); return -ENOENT; + }
- ret = drm_gem_handle_create(file_priv, obj, &handle); + /* drm_gem_handle_create_tail unlocks dev->object_name_lock. */ + ret = drm_gem_handle_create_tail(file_priv, obj, &handle); drm_gem_object_unreference_unlocked(obj); if (ret) return ret; diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 59a1cf1..9d82b33 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1665,6 +1665,9 @@ drm_gem_object_unreference_unlocked(struct drm_gem_object *obj) } }
+int drm_gem_handle_create_tail(struct drm_file *file_priv, + struct drm_gem_object *obj, + u32 *handlep); int drm_gem_handle_create(struct drm_file *file_priv, struct drm_gem_object *obj, u32 *handlep);
On Tue, Jul 16, 2013 at 09:12:09AM +0200, Daniel Vetter wrote:
The gem_flink_race/flink_name subtest exercise this race here. Like explained in the commit message strictly we don't need to care here, but having the same logic for dma-buf and flink names is imo worthwile.
But now I've spotted a little bug in the the dma-buf import code, so gotta write some dma-buf multithreaded testcases for that race now ;-) -Daniel
The export dma-buf cache is semantically similar to an flink name. So semantically it makes sense to treat it the same and remove the name (i.e. the dma_buf pointer) and its references when the last gem handle disappears.
Again we need to be careful, but double so: Not just could someone race and export with a gem close ioctl (so we need to recheck obj->handle_count again when assigning the new name), but multiple exports can also race against each another. This is prevented by holding the dev->object_name_lock across the entire section which touches obj->dma_buf.
With the new scheme we also need to reinstate the obj->dma_buf link at import time (in case the only reference userspace has held in-between was through the dma-buf fd and not through any native gem handle). For simplicity we don't check whether it's a native object but unconditionally set up that link - with the new scheme of removing the obj->dma_buf reference when the last handle disappears we can do that.
To make it clear that this is not just for exported buffers anymore als rename it from export_dma_buf to dma_buf.
To make sure that now one can race a fd_to_handle or handle_to_fd with gem_close we use the same tricks as in flink of extending the dev->object_name_locking critical section. With this change we finally have a guaranteed 1:1 relationship (at least for native objects) between gem objects and dma-bufs, even accounting for races (which can happen since the dma-buf itself holds a reference while in-flight).
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_fops.c | 1 + drivers/gpu/drm/drm_gem.c | 26 +++++++++++++++-- drivers/gpu/drm/drm_prime.c | 71 +++++++++++++++++++++++++++++++++++---------- include/drm/drmP.h | 12 ++++++-- 4 files changed, 90 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 3a24385..cea9bc5 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -555,6 +555,7 @@ int drm_release(struct inode *inode, struct file *filp) if (dev->driver->postclose) dev->driver->postclose(dev, file_priv);
+ if (drm_core_check_feature(dev, DRIVER_PRIME)) drm_prime_destroy_file_private(&file_priv->prime);
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index dd758c8..44ee16b 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -204,9 +204,16 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp) drm_prime_remove_buf_handle(&filp->prime, obj->import_attach->dmabuf); } - if (obj->export_dma_buf) { + + /* + * Note: obj->dma_buf can't disappear as long as we still hold a + * handle reference in obj->handle_count. + */ + if (obj->dma_buf) { + printk("removing exported handle for file %p, dmabuf %p\n", + filp, obj->dma_buf); drm_prime_remove_buf_handle(&filp->prime, - obj->export_dma_buf); + obj->dma_buf); } }
@@ -240,6 +247,15 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj) } }
+static void drm_gem_object_exported_dma_buf_free(struct drm_gem_object *obj) +{ + /* Unbreak the reference cycle if we have an exported dma_buf. */ + if (obj->dma_buf) { + dma_buf_put(obj->dma_buf); + obj->dma_buf = NULL; + } +} + static void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) { @@ -253,8 +269,10 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) */
mutex_lock(&obj->dev->object_name_lock); - if (--obj->handle_count == 0) + if (--obj->handle_count == 0) { drm_gem_object_handle_free(obj); + drm_gem_object_exported_dma_buf_free(obj); + } mutex_unlock(&obj->dev->object_name_lock);
drm_gem_object_unreference_unlocked(obj); @@ -647,6 +665,8 @@ drm_gem_release(struct drm_device *dev, struct drm_file *file_private) void drm_gem_object_release(struct drm_gem_object *obj) { + WARN_ON(obj->dma_buf); + if (obj->filp) fput(obj->filp); } diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index e4436c1..6d0755a 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -196,11 +196,8 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf) { struct drm_gem_object *obj = dma_buf->priv;
- if (obj->export_dma_buf == dma_buf) { - /* drop the reference on the export fd holds */ - obj->export_dma_buf = NULL; - drm_gem_object_unreference_unlocked(obj); - } + /* drop the reference on the export fd holds */ + drm_gem_object_unreference_unlocked(obj); } EXPORT_SYMBOL(drm_gem_dmabuf_release);
@@ -301,6 +298,38 @@ struct dma_buf *drm_gem_prime_export(struct drm_device *dev, } EXPORT_SYMBOL(drm_gem_prime_export);
+static struct dma_buf *export_and_register_object(struct drm_device *dev, + struct drm_gem_object *obj, + uint32_t flags) +{ + struct dma_buf *dmabuf; + + /* prevent races with concurrent gem_close. */ + if (obj->handle_count == 0) { + dma_buf_put(dmabuf); + dmabuf = ERR_PTR(-ENOENT); + return dmabuf; + } + + dmabuf = dev->driver->gem_prime_export(dev, obj, flags); + if (IS_ERR(dmabuf)) { + /* normally the created dma-buf takes ownership of the ref, + * but if that fails then drop the ref + */ + return dmabuf; + } + + /* + * Note that callers do not need to clean up the export cache + * since the check for obj->handle_count guarantees that someone + * will clean it up. + */ + obj->dma_buf = dmabuf; + get_dma_buf(obj->dma_buf); + + return dmabuf; +} + int drm_gem_prime_handle_to_fd(struct drm_device *dev, struct drm_file *file_priv, uint32_t handle, uint32_t flags, int *prime_fd) @@ -316,15 +345,20 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, /* re-export the original imported object */ if (obj->import_attach) { dmabuf = obj->import_attach->dmabuf; + get_dma_buf(dmabuf); goto out_have_obj; }
- if (obj->export_dma_buf) { - dmabuf = obj->export_dma_buf; + mutex_lock(&dev->object_name_lock); + if (obj->dma_buf) { + get_dma_buf(obj->dma_buf); + dmabuf = obj->dma_buf; + mutex_unlock(&dev->object_name_lock); goto out_have_obj; }
- dmabuf = dev->driver->gem_prime_export(dev, obj, flags); + dmabuf = export_and_register_object(dev, obj, flags); + mutex_unlock(&dev->object_name_lock); if (IS_ERR(dmabuf)) { /* normally the created dma-buf takes ownership of the ref, * but if that fails then drop the ref @@ -332,14 +366,13 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, ret = PTR_ERR(dmabuf); goto out; } - obj->export_dma_buf = dmabuf;
mutex_lock(&file_priv->prime.lock); /* if we've exported this buffer the cheat and add it to the import list * so we get the correct handle back */ ret = drm_prime_add_buf_handle(&file_priv->prime, - obj->export_dma_buf, handle); + dmabuf, handle); if (ret) goto fail_put_dmabuf;
@@ -352,7 +385,6 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, return 0;
out_have_obj: - get_dma_buf(dmabuf); ret = dma_buf_fd(dmabuf, flags); if (ret < 0) { dma_buf_put(dmabuf); @@ -368,8 +400,6 @@ fail_rm_handle: dmabuf); mutex_unlock(&file_priv->prime.lock); fail_put_dmabuf: - /* clear NOT to be checked when releasing dma_buf */ - obj->export_dma_buf = NULL; dma_buf_put(dmabuf); out: drm_gem_object_unreference_unlocked(obj); @@ -451,13 +481,22 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, goto out_put;
/* never seen this one, need to import */ + mutex_lock(&dev->object_name_lock); obj = dev->driver->gem_prime_import(dev, dma_buf); if (IS_ERR(obj)) { ret = PTR_ERR(obj); - goto out_put; + goto out_unlock; + } + + if (obj->dma_buf) { + WARN_ON(obj->dma_buf != dma_buf); + } else { + obj->dma_buf = dma_buf; + get_dma_buf(dma_buf); }
- ret = drm_gem_handle_create(file_priv, obj, handle); + /* drm_gem_handle_create_tail unlocks dev->object_name_lock. */ + ret = drm_gem_handle_create_tail(file_priv, obj, handle); drm_gem_object_unreference_unlocked(obj); if (ret) goto out_put; @@ -478,6 +517,8 @@ fail: * to detach.. which seems ok.. */ drm_gem_handle_delete(file_priv, *handle); +out_unlock: + mutex_lock(&dev->object_name_lock); out_put: dma_buf_put(dma_buf); mutex_unlock(&file_priv->prime.lock); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 9d82b33..c19cc2b 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -686,8 +686,16 @@ struct drm_gem_object {
void *driver_private;
- /* dma buf exported from this GEM object */ - struct dma_buf *export_dma_buf; + /** + * dma_buf - dma buf associated with this GEM object + * + * Pointer to the dma-buf associated with this gem object (either + * through importing or exporting). We break the resulting reference + * loop when the last gem handle for this object is released. + * + * Protected by obj->object_name_lock + */ + struct dma_buf *dma_buf;
/** * import_attach - dma buf attachment backing this object
with the reworking semantics and locking of the obj->dma_buf pointer this pointer is always set as long as there's still a gem handle around and a dma_buf associated with this gem object.
Also, the per file-priv lookup-cache for dma-buf importing is also unified between foreign and native objects.
Hence we don't need to special case the clean any more and can simply drop the clause which only runs for foreing objects, i.e. with obj->import_attach set.
Note that with this change (actually with the previous one to always set up obj->dma_buf even for foreign objects) it is no longer required to set obj->import_attach when importing a foreing object. So update comments accordingly, too.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_gem.c | 5 ----- include/drm/drmP.h | 5 +++++ 2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 44ee16b..a2b1470 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -200,11 +200,6 @@ EXPORT_SYMBOL(drm_gem_object_alloc); static void drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp) { - if (obj->import_attach) { - drm_prime_remove_buf_handle(&filp->prime, - obj->import_attach->dmabuf); - } - /* * Note: obj->dma_buf can't disappear as long as we still hold a * handle reference in obj->handle_count. diff --git a/include/drm/drmP.h b/include/drm/drmP.h index c19cc2b..13fe22a 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -706,6 +706,11 @@ struct drm_gem_object { * * The driver's ->gem_free_object callback is responsible for cleaning * up the dma_buf attachment and references acquired at import time. + * + * Note that the drm gem/prime core does not depend upon drivers setting + * this field any more. So for drivers where this doesn't make sense + * (e.g. virtual devices or a displaylink behind an usb bus) they can + * simply leave it as NULL. */ struct dma_buf_attachment *import_attach; };
Hi Daniel, sorry for being late.
2013/7/16 Daniel Vetter daniel.vetter@ffwll.ch
How about using stuffs of drm_prime instead of specific ones? Seem like that we could replace specific dmabuf stuffs with common ones of drm_prime, at least in case of Exynos: i.e. each driver can export a gem to a dmabuf through drm_gem_prime_export function of drm_prime instead of specific one. By doing so, I think we could remove duplicated codes of drivers, specific dmabuf stuffs. I'm not sure but it seems like that there is any reason you try to use existing stuffs with a little change: maybe the stuffs of drm_prime couldn't be used for all drm drivers commonly.
Thanks, Inki Dae
tricky. I've opted not to do that since last time around I've touched exynos a
bit it broke horribly ;-)
- The prime core should now no longer depend upon obj->import_attach being
On Sat, Jul 27, 2013 at 11:22 AM, Inki Dae inki.dae@samsung.com wrote:
I have to admit that I didn't really understand your email, mostly since you talk about "stuff" and "stuffs" and I didn't really grok which piece of code you actually mean. Can you pls elaborate, maybe taking the functions for exynos as an example? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
You have posted one patch that makes i915/exynos drivers use drm_gem_dmabuf_release function commonly. But my opinion is that we could use not only drm_gem_dmabuf_release function but also other functions of drm_prime commonly. I think that could be done simply like below,
In drivers/gpu/drm/i915/i915_dev.c, Static struct drm_driver driver = { ... .gem_prime_export = drm_gem_prime_export, .gem_prime_import = drm_gem_prime_import, ...
We are using driver specific dma_buf_ops, i915_dmabuf_ops for i915 and exynos_dmabuf_ops for exynos. So my opinion is to use drm_gem_prime_dmabuf_ops of drm_prime instead. However, for this, we have to change specific export and import functions to exported drm_prim's ones, drm_gem_prime_export and drm_gem_prime_import. And I thought maybe you caught that but you used only drm_gem_dmabuf_release function among them of drm_prime if there is no my missing point. So I guess maybe there is any reason in doing so.
Thanks, Inki Dae
drm: use common drm_gem_dmabuf_release in i915/exynos drivers
On Mon, Aug 05, 2013 at 11:02:48AM +0900, Inki Dae wrote:
Ah, now I understand.
Yeah, there's probably some room to share more code, otoh prime is still rather new so I prefer to keep code separate if it's not obviously the same code. That makes experimentation a bit easier. -Daniel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Daniel Vetter -
Dave Airlie -
David Herrmann -
Inki Dae -
Laurent Pinchart -
Patrik Jakobsson -
Rob Clark