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

2012/11/30 Inki Dae inki.dae@samsung.com

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)

3. gem close a. obj_refcount = 2 and release the obj_name b. obj_refcount = 1 once the obj_name release

4. 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 inki.dae@samsung.com

drivers/gpu/drm/drm_gem.c | 41 ++++++++++++++++++++++++++++++++++++++--- include/drm/drmP.h | 12 ++++++++++++ 2 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 24efae4..16e4b75 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,19 @@ 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 and not same process.
  

•    */
  

•   if (obj->name) {
  

•           if (file_priv->pid != current->pid)
  
  

This condition should be removed and placed with another. It's my mistake.

•                   atomic_inc(&obj->obj_name_count);
  

•           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 +476,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 +518,11 @@ 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);
  

•           if (file_priv->pid != current->pid)
  
  

ditto. For this, will re-send it.

•                   atomic_inc(&obj->obj_name_count);
  

•   }
    spin_unlock(&dev->object_name_lock);
    if (!obj)
            return -ENOENT;
  

@@ -612,9 +631,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;
  
  

-- 1.7.4.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel