On Fri, Apr 30, 2021 at 2:44 PM Tvrtko Ursulin
<tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
>
>
>
> 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.
They filter out no-op changes. I just meant that from look at
compute-runtime, it seems like sseu can change whenever.
-Daniel
> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
