| Message ID | 1354266615-14013-1-git-send-email-inki.dae@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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) > 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@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 >
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@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@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
2012/12/1 Daniel Vetter <daniel@ffwll.ch> > 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@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@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@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 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Sat, Dec 1, 2012 at 3:26 AM, Inki Dae <inki.dae@samsung.com> wrote: > 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. Like I've said, as long as there's a userspace handle around, the flink name should not disappear. This additional handle reference count is implemented with obj->handle_count, which is incremented in drm_gem_handle_create. This function is called both from flink and from open ioctls. And that reference is dropped again in drm_gem_object_release_handle and the two drm_gem_object_handle_unreference variants. So I guess there's a bug somewhere, so can you please provide a minimal testcase which showcases your issue? -Daniel
On Sat, Dec 1, 2012 at 12:30 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Sat, Dec 1, 2012 at 3:26 AM, Inki Dae <inki.dae@samsung.com> wrote: >> 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. > > Like I've said, as long as there's a userspace handle around, the > flink name should not disappear. This additional handle reference > count is implemented with obj->handle_count, which is incremented in > drm_gem_handle_create. This function is called both from flink and > from open ioctls. And that reference is dropped again in > drm_gem_object_release_handle and the two > drm_gem_object_handle_unreference variants. > > So I guess there's a bug somewhere, so can you please provide a > minimal testcase which showcases your issue? I've quickly implemented a testcase for what I believe is your use-case (i.e. sharing between 3 different open drm files, where we close the gem handle in the first fd before we open the flink name again using the 3rd drm fd handle). It seems to work. -Daniel
On Sat, Dec 1, 2012 at 1:11 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > I've quickly implemented a testcase for what I believe is your > use-case (i.e. sharing between 3 different open drm files, where we > close the gem handle in the first fd before we open the flink name > again using the 3rd drm fd handle). It seems to work. Now with link to testcase .... http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/commit/?id=6daae8bcb6b4a930c9677f87fa24675581b69072 Cheers, Daniel
2012/12/1 Daniel Vetter <daniel@ffwll.ch> > On Sat, Dec 1, 2012 at 1:11 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > I've quickly implemented a testcase for what I believe is your > > use-case (i.e. sharing between 3 different open drm files, where we > > close the gem handle in the first fd before we open the flink name > > again using the 3rd drm fd handle). It seems to work. > > Now with link to testcase .... > > http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/commit/?id=6daae8bcb6b4a930c9677f87fa24675581b69072 > > Cheers, Daniel > I have checked the above testcase and I found out there was my missing point. The missing point is that when drm_gem_handle_create() is called by drm_gem_open_ioctl(), obj->handle_count also is increased. As you mentioned, it seems like that our user side has some bug somewhere. Actually we had tested this on Xorg so this is complicated a little bit. Thanks for your checking. :) Thanks, Inki Dae And below addes my comments on your codes. /* obj->handle_count = 1, obj->refcount = 1 */ ret = ioctl(fd2, DRM_IOCTL_I915_GEM_CREATE, &create); assert(ret == 0); flink.handle = create.handle; /* obj->handle_count = 1, obj->refcount = 2, flink name is allocated */ /* flink_ioctl just increases only obj->refcount. */ ret = ioctl(fd2, DRM_IOCTL_GEM_FLINK, &flink); assert(ret == 0); gem_open.name = flink.name; /* obj->handle_count = 2, obj->refcount = 3 */ /* drm_gem_handle_create() increases obj->handle_count and obj->refcount. This was my missing point. */ ret = ioctl(fd, DRM_IOCTL_GEM_OPEN, &gem_open); assert(ret == 0); assert(gem_open.handle != 0); /* obj->handle_count = 1, obj->refcount = 2 */ /* obj->handle_count = 1 so flink name isn't released */ close(fd2); fd2 = drm_open_any(); gem_open.name = flink.name; /* obj->handle_count = 2, obj->refcount = 3 */ ret = ioctl(fd2, DRM_IOCTL_GEM_OPEN, &gem_open); assert(ret == 0); assert(gem_open.handle != 0); } And codes I added to release gem like below, /* obj->handle_count = 1, obj->refcount = 2 */ ioctl(fd2, XXX_GEM_CLOSE, handle) /* obj->handle_count = 0, obj->refcount = 1 when flink name is released and then obj->refcount = 0 so gem object can be released */ ioctl(fd, XXX_GEM_CLOSE, handle) > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
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;
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@samsung.com> --- drivers/gpu/drm/drm_gem.c | 41 ++++++++++++++++++++++++++++++++++++++--- include/drm/drmP.h | 12 ++++++++++++ 2 files changed, 50 insertions(+), 3 deletions(-)