Commit 219b4733 authored by Dave Airlie's avatar Dave Airlie Committed by Dave Airlie
Browse files

drm/prime: keep a reference from the handle to exported dma-buf (v6) 



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.
v5: always take a reference to the dma buf object, import or export.
(Imre Deak contributed this originally)
v6: square the circle, remove import vs export tracking now
that there is no difference

Reviewed-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org
Signed-off-by: default avatarDave Airlie <airlied@redhat.com>
parent c55b6b3d

drivers/gpu/drm/drm_gem.c

+2 −2
Original line number Original line Diff line number Diff line
@@ -205,11 +205,11 @@ static void 
drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp)

drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp)

{

{

	if (obj->import_attach) {

	if (obj->import_attach) {

		drm_prime_remove_imported_buf_handle(&filp->prime,

		drm_prime_remove_buf_handle(&filp->prime,

				obj->import_attach->dmabuf);

				obj->import_attach->dmabuf);

	}

	}

	if (obj->export_dma_buf) {

	if (obj->export_dma_buf) {

		drm_prime_remove_imported_buf_handle(&filp->prime,

		drm_prime_remove_buf_handle(&filp->prime,

				obj->export_dma_buf);

				obj->export_dma_buf);

	}

	}

}

}

drivers/gpu/drm/drm_prime.c

+40 −36
Original line number Original line Diff line number Diff line
@@ -62,6 +62,7 @@ struct drm_prime_member { 
	struct dma_buf *dma_buf;

	struct dma_buf *dma_buf;

	uint32_t handle;

	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);





static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,

static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,

		enum dma_data_direction dir)

		enum dma_data_direction dir)
@@ -200,7 +201,8 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, 
{

{

	struct drm_gem_object *obj;

	struct drm_gem_object *obj;

	void *buf;

	void *buf;

	int ret;

	int ret = 0;

	struct dma_buf *dmabuf;





	obj = drm_gem_object_lookup(dev, file_priv, handle);

	obj = drm_gem_object_lookup(dev, file_priv, handle);

	if (!obj)

	if (!obj)
@@ -209,43 +211,44 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, 
	mutex_lock(&file_priv->prime.lock);

	mutex_lock(&file_priv->prime.lock);

	/* re-export the original imported object */

	/* re-export the original imported object */

	if (obj->import_attach) {

	if (obj->import_attach) {

		get_dma_buf(obj->import_attach->dmabuf);

		dmabuf = obj->import_attach->dmabuf;

		*prime_fd = dma_buf_fd(obj->import_attach->dmabuf, flags);

		goto out_have_obj;

		drm_gem_object_unreference_unlocked(obj);

		mutex_unlock(&file_priv->prime.lock);

		return 0;

	}

	}





	if (obj->export_dma_buf) {

	if (obj->export_dma_buf) {

		get_dma_buf(obj->export_dma_buf);

		dmabuf = obj->export_dma_buf;

		*prime_fd = dma_buf_fd(obj->export_dma_buf, flags);

		goto out_have_obj;

		drm_gem_object_unreference_unlocked(obj);

	}

	} else {



	buf = dev->driver->gem_prime_export(dev, obj, flags);

	buf = dev->driver->gem_prime_export(dev, obj, flags);

	if (IS_ERR(buf)) {

	if (IS_ERR(buf)) {

		/* normally the created dma-buf takes ownership of the ref,

		/* normally the created dma-buf takes ownership of the ref,

		 * but if that fails then drop the ref

		 * but if that fails then drop the ref

		 */

		 */

			drm_gem_object_unreference_unlocked(obj);

		ret = PTR_ERR(buf);

			mutex_unlock(&file_priv->prime.lock);

		goto out;

			return PTR_ERR(buf);

	}

	}

	obj->export_dma_buf = buf;

	obj->export_dma_buf = buf;

		*prime_fd = dma_buf_fd(buf, flags);



	}

	/* if we've exported this buffer the cheat and add it to the import list

	/* if we've exported this buffer the cheat and add it to the import list

	 * so we get the correct handle back

	 * so we get the correct handle back

	 */

	 */

	ret = drm_prime_add_imported_buf_handle(&file_priv->prime,

	ret = drm_prime_add_buf_handle(&file_priv->prime,

				       obj->export_dma_buf, handle);

				       obj->export_dma_buf, handle);

	if (ret) {

	if (ret)

		drm_gem_object_unreference_unlocked(obj);

		goto out;

		mutex_unlock(&file_priv->prime.lock);

		return ret;

	}





	*prime_fd = dma_buf_fd(buf, flags);

	mutex_unlock(&file_priv->prime.lock);

	mutex_unlock(&file_priv->prime.lock);

	return 0;

	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);

EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
@@ -314,7 +317,7 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, 




	mutex_lock(&file_priv->prime.lock);

	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);

			dma_buf, handle);

	if (!ret) {

	if (!ret) {

		ret = 0;

		ret = 0;
@@ -333,7 +336,7 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, 
	if (ret)

	if (ret)

		goto out_put;

		goto out_put;





	ret = drm_prime_add_imported_buf_handle(&file_priv->prime,

	ret = drm_prime_add_buf_handle(&file_priv->prime,

			dma_buf, *handle);

			dma_buf, *handle);

	if (ret)

	if (ret)

		goto fail;

		goto fail;
@@ -487,7 +490,7 @@ void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv) 
}

}

EXPORT_SYMBOL(drm_prime_destroy_file_private);

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;

	struct drm_prime_member *member;
@@ -495,14 +498,14 @@ int drm_prime_add_imported_buf_handle(struct drm_prime_file_private *prime_fpriv 
	if (!member)

	if (!member)

		return -ENOMEM;

		return -ENOMEM;





	get_dma_buf(dma_buf);

	member->dma_buf = dma_buf;

	member->dma_buf = dma_buf;

	member->handle = handle;

	member->handle = handle;

	list_add(&member->entry, &prime_fpriv->head);

	list_add(&member->entry, &prime_fpriv->head);

	return 0;

	return 0;

}

}

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)

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;

	struct drm_prime_member *member;
@@ -514,19 +517,20 @@ int drm_prime_lookup_imported_buf_handle(struct drm_prime_file_private *prime_fp 
	}

	}

	return -ENOENT;

	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;

	struct drm_prime_member *member, *safe;





	mutex_lock(&prime_fpriv->lock);

	mutex_lock(&prime_fpriv->lock);

	list_for_each_entry_safe(member, safe, &prime_fpriv->head, entry) {

	list_for_each_entry_safe(member, safe, &prime_fpriv->head, entry) {

		if (member->dma_buf == dma_buf) {

		if (member->dma_buf == dma_buf) {

			dma_buf_put(dma_buf);

			list_del(&member->entry);

			list_del(&member->entry);

			kfree(member);

			kfree(member);

		}

		}

	}

	}

	mutex_unlock(&prime_fpriv->lock);

	mutex_unlock(&prime_fpriv->lock);

}

}

EXPORT_SYMBOL(drm_prime_remove_imported_buf_handle);
 
EXPORT_SYMBOL(drm_prime_remove_buf_handle);

include/drm/drmP.h

+2 −3
Original line number Original line Diff line number Diff line
@@ -1593,9 +1593,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_init_file_private(struct drm_prime_file_private *prime_fpriv);

void drm_prime_destroy_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_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_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf);

void drm_prime_remove_imported_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_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,

int drm_prime_lookup_obj(struct drm_device *dev, struct dma_buf *buf,

CRA Git | Maintained and supported by SUSTech CRA and CCSE