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

[Tvrtko Ursulin]

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? Should it instead assert it is set? 
Because if it is not set signalling on the new engine won't be enabled.

Regards,

Tvrtko