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

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand <jason@jlekstrand.net> wrote:
>
> On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter <daniel@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@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@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

So it's kinda not needed. But also we're asking umd to switch over to
CTX_CREATE_EXT, and if sseu doesn't work for that media team will be
puzzled. And we've confused them enough already with our uapis.

Another idea: proto_set_sseu just stores the uapi struct and a note
that it's set, and checks nothing. To validate sseu on proto context
we do (but only when an sseu parameter is set):
1. finalize the context
2. call the real set_sseu for validation
3. throw the finalized context away again, it was just for validating
the overall thing

That way we don't have to consider all the interactions of setting
sseu and engines in any order on proto context, validation code is
guaranteed shared. Only downside is that there's a slight chance in
behaviour: SSEU, then setting another engine in that slot will fail
instead of throwing the sseu parameters away. That's the right thing
for CTX_CREATE_EXT anyway, and current userspace doesn't care.

Thoughts?

> I'm also going to send it to trybot.

If you resend pls include all my r-b, I think some got lost in v4.
Also, in the kernel at least we expect minimal commit message with a
bit of context, there's no Part-of: link pointing at the entire MR
with overview and discussion, the patchwork Link: we add is a pretty
bad substitute. Some of the new patches in v4 are a bit too terse on
that.

And finally I'm still not a big fan of the add/remove split over
patches, but oh well.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel