[PATCH 2/4] drm/i915: Retire requests along rings

[Tvrtko Ursulin]

On 20/04/2018 15:42, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-04-20 15:28:35)
>>
>> On 20/04/2018 12:35, Chris Wilson wrote:
>>> In the next patch, rings are the central timeline as requests may jump
>>> between engines. Therefore in the future as we retire in order along the
>>> engine timeline, we may retire out-of-order within a ring (as the ring now
>>> occurs along multiple engines), leading to much hilarity in miscomputing
>>> the position of ring->head.
>>
>> Will we be retiring out of order within a ring? I thought we said not,
>> that is no parallel execution of a single ring on two engines. And if we
>> won't then I don't see how ring is retired out of order.
> 
> Within a ring? No. The problem occurred (without this patch) as the ring
> popped up on different engines, and so advance_ring() was called out of
> sync (but in order on each engine ofc). As a result ring->head jumped
> all over the place.

Aaah I get it. Hm, that kind of gives away something about how you 
implemented the virtual engine, no? So I am not sure splitting this 
ahead of the rest is the best approach.

>>>    void i915_retire_requests(struct drm_i915_private *i915)
>>>    {
>>> -     struct intel_engine_cs *engine;
>>> -     enum intel_engine_id id;
>>> +     struct intel_ring *ring, *next;
>>>    
>>>        lockdep_assert_held(&i915->drm.struct_mutex);
>>>    
>>>        if (!i915->gt.active_requests)
>>>                return;
>>>    
>>> -     for_each_engine(engine, i915, id)
>>> -             engine_retire_requests(engine);
>>> +     list_for_each_entry_safe(ring, next, &i915->gt.rings, link)
>>> +             ring_retire_requests(ring);
>>
>> This is the less efficient bit - but I actually cannot decide if it
>> matters in practice (to walk potentially so many more lists), or the
>> removal of irq spinlock wins. For real-world use it probably doesn't
>> matter. For pathological IGTs we probably don't care.
> 
> Aye. i915_retire_requests() should never be on the critical path; it's
> used as the catch up after idling, or by the idle-worker to flush the
> requests. It is run under struct_mutex though, and as such not entirely
> painless. Solving requests + struct_mutex will certainly be interesting.

Ignoring the really uninteresting call sites, it is also called on 
shrinking and eviction (and perf - but I did not try to understand why). 
So that might have some impact, although still wouldn't dominate I think.

Regards,

Tvrtko