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 at redhat.com>
>
> Reviewed-by: Daniel Vetter <daniel.vetter at 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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
