[RESEND,RFC,v1] drm: add reference count of gem object name

Commit Message

Inki Dae Nov. 30, 2012, 1:58 p.m. UTC

From: Inki Dae <[e-mail]>

Hello, all.

This patch introduces reference count of gem object name and resolves
the below issue.

In case that proces A shares its own gem object with process B,
process B opens a gem object name from process A to get its own
gem handle. But if process A closes the gem handle, the gem object
name to this is also released. So the gem object name that process
B referring to becomes invalid. I'm not sure that this is issue or
not but at least, gem object name IS NOT process-unique but IS
gem object-unique. So I think the gem object name shared by process A
should be preserved as long as the gem object has not been released.

Below is simple example.

at P1:
1. gem create	=>	obj_refcount = 1
2. gem flink	=>	obj_refcount = 2  (allocate obj_name)
5. gem close
	a. obj_refcount = 2 and release the obj_name
	b. obj_refcount = 1 once the obj_name release

3. and share it with P2

at P2:
4. gem open	=>	obj_refcount = 3
6. the obj_name from P1 is invalid.
7. gem close	=>	obj_refcount = 0(released)

And with this patch,

at P1:
1. gem create	=>	obj_refcount = 1, name_refcount = 0
2. gem flink	=>	obj_refcount = 2, name_refcount = 1  (allocate obj_name)
5. gem close	=>	obj_refcount = 2, name_refcount = 1

3. and share it with P2

at P2:
4. gem open	=>	obj_refcount = 3, name_refcount = 2
6. the gem object name from P1 is valid.
7. gem close
	a. obj_refcount = 1, name_refcount = 0(released)
	b. obj_refcount = 0(released) once name_ref_count = 0

There may be my missing point so please give me any advice.

Thanks,
Inki Dae

Signed-off-by: Inki Dae <[e-mail]>
---
 drivers/gpu/drm/drm_gem.c | 37 ++++++++++++++++++++++++++++++++++---
 include/drm/drmP.h        | 12 ++++++++++++
 2 files changed, 46 insertions(+), 3 deletions(-)

Patch

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 24efae4..946f46c 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -145,6 +145,7 @@  int drm_gem_object_init(struct drm_device *dev,
 
 	kref_init(&obj->refcount);
 	atomic_set(&obj->handle_count, 0);
+	atomic_set(&obj->obj_name_count, 0);
 	obj->size = size;
 
 	return 0;
@@ -166,6 +167,7 @@  int drm_gem_private_object_init(struct drm_device *dev,
 
 	kref_init(&obj->refcount);
 	atomic_set(&obj->handle_count, 0);
+	atomic_set(&obj->obj_name_count, 0);
 	obj->size = size;
 
 	return 0;
@@ -452,6 +454,16 @@  drm_gem_flink_ioctl(struct drm_device *dev, void *data,
 		return -ENOENT;
 
 again:
+	/*
+	 * return current object name increasing reference count of
+	 * this object name if exists already.
+	 */
+	if (obj->name) {
+		args->name = (uint64_t)obj->name;
+		drm_gem_object_unreference_unlocked(obj);
+		return 0;
+	}
+
 	if (idr_pre_get(&dev->object_name_idr, GFP_KERNEL) == 0) {
 		ret = -ENOMEM;
 		goto err;
@@ -461,6 +473,7 @@  again:
 	if (!obj->name) {
 		ret = idr_get_new_above(&dev->object_name_idr, obj, 1,
 					&obj->name);
+		atomic_set(&obj->obj_name_count, 1);
 		args->name = (uint64_t) obj->name;
 		spin_unlock(&dev->object_name_lock);
 
@@ -502,8 +515,10 @@  drm_gem_open_ioctl(struct drm_device *dev, void *data,
 
 	spin_lock(&dev->object_name_lock);
 	obj = idr_find(&dev->object_name_idr, (int) args->name);
-	if (obj)
+	if (obj) {
 		drm_gem_object_reference(obj);
+		atomic_inc(&obj->obj_name_count);
+	}
 	spin_unlock(&dev->object_name_lock);
 	if (!obj)
 		return -ENOENT;
@@ -612,9 +627,25 @@  void drm_gem_object_handle_free(struct drm_gem_object *obj)
 	/* Remove any name for this object */
 	spin_lock(&dev->object_name_lock);
 	if (obj->name) {
-		idr_remove(&dev->object_name_idr, obj->name);
-		obj->name = 0;
+		/*
+		 * The name of this object could being referenced
+		 * by another process so remove the name after checking
+		 * the obj_name_count of this object.
+		 */
+		if (atomic_dec_and_test(&obj->obj_name_count)) {
+			idr_remove(&dev->object_name_idr, obj->name);
+			obj->name = 0;
+		} else {
+			/*
+			 * this object name is being referenced by other yet
+			 * so do not unreference this.
+			 */
+			spin_unlock(&dev->object_name_lock);
+			return;
+		}
+
 		spin_unlock(&dev->object_name_lock);
+
 		/*
 		 * The object name held a reference to this object, drop
 		 * that now.
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index fad21c9..27657b8 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -628,6 +628,18 @@  struct drm_gem_object {
 	/** Handle count of this object. Each handle also holds a reference */
 	atomic_t handle_count; /* number of handles on this object */
 
+	/*
+	 * Name count of this object.
+	 *
+	 * This count is initialized as 0 with drm_gem_object_init or
+	 * drm_gem_private_object_init call. After that, this count is
+	 * increased if the name of this object exists already
+	 * otherwise is set to 1 at flink. And finally, the name of
+	 * this object will be released when this count reaches 0
+	 * by gem close.
+	 */
+	atomic_t obj_name_count;
+
 	/** Related drm device */
 	struct drm_device *dev;