[Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation

[Daniel Vetter]

On Fri, Apr 30, 2021 at 1:58 PM Tvrtko Ursulin
<tvrtko.ursulin at linux.intel.com> wrote:
>
>
> On 30/04/2021 07:53, Daniel Vetter wrote:
> > On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand <jason at jlekstrand.net> wrote:
> >>
> >> On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter <daniel at ffwll.ch> 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 at ffwll.ch> 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 at ffwll.ch> 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.

Or do you mean this is defacto dead code? this = compute setting it
before every batch I mean here.
-Daniel




--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch