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

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Dec 13, 2018 at 12:24:57PM +0000, Koenig, Christian wrote:
> Am 13.12.18 um 13:21 schrieb Chris Wilson:
> > Quoting Koenig, Christian (2018-12-13 12:11:10)
> >> 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.
> > I don't want a blocking wait at all.
> 
> Ok I wasn't clear enough :) That is exactly what I would NAK!
> 
> The wait must be blocking or otherwise you would allow wait-before-signal.

Well the current implementation is pulling a rather big trick on readers
in this regard: It looks like a dma_fence, it's implemented as one even,
heck you even open-code a dma_fence_wait here.

Except the semantics are completely different.

So aside from the discussion whether we really want to fully chain them I
think it just doesn't make sense to implement the "wait for fence submit"
as a dma_fence wait. And I'd outright remove that part from the uapi, and
force the wait. The current radv/anv plans I discussed with Jason was that
we'd have a separate submit thread, and hence unconditionally blocking
until the fence has materialized is the right thing to do. Even allowing
that option, either through a flag, or making these things look like
dma_fences (they are _not_) just tricks folks into misunderstanding what's
going on. Code sharing just because the code looks similar is imo a really
bad idea, when the semantics are entirely different (that was also the
reason behind not reusing all the cpu event stuff for dma_fence, they're
not normal cpu events).
-Daniel

> 
> Christian.
> 
> > -Chris
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel