[Intel-gfx] [PATCH 4/4] drm/i915: Implement SINGLE_TIMELINE with a syncobj

[Jason Ekstrand]

On Tue, Mar 23, 2021 at 4:35 AM Tvrtko Ursulin
<tvrtko.ursulin at linux.intel.com> wrote:
>
>
> On 22/03/2021 16:10, Jason Ekstrand wrote:
> > On Mon, Mar 22, 2021 at 7:28 AM Tvrtko Ursulin
>
> [snip]
>
> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> >>> index 96403130a373d..2c56796f6a71b 100644
> >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> >>> @@ -3295,6 +3295,15 @@ i915_gem_do_execbuffer(struct drm_device *dev,
> >>>                goto err_vma;
> >>>        }
> >>>
> >>> +     if (eb.gem_context->syncobj) {
> >>> +             struct dma_fence *fence;
> >>> +
> >>> +             fence = drm_syncobj_fence_get(eb.gem_context->syncobj);
> >>
> >> Who drops this reference?
> >
> > i915_request_await_dma_fence() below consumes a reference.
>
> Not sure, please check on difference wrt input fence handling.

Gah.  You were right.  It takes a reference if it needs one.  I
thought I was being symmetric with the other syncobj usage but it was
well hidden inside our confusing tear-down paths.

> >>> +             err = i915_request_await_dma_fence(eb.request, fence);
> >>> +             if (err)
> >>> +                     goto err_ext;
> >>> +     }
> >>> +
> >>>        if (in_fence) {
> >>>                if (args->flags & I915_EXEC_FENCE_SUBMIT)
> >>>                        err = i915_request_await_execution(eb.request,
> >>> @@ -3351,6 +3360,12 @@ i915_gem_do_execbuffer(struct drm_device *dev,
> >>>                        fput(out_fence->file);
> >>>                }
> >>>        }
> >>> +
> >>> +     if (eb.gem_context->syncobj) {

At Daniel's request, I've wrapped these in unlikely()

> >>> +             drm_syncobj_replace_fence(eb.gem_context->syncobj,
> >>> +                                       &eb.request->fence);
> >>> +     }
> >>> +
> >>>        i915_request_put(eb.request);
> >>>
> >>>    err_vma:
> >>>
> >>
> >> So essentially moving the synchronisation to top level which is extra
> >> work, but given limited and questionable usage of the uapi may be
> >> acceptable. Need full picture on motivation to understand.
> >
> > For one thing, the GuC scheduler doesn't natively have a concept of
> > "timelines" which can be shared like this.  To work with the GuC
>
> Confused - neither does execlists. It is handled in common layer in
> i915. GuC scheduler has the same concept of one hw context is one
> timeline because that is the hw concept and not backend specific.
>
> > scheduler as currently proposed in DII, they've asked the media driver
> > to stop using this flag in favor of passing a sync file from batch to
> > batch.  If we want to slide GuC scheduling in smoothly, we've got to
> > keep it working.  This means either making timelines a concept there
> > or doing an emulation like this.
>
> Hm not aware and don't see that GuC backend can't or doesn't implement
> this. Perhaps this would be best discussed once GuC patches are posted.
>
> >> Semantics are also not 1:1 since dma fence context will be different.
> >
> > Could you elaborate?
>
> Exported dma fence context as an "timeline" id will be single with the
> current patch and multiple contexts with this implementation.
>
> Daniel also raised another difference caused by lack of serialisation
> due multiple tl->mutex here.
>
> I don't think this is important, it was never part of a contract what
> happens with racing execbufs, but it is definitely required covering
> both topics in the commit message.

I've updated the commit message as follows:

    drm/i915: Implement SINGLE_TIMELINE with a syncobj (v2)

    This API is entirely unnecessary and I'd love to get rid of it.  If
    userspace wants a single timeline across multiple contexts, they can
    either use implicit synchronization or a syncobj, both of which existed
    at the time this feature landed.  The justification given at the time
    was that it would help GL drivers which are inherently single-timeline.
    However, neither of our GL drivers actually wanted the feature.  i965
    was already in maintenance mode at the time and iris uses syncobj for
    everything.

    Unfortunately, as much as I'd love to get rid of it, it is used by the
    media driver so we can't do that.  We can, however, do the next-best
    thing which is to embed a syncobj in the context and do exactly what
    we'd expect from userspace internally.  This isn't an entirely identical
    implementation because it's no longer atomic if userspace races with
    itself by calling execbuffer2 twice simultaneously from different
    threads.  It won't crash in that case; it just doesn't guarantee any
    ordering between those two submits.

    Moving SINGLE_TIMELINE to a syncobj emulation has a couple of technical
    advantages beyond mere annoyance.  One is that intel_timeline is no
    longer an api-visible object and can remain entirely an implementation
    detail.  This may be advantageous as we make scheduler changes going
    forward.  Second is that, together with deleting the CLONE_CONTEXT API,
    we should now have a 1:1 mapping between intel_context and
    intel_timeline which may help us reduce locking.

I hope that clears up some of the confusion and is less bothersome.

--Jason