[Intel-gfx] [PATCH 2/4] drm/i915/gt: Check for arbitration after writing start seqno

[Tvrtko Ursulin]

On 11/01/2021 16:10, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2021-01-11 16:03:48)
>>
>> On 11/01/2021 10:57, Chris Wilson wrote:
>>> On the off chance that we need to arbitrate before launching the
>>> payload, perform the check after we signal the request is ready to
>>> start. Assuming instantaneous processing of the CS event, the request
>>> will then be treated as having started when we make the decisions as to
>>> how to process that CS event.
>>
>> What is the wider context with this change?
> 
> Just thinking about the impact of MI_ARB_ONOFF. It's the next patch that
> sparked the curiosity at that is trying to address a missed arbitration
> point on the semaphore-wait miss.
> 
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> ---
>>>    drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 12 ++++++------
>>>    1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
>>> index 2e36e0a9d8a6..9a182652a35e 100644
>>> --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
>>> +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
>>> @@ -361,19 +361,19 @@ int gen8_emit_init_breadcrumb(struct i915_request *rq)
>>>        if (IS_ERR(cs))
>>>                return PTR_ERR(cs);
>>>    
>>> +     *cs++ = MI_STORE_DWORD_IMM_GEN4 | MI_USE_GGTT;
>>> +     *cs++ = hwsp_offset(rq);
>>> +     *cs++ = 0;
>>> +     *cs++ = rq->fence.seqno - 1;
>>> +
>>
>> Strictly this movement even makes the existing comment (below) more correct.
>>
>>>        /*
>>>         * Check if we have been preempted before we even get started.
>>>         *
>>>         * After this point i915_request_started() reports true, even if
>>>         * we get preempted and so are no longer running.
>>>         */
>>> -     *cs++ = MI_ARB_CHECK;
>>>        *cs++ = MI_NOOP;
>>> -
>>> -     *cs++ = MI_STORE_DWORD_IMM_GEN4 | MI_USE_GGTT;
>>> -     *cs++ = hwsp_offset(rq);
>>> -     *cs++ = 0;
>>> -     *cs++ = rq->fence.seqno - 1;
>>> +     *cs++ = MI_ARB_CHECK;
>>
>> Special reason to have NOOP before MI_ARB_CHECK or would more common
>> NOOP padding at the end be suitable?
> 
> The so small it's barely a reason was to put as much distance (those
> whole 6 cycles) between the store and the arbitration point.

Overall on the patch, there could be slight difference on how 
i915_request_started reports true and would now allow to be preempted 
after that point, even if the user payload itself would not be 
preemptable. Obviously that applies on Gen8, maybe on later Gens with 
like non-preemptible media batches or so. I can't think that it would 
(or should) cause a problem though, just thinking out loud. So don't 
know really, sounds safe to experiment.

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

Regards,

Tvrtko