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