[Intel-gfx] [PATCH 16/38] drm/i915: Introduce a context barrier callback

[Tvrtko Ursulin]

On 01/03/2019 19:03, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-03-01 16:12:33)
>>
>> On 01/03/2019 14:03, Chris Wilson wrote:
>>> +     counter = 0;
>>> +     err = context_barrier_task(ctx, 0, mock_barrier_task, &counter);
>>> +     if (err) {
>>> +             pr_err("Failed at line %d, err=%d\n", __LINE__, err);
>>> +             goto out;
>>> +     }
>>> +     if (counter == 0) {
>>> +             pr_err("Did not retire immediately with 0 engines\n");
>>> +             err = -EINVAL;
>>> +             goto out;
>>> +     }
>>> +
>>> +     counter = 0;
>>> +     err = context_barrier_task(ctx, -1, mock_barrier_task, &counter);
>>> +     if (err) {
>>> +             pr_err("Failed at line %d, err=%d\n", __LINE__, err);
>>> +             goto out;
>>> +     }
>>> +     if (counter == 0) {
>>> +             pr_err("Did not retire immediately for all inactive engines\n");
>>
>> Why would this one retire immediately? It will send requests down the
>> pipe, no? So don't you actually need to wait for the tracker to be
>> signalled and that counter == num_engines?
> 
> Nothing has used the context at this point, so we don't emit a request
> on any engine, and the barrier can be immediately executed.

Yep, that it uses ctx->active_engines slipped my mind.

>>> +             err = -EINVAL;
>>> +             goto out;
>>> +     }
>>> +
>>> +     rq = ERR_PTR(-ENODEV);
>>> +     with_intel_runtime_pm(i915, wakeref)
>>> +             rq = i915_request_alloc(i915->engine[RCS], ctx);
>>> +     if (IS_ERR(rq)) {
>>> +             pr_err("Request allocation failed!\n");
>>> +             goto out;
>>> +     }
>>> +     i915_request_add(rq);
>>
>> Doesn't this need to go under the wakeref as well?
> 
> No, we only need the wakeref to start (that's only to avoid blocking
> inside request_alloc --- yeah, that's not exactly how it works, we'll be
> back later to fix that!). The request then carries the GT wakeref with it.
> 
>>> +     GEM_BUG_ON(list_empty(&ctx->active_engines));
>>> +
>>> +     counter = 0;
>>> +     context_barrier_inject_fault = BIT(RCS);
>>> +     err = context_barrier_task(ctx, -1, mock_barrier_task, &counter);
>>> +     context_barrier_inject_fault = 0;
>>> +     if (err == -ENXIO)
>>> +             err = 0;
>>> +     else
>>> +             pr_err("Did not hit fault injection!\n");
>>> +     if (counter != 0) {
>>> +             pr_err("Invoked callback on error!\n");
>>> +             err = -EIO;
>>> +     }
>>> +     if (err)
>>> +             goto out;
>>> +
>>> +     counter = 0;
>>> +     err = context_barrier_task(ctx, -1, mock_barrier_task, &counter);
>>> +     if (err) {
>>> +             pr_err("Failed at line %d, err=%d\n", __LINE__, err);
>>> +             goto out;
>>> +     }
>>> +     mock_device_flush(i915);
>>> +     if (counter == 0) {
>>> +             pr_err("Did not retire on each active engines\n");
>>> +             err = -EINVAL;
>>> +             goto out;
>>> +     }
>>
>> This one is inline with my understanding, and the context_barrier_task
>> arguments are the same as the one above.. hm.. I am confused.
> 
> This time it is active, so we actually have to wait as the barrier waits
> for the GPU before firing.

Yep.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko