Messages in this thread | | | Subject | Re: [PATCH] Revert "drm/syncobj: use dma_fence_get_stub" | From | Christian König <> | Date | Thu, 8 Apr 2021 09:02:53 +0200 |
| |
Am 08.04.21 um 06:59 schrieb David Stevens: > From: David Stevens <stevensd@chromium.org> > > This reverts commit 86bbd89d5da66fe760049ad3f04adc407ec0c4d6. > > Using the singleton stub fence in drm_syncobj_assign_null_handle means > that all syncobjs created in an already signaled state or any syncobjs > signaled by userspace will reference the singleton fence when exported > to a sync_file. If those sync_files are queried with SYNC_IOC_FILE_INFO, > then the timestamp_ns value returned will correspond to whenever the > singleton stub fence was first initialized. This can break the ability > of userspace to use timestamps of these fences, as the singleton stub > fence's timestamp bears no relationship to any meaningful event.
And why exactly is having the timestamp of the call to drm_syncobj_assign_null_handle() better?
Additional if you really need that please don't revert the patch. Instead provide a function which returns a newly initialized stub fence in the dma_fence.c code.
Regards, Christian.
> > Signed-off-by: David Stevens <stevensd@chromium.org> > --- > drivers/gpu/drm/drm_syncobj.c | 58 ++++++++++++++++++++++++++--------- > 1 file changed, 44 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > index 349146049849..7cc11f1a83f4 100644 > --- a/drivers/gpu/drm/drm_syncobj.c > +++ b/drivers/gpu/drm/drm_syncobj.c > @@ -211,6 +211,21 @@ struct syncobj_wait_entry { > static void syncobj_wait_syncobj_func(struct drm_syncobj *syncobj, > struct syncobj_wait_entry *wait); > > +struct drm_syncobj_stub_fence { > + struct dma_fence base; > + spinlock_t lock; > +}; > + > +static const char *drm_syncobj_stub_fence_get_name(struct dma_fence *fence) > +{ > + return "syncobjstub"; > +} > + > +static const struct dma_fence_ops drm_syncobj_stub_fence_ops = { > + .get_driver_name = drm_syncobj_stub_fence_get_name, > + .get_timeline_name = drm_syncobj_stub_fence_get_name, > +}; > + > /** > * drm_syncobj_find - lookup and reference a sync object. > * @file_private: drm file private pointer > @@ -344,18 +359,24 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, > } > EXPORT_SYMBOL(drm_syncobj_replace_fence); > > -/** > - * drm_syncobj_assign_null_handle - assign a stub fence to the sync object > - * @syncobj: sync object to assign the fence on > - * > - * Assign a already signaled stub fence to the sync object. > - */ > -static void drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj) > +static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj) > { > - struct dma_fence *fence = dma_fence_get_stub(); > + struct drm_syncobj_stub_fence *fence; > > - drm_syncobj_replace_fence(syncobj, fence); > - dma_fence_put(fence); > + fence = kzalloc(sizeof(*fence), GFP_KERNEL); > + if (fence == NULL) > + return -ENOMEM; > + > + spin_lock_init(&fence->lock); > + dma_fence_init(&fence->base, &drm_syncobj_stub_fence_ops, > + &fence->lock, 0, 0); > + dma_fence_signal(&fence->base); > + > + drm_syncobj_replace_fence(syncobj, &fence->base); > + > + dma_fence_put(&fence->base); > + > + return 0; > } > > /* 5s default for wait submission */ > @@ -469,6 +490,7 @@ EXPORT_SYMBOL(drm_syncobj_free); > int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags, > struct dma_fence *fence) > { > + int ret; > struct drm_syncobj *syncobj; > > syncobj = kzalloc(sizeof(struct drm_syncobj), GFP_KERNEL); > @@ -479,8 +501,13 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags, > INIT_LIST_HEAD(&syncobj->cb_list); > spin_lock_init(&syncobj->lock); > > - if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) > - drm_syncobj_assign_null_handle(syncobj); > + if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) { > + ret = drm_syncobj_assign_null_handle(syncobj); > + if (ret < 0) { > + drm_syncobj_put(syncobj); > + return ret; > + } > + } > > if (fence) > drm_syncobj_replace_fence(syncobj, fence); > @@ -1322,8 +1349,11 @@ drm_syncobj_signal_ioctl(struct drm_device *dev, void *data, > if (ret < 0) > return ret; > > - for (i = 0; i < args->count_handles; i++) > - drm_syncobj_assign_null_handle(syncobjs[i]); > + for (i = 0; i < args->count_handles; i++) { > + ret = drm_syncobj_assign_null_handle(syncobjs[i]); > + if (ret < 0) > + break; > + } > > drm_syncobj_array_free(syncobjs, args->count_handles); >
| |