Re: [Intel-gfx] [PATCH 06/10] drm/syncobj: use the timeline point in drm_syncobj_find_fence v3

["Koenig, Christian" <Christian.Koenig-5C7GfCeVMHo@public.gmane.org>]

Am 13.12.18 um 12:37 schrieb Chris Wilson:
> Quoting Chunming Zhou (2018-12-11 10:34:45)
>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>>
>> Implement finding the right timeline point in drm_syncobj_find_fence.
>>
>> v2: return -EINVAL when the point is not submitted yet.
>> v3: fix reference counting bug, add flags handling as well
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/drm_syncobj.c | 43 ++++++++++++++++++++++++++++++++---
>>   1 file changed, 40 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
>> index 76ce13dafc4d..d964b348ecba 100644
>> --- a/drivers/gpu/drm/drm_syncobj.c
>> +++ b/drivers/gpu/drm/drm_syncobj.c
>> @@ -231,16 +231,53 @@ int drm_syncobj_find_fence(struct drm_file *file_private,
>>                             struct dma_fence **fence)
>>   {
>>          struct drm_syncobj *syncobj = drm_syncobj_find(file_private, handle);
>> -       int ret = 0;
>> +       struct syncobj_wait_entry wait;
>> +       int ret;
>>   
>>          if (!syncobj)
>>                  return -ENOENT;
>>   
>>          *fence = drm_syncobj_fence_get(syncobj);
>> -       if (!*fence) {
>> +       drm_syncobj_put(syncobj);
>> +
>> +       if (*fence) {
>> +               ret = dma_fence_chain_find_seqno(fence, point);
>> +               if (!ret)
>> +                       return 0;
>> +               dma_fence_put(*fence);
>> +       } else {
>>                  ret = -EINVAL;
>>          }
>> -       drm_syncobj_put(syncobj);
>> +
>> +       if (!(flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT))
>> +               return ret;
>> +
>> +       memset(&wait, 0, sizeof(wait));
>> +       wait.task = current;
>> +       wait.point = point;
>> +       drm_syncobj_fence_add_wait(syncobj, &wait);
>> +
>> +       do {
>> +               set_current_state(TASK_INTERRUPTIBLE);
>> +               if (wait.fence) {
>> +                       ret = 0;
>> +                       break;
>> +               }
>> +
>> +               if (signal_pending(current)) {
>> +                       ret = -ERESTARTSYS;
>> +                       break;
>> +               }
>> +
>> +               schedule();
>> +       } while (1);
> I've previously used a dma_fence_proxy so that we could do nonblocking
> waits on future submits. That would be preferrable (a requirement for
> our stupid BKL-driven code).

That is exactly what I would definitely NAK.

I would rather say we should come up with a wait_multiple_events() macro 
and completely nuke the custom implementation of this in:
1. dma_fence_default_wait and dma_fence_wait_any_timeout
2. the radeon fence implementation
3. the nouveau fence implementation
4. the syncobj code

Cause all of them do exactly the same. The dma_fence implementation 
unfortunately came up with a custom event handling mechanism instead of 
extending the core Linux wait_event() system.

This in turn lead to a lot of this duplicated handling.

Christian.

> -Chris

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx