| Message ID | 1366173482-30764-1-git-send-email-airlied@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 17, 2013 at 02:38:02PM +1000, Dave Airlie wrote: > Currently we have a problem with this: > 1. i915: create gem object > 2. i915: export gem object to prime > 3. radeon: import gem object > 4. close prime fd > 5. radeon: unref object > 6. i915: unref object > > i915 has an imported object reference in its file priv, that isn't > cleaned up properly until fd close. The reference gets added at step 2, > but at step 6 we don't have enough info to clean it up. > > The solution is to take a reference on the dma-buf when we export it, > and drop the reference when the gem handle goes away. > > So 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. > > This patch isn't meant to fix any other problem or bikesheds, and it doesn't > fix any races with other scenarios. > > 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. > > v2.1: rebase this patch, write better commit msg. > v3: cleanup error handling, track import vs export in linked list, > these two patches were separate previously, but seem to work better > like this. > v4: danvet is correct, this code is no longer useful, since the buffer > better exist, so remove it. > > Signed-off-by: Dave Airlie <airlied@redhat.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> On bikeshed review level I wonder whether we shouldn't just grab a reference unconditonally when inserting it into the handle_to_fd lookup lists. But I need to think through a few other cases and apparently the self-import test is still a bit broken. So this is material for follow-up patches. -Daniel > --- > drivers/gpu/drm/drm_gem.c | 4 +- > drivers/gpu/drm/drm_prime.c | 95 +++++++++++++++++++++++++++++---------------- > include/drm/drmP.h | 4 +- > 3 files changed, 65 insertions(+), 38 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index af779ae..cf919e3 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -205,11 +205,11 @@ static void > drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp) > { > if (obj->import_attach) { > - drm_prime_remove_imported_buf_handle(&filp->prime, > + drm_prime_remove_buf_handle(&filp->prime, > obj->import_attach->dmabuf); > } > if (obj->export_dma_buf) { > - drm_prime_remove_imported_buf_handle(&filp->prime, > + drm_prime_remove_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 366910d..b4ed808 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -57,10 +57,14 @@ > * use the drm_gem_prime_{import,export} helpers. > */ > > +#define PRIME_IMPORT 1 > +#define PRIME_EXPORT 2 > + > struct drm_prime_member { > struct list_head entry; > struct dma_buf *dma_buf; > uint32_t handle; > + int type; > }; > > static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach, > @@ -157,6 +161,8 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = { > .vunmap = drm_gem_dmabuf_vunmap, > }; > > +static int drm_prime_add_exported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle); > + > /** > * DOC: PRIME Helpers > * > @@ -200,7 +206,9 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, > { > struct drm_gem_object *obj; > void *buf; > - int ret; > + int ret = 0; > + struct dma_buf *dmabuf; > + uint32_t exp_handle; > > obj = drm_gem_object_lookup(dev, file_priv, handle); > if (!obj) > @@ -209,43 +217,44 @@ 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); > - 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); > + 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 > + */ > + ret = PTR_ERR(buf); > + goto out; > } > + 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); > - if (ret) { > - drm_gem_object_unreference_unlocked(obj); > - mutex_unlock(&file_priv->prime.lock); > - return ret; > - } > + ret = drm_prime_add_exported_buf_handle(&file_priv->prime, > + obj->export_dma_buf, handle); > + if (ret) > + goto out; > > + *prime_fd = dma_buf_fd(buf, flags); > mutex_unlock(&file_priv->prime.lock); > return 0; > + > +out_have_obj: > + get_dma_buf(dmabuf); > + *prime_fd = dma_buf_fd(dmabuf, flags); > +out: > + drm_gem_object_unreference_unlocked(obj); > + mutex_unlock(&file_priv->prime.lock); > + return ret; > } > EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); > > @@ -314,7 +323,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; > @@ -491,7 +500,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, int type) > { > struct drm_prime_member *member; > > @@ -501,12 +510,25 @@ int drm_prime_add_imported_buf_handle(struct drm_prime_file_private *prime_fpriv > > member->dma_buf = dma_buf; > member->handle = handle; > + member->type = type; > 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, PRIME_IMPORT); > +} > 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, PRIME_EXPORT); > +} > + > +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; > > @@ -518,19 +540,24 @@ 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) > +void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf) > { > struct drm_prime_member *member, *safe; > > mutex_lock(&prime_fpriv->lock); > list_for_each_entry_safe(member, safe, &prime_fpriv->head, entry) { > if (member->dma_buf == dma_buf) { > + if (member->type == PRIME_EXPORT) > + dma_buf_put(dma_buf); > list_del(&member->entry); > kfree(member); > } > } > mutex_unlock(&prime_fpriv->lock); > } > -EXPORT_SYMBOL(drm_prime_remove_imported_buf_handle); > +EXPORT_SYMBOL(drm_prime_remove_buf_handle); > + > + > + > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 2d94d74..69712a6 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -1594,8 +1594,8 @@ 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); > -void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf); > +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_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.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, 2013-04-17 at 10:21 +0200, Daniel Vetter wrote: > On Wed, Apr 17, 2013 at 02:38:02PM +1000, Dave Airlie wrote: > > Currently we have a problem with this: > > 1. i915: create gem object > > 2. i915: export gem object to prime > > 3. radeon: import gem object > > 4. close prime fd > > 5. radeon: unref object > > 6. i915: unref object > > > > i915 has an imported object reference in its file priv, that isn't > > cleaned up properly until fd close. The reference gets added at step 2, > > but at step 6 we don't have enough info to clean it up. > > > > The solution is to take a reference on the dma-buf when we export it, > > and drop the reference when the gem handle goes away. > > > > So 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. > > > > This patch isn't meant to fix any other problem or bikesheds, and it doesn't > > fix any races with other scenarios. > > > > 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. > > > > v2.1: rebase this patch, write better commit msg. > > v3: cleanup error handling, track import vs export in linked list, > > these two patches were separate previously, but seem to work better > > like this. > > v4: danvet is correct, this code is no longer useful, since the buffer > > better exist, so remove it. > > > > Signed-off-by: Dave Airlie <airlied@redhat.com> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > On bikeshed review level I wonder whether we shouldn't just grab a > reference unconditonally when inserting it into the handle_to_fd lookup > lists. But I need to think through a few other cases and apparently the > self-import test is still a bit broken. So this is material for follow-up > patches. Yes, this is needed, otherwise closing the prime fd will leave stale pointers to the dma buf in the importer's lookup table. I've sent an update to igt/prime_self_test to intel-gfx for showing this. --Imre > -Daniel > > > --- > > drivers/gpu/drm/drm_gem.c | 4 +- > > drivers/gpu/drm/drm_prime.c | 95 +++++++++++++++++++++++++++++---------------- > > include/drm/drmP.h | 4 +- > > 3 files changed, 65 insertions(+), 38 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > > index af779ae..cf919e3 100644 > > --- a/drivers/gpu/drm/drm_gem.c > > +++ b/drivers/gpu/drm/drm_gem.c > > @@ -205,11 +205,11 @@ static void > > drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp) > > { > > if (obj->import_attach) { > > - drm_prime_remove_imported_buf_handle(&filp->prime, > > + drm_prime_remove_buf_handle(&filp->prime, > > obj->import_attach->dmabuf); > > } > > if (obj->export_dma_buf) { > > - drm_prime_remove_imported_buf_handle(&filp->prime, > > + drm_prime_remove_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 366910d..b4ed808 100644 > > --- a/drivers/gpu/drm/drm_prime.c > > +++ b/drivers/gpu/drm/drm_prime.c > > @@ -57,10 +57,14 @@ > > * use the drm_gem_prime_{import,export} helpers. > > */ > > > > +#define PRIME_IMPORT 1 > > +#define PRIME_EXPORT 2 > > + > > struct drm_prime_member { > > struct list_head entry; > > struct dma_buf *dma_buf; > > uint32_t handle; > > + int type; > > }; > > > > static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach, > > @@ -157,6 +161,8 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = { > > .vunmap = drm_gem_dmabuf_vunmap, > > }; > > > > +static int drm_prime_add_exported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle); > > + > > /** > > * DOC: PRIME Helpers > > * > > @@ -200,7 +206,9 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, > > { > > struct drm_gem_object *obj; > > void *buf; > > - int ret; > > + int ret = 0; > > + struct dma_buf *dmabuf; > > + uint32_t exp_handle; > > > > obj = drm_gem_object_lookup(dev, file_priv, handle); > > if (!obj) > > @@ -209,43 +217,44 @@ 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); > > - 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); > > + 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 > > + */ > > + ret = PTR_ERR(buf); > > + goto out; > > } > > + 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); > > - if (ret) { > > - drm_gem_object_unreference_unlocked(obj); > > - mutex_unlock(&file_priv->prime.lock); > > - return ret; > > - } > > + ret = drm_prime_add_exported_buf_handle(&file_priv->prime, > > + obj->export_dma_buf, handle); > > + if (ret) > > + goto out; > > > > + *prime_fd = dma_buf_fd(buf, flags); > > mutex_unlock(&file_priv->prime.lock); > > return 0; > > + > > +out_have_obj: > > + get_dma_buf(dmabuf); > > + *prime_fd = dma_buf_fd(dmabuf, flags); > > +out: > > + drm_gem_object_unreference_unlocked(obj); > > + mutex_unlock(&file_priv->prime.lock); > > + return ret; > > } > > EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); > > > > @@ -314,7 +323,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; > > @@ -491,7 +500,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, int type) > > { > > struct drm_prime_member *member; > > > > @@ -501,12 +510,25 @@ int drm_prime_add_imported_buf_handle(struct drm_prime_file_private *prime_fpriv > > > > member->dma_buf = dma_buf; > > member->handle = handle; > > + member->type = type; > > 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, PRIME_IMPORT); > > +} > > 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, PRIME_EXPORT); > > +} > > + > > +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; > > > > @@ -518,19 +540,24 @@ 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) > > +void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf) > > { > > struct drm_prime_member *member, *safe; > > > > mutex_lock(&prime_fpriv->lock); > > list_for_each_entry_safe(member, safe, &prime_fpriv->head, entry) { > > if (member->dma_buf == dma_buf) { > > + if (member->type == PRIME_EXPORT) > > + dma_buf_put(dma_buf); > > list_del(&member->entry); > > kfree(member); > > } > > } > > mutex_unlock(&prime_fpriv->lock); > > } > > -EXPORT_SYMBOL(drm_prime_remove_imported_buf_handle); > > +EXPORT_SYMBOL(drm_prime_remove_buf_handle); > > + > > + > > + > > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > > index 2d94d74..69712a6 100644 > > --- a/include/drm/drmP.h > > +++ b/include/drm/drmP.h > > @@ -1594,8 +1594,8 @@ 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); > > -void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf); > > +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_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.2 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Thu, Apr 18, 2013 at 2:16 AM, Imre Deak <imre.deak@intel.com> wrote: > On Wed, 2013-04-17 at 10:21 +0200, Daniel Vetter wrote: >> On Wed, Apr 17, 2013 at 02:38:02PM +1000, Dave Airlie wrote: >> > Currently we have a problem with this: >> > 1. i915: create gem object >> > 2. i915: export gem object to prime >> > 3. radeon: import gem object >> > 4. close prime fd >> > 5. radeon: unref object >> > 6. i915: unref object >> > >> > i915 has an imported object reference in its file priv, that isn't >> > cleaned up properly until fd close. The reference gets added at step 2, >> > but at step 6 we don't have enough info to clean it up. >> > >> > The solution is to take a reference on the dma-buf when we export it, >> > and drop the reference when the gem handle goes away. >> > >> > So 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. >> > >> > This patch isn't meant to fix any other problem or bikesheds, and it doesn't >> > fix any races with other scenarios. >> > >> > 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. >> > >> > v2.1: rebase this patch, write better commit msg. >> > v3: cleanup error handling, track import vs export in linked list, >> > these two patches were separate previously, but seem to work better >> > like this. >> > v4: danvet is correct, this code is no longer useful, since the buffer >> > better exist, so remove it. >> > >> > Signed-off-by: Dave Airlie <airlied@redhat.com> >> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> >> On bikeshed review level I wonder whether we shouldn't just grab a >> reference unconditonally when inserting it into the handle_to_fd lookup >> lists. But I need to think through a few other cases and apparently the >> self-import test is still a bit broken. So this is material for follow-up >> patches. > > Yes, this is needed, otherwise closing the prime fd will leave stale > pointers to the dma buf in the importer's lookup table. I've sent an > update to igt/prime_self_test to intel-gfx for showing this. > Okay I've spent time on this today and yes I agree, so I'll post v5 of my patch with that in it. I'd like to also merge your other patch, it seems fine. Dave.
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index af779ae..cf919e3 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -205,11 +205,11 @@ static void drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp) { if (obj->import_attach) { - drm_prime_remove_imported_buf_handle(&filp->prime, + drm_prime_remove_buf_handle(&filp->prime, obj->import_attach->dmabuf); } if (obj->export_dma_buf) { - drm_prime_remove_imported_buf_handle(&filp->prime, + drm_prime_remove_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 366910d..b4ed808 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -57,10 +57,14 @@ * use the drm_gem_prime_{import,export} helpers. */ +#define PRIME_IMPORT 1 +#define PRIME_EXPORT 2 + struct drm_prime_member { struct list_head entry; struct dma_buf *dma_buf; uint32_t handle; + int type; }; static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach, @@ -157,6 +161,8 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = { .vunmap = drm_gem_dmabuf_vunmap, }; +static int drm_prime_add_exported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle); + /** * DOC: PRIME Helpers * @@ -200,7 +206,9 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, { struct drm_gem_object *obj; void *buf; - int ret; + int ret = 0; + struct dma_buf *dmabuf; + uint32_t exp_handle; obj = drm_gem_object_lookup(dev, file_priv, handle); if (!obj) @@ -209,43 +217,44 @@ 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); - 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); + 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 + */ + ret = PTR_ERR(buf); + goto out; } + 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); - if (ret) { - drm_gem_object_unreference_unlocked(obj); - mutex_unlock(&file_priv->prime.lock); - return ret; - } + ret = drm_prime_add_exported_buf_handle(&file_priv->prime, + obj->export_dma_buf, handle); + if (ret) + goto out; + *prime_fd = dma_buf_fd(buf, flags); mutex_unlock(&file_priv->prime.lock); return 0; + +out_have_obj: + get_dma_buf(dmabuf); + *prime_fd = dma_buf_fd(dmabuf, flags); +out: + drm_gem_object_unreference_unlocked(obj); + mutex_unlock(&file_priv->prime.lock); + return ret; } EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); @@ -314,7 +323,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; @@ -491,7 +500,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, int type) { struct drm_prime_member *member; @@ -501,12 +510,25 @@ int drm_prime_add_imported_buf_handle(struct drm_prime_file_private *prime_fpriv member->dma_buf = dma_buf; member->handle = handle; + member->type = type; 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, PRIME_IMPORT); +} 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, PRIME_EXPORT); +} + +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; @@ -518,19 +540,24 @@ 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) +void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf) { struct drm_prime_member *member, *safe; mutex_lock(&prime_fpriv->lock); list_for_each_entry_safe(member, safe, &prime_fpriv->head, entry) { if (member->dma_buf == dma_buf) { + if (member->type == PRIME_EXPORT) + dma_buf_put(dma_buf); list_del(&member->entry); kfree(member); } } mutex_unlock(&prime_fpriv->lock); } -EXPORT_SYMBOL(drm_prime_remove_imported_buf_handle); +EXPORT_SYMBOL(drm_prime_remove_buf_handle); + + + diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 2d94d74..69712a6 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1594,8 +1594,8 @@ 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); -void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf); +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_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,
Currently we have a problem with this: 1. i915: create gem object 2. i915: export gem object to prime 3. radeon: import gem object 4. close prime fd 5. radeon: unref object 6. i915: unref object i915 has an imported object reference in its file priv, that isn't cleaned up properly until fd close. The reference gets added at step 2, but at step 6 we don't have enough info to clean it up. The solution is to take a reference on the dma-buf when we export it, and drop the reference when the gem handle goes away. So 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. This patch isn't meant to fix any other problem or bikesheds, and it doesn't fix any races with other scenarios. 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. v2.1: rebase this patch, write better commit msg. v3: cleanup error handling, track import vs export in linked list, these two patches were separate previously, but seem to work better like this. v4: danvet is correct, this code is no longer useful, since the buffer better exist, so remove it. Signed-off-by: Dave Airlie <airlied@redhat.com> --- drivers/gpu/drm/drm_gem.c | 4 +- drivers/gpu/drm/drm_prime.c | 95 +++++++++++++++++++++++++++++---------------- include/drm/drmP.h | 4 +- 3 files changed, 65 insertions(+), 38 deletions(-)