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
