2012/11/30 Inki Dae <inki.dae at 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)
> 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 <inki.dae at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/dri-devel/attachments/20121130/8df39b7e/attachment-0001.html>
