Messages in this thread | | | Subject | Re: [PATCH] Revert "drm/syncobj: use dma_fence_get_stub" | From | Christian König <> | Date | Thu, 8 Apr 2021 11:34:48 +0200 |
| |
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); >>>
| |