Re: [Intel-gfx] [RFC 2/2] drm/doc/rfc: i915 new parallel submission uAPI plan

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 21/05/2021 17:48, Matthew Brost wrote:
> On Fri, May 21, 2021 at 01:00:54PM +0100, Tvrtko Ursulin wrote:

[snip]

>>> + * enables parallel submission across multiple engine classes. In this case each
>>> + * context's logical engine mask indicates where that context can placed. It is
>>> + * implied in this mode that all contexts have mutual exclusive placement (e.g.
>>> + * if one context is running CS0 no other contexts can run on CS0).
>>
>> I think talk about logical context and its mask is too implementation detail
>> at the uapi level. Instead I would suggest more userspace programmer centric
>> description.
> 
> Ok, can you give me suggestion? Writing DOC isn't really my strength.

Yeah, not mine either. Maybe we need to hire a technical writer. :)

I think in general I would just talk a bit how until now submission was 
along a single engine only and this is adding a wide submission model, 
expanding how it works with more details and only then talking about 
logical contexts if needed.

It depends a bit whether our userspace clients still predominantly think 
in terms of engines, or is it contexts? I don't have an answer there.

It probably isn't the most important thing and probably with a few 
tweaks of what you have it can be good enough. Key probably is simply 
coming up with as intuitive as possible diagrams, with consistent 
naming, showing how the wide engine is built and how it works.

>>> + *
>>> + * Example 1 pseudo code:
>>> + * CSX[Y] = engine class X, logical instance Y
>>> + * INVALID = I915_ENGINE_CLASS_INVALID, I915_ENGINE_CLASS_INVALID_NONE
>>> + * set_engines(INVALID)
>>> + * set_parallel(engine_index=0, width=2, num_siblings=2,
>>> + *		engines=CS0[0],CS0[1],CS1[0],CS1[1])
>>> + *
>>> + * Results in the following valid placements:
>>> + * CS0[0], CS1[0]
>>> + * CS0[0], CS1[1]
>>> + * CS0[1], CS1[0]
>>> + * CS0[1], CS1[1]
>>> + *
>>> + * This can also be though of as 2 virtual engines:
>>> + * VE[0] = CS0[0], CS0[1]
>>> + * VE[1] = CS1[0], CS1[1]
>>
>> Ah okay so essentially similar to what I was proposing a year ago. But then
>> it is no longer "set_parallel" really. It is one slot in the engine map,
>> right, with the idea to super class intel_context in the implementation?
>>
> 
> Yes, it is bascially a super class intel_context. In the implementation is
> parent-child with the parent having a linked list of child intel_contexts.
>   
>> So really a wide virtual engine, as opposed to single one. In which case I
>> think it makes sense to stay close to the existing naming of the
>> load_balance extension for consistency. Load_balance_wide?
>> Load_balance_parallel? Multi?
> 
> I like the way is named but I also don't want to argue about this as I don't
> really care. If someone else says this should be renamed, let's do it.

I don't care too much apart from a general desire for more consistency 
and fewer terms in use.

>> I also have to say the notation "CS0[0]" - I who know this problem space am
>> finding it hard to penetrate what that actually means. (Also uppercase IMO
>> makes it hard to read, but maybe it is just me.)
>>
> 
> Yea, now I think about it CS0[0] is bad because of using numbers twice. How
> about CSX[0] & CSY[1]? I used upper case because in the i915 all engine classes
> defines are upper case but agree it might be easier to read it lower case.

What would X and Y represent? Or if I paste this part:

  * VE[0] = CS0[0], CS0[1]
  * VE[1] = CS1[0], CS1[1]

Which index is engine instance and what is the other index?

>> Looking a bit lower below, extension seems to be taking a 2d array of
>> class:instance pairs, right? If so then reading these docs in order, or even
>> just looking further down, I don't think that is explicitly called out
>> clearly enough.
>>
>> So I think a paragraph or two explaining clearly how the 2d array of engines
>> corresponds to the allowed engines for full virtual engine width. Or maybe
>> just a 2d diagram?
>>
>>    2-wide virtual engine:
>>      .engines = [
>>        /* channel 0 allowed engines: */  [cs0, cs1],
>>        /* channel 1 allowed engines: */  [cs0, cs1]
>>       ]
>>
>> Not sure if that's better.
>>
> 
> Yes, it is a 2-d array. Agree the explaination could be better.
> 
>> Also to be noted, this only allows uniform number of allowed engines per
>> channel. I am not saying we need the non-uniform setup today but with bonds
>> there isn't this limitation.
>>
> 
> Not exactly. You could do something like this.
> 
> witdth = 2
> siblings = 2
> engines = CSX[0], CSX[1], CSY[0], INVALID
> 
> This would allow a placement of:
> 
> CSX[0], CSY[0]
> CSX[1], CSY[0]
> 
> In this case the siblings is just a max value of each entry.

Okay fair, did not think about that or saw it mentioned.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx