| Message ID | 1355974747-24271-1-git-send-email-airlied@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 20, 2012 at 01:39:07PM +1000, Dave Airlie wrote: > From: Dave Airlie <airlied@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@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Dec 20, 2012 at 01:39:07PM +1000, Dave Airlie wrote: > From: Dave Airlie <airlied@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@redhat.com> [snip] > -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); We need some form of protection to ensure that the removal of the export_dma_buf and ref dropping is atomic vs. lookups. Atm there's no protection, which means if you e.g. transfer a gem_bo from one process to another with dma_buf (dropping the gem_handle right away), we'll get a leak: [first proces] 1. handle_to_fd 2. gem_close, dropping the above internal dma-buf ref 3. pass dma-buf to other process [other process] 4. fd_to_handle see export_dma_buf != NULL, doesn't re-add export reference 5. close dma_buf fd, freeing it 6. gem_close tries to unref the now free'd dma_buf For added fun, try to careful race steps 2. against 4. (or for a different kind of fun, against 5. but that requires more evil afterwards to blow up). Cheers, Daniel
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); } } 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); + 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)) { + 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,