[Intel-gfx] [PATCH v2 1/6] drm/i915: Stop tracking timeline->inflight_seqnos

[Tvrtko Ursulin]

On 24/04/2018 11:40, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-04-24 11:14:21)
>>
>> On 23/04/2018 19:08, Chris Wilson wrote:
>>> -static int reserve_engine(struct intel_engine_cs *engine)
>>> +static int reserve_gt(struct drm_i915_private *i915)
>>>    {
>>> -     struct drm_i915_private *i915 = engine->i915;
>>> -     u32 active = ++engine->timeline->inflight_seqnos;
>>> -     u32 seqno = engine->timeline->seqno;
>>>        int ret;
>>>    
>>> -     /* Reservation is fine until we need to wrap around */
>>> -     if (unlikely(add_overflows(seqno, active))) {
>>> +     /*
>>> +      * Reservation is fine until we may need to wrap around
>>> +      *
>>> +      * By incrementing the serial for every request, we know that no
>>> +      * individual engine may exceed that serial (as each is reset to 0
>>> +      * on any wrap). This protects even the most pessimistic of migrations
>>> +      * of every request from all engines onto just one.
>>> +      */
>>
>> I didn't really figure out what was wrong with v1? Neither could handle
>> more than four billion of simultaneously active requests - but I thought
>> that should not concern us. :)
> 
> It was still using the local engine->timeline.seqno as it's base. If we
> swapped from one at 0 to another at U32_MAX, we would overflow much
> later in submission; after the point of no return.

By swapped you already refer to engine change? Ok, I can see that yes. 
In this case global counter does prevent that.

In the light of that, what is your current thinking with regards to 
mixing engine classes?

If the thinking is still to only allow within a class then per-class 
seqno counter would be an option.

Regards,

Tvrtko

>> I don't quite get wrapping based on otherwise unused counter.
>>
>> v1 made sense to wrap then any engine timeline couldn't handle all
>> active requests without wrapping.
> 
> Emphasis on *any*, not just the current engine.
> -Chris
>