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);