On Fri, 2013-04-19 at 11:11 +1000, Dave Airlie wrote:
Currently we have a problem with this:
- i915: create gem object
- i915: export gem object to prime
- radeon: import gem object
- close prime fd
- radeon: unref object
- 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. v5: always take a reference to the dma buf object, import or export. (Imre Deak contributed this originally)
Signed-off-by: Dave Airlie airlied@redhat.com
drm_prime_destroy_file_private() lacks a drm_buf_put(). A separate issue, but the list should actually be empty at that point so perhaps we should warn if it's not.
bikeshed: we don't really need the PRIME_{EXPORT,IMPORT} tracking.
Otherwise looks ok: Reviewed-by: Imre Deak imre.deak@intel.com
drivers/gpu/drm/drm_gem.c | 4 +- drivers/gpu/drm/drm_prime.c | 93 ++++++++++++++++++++++++++++----------------- include/drm/drmP.h | 4 +- 3 files changed, 63 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);
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..eb7c38d 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,8 @@ 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;
obj = drm_gem_object_lookup(dev, file_priv, handle); if (!obj)
@@ -209,43 +216,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 +322,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 +499,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;
@@ -499,14 +507,27 @@ int drm_prime_add_imported_buf_handle(struct drm_prime_file_private *prime_fpriv if (!member) return -ENOMEM;
- get_dma_buf(dma_buf); 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 */
- 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 +539,23 @@ 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) {
dma_buf_put(dma_buf); list_del(&member->entry); kfree(member);} -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,