| Message ID | 20210408045926.3202160-1-stevensd@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Revert "drm/syncobj: use dma_fence_get_stub" | expand |
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); >
On Thu, Apr 8, 2021 at 4:03 PM Christian König <christian.koenig@amd.com> wrote: > > 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? The timestamp returned by SYNC_IOC_FILE_INFO is the "timestamp of status change in nanoseconds". If userspace signals the fence with DRM_IOCTL_SYNCOBJ_SIGNAL, then a timestamp from drm_syncobj_assign_null_handle corresponds to the status change. If userspace sets DRM_SYNCOBJ_CREATE_SIGNALED when creating a fence, then the status change happens immediately upon creation, which again corresponds to when drm_syncobj_assign_null_handle gets called. > 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. Ack. -David > 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); > > >
Am 08.04.21 um 11:30 schrieb David Stevens: > On Thu, Apr 8, 2021 at 4:03 PM Christian König <christian.koenig@amd.com> wrote: >> 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? > The timestamp returned by SYNC_IOC_FILE_INFO is the "timestamp of > status change in nanoseconds". If userspace signals the fence with > DRM_IOCTL_SYNCOBJ_SIGNAL, then a timestamp from > drm_syncobj_assign_null_handle corresponds to the status change. If > userspace sets DRM_SYNCOBJ_CREATE_SIGNALED when creating a fence, then > the status change happens immediately upon creation, which again > corresponds to when drm_syncobj_assign_null_handle gets called. Ok, that makes sense. > >> 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. > Ack. Just add a something like dma_fence_get_new_stub() with kmalloc(), dma_fence_init() and dma_fence_signal(). Shouldn't be more than a six liner. Thanks, Christian. > > -David > >> 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); >>>
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);