2012/12/1 Daniel Vetter <daniel@xxxxxxxx>
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.
Right, this is becasue now drm gem framework doesn't gaurantee that.
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.
Right, actually, I tried to resolve that but couldn't find good way. Anyway what this patch tries to do is different.
As you know, when the handle is closed, the flink name is also released even though another process opened the flink name to get its own handle.
So the flink name becomes invalid. This is the issue I pointed out and this is the fixup this patch tries to do.
I think flink name should have dependency of the gem object rather than process.
In other words, gem flink name couldn't be used as global key becasue it can't gurantee that the fink name is pointing to same gem object.
This patch makes the gem flink name valid as long as the gem object isn't released instead of handle.
And this is the reason that we need this patch.
Thanks,
Inki Dae
Cheers, Daniel
--
On Fri, Nov 30, 2012 at 10:10 AM, Inki Dae <inki.dae@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
