From patchwork Wed Apr 10 00:56:43 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dave Airlie X-Patchwork-Id: 2418891 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by patchwork1.kernel.org (Postfix) with ESMTP id C1CBF3FD8C for ; Wed, 10 Apr 2013 00:57:44 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id C11C5E63E2 for ; Tue, 9 Apr 2013 17:57:44 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by gabe.freedesktop.org (Postfix) with ESMTP id 67885E602B for ; Tue, 9 Apr 2013 17:56:48 -0700 (PDT) Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3A0ulJ7011060 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 9 Apr 2013 20:56:48 -0400 Received: from prime.bne.redhat.com (dhcp-40-183.bne.redhat.com [10.64.40.183]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r3A0ujTV002835 for ; Tue, 9 Apr 2013 20:56:47 -0400 From: Dave Airlie To: dri-devel@lists.freedesktop.org Subject: [PATCH] drm/prime: keep a reference from the handle to exported dma-buf (v2.1) Date: Wed, 10 Apr 2013 10:56:43 +1000 Message-Id: <1365555403-31838-2-git-send-email-airlied@gmail.com> In-Reply-To: <1365555403-31838-1-git-send-email-airlied@gmail.com> References: <1365555403-31838-1-git-send-email-airlied@gmail.com> X-Scanned-By: MIMEDefang 2.67 on 10.5.11.11 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org Errors-To: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org 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. Signed-off-by: Dave Airlie Acked-by: Imre Deak --- drivers/gpu/drm/drm_gem.c | 2 +- drivers/gpu/drm/drm_prime.c | 94 ++++++++++++++++++++++++++++++++------------- include/drm/drmP.h | 3 +- 3 files changed, 71 insertions(+), 28 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index af779ae..f0f7a86 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 366910d..05ee250 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -157,6 +157,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 * @@ -201,6 +203,8 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, struct drm_gem_object *obj; void *buf; int ret; + struct dma_buf *dmabuf; + uint32_t exp_handle; obj = drm_gem_object_lookup(dev, file_priv, handle); if (!obj) @@ -209,41 +213,52 @@ 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: + 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; } @@ -314,7 +329,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 +506,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) +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; @@ -504,9 +519,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; @@ -518,9 +545,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) +void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf) { struct drm_prime_member *member, *safe; @@ -533,4 +560,19 @@ 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) +{ + return 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 2d94d74..0466808 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1594,8 +1594,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,