[Intel-gfx] [PATCH 1/5] drm/i915/execlists: Refactor -EIO markup of hung requests

[Tvrtko Ursulin]

On 23/09/2019 10:35, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-09-23 10:27:01)
>>
>> On 21/09/2019 10:55, Chris Wilson wrote:
>>> Pull setting -EIO on the hung requests into its own utility function.
>>>
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> ---
>>>    drivers/gpu/drm/i915/gt/intel_lrc.c | 32 +++++++++++++++--------------
>>>    1 file changed, 17 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
>>> index 1a2b71157f08..53e823d36b28 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
>>> @@ -234,6 +234,13 @@ static void execlists_init_reg_state(u32 *reg_state,
>>>                                     struct intel_engine_cs *engine,
>>>                                     struct intel_ring *ring);
>>>    
>>> +static void mark_eio(struct i915_request *rq)
>>> +{
>>> +     if (!i915_request_signaled(rq))
>>> +             dma_fence_set_error(&rq->fence, -EIO);
>>> +     i915_request_mark_complete(rq);
>>> +}
>>> +
>>>    static inline u32 intel_hws_preempt_address(struct intel_engine_cs *engine)
>>>    {
>>>        return (i915_ggtt_offset(engine->status_page.vma) +
>>> @@ -2501,12 +2508,8 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
>>>        __execlists_reset(engine, true);
>>>    
>>>        /* Mark all executing requests as skipped. */
>>> -     list_for_each_entry(rq, &engine->active.requests, sched.link) {
>>> -             if (!i915_request_signaled(rq))
>>> -                     dma_fence_set_error(&rq->fence, -EIO);
>>> -
>>> -             i915_request_mark_complete(rq);
>>> -     }
>>> +     list_for_each_entry(rq, &engine->active.requests, sched.link)
>>> +             mark_eio(rq);
>>>    
>>>        /* Flush the queued requests to the timeline list (for retiring). */
>>>        while ((rb = rb_first_cached(&execlists->queue))) {
>>> @@ -2514,10 +2517,8 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
>>>                int i;
>>>    
>>>                priolist_for_each_request_consume(rq, rn, p, i) {
>>> -                     list_del_init(&rq->sched.link);
>>
>> list_del_init not needed any more? Should be mentioned in the commit
>> message.
> 
> It's not been needed for a long time; only just noticed it was still
> there.
>   
>>> +                     mark_eio(rq);
>>>                        __i915_request_submit(rq);
>>> -                     dma_fence_set_error(&rq->fence, -EIO);
>>> -                     i915_request_mark_complete(rq);
>>
>> I am also curious about Mika's question - if the change in ordering of
>> submit vs mark_complete is important it should be mentioned in the commit.
> 
> It's not important as the gpu is wedged, it just ties together with the
> next patch where if already completed, we take a short-path through
> __i915_request_submit.

Okay, after reading the next patch I see what will happen.

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

(Keep r-b after list_del respin.)

Regards,

Tvrtko