Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation
On Fri, Apr 30, 2021 at 6:57 PM Jason Ekstrand wrote:
>
> On Fri, Apr 30, 2021 at 11:33 AM Daniel Vetter wrote:
> >
> > On Fri, Apr 30, 2021 at 6:27 PM Jason Ekstrand wrote:
> > >
> > > On Fri, Apr 30, 2021 at 1:53 AM Daniel Vetter wrote:
> > > >
> > > > On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand
> > > > wrote:
> > > > >
> > > > > On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter 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
> > > > > > > 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
> > > > > > > > > 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.
> > >
> > > I think that's already mostly taken care of. When set_engines
> > > happens, we throw away the old array of engines and start with a new
> > > one where everything has been memset to 0. The one remaining problem
> > > is that, if userspace resets the engine set, we need to memset
> > > legacy_rcs_sseu to 0. I've added that.
> > >
> > > > 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.
> > >
> > > Again, I think that's handled. See above.
> > >
> > > > 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 o
Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation
On Fri, Apr 30, 2021 at 11:33 AM Daniel Vetter wrote:
>
> On Fri, Apr 30, 2021 at 6:27 PM Jason Ekstrand wrote:
> >
> > On Fri, Apr 30, 2021 at 1:53 AM Daniel Vetter wrote:
> > >
> > > On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand
> > > wrote:
> > > >
> > > > On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter 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
> > > > > > 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
> > > > > > > > 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.
> >
> > I think that's already mostly taken care of. When set_engines
> > happens, we throw away the old array of engines and start with a new
> > one where everything has been memset to 0. The one remaining problem
> > is that, if userspace resets the engine set, we need to memset
> > legacy_rcs_sseu to 0. I've added that.
> >
> > > 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.
> >
> > Again, I think that's handled. See above.
> >
> > > 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 thought about that. The problem is that they can set_sseu multiple
> > times on different engines. This means we'
Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation
On Fri, Apr 30, 2021 at 6:27 PM Jason Ekstrand wrote:
>
> On Fri, Apr 30, 2021 at 1:53 AM Daniel Vetter wrote:
> >
> > On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand
> > wrote:
> > >
> > > On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter 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 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
> > > > > > > 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.
>
> I think that's already mostly taken care of. When set_engines
> happens, we throw away the old array of engines and start with a new
> one where everything has been memset to 0. The one remaining problem
> is that, if userspace resets the engine set, we need to memset
> legacy_rcs_sseu to 0. I've added that.
>
> > 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.
>
> Again, I think that's handled. See above.
>
> > 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 thought about that. The problem is that they can set_sseu multiple
> times on different engines. This means we'd have to effectively build
> up an arbitrary list of SSEU set operations and replay it. I'm not
> sure how I feel about building up a big data structure.
Hm, but how does this work with proto ctx then? I've only seen a
single sseu param set in the patch you linked.
> > > I'm also going to send it
Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation
On Fri, Apr 30, 2021 at 1:53 AM Daniel Vetter wrote:
>
> On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand wrote:
> >
> > On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter 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 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
> > > > > > 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.
I think that's already mostly taken care of. When set_engines
happens, we throw away the old array of engines and start with a new
one where everything has been memset to 0. The one remaining problem
is that, if userspace resets the engine set, we need to memset
legacy_rcs_sseu to 0. I've added that.
> 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.
Again, I think that's handled. See above.
> 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 thought about that. The problem is that they can set_sseu multiple
times on different engines. This means we'd have to effectively build
up an arbitrary list of SSEU set operations and replay it. I'm not
sure how I feel about building up a big data structure.
> > 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.
I'll try and dig those up.
> 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 t
Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation
On 30/04/2021 14:07, Daniel Vetter wrote:
On Fri, Apr 30, 2021 at 2:44 PM Tvrtko Ursulin
wrote:
On 30/04/2021 13:30, Daniel Vetter wrote:
On Fri, Apr 30, 2021 at 1:58 PM Tvrtko Ursulin
wrote:
On 30/04/2021 07:53, Daniel Vetter wrote:
On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand wrote:
On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter 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 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 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.
i915 does it as well for good measure - since the penalty is global we
have to. So I guess they don't know when VME block will be used ie it is
per batch.
Regards,
Tvrtko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation
On Fri, Apr 30, 2021 at 2:44 PM Tvrtko Ursulin
wrote:
>
>
>
> On 30/04/2021 13:30, Daniel Vetter wrote:
> > On Fri, Apr 30, 2021 at 1:58 PM Tvrtko Ursulin
> > wrote:
> >> On 30/04/2021 07:53, Daniel Vetter wrote:
> >>> On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand
> >>> wrote:
>
> On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter 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 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
> 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
>
Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation
On 30/04/2021 13:30, Daniel Vetter wrote:
On Fri, Apr 30, 2021 at 1:58 PM Tvrtko Ursulin
wrote:
On 30/04/2021 07:53, Daniel Vetter wrote:
On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand wrote:
On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter 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 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 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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation
On Fri, Apr 30, 2021 at 1:58 PM Tvrtko Ursulin
wrote:
>
>
> On 30/04/2021 07:53, Daniel Vetter wrote:
> > On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand
> > wrote:
> >>
> >> On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter 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 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 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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation
On 30/04/2021 07:53, Daniel Vetter wrote:
On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand wrote:
On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter 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 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 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.
Regards,
Tvrtko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation
On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand wrote:
>
> On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter 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 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
> > > > > 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
Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation
On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter 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 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 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
I'm also going to send it to trybot.
--Jason
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation
On Thu, Apr 29, 2021 at 02:01:16PM -0500, Jason Ekstrand wrote:
> On Thu, Apr 29, 2021 at 1:56 PM Daniel Vetter 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 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.
-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
Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation
On Thu, Apr 29, 2021 at 1:56 PM Daniel Vetter 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 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.
--Jason
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation
On Thu, Apr 29, 2021 at 01:16:04PM -0500, Jason Ekstrand wrote:
> On Thu, Apr 29, 2021 at 10:51 AM Daniel Vetter 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?
-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
Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation
On Thu, Apr 29, 2021 at 10:51 AM Daniel Vetter wrote:
>
> Yeah this needs some text to explain what/why you're doing this, and maybe
> some rough sketch of the locking design.
Yup. Will add.
>
> On Fri, Apr 23, 2021 at 05:31:26PM -0500, Jason Ekstrand wrote:
> > Signed-off-by: Jason Ekstrand
> > ---
> > drivers/gpu/drm/i915/gem/i915_gem_context.c | 657 --
> > drivers/gpu/drm/i915/gem/i915_gem_context.h | 3 +
> > .../gpu/drm/i915/gem/i915_gem_context_types.h | 26 +
> > .../gpu/drm/i915/gem/selftests/mock_context.c | 5 +-
> > drivers/gpu/drm/i915/i915_drv.h | 17 +-
> > 5 files changed, 648 insertions(+), 60 deletions(-)
>
> So I think the patch split here is a bit unfortunate, because you're
> adding the new vm/engine validation code for proto context here, but the
> old stuff is only removed in the next patches that make vm/engines
> immutable after first use.
Yes, it's very unfortunate. I'm reworking things now to have a
different split which I think makes more sense but actually separates
the add from the remove even further. :-(
> I think a better split would be if this patch here only has all the
> scaffolding. You already have the EOPNOTSUPP fallback (which I hope gets
> removed), so moving the conversion entirely to later patches should be all
> fine.
>
> Or do I miss something?
>
> I think the only concern I'm seeing is that bisectability might be a bit
> lost, because we finalize the context in some cases in setparam. And if we
> do the conversion in a different order than the one media uses for its
> setparam, then later setparam might fail because the context is finalized
> already. But also
> - it's just bisectability of media functionality I think
> - just check which order media calls CTX_SETPARAM and use that to do the
> conversion
>
> And we should be fine ... I think?
Before we go down that path, let's what you think of my new ordering.
> Some more thoughts below, but the proto ctx stuff itself looks fine.
>
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > index db9153e0f85a7..aa8e61211924f 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > @@ -193,8 +193,15 @@ static int validate_priority(struct drm_i915_private
> > *i915,
> >
> > static void proto_context_close(struct i915_gem_proto_context *pc)
> > {
> > + int i;
> > +
> > if (pc->vm)
> > i915_vm_put(pc->vm);
> > + if (pc->user_engines) {
> > + for (i = 0; i < pc->num_user_engines; i++)
> > + kfree(pc->user_engines[i].siblings);
> > + kfree(pc->user_engines);
> > + }
> > kfree(pc);
> > }
> >
> > @@ -274,12 +281,417 @@ proto_context_create(struct drm_i915_private *i915,
> > unsigned int flags)
> > proto_context_set_persistence(i915, pc, true);
> > pc->sched.priority = I915_PRIORITY_NORMAL;
> >
> > + pc->num_user_engines = -1;
> > + pc->user_engines = NULL;
> > +
> > if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE)
> > pc->single_timeline = true;
> >
> > return pc;
> > }
> >
> > +static int proto_context_register_locked(struct drm_i915_file_private
> > *fpriv,
> > + struct i915_gem_proto_context *pc,
> > + u32 *id)
> > +{
> > + int ret;
> > + void *old;
>
> assert_lock_held just for consistency.
Done.
> > +
> > + ret = xa_alloc(&fpriv->context_xa, id, NULL, xa_limit_32b,
> > GFP_KERNEL);
> > + if (ret)
> > + return ret;
> > +
> > + old = xa_store(&fpriv->proto_context_xa, *id, pc, GFP_KERNEL);
> > + if (xa_is_err(old)) {
> > + xa_erase(&fpriv->context_xa, *id);
> > + return xa_err(old);
> > + }
> > + GEM_BUG_ON(old);
> > +
> > + return 0;
> > +}
> > +
> > +static int proto_context_register(struct drm_i915_file_private *fpriv,
> > + struct i915_gem_proto_context *pc,
> > + u32 *id)
> > +{
> > + int ret;
> > +
> > + mutex_lock(&fpriv->proto_context_lock);
> > + ret = proto_context_register_locked(fpriv, pc, id);
> > + mutex_unlock(&fpriv->proto_context_lock);
> > +
> > + return ret;
> > +}
> > +
> > +static int set_proto_ctx_vm(struct drm_i915_file_private *fpriv,
> > + struct i915_gem_proto_context *pc,
> > + const struct drm_i915_gem_context_param *args)
> > +{
> > + struct i915_address_space *vm;
> > +
> > + if (args->size)
> > + return -EINVAL;
> > +
> > + if (!pc->vm)
> > + return -ENODEV;
> > +
> > + if (upper_32_bits(args->value))
> > + return -ENOENT;
> > +
> > + rcu_read_lock();
> > + vm = xa_load(&fpriv->vm_xa, args->value);
> > + if (vm && !kref_get_unless_zero(&vm-
Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation
Yeah this needs some text to explain what/why you're doing this, and maybe
some rough sketch of the locking design.
On Fri, Apr 23, 2021 at 05:31:26PM -0500, Jason Ekstrand wrote:
> Signed-off-by: Jason Ekstrand
> ---
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 657 --
> drivers/gpu/drm/i915/gem/i915_gem_context.h | 3 +
> .../gpu/drm/i915/gem/i915_gem_context_types.h | 26 +
> .../gpu/drm/i915/gem/selftests/mock_context.c | 5 +-
> drivers/gpu/drm/i915/i915_drv.h | 17 +-
> 5 files changed, 648 insertions(+), 60 deletions(-)
So I think the patch split here is a bit unfortunate, because you're
adding the new vm/engine validation code for proto context here, but the
old stuff is only removed in the next patches that make vm/engines
immutable after first use.
I think a better split would be if this patch here only has all the
scaffolding. You already have the EOPNOTSUPP fallback (which I hope gets
removed), so moving the conversion entirely to later patches should be all
fine.
Or do I miss something?
I think the only concern I'm seeing is that bisectability might be a bit
lost, because we finalize the context in some cases in setparam. And if we
do the conversion in a different order than the one media uses for its
setparam, then later setparam might fail because the context is finalized
already. But also
- it's just bisectability of media functionality I think
- just check which order media calls CTX_SETPARAM and use that to do the
conversion
And we should be fine ... I think?
Some more thoughts below, but the proto ctx stuff itself looks fine.
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index db9153e0f85a7..aa8e61211924f 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -193,8 +193,15 @@ static int validate_priority(struct drm_i915_private
> *i915,
>
> static void proto_context_close(struct i915_gem_proto_context *pc)
> {
> + int i;
> +
> if (pc->vm)
> i915_vm_put(pc->vm);
> + if (pc->user_engines) {
> + for (i = 0; i < pc->num_user_engines; i++)
> + kfree(pc->user_engines[i].siblings);
> + kfree(pc->user_engines);
> + }
> kfree(pc);
> }
>
> @@ -274,12 +281,417 @@ proto_context_create(struct drm_i915_private *i915,
> unsigned int flags)
> proto_context_set_persistence(i915, pc, true);
> pc->sched.priority = I915_PRIORITY_NORMAL;
>
> + pc->num_user_engines = -1;
> + pc->user_engines = NULL;
> +
> if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE)
> pc->single_timeline = true;
>
> return pc;
> }
>
> +static int proto_context_register_locked(struct drm_i915_file_private *fpriv,
> + struct i915_gem_proto_context *pc,
> + u32 *id)
> +{
> + int ret;
> + void *old;
assert_lock_held just for consistency.
> +
> + ret = xa_alloc(&fpriv->context_xa, id, NULL, xa_limit_32b, GFP_KERNEL);
> + if (ret)
> + return ret;
> +
> + old = xa_store(&fpriv->proto_context_xa, *id, pc, GFP_KERNEL);
> + if (xa_is_err(old)) {
> + xa_erase(&fpriv->context_xa, *id);
> + return xa_err(old);
> + }
> + GEM_BUG_ON(old);
> +
> + return 0;
> +}
> +
> +static int proto_context_register(struct drm_i915_file_private *fpriv,
> + struct i915_gem_proto_context *pc,
> + u32 *id)
> +{
> + int ret;
> +
> + mutex_lock(&fpriv->proto_context_lock);
> + ret = proto_context_register_locked(fpriv, pc, id);
> + mutex_unlock(&fpriv->proto_context_lock);
> +
> + return ret;
> +}
> +
> +static int set_proto_ctx_vm(struct drm_i915_file_private *fpriv,
> + struct i915_gem_proto_context *pc,
> + const struct drm_i915_gem_context_param *args)
> +{
> + struct i915_address_space *vm;
> +
> + if (args->size)
> + return -EINVAL;
> +
> + if (!pc->vm)
> + return -ENODEV;
> +
> + if (upper_32_bits(args->value))
> + return -ENOENT;
> +
> + rcu_read_lock();
> + vm = xa_load(&fpriv->vm_xa, args->value);
> + if (vm && !kref_get_unless_zero(&vm->ref))
> + vm = NULL;
> + rcu_read_unlock();
> + if (!vm)
> + return -ENOENT;
> +
> + i915_vm_put(pc->vm);
> + pc->vm = vm;
> +
> + return 0;
> +}
> +
> +struct set_proto_ctx_engines {
> + struct drm_i915_private *i915;
> + unsigned num_engines;
> + struct i915_gem_proto_engine *engines;
> +};
> +
> +static int
> +set_proto_ctx_engines_balance(struct i915_user_extension __user *base,
> + void *data)
> +{
> + struct i915_context_engines_load_balance __user *ext =
> + contain
Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation
Hi Jason,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on drm-tip/drm-tip drm-exynos/exynos-drm-next
next-20210423]
[cannot apply to tegra-drm/drm/tegra/for-next drm/drm-next v5.12-rc8]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Jason-Ekstrand/drm-i915-gem-ioctl-clean-ups/20210424-063511
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-rhel-8.3 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
#
https://github.com/0day-ci/linux/commit/e00622bd8a3f3eccbb22721c2f8857bdfb7d5d9d
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Jason-Ekstrand/drm-i915-gem-ioctl-clean-ups/20210424-063511
git checkout e00622bd8a3f3eccbb22721c2f8857bdfb7d5d9d
# save the attached .config to linux build tree
make W=1 W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/i915/gem/i915_gem_context.c:2439:1: warning: no previous
>> prototype for 'lazy_create_context_locked' [-Wmissing-prototypes]
2439 | lazy_create_context_locked(struct drm_i915_file_private *file_priv,
| ^~
vim +/lazy_create_context_locked +2439
drivers/gpu/drm/i915/gem/i915_gem_context.c
2437
2438 struct i915_gem_context *
> 2439 lazy_create_context_locked(struct drm_i915_file_private *file_priv,
2440 struct i915_gem_proto_context *pc, u32 id)
2441 {
2442 struct i915_gem_context *ctx;
2443 void *old;
2444
2445 ctx = i915_gem_create_context(file_priv->dev_priv, pc);
2446 if (IS_ERR(ctx))
2447 return ctx;
2448
2449 gem_context_register(ctx, file_priv, id);
2450
2451 old = xa_erase(&file_priv->proto_context_xa, id);
2452 GEM_BUG_ON(old != pc);
2453 proto_context_close(pc);
2454
2455 /* One for the xarray and one for the caller */
2456 return i915_gem_context_get(ctx);
2457 }
2458
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation
Hi Jason,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on drm-tip/drm-tip drm-exynos/exynos-drm-next
next-20210423]
[cannot apply to tegra-drm/drm/tegra/for-next drm/drm-next v5.12-rc8]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Jason-Ekstrand/drm-i915-gem-ioctl-clean-ups/20210424-063511
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-a001-20210423 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
#
https://github.com/0day-ci/linux/commit/e00622bd8a3f3eccbb22721c2f8857bdfb7d5d9d
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Jason-Ekstrand/drm-i915-gem-ioctl-clean-ups/20210424-063511
git checkout e00622bd8a3f3eccbb22721c2f8857bdfb7d5d9d
# save the attached .config to linux build tree
make W=1 W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
>> drivers/gpu/drm/i915/gem/i915_gem_context.c:2439:1: error: no previous
>> prototype for 'lazy_create_context_locked' [-Werror=missing-prototypes]
2439 | lazy_create_context_locked(struct drm_i915_file_private *file_priv,
| ^~
cc1: all warnings being treated as errors
vim +/lazy_create_context_locked +2439
drivers/gpu/drm/i915/gem/i915_gem_context.c
2437
2438 struct i915_gem_context *
> 2439 lazy_create_context_locked(struct drm_i915_file_private *file_priv,
2440 struct i915_gem_proto_context *pc, u32 id)
2441 {
2442 struct i915_gem_context *ctx;
2443 void *old;
2444
2445 ctx = i915_gem_create_context(file_priv->dev_priv, pc);
2446 if (IS_ERR(ctx))
2447 return ctx;
2448
2449 gem_context_register(ctx, file_priv, id);
2450
2451 old = xa_erase(&file_priv->proto_context_xa, id);
2452 GEM_BUG_ON(old != pc);
2453 proto_context_close(pc);
2454
2455 /* One for the xarray and one for the caller */
2456 return i915_gem_context_get(ctx);
2457 }
2458
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
