<br><br><div class="gmail_quote">2012/12/1 Daniel Vetter <span dir="ltr"><<a href="mailto:daniel@ffwll.ch" target="_blank">daniel@ffwll.ch</a>></span><br><blockquote style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid" class="gmail_quote">
Hi,<br>
<br>
tbh I don't follow what exactly you're trying to fix, but the rules is:<br>
<br>
The flink name stays around as long as there's a userspace handle, and<br>
will be deleted once the last userspace handle is closed.<br></blockquote><blockquote style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid" class="gmail_quote">
<br>
Which means that for process->process gem passing the sender _must_<br>
keep the bo handle open until the receiver has confirmed that it has<br>
opened the flink name.</blockquote><div> </div><div>Right, this is becasue now drm gem framework doesn't gaurantee that.</div><div> </div><blockquote style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid" class="gmail_quote">
dma-buf makes this much easier, since the fd<br>
you can passs over a unix socket holds a reference of its own. But for<br>
gem flink name there's no way to create the same semantics without<br>
creating resource leaks somewhere. So consider the idea nacked.<br></blockquote><div> </div><div>Right, actually, I tried to resolve that but couldn't find good way. Anyway what this patch tries to do is different.</div>
<div> </div><div>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.</div><div>So the flink name becomes invalid. This is the issue I pointed out and this is the fixup this patch tries to do.</div>
<div> </div><div>I think flink name should have dependency of the gem object rather than process.</div><div>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.</div>
<div>This patch makes the gem flink name valid as long as the gem object isn't released instead of handle.</div><div>And this is the reason that we need this patch.</div></div><div class="gmail_quote"><div> </div><div>
Thanks,</div><div>Inki Dae</div><div> </div><blockquote style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid" class="gmail_quote">
<br>
Cheers, Daniel<br>
<div class="HOEnZb"><div class="h5"><br>
On Fri, Nov 30, 2012 at 10:10 AM, Inki Dae <<a href="mailto:inki.dae@samsung.com">inki.dae@samsung.com</a>> wrote:<br>
> Hello, all.<br>
><br>
> This patch introduces reference count of gem object name and resolves<br>
> the below issue.<br>
><br>
> In case that proces A shares its own gem object with process B,<br>
> process B opens a gem object name from process A to get its own<br>
> gem handle. But if process A closes the gem handle, the gem object<br>
> name to this is also released. So the gem object name that process<br>
> B referring to becomes invalid. I'm not sure that this is issue or<br>
> not but at least, gem object name IS NOT process-unique but IS<br>
> gem object-unique. So I think the gem object name shared by process A<br>
> should be preserved as long as the gem object has not been released.<br>
><br>
> Below is simple example.<br>
><br>
> at P1:<br>
> 1. gem create => obj_refcount = 1<br>
> 2. gem flink => obj_refcount = 2 (allocate obj_name)<br>
> 5. gem close<br>
> a. obj_refcount = 2 and release the obj_name<br>
> b. obj_refcount = 1 once the obj_name release<br>
><br>
> 3. and share it with P2<br>
><br>
> at P2:<br>
> 4. gem open => obj_refcount = 3<br>
> 6. the obj_name from P1 is invalid.<br>
> 7. gem close => obj_refcount = 0(released)<br>
><br>
> And with this patch,<br>
><br>
> at P1:<br>
> 1. gem create => obj_refcount = 1, name_refcount = 0<br>
> 2. gem flink => obj_refcount = 2, name_refcount = 1 (allocate obj_name)<br>
> 5. gem close => obj_refcount = 2, name_refcount = 1<br>
><br>
> 3. and share it with P2<br>
><br>
> at P2:<br>
> 4. gem open => obj_refcount = 3, name_refcount = 2<br>
> 6. the gem object name from P1 is valid.<br>
> 7. gem close<br>
> a. obj_refcount = 1, name_refcount = 0(released)<br>
> b. obj_refcount = 0(released) once name_ref_count = 0<br>
><br>
> There may be my missing point so please give me any advice.<br>
><br>
> Thanks,<br>
> Inki Dae<br>
><br>
> Signed-off-by: Inki Dae <<a href="mailto:inki.dae@samsung.com">inki.dae@samsung.com</a>><br>
> ---<br>
> drivers/gpu/drm/drm_gem.c | 41 ++++++++++++++++++++++++++++++++++++++---<br>
> include/drm/drmP.h | 12 ++++++++++++<br>
> 2 files changed, 50 insertions(+), 3 deletions(-)<br>
><br>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c<br>
> index 24efae4..16e4b75 100644<br>
> --- a/drivers/gpu/drm/drm_gem.c<br>
> +++ b/drivers/gpu/drm/drm_gem.c<br>
> @@ -145,6 +145,7 @@ int drm_gem_object_init(struct drm_device *dev,<br>
><br>
> kref_init(&obj->refcount);<br>
> atomic_set(&obj->handle_count, 0);<br>
> + atomic_set(&obj->obj_name_count, 0);<br>
> obj->size = size;<br>
><br>
> return 0;<br>
> @@ -166,6 +167,7 @@ int drm_gem_private_object_init(struct drm_device *dev,<br>
><br>
> kref_init(&obj->refcount);<br>
> atomic_set(&obj->handle_count, 0);<br>
> + atomic_set(&obj->obj_name_count, 0);<br>
> obj->size = size;<br>
><br>
> return 0;<br>
> @@ -452,6 +454,19 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data,<br>
> return -ENOENT;<br>
><br>
> again:<br>
> + /*<br>
> + * return current object name increasing reference count of<br>
> + * this object name if exists already and not same process.<br>
> + */<br>
> + if (obj->name) {<br>
> + if (file_priv->pid != current->pid)<br>
> + atomic_inc(&obj->obj_name_count);<br>
> +<br>
> + args->name = (uint64_t)obj->name;<br>
> + drm_gem_object_unreference_unlocked(obj);<br>
> + return 0;<br>
> + }<br>
> +<br>
> if (idr_pre_get(&dev->object_name_idr, GFP_KERNEL) == 0) {<br>
> ret = -ENOMEM;<br>
> goto err;<br>
> @@ -461,6 +476,7 @@ again:<br>
> if (!obj->name) {<br>
> ret = idr_get_new_above(&dev->object_name_idr, obj, 1,<br>
> &obj->name);<br>
> + atomic_set(&obj->obj_name_count, 1);<br>
> args->name = (uint64_t) obj->name;<br>
> spin_unlock(&dev->object_name_lock);<br>
><br>
> @@ -502,8 +518,11 @@ drm_gem_open_ioctl(struct drm_device *dev, void *data,<br>
><br>
> spin_lock(&dev->object_name_lock);<br>
> obj = idr_find(&dev->object_name_idr, (int) args->name);<br>
> - if (obj)<br>
> + if (obj) {<br>
> drm_gem_object_reference(obj);<br>
> + if (file_priv->pid != current->pid)<br>
> + atomic_inc(&obj->obj_name_count);<br>
> + }<br>
> spin_unlock(&dev->object_name_lock);<br>
> if (!obj)<br>
> return -ENOENT;<br>
> @@ -612,9 +631,25 @@ void drm_gem_object_handle_free(struct drm_gem_object *obj)<br>
> /* Remove any name for this object */<br>
> spin_lock(&dev->object_name_lock);<br>
> if (obj->name) {<br>
> - idr_remove(&dev->object_name_idr, obj->name);<br>
> - obj->name = 0;<br>
> + /*<br>
> + * The name of this object could being referenced<br>
> + * by another process so remove the name after checking<br>
> + * the obj_name_count of this object.<br>
> + */<br>
> + if (atomic_dec_and_test(&obj->obj_name_count)) {<br>
> + idr_remove(&dev->object_name_idr, obj->name);<br>
> + obj->name = 0;<br>
> + } else {<br>
> + /*<br>
> + * this object name is being referenced by other yet<br>
> + * so do not unreference this.<br>
> + */<br>
> + spin_unlock(&dev->object_name_lock);<br>
> + return;<br>
> + }<br>
> +<br>
> spin_unlock(&dev->object_name_lock);<br>
> +<br>
> /*<br>
> * The object name held a reference to this object, drop<br>
> * that now.<br>
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h<br>
> index fad21c9..27657b8 100644<br>
> --- a/include/drm/drmP.h<br>
> +++ b/include/drm/drmP.h<br>
> @@ -628,6 +628,18 @@ struct drm_gem_object {<br>
> /** Handle count of this object. Each handle also holds a reference */<br>
> atomic_t handle_count; /* number of handles on this object */<br>
><br>
> + /*<br>
> + * Name count of this object.<br>
> + *<br>
> + * This count is initialized as 0 with drm_gem_object_init or<br>
> + * drm_gem_private_object_init call. After that, this count is<br>
> + * increased if the name of this object exists already<br>
> + * otherwise is set to 1 at flink. And finally, the name of<br>
> + * this object will be released when this count reaches 0<br>
> + * by gem close.<br>
> + */<br>
> + atomic_t obj_name_count;<br>
> +<br>
> /** Related drm device */<br>
> struct drm_device *dev;<br>
><br>
> --<br>
> 1.7.4.1<br>
><br>
> _______________________________________________<br>
> dri-devel mailing list<br>
> <a href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a><br>
> <a href="http://lists.freedesktop.org/mailman/listinfo/dri-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a><br>
<br>
<br>
<br>
</div></div><span class="HOEnZb"><font color="#888888">--<br>
Daniel Vetter<br>
Software Engineer, Intel Corporation<br>
+41 (0) 79 365 57 48 - <a href="http://blog.ffwll.ch" target="_blank">http://blog.ffwll.ch</a><br>
</font></span><div class="HOEnZb"><div class="h5">_______________________________________________<br>
dri-devel mailing list<br>
<a href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/dri-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a><br>
</div></div></blockquote></div><br>