[Intel-gfx] [PATCH v2] drm/i915/gt: Transfer old virtual breadcrumbs to irq_worker

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-05-13 09:10:48)
> 
> On 12/05/2020 17:20, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2020-05-12 17:07:23)
> >>
> >> On 12/05/2020 16:52, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2020-05-12 16:17:30)
> >>>>
> >>>> On 12/05/2020 14:22, Chris Wilson wrote:
> >>>>> -     spin_lock(&old->breadcrumbs.irq_lock);
> >>>>> -     if (!list_empty(&ve->context.signal_link)) {
> >>>>> -             list_del_init(&ve->context.signal_link);
> >>>>> -
> >>>>> -             /*
> >>>>> -              * We cannot acquire the new engine->breadcrumbs.irq_lock
> >>>>> -              * (as we are holding a breadcrumbs.irq_lock already),
> >>>>> -              * so attach this request to the signaler on submission.
> >>>>> -              * The queued irq_work will occur when we finally drop
> >>>>> -              * the engine->active.lock after dequeue.
> >>>>> -              */
> >>>>> -             set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &rq->fence.flags);
> >>>>> -
> >>>>> -             /* Also transfer the pending irq_work for the old breadcrumb. */
> >>>>> -             intel_engine_signal_breadcrumbs(rq->engine);
> >>>>> -     }
> >>>>> -     spin_unlock(&old->breadcrumbs.irq_lock);
> >>>>> +     intel_engine_transfer_breadcrumbs(ve->siblings[0], &ve->context);
> >>>>
> >>>> But isn't ve->siblings[0] the old engine at this point so new target
> >>>> engine would have to be explicitly passed in?
> >>>
> >>> ve->siblings[0] is the old engine, which is holding the completed
> >>> requests and their signals. Since their rq->engine == ve->siblings[0]
> >>> and we can't update rq->engine as we can't take the required locks, we
> >>> need to keep the breadcrumbs relative to ve->siblings[0] and not the new
> >>> engine (the i915_request_cancel_breadcrumb conundrum).
> >>
> >> Who then enables breadcrumbs on the new engine?
> > 
> > If we enable signaling on the request we are about to submit, that
> > will enable the breadcrumbs on the new engine. We've cleared out
> > ce->signals and removed it from the old engine, so as soon as we want to
> > enable a new breadcrumb, it will be attached to the [new] rq->engine.
> 
> So current code was wrong to think it needed to set 
> DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT?

That was a trick to try and use the next request to attach the
ve->context as a signaler on the new engine, and use that engine to
execute the irq_work. It was wrong because of how
i915_request_enable_breadcrumb() only attaches the context to the engine
when the list of breadcrumbs is empty. (It would not be empty as we know
the list is not empty and there may not be enough time on another cpu to
drain it.)

> Should it instead assert it is set? 
> Because if it is not set signalling on the new engine won't be enabled.

Not DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT itself, since we do allow waiters
to be attached prior to execution. !I915_FENCE_FLAG_ACTIVE is a
candidate, but it's a bit of a random spot to assert it.
-Chris