On 30/04/2021 13:30, Daniel Vetter wrote:
On Fri, Apr 30, 2021 at 1:58 PM Tvrtko Ursulin
<tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
On 30/04/2021 07:53, Daniel Vetter wrote:
On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand <jason@xxxxxxxxxxxxxx> wrote:
On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter <daniel@xxxxxxxx> wrote:
On Thu, Apr 29, 2021 at 02:01:16PM -0500, Jason Ekstrand wrote:
On Thu, Apr 29, 2021 at 1:56 PM Daniel Vetter <daniel@xxxxxxxx> wrote:
On Thu, Apr 29, 2021 at 01:16:04PM -0500, Jason Ekstrand wrote:
On Thu, Apr 29, 2021 at 10:51 AM Daniel Vetter <daniel@xxxxxxxx> wrote:
+ ret = set_proto_ctx_param(file_priv, pc, args);
I think we should have a FIXME here of not allowing this on some future
platforms because just use CTX_CREATE_EXT.
Done.
+ if (ret == -ENOTSUPP) {
+ /* Some params, specifically SSEU, can only be set on fully
I think this needs a FIXME: that this only holds during the conversion?
Otherwise we kinda have a bit a problem me thinks ...
I'm not sure what you mean by that.
Well I'm at least assuming that we wont have this case anymore, i.e.
there's only two kinds of parameters:
- those which are valid only on proto context
- those which are valid on both (like priority)
This SSEU thing looks like a 3rd parameter, which is only valid on
finalized context. That feels all kinds of wrong. Will it stay? If yes
*ugh* and why?
Because I was being lazy. The SSEU stuff is a fairly complex param to
parse and it's always set live. I can factor out the SSEU parsing
code if you want and it shouldn't be too bad in the end.
Yeah I think the special case here is a bit too jarring.
I rolled a v5 that allows you to set SSEU as a create param. I'm not
a huge fan of that much code duplication for the SSEU set but I guess
that's what we get for deciding to "unify" our context creation
parameter path with our on-the-fly parameter path....
You can look at it here:
https://gitlab.freedesktop.org/jekstrand/linux/-/commit/c805f424a3374b2de405b7fc651eab551df2cdaf#474deb1194892a272db022ff175872d42004dfda_283_588
Hm yeah the duplication of the render engine check is a bit annoying.
What's worse, if you tthrow another set_engines on top it's probably
all wrong then. The old thing solved that by just throwing that
intel_context away.
You're also not keeping the engine id in the proto ctx for this, so
there's probably some gaps there. We'd need to clear the SSEU if
userspace puts another context there. But also no userspace does that.
Plus cursory review of userspace show
- mesa doesn't set this
- compute sets its right before running the batch
- media sets it as the last thing of context creation
Noticed a long sub-thread so looked inside..
SSEU is a really an interesting one.
For current userspace limiting to context creation is fine, since it is
only allowed for Icelake/VME use case. But if you notice the comment inside:
/* ABI restriction - VME use case only. */
It is a hint there was, or could be, more to this uapi than that.
And from memory I think limiting to creation time will nip the hopes
media had to use this dynamically on other platforms in the bud. So not
that good really. They had convincing numbers what gets significantly
better if we allowed dynamic control to this, just that as always, open
source userspace was not there so we never allowed it. However if you
come up with a new world order where it can only be done at context
creation, as said already, the possibility for that improvement (aka
further improving the competitive advantage) is most likely dashed.
Hm are you sure that this is create-time only? media-driver uses it
like that, but from my checking compute-runtime updates SSEU mode
before every execbuf call. So it very much looked like we have to keep
this dynamic.
Ah okay, I assumed it's more of the overall drive to eliminate
set_param. If sseu set_param stays then it's fine for what I had in mind.
Or do you mean this is defacto dead code? this = compute setting it
before every batch I mean here.
No idea, wasn't aware of the compute usage.
Before every execbuf is not very ideal though since we have to inject a
foreign context operation to update context image, which means stream of
work belonging to the context cannot be coalesced (assuming it could to
start with). There is also a hw cost to reconfigure the sseu which adds
latency on top.
Anyway, I was only aware of the current media usage, which is static as
you say, and future/wishlist media usage, which would be dynamic, but a
complicated story to get right (partly due downsides mentioned in the
previous paragraph mean balancing benefit vs cost of dynamic sseu is not
easy).
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
