[RFC v1] drm: add reference count of gem object name
Daniel Vetter
daniel at ffwll.ch
Fri Nov 30 08:26:06 PST 2012
Hi,
tbh I don't follow what exactly you're trying to fix, but the rules is:
The flink name stays around as long as there's a userspace handle, and
will be deleted once the last userspace handle is closed.
Which means that for process->process gem passing the sender _must_
keep the bo handle open until the receiver has confirmed that it has
opened the flink name. dma-buf makes this much easier, since the fd
you can passs over a unix socket holds a reference of its own. But for
gem flink name there's no way to create the same semantics without
creating resource leaks somewhere. So consider the idea nacked.
Cheers, Daniel
On Fri, Nov 30, 2012 at 10:10 AM, Inki Dae <inki.dae at samsung.com> wrote:
> 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)
> + 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)
> + 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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the dri-devel
mailing list