[PATCH] drm/prime: keep a reference from the handle to exported dma-buf (v2)
Daniel Vetter
daniel at ffwll.ch
Thu Dec 20 06:27:25 PST 2012
On Thu, Dec 20, 2012 at 01:39:07PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied at redhat.com>
>
> This is likely the simplest solution to the problem, and seems
> to work fine.
>
> When we export a dma_buf from a gem object, we keep track of it
> with the handle, we take a reference to the dma_buf. When we close
> the handle (i.e. userspace is finished with the buffer), we drop
> the reference to the dma_buf, and it gets collected.
>
> I'd like to refrain from too much in this patch as I'd like it to
> go to stable, so future cleanups should look at maybe reducing
> the obj storing two pointers etc.
>
> v1.1: move export symbol line back up.
>
> v2: okay I had to do a bit more, as the first patch showed a leak
> on one of my tests, that I found using the dma-buf debugfs support,
> the problem case is exporting a buffer twice with the same handle,
> we'd add another export handle for it unnecessarily, however
> we now fail if we try to export the same object with a different gem handle,
> however I'm not sure if that is a case I want to support, and I've
> gotten the code to WARN_ON if we hit something like that.
>
> Signed-off-by: Dave Airlie <airlied at redhat.com>
Imo still slightly wrong for the case of self-imports on the same device,
but different fds, i.e. sharing buffers in a wayland/dri3 kind of setup.
> ---
> drivers/gpu/drm/drm_gem.c | 2 +-
> drivers/gpu/drm/drm_prime.c | 95 ++++++++++++++++++++++++++++++++-------------
> include/drm/drmP.h | 3 +-
> 3 files changed, 72 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 24efae4..e221b7b 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -209,7 +209,7 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp)
> obj->import_attach->dmabuf);
> }
> if (obj->export_dma_buf) {
> - drm_prime_remove_imported_buf_handle(&filp->prime,
> + drm_prime_remove_exported_buf_handle(&filp->prime,
> obj->export_dma_buf);
I think this here belongs into drm_gem_object_handle_free i.e. once we've
dropped the last userspace gem handle, not when we drop any userspace
handle. This way you should be able to export/import dma_bufs as much as
you like and also use them for inter-proc communication with fd, passing.
And as long as we keep a gem handle around we cache the exported dma-buf
and hand back the same one to userspace.
If the only userspace references is the dma-buf fd itself, userspace can
reimport the dma-buf, which will be a bit tricky with the curent approach,
since we need to figure out whether it's a an imported one form another
driver or one originally exported from ourselves at import time. Probably
easier to grab the dma-buf reference for the lookup cache unconditionally
and unify the two cases a bit.
Then the rule would be that as long as we have a gem handle around, we
hold an additional reference onto the dma-buf object.
Since larger issues are still open, I haven't checked for import/export
vs. gem handle open/close races yet. Two small things below.
> }
> }
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 7f12573..a9ddea9 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -62,6 +62,8 @@ struct drm_prime_member {
> uint32_t handle;
> };
>
> +static int drm_prime_add_exported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle);
Line wrapping.
> +
> 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)
> @@ -69,6 +71,7 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
> struct drm_gem_object *obj;
> void *buf;
> int ret;
> + struct dma_buf *dmabuf;
>
> obj = drm_gem_object_lookup(dev, file_priv, handle);
> if (!obj)
> @@ -77,41 +80,57 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
> mutex_lock(&file_priv->prime.lock);
> /* re-export the original imported object */
> if (obj->import_attach) {
> - get_dma_buf(obj->import_attach->dmabuf);
> - *prime_fd = dma_buf_fd(obj->import_attach->dmabuf, flags);
> - drm_gem_object_unreference_unlocked(obj);
> - mutex_unlock(&file_priv->prime.lock);
> - return 0;
> + dmabuf = obj->import_attach->dmabuf;
> + goto out_have_obj;
> }
>
> if (obj->export_dma_buf) {
> - get_dma_buf(obj->export_dma_buf);
> - *prime_fd = dma_buf_fd(obj->export_dma_buf, flags);
> + dmabuf = obj->export_dma_buf;
> + goto out_have_obj;
> + }
> +
> + buf = dev->driver->gem_prime_export(dev, obj, flags);
> + if (IS_ERR(buf)) {
> + /* normally the created dma-buf takes ownership of the ref,
> + * but if that fails then drop the ref
> + */
> drm_gem_object_unreference_unlocked(obj);
> - } else {
> - buf = dev->driver->gem_prime_export(dev, obj, flags);
> - if (IS_ERR(buf)) {
> - /* normally the created dma-buf takes ownership of the ref,
> - * but if that fails then drop the ref
> - */
> - drm_gem_object_unreference_unlocked(obj);
> - mutex_unlock(&file_priv->prime.lock);
> - return PTR_ERR(buf);
> - }
> - obj->export_dma_buf = buf;
> - *prime_fd = dma_buf_fd(buf, flags);
> + mutex_unlock(&file_priv->prime.lock);
> + return PTR_ERR(buf);
> }
> + obj->export_dma_buf = buf;
> +
> /* 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_imported_buf_handle(&file_priv->prime,
> - obj->export_dma_buf, handle);
> + ret = drm_prime_add_exported_buf_handle(&file_priv->prime,
> + obj->export_dma_buf, handle);
> if (ret) {
> drm_gem_object_unreference_unlocked(obj);
> mutex_unlock(&file_priv->prime.lock);
> return ret;
> }
> + *prime_fd = dma_buf_fd(buf, flags);
> + mutex_unlock(&file_priv->prime.lock);
> + return 0;
>
> +out_have_obj:
> + /* we should have a buf handle for this case */
> + {
> + uint32_t exp_handle;
> + get_dma_buf(dmabuf);
> + ret = drm_prime_lookup_buf_handle(&file_priv->prime,
> + dmabuf,
> + &exp_handle);
> + if (WARN_ON(ret == -ENOENT || exp_handle != handle)) {
I don't quite understand what this test here's supposed to do. Is this to
protect against concurrent races of userspace removing the last handle
while we set up a export/reexport?
> + dma_buf_put(dmabuf);
> + drm_gem_object_unreference_unlocked(obj);
> + mutex_unlock(&file_priv->prime.lock);
> + return -EINVAL;
> + }
> + }
> + *prime_fd = dma_buf_fd(dmabuf, flags);
> + drm_gem_object_unreference_unlocked(obj);
> mutex_unlock(&file_priv->prime.lock);
> return 0;
> }
> @@ -130,7 +149,7 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
>
> mutex_lock(&file_priv->prime.lock);
>
> - ret = drm_prime_lookup_imported_buf_handle(&file_priv->prime,
> + ret = drm_prime_lookup_buf_handle(&file_priv->prime,
> dma_buf, handle);
> if (!ret) {
> ret = 0;
> @@ -307,7 +326,7 @@ void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv)
> }
> EXPORT_SYMBOL(drm_prime_destroy_file_private);
>
> -int drm_prime_add_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle)
> +static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle)
> {
> struct drm_prime_member *member;
>
> @@ -320,9 +339,21 @@ int drm_prime_add_imported_buf_handle(struct drm_prime_file_private *prime_fpriv
> list_add(&member->entry, &prime_fpriv->head);
> return 0;
> }
> +
> +int drm_prime_add_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle)
> +{
> + return drm_prime_add_buf_handle(prime_fpriv, dma_buf, handle);
> +}
> EXPORT_SYMBOL(drm_prime_add_imported_buf_handle);
>
> -int drm_prime_lookup_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t *handle)
> +static int drm_prime_add_exported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle)
> +{
> + /* take a reference to the buf handle for this case */
> + get_dma_buf(dma_buf);
> + return drm_prime_add_buf_handle(prime_fpriv, dma_buf, handle);
> +}
> +
> +int drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t *handle)
> {
> struct drm_prime_member *member;
>
> @@ -334,9 +365,9 @@ int drm_prime_lookup_imported_buf_handle(struct drm_prime_file_private *prime_fp
> }
> return -ENOENT;
> }
> -EXPORT_SYMBOL(drm_prime_lookup_imported_buf_handle);
> +EXPORT_SYMBOL(drm_prime_lookup_buf_handle);
>
> -void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)
> +static void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)
> {
> struct drm_prime_member *member, *safe;
>
> @@ -349,4 +380,16 @@ void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_f
> }
> mutex_unlock(&prime_fpriv->lock);
> }
> +
> +void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)
> +{
> + drm_prime_remove_buf_handle(prime_fpriv, dma_buf);
> +}
> EXPORT_SYMBOL(drm_prime_remove_imported_buf_handle);
> +
> +void drm_prime_remove_exported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf)
> +{
> + drm_prime_remove_buf_handle(prime_fpriv, dma_buf);
> + dma_buf_put(dma_buf);
> +}
> +EXPORT_SYMBOL(drm_prime_remove_exported_buf_handle);
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index fad21c9..38d3177 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1560,8 +1560,9 @@ extern void drm_prime_gem_destroy(struct drm_gem_object *obj, struct sg_table *s
> void drm_prime_init_file_private(struct drm_prime_file_private *prime_fpriv);
> void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv);
> int drm_prime_add_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle);
> -int drm_prime_lookup_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t *handle);
> +int drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t *handle);
> void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf);
> +void drm_prime_remove_exported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf);
>
> int drm_prime_add_dma_buf(struct drm_device *dev, struct drm_gem_object *obj);
> int drm_prime_lookup_obj(struct drm_device *dev, struct dma_buf *buf,
> --
> 1.8.0.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the dri-devel
mailing list