Re: [Intel-gfx] [PATCH 08/21] drm/i915/gem: Disallow bonding of virtual engines
On Sat, May 01, 2021 at 10:17:46AM -0700, Matthew Brost wrote: > On Fri, Apr 30, 2021 at 12:11:07PM +0200, Daniel Vetter wrote: > > On Thu, Apr 29, 2021 at 09:03:48PM -0700, Matthew Brost wrote: > > > On Thu, Apr 29, 2021 at 02:14:19PM +0200, Daniel Vetter wrote: > > > > On Wed, Apr 28, 2021 at 01:17:27PM -0500, Jason Ekstrand wrote: > > > > > On Wed, Apr 28, 2021 at 1:02 PM Matthew Brost > > > > > wrote: > > > > > > > > > > > > On Wed, Apr 28, 2021 at 12:46:07PM -0500, Jason Ekstrand wrote: > > > > > > > On Wed, Apr 28, 2021 at 12:26 PM Matthew Brost > > > > > > > wrote: > > > > > > > > Jumping on here mid-thread. For what is is worth to make > > > > > > > > execlists work > > > > > > > > with the upcoming parallel submission extension I leveraged > > > > > > > > some of the > > > > > > > > existing bonding code so I wouldn't be too eager to delete this > > > > > > > > code > > > > > > > > until that lands. > > > > > > > > > > > > > > Mind being a bit more specific about that? The motivation for > > > > > > > this > > > > > > > patch is that the current bonding handling and uAPI is, well, > > > > > > > very odd > > > > > > > and confusing IMO. It doesn't let you create sets of bonded > > > > > > > engines. > > > > > > > Instead you create engines and then bond them together after the > > > > > > > fact. > > > > > > > I didn't want to blindly duplicate those oddities with the > > > > > > > proto-ctx > > > > > > > stuff unless they were useful. With parallel submit, I would > > > > > > > expect > > > > > > > we want a more explicit API where you specify a set of engine > > > > > > > class/instance pairs to bond together into a single engine > > > > > > > similar to > > > > > > > how the current balancing API works. > > > > > > > > > > > > > > Of course, that's all focused on the API and not the internals. > > > > > > > But, > > > > > > > again, I'm not sure how we want things to look internally. What > > > > > > > we've > > > > > > > got now doesn't seem great for the GuC submission model but I'm > > > > > > > very > > > > > > > much not the expert there. I don't want to be working at cross > > > > > > > purposes to you and I'm happy to leave bits if you think they're > > > > > > > useful. But I thought I was clearing things away so that you can > > > > > > > put > > > > > > > in what you actually want for GuC/parallel submit. > > > > > > > > > > > > > > > > > > > Removing all the UAPI things are fine but I wouldn't delete some of > > > > > > the > > > > > > internal stuff (e.g. intel_virtual_engine_attach_bond, bond > > > > > > intel_context_ops, the hook for a submit fence, etc...) as that will > > > > > > still likely be used for the new parallel submission interface with > > > > > > execlists. As you say the new UAPI wont allow crazy configurations, > > > > > > only simple ones. > > > > > > > > > > I'm fine with leaving some of the internal bits for a little while if > > > > > it makes pulling the GuC scheduler in easier. I'm just a bit > > > > > skeptical of why you'd care about SUBMIT_FENCE. :-) Daniel, any > > > > > thoughts? > > > > > > > > Yeah I'm also wondering why we need this. Essentially your insight (and > > > > Tony Ye from media team confirmed) is that media umd never uses bonded > > > > on > > > > virtual engines. > > > > > > > > > > Well you should use virtual engines with parallel submission interface > > > if are you using it correctly. > > > > > > e.g. You want a 2 wide parallel submission and there are 4 engine > > > instances. > > > > > > You'd create 2 VEs: > > > > > > A: 0, 2 > > > B: 1, 3 > > > set_parallel > > > > So tbh I'm not really liking this part. At least my understanding is that > > with GuC this is really one overall virtual engine, backed by a multi-lrc. > > > > So it should fill one engine slot, not fill multiple virtual engines and > > then be an awkward thing wrapped on top. > > > > I think (but maybe my understanding of GuC and the parallel submit execbuf > > interface is wrong) that the parallel engine should occupy a single VE > > slot, not require additional VE just for fun (maybe the execlist backend > > would require that internally, but that should not leak into the higher > > levels, much less the uapi). And you submit your multi-batch execbuf on > > that single parallel VE, which then gets passed to GuC as a multi-LRC. > > Internally in the backend there's a bit of fan-out to put the right > > MI_BB_START into the right rings and all that, but again I think that > > should be backend concerns. > > > > Or am I missing something big here? > > Unfortunately that is not how the interface works. The user must > configure the engine set with either physical or virtual engines which > determine the valid placements of each BB (LRC, ring, whatever we want > to call it) and call the set parallel extension which validations engine > layout. After that the engines are ready be used with multi-BB > submission in single IOCTL. >
Re: [Intel-gfx] [PATCH 08/21] drm/i915/gem: Disallow bonding of virtual engines
On Fri, Apr 30, 2021 at 12:11:07PM +0200, Daniel Vetter wrote: > On Thu, Apr 29, 2021 at 09:03:48PM -0700, Matthew Brost wrote: > > On Thu, Apr 29, 2021 at 02:14:19PM +0200, Daniel Vetter wrote: > > > On Wed, Apr 28, 2021 at 01:17:27PM -0500, Jason Ekstrand wrote: > > > > On Wed, Apr 28, 2021 at 1:02 PM Matthew Brost > > > > wrote: > > > > > > > > > > On Wed, Apr 28, 2021 at 12:46:07PM -0500, Jason Ekstrand wrote: > > > > > > On Wed, Apr 28, 2021 at 12:26 PM Matthew Brost > > > > > > wrote: > > > > > > > Jumping on here mid-thread. For what is is worth to make > > > > > > > execlists work > > > > > > > with the upcoming parallel submission extension I leveraged some > > > > > > > of the > > > > > > > existing bonding code so I wouldn't be too eager to delete this > > > > > > > code > > > > > > > until that lands. > > > > > > > > > > > > Mind being a bit more specific about that? The motivation for this > > > > > > patch is that the current bonding handling and uAPI is, well, very > > > > > > odd > > > > > > and confusing IMO. It doesn't let you create sets of bonded > > > > > > engines. > > > > > > Instead you create engines and then bond them together after the > > > > > > fact. > > > > > > I didn't want to blindly duplicate those oddities with the proto-ctx > > > > > > stuff unless they were useful. With parallel submit, I would expect > > > > > > we want a more explicit API where you specify a set of engine > > > > > > class/instance pairs to bond together into a single engine similar > > > > > > to > > > > > > how the current balancing API works. > > > > > > > > > > > > Of course, that's all focused on the API and not the internals. > > > > > > But, > > > > > > again, I'm not sure how we want things to look internally. What > > > > > > we've > > > > > > got now doesn't seem great for the GuC submission model but I'm very > > > > > > much not the expert there. I don't want to be working at cross > > > > > > purposes to you and I'm happy to leave bits if you think they're > > > > > > useful. But I thought I was clearing things away so that you can > > > > > > put > > > > > > in what you actually want for GuC/parallel submit. > > > > > > > > > > > > > > > > Removing all the UAPI things are fine but I wouldn't delete some of > > > > > the > > > > > internal stuff (e.g. intel_virtual_engine_attach_bond, bond > > > > > intel_context_ops, the hook for a submit fence, etc...) as that will > > > > > still likely be used for the new parallel submission interface with > > > > > execlists. As you say the new UAPI wont allow crazy configurations, > > > > > only simple ones. > > > > > > > > I'm fine with leaving some of the internal bits for a little while if > > > > it makes pulling the GuC scheduler in easier. I'm just a bit > > > > skeptical of why you'd care about SUBMIT_FENCE. :-) Daniel, any > > > > thoughts? > > > > > > Yeah I'm also wondering why we need this. Essentially your insight (and > > > Tony Ye from media team confirmed) is that media umd never uses bonded on > > > virtual engines. > > > > > > > Well you should use virtual engines with parallel submission interface > > if are you using it correctly. > > > > e.g. You want a 2 wide parallel submission and there are 4 engine > > instances. > > > > You'd create 2 VEs: > > > > A: 0, 2 > > B: 1, 3 > > set_parallel > > So tbh I'm not really liking this part. At least my understanding is that > with GuC this is really one overall virtual engine, backed by a multi-lrc. > > So it should fill one engine slot, not fill multiple virtual engines and > then be an awkward thing wrapped on top. > > I think (but maybe my understanding of GuC and the parallel submit execbuf > interface is wrong) that the parallel engine should occupy a single VE > slot, not require additional VE just for fun (maybe the execlist backend > would require that internally, but that should not leak into the higher > levels, much less the uapi). And you submit your multi-batch execbuf on > that single parallel VE, which then gets passed to GuC as a multi-LRC. > Internally in the backend there's a bit of fan-out to put the right > MI_BB_START into the right rings and all that, but again I think that > should be backend concerns. > > Or am I missing something big here? Unfortunately that is not how the interface works. The user must configure the engine set with either physical or virtual engines which determine the valid placements of each BB (LRC, ring, whatever we want to call it) and call the set parallel extension which validations engine layout. After that the engines are ready be used with multi-BB submission in single IOCTL. We discussed this internally with the i915 developers + with the media for like 6 months and this is where we landed after some very contentious discussions. One of the proposals was pretty similar to your understanding but got NACK'd as it was too specific to what our HW can do / what the UMDs need rather than being able to
Re: [Intel-gfx] [PATCH 08/21] drm/i915/gem: Disallow bonding of virtual engines
On Thu, Apr 29, 2021 at 09:03:48PM -0700, Matthew Brost wrote: > On Thu, Apr 29, 2021 at 02:14:19PM +0200, Daniel Vetter wrote: > > On Wed, Apr 28, 2021 at 01:17:27PM -0500, Jason Ekstrand wrote: > > > On Wed, Apr 28, 2021 at 1:02 PM Matthew Brost > > > wrote: > > > > > > > > On Wed, Apr 28, 2021 at 12:46:07PM -0500, Jason Ekstrand wrote: > > > > > On Wed, Apr 28, 2021 at 12:26 PM Matthew Brost > > > > > wrote: > > > > > > Jumping on here mid-thread. For what is is worth to make execlists > > > > > > work > > > > > > with the upcoming parallel submission extension I leveraged some of > > > > > > the > > > > > > existing bonding code so I wouldn't be too eager to delete this code > > > > > > until that lands. > > > > > > > > > > Mind being a bit more specific about that? The motivation for this > > > > > patch is that the current bonding handling and uAPI is, well, very odd > > > > > and confusing IMO. It doesn't let you create sets of bonded engines. > > > > > Instead you create engines and then bond them together after the fact. > > > > > I didn't want to blindly duplicate those oddities with the proto-ctx > > > > > stuff unless they were useful. With parallel submit, I would expect > > > > > we want a more explicit API where you specify a set of engine > > > > > class/instance pairs to bond together into a single engine similar to > > > > > how the current balancing API works. > > > > > > > > > > Of course, that's all focused on the API and not the internals. But, > > > > > again, I'm not sure how we want things to look internally. What we've > > > > > got now doesn't seem great for the GuC submission model but I'm very > > > > > much not the expert there. I don't want to be working at cross > > > > > purposes to you and I'm happy to leave bits if you think they're > > > > > useful. But I thought I was clearing things away so that you can put > > > > > in what you actually want for GuC/parallel submit. > > > > > > > > > > > > > Removing all the UAPI things are fine but I wouldn't delete some of the > > > > internal stuff (e.g. intel_virtual_engine_attach_bond, bond > > > > intel_context_ops, the hook for a submit fence, etc...) as that will > > > > still likely be used for the new parallel submission interface with > > > > execlists. As you say the new UAPI wont allow crazy configurations, > > > > only simple ones. > > > > > > I'm fine with leaving some of the internal bits for a little while if > > > it makes pulling the GuC scheduler in easier. I'm just a bit > > > skeptical of why you'd care about SUBMIT_FENCE. :-) Daniel, any > > > thoughts? > > > > Yeah I'm also wondering why we need this. Essentially your insight (and > > Tony Ye from media team confirmed) is that media umd never uses bonded on > > virtual engines. > > > > Well you should use virtual engines with parallel submission interface > if are you using it correctly. > > e.g. You want a 2 wide parallel submission and there are 4 engine > instances. > > You'd create 2 VEs: > > A: 0, 2 > B: 1, 3 > set_parallel So tbh I'm not really liking this part. At least my understanding is that with GuC this is really one overall virtual engine, backed by a multi-lrc. So it should fill one engine slot, not fill multiple virtual engines and then be an awkward thing wrapped on top. I think (but maybe my understanding of GuC and the parallel submit execbuf interface is wrong) that the parallel engine should occupy a single VE slot, not require additional VE just for fun (maybe the execlist backend would require that internally, but that should not leak into the higher levels, much less the uapi). And you submit your multi-batch execbuf on that single parallel VE, which then gets passed to GuC as a multi-LRC. Internally in the backend there's a bit of fan-out to put the right MI_BB_START into the right rings and all that, but again I think that should be backend concerns. Or am I missing something big here? > For GuC submission we just configure context and the GuC load balances > it. > > For execlists we'd need to create bonds. > > Also likely the reason virtual engines wasn't used with the old > interface was we only had 2 instances max per class so no need for > virtual engines. If they used it for my above example if they were using > the interface correctly they would have to use virtual engines too. They do actually use virtual engines, it's just the virtual engine only contains a single one, and internally i915 folds that into the hw engine directly. So we can take away the entire implementation complexity. Also I still think for execlist we shouldn't bother with trying to enable parallel submit. Or at least only way down if there's no other reasonable option. > > So the only thing we need is the await_fence submit_fence logic to stall > > the subsequent patches just long enough. I think that stays. > > > > My implementation, for the new parallel submission interface, with > execlists used a bonds + priority boosts to
Re: [Intel-gfx] [PATCH 08/21] drm/i915/gem: Disallow bonding of virtual engines
On Thu, Apr 29, 2021 at 02:14:19PM +0200, Daniel Vetter wrote: > On Wed, Apr 28, 2021 at 01:17:27PM -0500, Jason Ekstrand wrote: > > On Wed, Apr 28, 2021 at 1:02 PM Matthew Brost > > wrote: > > > > > > On Wed, Apr 28, 2021 at 12:46:07PM -0500, Jason Ekstrand wrote: > > > > On Wed, Apr 28, 2021 at 12:26 PM Matthew Brost > > > > wrote: > > > > > Jumping on here mid-thread. For what is is worth to make execlists > > > > > work > > > > > with the upcoming parallel submission extension I leveraged some of > > > > > the > > > > > existing bonding code so I wouldn't be too eager to delete this code > > > > > until that lands. > > > > > > > > Mind being a bit more specific about that? The motivation for this > > > > patch is that the current bonding handling and uAPI is, well, very odd > > > > and confusing IMO. It doesn't let you create sets of bonded engines. > > > > Instead you create engines and then bond them together after the fact. > > > > I didn't want to blindly duplicate those oddities with the proto-ctx > > > > stuff unless they were useful. With parallel submit, I would expect > > > > we want a more explicit API where you specify a set of engine > > > > class/instance pairs to bond together into a single engine similar to > > > > how the current balancing API works. > > > > > > > > Of course, that's all focused on the API and not the internals. But, > > > > again, I'm not sure how we want things to look internally. What we've > > > > got now doesn't seem great for the GuC submission model but I'm very > > > > much not the expert there. I don't want to be working at cross > > > > purposes to you and I'm happy to leave bits if you think they're > > > > useful. But I thought I was clearing things away so that you can put > > > > in what you actually want for GuC/parallel submit. > > > > > > > > > > Removing all the UAPI things are fine but I wouldn't delete some of the > > > internal stuff (e.g. intel_virtual_engine_attach_bond, bond > > > intel_context_ops, the hook for a submit fence, etc...) as that will > > > still likely be used for the new parallel submission interface with > > > execlists. As you say the new UAPI wont allow crazy configurations, > > > only simple ones. > > > > I'm fine with leaving some of the internal bits for a little while if > > it makes pulling the GuC scheduler in easier. I'm just a bit > > skeptical of why you'd care about SUBMIT_FENCE. :-) Daniel, any > > thoughts? > > Yeah I'm also wondering why we need this. Essentially your insight (and > Tony Ye from media team confirmed) is that media umd never uses bonded on > virtual engines. > Well you should use virtual engines with parallel submission interface if are you using it correctly. e.g. You want a 2 wide parallel submission and there are 4 engine instances. You'd create 2 VEs: A: 0, 2 B: 1, 3 set_parallel For GuC submission we just configure context and the GuC load balances it. For execlists we'd need to create bonds. Also likely the reason virtual engines wasn't used with the old interface was we only had 2 instances max per class so no need for virtual engines. If they used it for my above example if they were using the interface correctly they would have to use virtual engines too. > So the only thing we need is the await_fence submit_fence logic to stall > the subsequent patches just long enough. I think that stays. > My implementation, for the new parallel submission interface, with execlists used a bonds + priority boosts to ensure both are present at the same time. This was used for both non-virtual and virtual engines. This was never reviewed though and the code died on the list. > All the additional logic with the cmpxchg lockless trickery and all that > isn't needed, because we _never_ have to select an engine for bonded > submission: It's always the single one available. > > This would mean that for execlist parallel submit we can apply a > limitation (beyond what GuC supports perhaps) and it's all ok. With that > everything except the submit fence await logic itself can go I think. > > Also one for Matt: We decided to ZBB implementing parallel submit on > execlist, it's going to be just for GuC. At least until someone starts > screaming really loudly. If this is the case, then bonds can be deleted. Matt > > Cheers, 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 08/21] drm/i915/gem: Disallow bonding of virtual engines
On Thu, Apr 29, 2021 at 7:54 AM Tvrtko Ursulin wrote: > > > On 29/04/2021 13:24, Daniel Vetter wrote: > > On Wed, Apr 28, 2021 at 04:51:19PM +0100, Tvrtko Ursulin wrote: > >> > >> On 23/04/2021 23:31, Jason Ekstrand wrote: > >>> This adds a bunch of complexity which the media driver has never > >>> actually used. The media driver does technically bond a balanced engine > >>> to another engine but the balanced engine only has one engine in the > >>> sibling set. This doesn't actually result in a virtual engine. > >> > >> For historical reference, this is not because uapi was over-engineered but > >> because certain SKUs never materialized. > > > > Jason said that for SKU with lots of media engines media-driver sets up a > > set of ctx in userspace with all the pairings (and I guess then load > > balances in userspace or something like that). Tony Ye also seems to have > > confirmed that. So I'm not clear on which SKU this is? > > Not sure if I should disclose it here. But anyway, platform which is > currently in upstream and was supposed to be the first to use this uapi > was supposed to have at least 4 vcs engines initially, or even 8 vcs + 4 > vecs at some point. That was the requirement uapi was designed for. For > that kind of platform there were supposed to be two virtual engines > created, with bonding, for instance parent = [vcs0, vcs2], child = > [vcs1, vcs3]; bonds = [vcs0 - vcs1, vcs2 - vcs3]. With more engines the > merrier. I've added the following to the commit message: This functionality was originally added to handle cases where we may have more than two video engines and media might want to load-balance their bonded submits by, for instance, submitting to a balanced vcs0-1 as the primary and then vcs2-3 as the secondary. However, no such hardware has shipped thus far and, if we ever want to enable such use-cases in the future, we'll use the up-and-coming parallel submit API which targets GuC submission. --Jason > Userspace load balancing, from memory, came into the picture only as a > consequence of balancing between two types of media pipelines which was > either working around the rcs contention or lack of sfc, or both. Along > the lines of - one stage of a media pipeline can be done either as GPGPU > work, or on the media engine, and so userspace was deciding to spawn "a > bit of these and a bit of those" to utilise all the GPU blocks. Not > really about frame split virtual engines and bonding, but completely > different load balancing, between gpgpu and fixed pipeline. > > Or maybe the real deal is only future platforms, and there we have GuC > > scheduler backend. > > Yes, because SKUs never materialised. > > > Not against adding a bit more context to the commit message, but we need > > to make sure what we put there is actually correct. Maybe best to ask > > Tony/Carl as part of getting an ack from them. > > I think there is no need - fact uapi was designed for way more engines > than we got to have is straight forward enough. > > Only unasked for flexibility in the uapi was the fact bonding can > express any dependency and not only N consecutive engines as media fixed > function needed at the time. I say "at the time" because in fact the > "consecutive" engines requirement also got more complicated / broken in > a following gen (via fusing and logical instance remapping), proving the > point having the uapi disassociated from the hw limitations of the _day_ > was a good call. > > Regards, > > Tvrtko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 08/21] drm/i915/gem: Disallow bonding of virtual engines
On 29/04/2021 13:24, Daniel Vetter wrote: On Wed, Apr 28, 2021 at 04:51:19PM +0100, Tvrtko Ursulin wrote: On 23/04/2021 23:31, Jason Ekstrand wrote: This adds a bunch of complexity which the media driver has never actually used. The media driver does technically bond a balanced engine to another engine but the balanced engine only has one engine in the sibling set. This doesn't actually result in a virtual engine. For historical reference, this is not because uapi was over-engineered but because certain SKUs never materialized. Jason said that for SKU with lots of media engines media-driver sets up a set of ctx in userspace with all the pairings (and I guess then load balances in userspace or something like that). Tony Ye also seems to have confirmed that. So I'm not clear on which SKU this is? Not sure if I should disclose it here. But anyway, platform which is currently in upstream and was supposed to be the first to use this uapi was supposed to have at least 4 vcs engines initially, or even 8 vcs + 4 vecs at some point. That was the requirement uapi was designed for. For that kind of platform there were supposed to be two virtual engines created, with bonding, for instance parent = [vcs0, vcs2], child = [vcs1, vcs3]; bonds = [vcs0 - vcs1, vcs2 - vcs3]. With more engines the merrier. Userspace load balancing, from memory, came into the picture only as a consequence of balancing between two types of media pipelines which was either working around the rcs contention or lack of sfc, or both. Along the lines of - one stage of a media pipeline can be done either as GPGPU work, or on the media engine, and so userspace was deciding to spawn "a bit of these and a bit of those" to utilise all the GPU blocks. Not really about frame split virtual engines and bonding, but completely different load balancing, between gpgpu and fixed pipeline. Or maybe the real deal is only future platforms, and there we have GuC scheduler backend. Yes, because SKUs never materialised. Not against adding a bit more context to the commit message, but we need to make sure what we put there is actually correct. Maybe best to ask Tony/Carl as part of getting an ack from them. I think there is no need - fact uapi was designed for way more engines than we got to have is straight forward enough. Only unasked for flexibility in the uapi was the fact bonding can express any dependency and not only N consecutive engines as media fixed function needed at the time. I say "at the time" because in fact the "consecutive" engines requirement also got more complicated / broken in a following gen (via fusing and logical instance remapping), proving the point having the uapi disassociated from the hw limitations of the _day_ was a good call. Regards, Tvrtko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 08/21] drm/i915/gem: Disallow bonding of virtual engines
On Wed, Apr 28, 2021 at 04:51:19PM +0100, Tvrtko Ursulin wrote:
>
> On 23/04/2021 23:31, Jason Ekstrand wrote:
> > This adds a bunch of complexity which the media driver has never
> > actually used. The media driver does technically bond a balanced engine
> > to another engine but the balanced engine only has one engine in the
> > sibling set. This doesn't actually result in a virtual engine.
>
> For historical reference, this is not because uapi was over-engineered but
> because certain SKUs never materialized.
Jason said that for SKU with lots of media engines media-driver sets up a
set of ctx in userspace with all the pairings (and I guess then load
balances in userspace or something like that). Tony Ye also seems to have
confirmed that. So I'm not clear on which SKU this is?
Or maybe the real deal is only future platforms, and there we have GuC
scheduler backend.
Not against adding a bit more context to the commit message, but we need
to make sure what we put there is actually correct. Maybe best to ask
Tony/Carl as part of getting an ack from them.
-Daniel
>
> Regards,
>
> Tvrtko
>
> > Unless some userspace badly wants it, there's no good reason to support
> > this case. This makes I915_CONTEXT_ENGINES_EXT_BOND a total no-op. We
> > leave the validation code in place in case we ever decide we want to do
> > something interesting with the bonding information.
> >
> > Signed-off-by: Jason Ekstrand
> > ---
> > drivers/gpu/drm/i915/gem/i915_gem_context.c | 18 +-
> > .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 2 +-
> > drivers/gpu/drm/i915/gt/intel_engine_types.h | 7 -
> > .../drm/i915/gt/intel_execlists_submission.c | 100
> > .../drm/i915/gt/intel_execlists_submission.h | 4 -
> > drivers/gpu/drm/i915/gt/selftest_execlists.c | 229 --
> > 6 files changed, 7 insertions(+), 353 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > index e8179918fa306..5f8d0faf783aa 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > @@ -1553,6 +1553,12 @@ set_engines__bond(struct i915_user_extension __user
> > *base, void *data)
> > }
> > virtual = set->engines->engines[idx]->engine;
> > + if (intel_engine_is_virtual(virtual)) {
> > + drm_dbg(>drm,
> > + "Bonding with virtual engines not allowed\n");
> > + return -EINVAL;
> > + }
> > +
> > err = check_user_mbz(>flags);
> > if (err)
> > return err;
> > @@ -1593,18 +1599,6 @@ set_engines__bond(struct i915_user_extension __user
> > *base, void *data)
> > n, ci.engine_class, ci.engine_instance);
> > return -EINVAL;
> > }
> > -
> > - /*
> > -* A non-virtual engine has no siblings to choose between; and
> > -* a submit fence will always be directed to the one engine.
> > -*/
> > - if (intel_engine_is_virtual(virtual)) {
> > - err = intel_virtual_engine_attach_bond(virtual,
> > - master,
> > - bond);
> > - if (err)
> > - return err;
> > - }
> > }
> > return 0;
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > index d640bba6ad9ab..efb2fa3522a42 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > @@ -3474,7 +3474,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
> > if (args->flags & I915_EXEC_FENCE_SUBMIT)
> > err = i915_request_await_execution(eb.request,
> >in_fence,
> > -
> > eb.engine->bond_execute);
> > + NULL);
> > else
> > err = i915_request_await_dma_fence(eb.request,
> >in_fence);
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > index 883bafc449024..68cfe5080325c 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > @@ -446,13 +446,6 @@ struct intel_engine_cs {
> > */
> > void(*submit_request)(struct i915_request *rq);
> > - /*
> > -* Called on signaling of a SUBMIT_FENCE, passing along the signaling
> > -* request down to the bonded pairs.
> > -*/
> > - void(*bond_execute)(struct i915_request *rq,
> > - struct dma_fence *signal);
> > -
> > /*
>
Re: [Intel-gfx] [PATCH 08/21] drm/i915/gem: Disallow bonding of virtual engines
On Wed, Apr 28, 2021 at 01:17:27PM -0500, Jason Ekstrand wrote: > On Wed, Apr 28, 2021 at 1:02 PM Matthew Brost wrote: > > > > On Wed, Apr 28, 2021 at 12:46:07PM -0500, Jason Ekstrand wrote: > > > On Wed, Apr 28, 2021 at 12:26 PM Matthew Brost > > > wrote: > > > > Jumping on here mid-thread. For what is is worth to make execlists work > > > > with the upcoming parallel submission extension I leveraged some of the > > > > existing bonding code so I wouldn't be too eager to delete this code > > > > until that lands. > > > > > > Mind being a bit more specific about that? The motivation for this > > > patch is that the current bonding handling and uAPI is, well, very odd > > > and confusing IMO. It doesn't let you create sets of bonded engines. > > > Instead you create engines and then bond them together after the fact. > > > I didn't want to blindly duplicate those oddities with the proto-ctx > > > stuff unless they were useful. With parallel submit, I would expect > > > we want a more explicit API where you specify a set of engine > > > class/instance pairs to bond together into a single engine similar to > > > how the current balancing API works. > > > > > > Of course, that's all focused on the API and not the internals. But, > > > again, I'm not sure how we want things to look internally. What we've > > > got now doesn't seem great for the GuC submission model but I'm very > > > much not the expert there. I don't want to be working at cross > > > purposes to you and I'm happy to leave bits if you think they're > > > useful. But I thought I was clearing things away so that you can put > > > in what you actually want for GuC/parallel submit. > > > > > > > Removing all the UAPI things are fine but I wouldn't delete some of the > > internal stuff (e.g. intel_virtual_engine_attach_bond, bond > > intel_context_ops, the hook for a submit fence, etc...) as that will > > still likely be used for the new parallel submission interface with > > execlists. As you say the new UAPI wont allow crazy configurations, > > only simple ones. > > I'm fine with leaving some of the internal bits for a little while if > it makes pulling the GuC scheduler in easier. I'm just a bit > skeptical of why you'd care about SUBMIT_FENCE. :-) Daniel, any > thoughts? Yeah I'm also wondering why we need this. Essentially your insight (and Tony Ye from media team confirmed) is that media umd never uses bonded on virtual engines. So the only thing we need is the await_fence submit_fence logic to stall the subsequent patches just long enough. I think that stays. All the additional logic with the cmpxchg lockless trickery and all that isn't needed, because we _never_ have to select an engine for bonded submission: It's always the single one available. This would mean that for execlist parallel submit we can apply a limitation (beyond what GuC supports perhaps) and it's all ok. With that everything except the submit fence await logic itself can go I think. Also one for Matt: We decided to ZBB implementing parallel submit on execlist, it's going to be just for GuC. At least until someone starts screaming really loudly. Cheers, 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 08/21] drm/i915/gem: Disallow bonding of virtual engines
On Wed, Apr 28, 2021 at 1:02 PM Matthew Brost wrote:
>
> On Wed, Apr 28, 2021 at 12:46:07PM -0500, Jason Ekstrand wrote:
> > On Wed, Apr 28, 2021 at 12:26 PM Matthew Brost
> > wrote:
> > >
> > > On Wed, Apr 28, 2021 at 12:18:29PM -0500, Jason Ekstrand wrote:
> > > > On Wed, Apr 28, 2021 at 5:13 AM Daniel Vetter wrote:
> > > > >
> > > > > On Tue, Apr 27, 2021 at 08:51:08AM -0500, Jason Ekstrand wrote:
> > > > > > On Fri, Apr 23, 2021 at 5:31 PM Jason Ekstrand
> > > > > > wrote:
> > > > > > >
> > > > > > > This adds a bunch of complexity which the media driver has never
> > > > > > > actually used. The media driver does technically bond a balanced
> > > > > > > engine
> > > > > > > to another engine but the balanced engine only has one engine in
> > > > > > > the
> > > > > > > sibling set. This doesn't actually result in a virtual engine.
> > > > > > >
> > > > > > > Unless some userspace badly wants it, there's no good reason to
> > > > > > > support
> > > > > > > this case. This makes I915_CONTEXT_ENGINES_EXT_BOND a total
> > > > > > > no-op. We
> > > > > > > leave the validation code in place in case we ever decide we want
> > > > > > > to do
> > > > > > > something interesting with the bonding information.
> > > > > > >
> > > > > > > Signed-off-by: Jason Ekstrand
> > > > > > > ---
> > > > > > > drivers/gpu/drm/i915/gem/i915_gem_context.c | 18 +-
> > > > > > > .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 2 +-
> > > > > > > drivers/gpu/drm/i915/gt/intel_engine_types.h | 7 -
> > > > > > > .../drm/i915/gt/intel_execlists_submission.c | 100
> > > > > > > .../drm/i915/gt/intel_execlists_submission.h | 4 -
> > > > > > > drivers/gpu/drm/i915/gt/selftest_execlists.c | 229
> > > > > > > --
> > > > > > > 6 files changed, 7 insertions(+), 353 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > > > > > b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > > > > > index e8179918fa306..5f8d0faf783aa 100644
> > > > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > > > > > @@ -1553,6 +1553,12 @@ set_engines__bond(struct
> > > > > > > i915_user_extension __user *base, void *data)
> > > > > > > }
> > > > > > > virtual = set->engines->engines[idx]->engine;
> > > > > > >
> > > > > > > + if (intel_engine_is_virtual(virtual)) {
> > > > > > > + drm_dbg(>drm,
> > > > > > > + "Bonding with virtual engines not
> > > > > > > allowed\n");
> > > > > > > + return -EINVAL;
> > > > > > > + }
> > > > > > > +
> > > > > > > err = check_user_mbz(>flags);
> > > > > > > if (err)
> > > > > > > return err;
> > > > > > > @@ -1593,18 +1599,6 @@ set_engines__bond(struct
> > > > > > > i915_user_extension __user *base, void *data)
> > > > > > > n, ci.engine_class,
> > > > > > > ci.engine_instance);
> > > > > > > return -EINVAL;
> > > > > > > }
> > > > > > > -
> > > > > > > - /*
> > > > > > > -* A non-virtual engine has no siblings to choose
> > > > > > > between; and
> > > > > > > -* a submit fence will always be directed to the
> > > > > > > one engine.
> > > > > > > -*/
> > > > > > > - if (intel_engine_is_virtual(virtual)) {
> > > > > > > - err =
> > > > > > > intel_virtual_engine_attach_bond(virtual,
> > > > > > > -
> > > > > > > master,
> > > > > > > -
> > > > > > > bond);
> > > > > > > - if (err)
> > > > > > > - return err;
> > > > > > > - }
> > > > > > > }
> > > > > > >
> > > > > > > return 0;
> > > > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > > > > > b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > > > > > index d640bba6ad9ab..efb2fa3522a42 100644
> > > > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > > > > > @@ -3474,7 +3474,7 @@ i915_gem_do_execbuffer(struct drm_device
> > > > > > > *dev,
> > > > > > > if (args->flags & I915_EXEC_FENCE_SUBMIT)
> > > > > > > err =
> > > > > > > i915_request_await_execution(eb.request,
> > > > > > >
> > > > > > > in_fence,
> > > > > > > -
> > > > > > > eb.engine->bond_execute);
> > > > > > > + NULL);
> > > > > > > else
> > > > > > > err =
> > > > > > >
Re: [Intel-gfx] [PATCH 08/21] drm/i915/gem: Disallow bonding of virtual engines
On Wed, Apr 28, 2021 at 12:46:07PM -0500, Jason Ekstrand wrote:
> On Wed, Apr 28, 2021 at 12:26 PM Matthew Brost
> wrote:
> >
> > On Wed, Apr 28, 2021 at 12:18:29PM -0500, Jason Ekstrand wrote:
> > > On Wed, Apr 28, 2021 at 5:13 AM Daniel Vetter wrote:
> > > >
> > > > On Tue, Apr 27, 2021 at 08:51:08AM -0500, Jason Ekstrand wrote:
> > > > > On Fri, Apr 23, 2021 at 5:31 PM Jason Ekstrand
> > > > > wrote:
> > > > > >
> > > > > > This adds a bunch of complexity which the media driver has never
> > > > > > actually used. The media driver does technically bond a balanced
> > > > > > engine
> > > > > > to another engine but the balanced engine only has one engine in the
> > > > > > sibling set. This doesn't actually result in a virtual engine.
> > > > > >
> > > > > > Unless some userspace badly wants it, there's no good reason to
> > > > > > support
> > > > > > this case. This makes I915_CONTEXT_ENGINES_EXT_BOND a total no-op.
> > > > > > We
> > > > > > leave the validation code in place in case we ever decide we want
> > > > > > to do
> > > > > > something interesting with the bonding information.
> > > > > >
> > > > > > Signed-off-by: Jason Ekstrand
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/gem/i915_gem_context.c | 18 +-
> > > > > > .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 2 +-
> > > > > > drivers/gpu/drm/i915/gt/intel_engine_types.h | 7 -
> > > > > > .../drm/i915/gt/intel_execlists_submission.c | 100
> > > > > > .../drm/i915/gt/intel_execlists_submission.h | 4 -
> > > > > > drivers/gpu/drm/i915/gt/selftest_execlists.c | 229
> > > > > > --
> > > > > > 6 files changed, 7 insertions(+), 353 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > > > > b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > > > > index e8179918fa306..5f8d0faf783aa 100644
> > > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > > > > @@ -1553,6 +1553,12 @@ set_engines__bond(struct i915_user_extension
> > > > > > __user *base, void *data)
> > > > > > }
> > > > > > virtual = set->engines->engines[idx]->engine;
> > > > > >
> > > > > > + if (intel_engine_is_virtual(virtual)) {
> > > > > > + drm_dbg(>drm,
> > > > > > + "Bonding with virtual engines not
> > > > > > allowed\n");
> > > > > > + return -EINVAL;
> > > > > > + }
> > > > > > +
> > > > > > err = check_user_mbz(>flags);
> > > > > > if (err)
> > > > > > return err;
> > > > > > @@ -1593,18 +1599,6 @@ set_engines__bond(struct i915_user_extension
> > > > > > __user *base, void *data)
> > > > > > n, ci.engine_class,
> > > > > > ci.engine_instance);
> > > > > > return -EINVAL;
> > > > > > }
> > > > > > -
> > > > > > - /*
> > > > > > -* A non-virtual engine has no siblings to choose
> > > > > > between; and
> > > > > > -* a submit fence will always be directed to the
> > > > > > one engine.
> > > > > > -*/
> > > > > > - if (intel_engine_is_virtual(virtual)) {
> > > > > > - err =
> > > > > > intel_virtual_engine_attach_bond(virtual,
> > > > > > -
> > > > > > master,
> > > > > > -
> > > > > > bond);
> > > > > > - if (err)
> > > > > > - return err;
> > > > > > - }
> > > > > > }
> > > > > >
> > > > > > return 0;
> > > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > > > > b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > > > > index d640bba6ad9ab..efb2fa3522a42 100644
> > > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > > > > @@ -3474,7 +3474,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
> > > > > > if (args->flags & I915_EXEC_FENCE_SUBMIT)
> > > > > > err =
> > > > > > i915_request_await_execution(eb.request,
> > > > > >in_fence,
> > > > > > -
> > > > > > eb.engine->bond_execute);
> > > > > > + NULL);
> > > > > > else
> > > > > > err =
> > > > > > i915_request_await_dma_fence(eb.request,
> > > > > >
> > > > > > in_fence);
> > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > > > > b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > > > > index
Re: [Intel-gfx] [PATCH 08/21] drm/i915/gem: Disallow bonding of virtual engines
On Wed, Apr 28, 2021 at 12:26 PM Matthew Brost wrote:
>
> On Wed, Apr 28, 2021 at 12:18:29PM -0500, Jason Ekstrand wrote:
> > On Wed, Apr 28, 2021 at 5:13 AM Daniel Vetter wrote:
> > >
> > > On Tue, Apr 27, 2021 at 08:51:08AM -0500, Jason Ekstrand wrote:
> > > > On Fri, Apr 23, 2021 at 5:31 PM Jason Ekstrand
> > > > wrote:
> > > > >
> > > > > This adds a bunch of complexity which the media driver has never
> > > > > actually used. The media driver does technically bond a balanced
> > > > > engine
> > > > > to another engine but the balanced engine only has one engine in the
> > > > > sibling set. This doesn't actually result in a virtual engine.
> > > > >
> > > > > Unless some userspace badly wants it, there's no good reason to
> > > > > support
> > > > > this case. This makes I915_CONTEXT_ENGINES_EXT_BOND a total no-op.
> > > > > We
> > > > > leave the validation code in place in case we ever decide we want to
> > > > > do
> > > > > something interesting with the bonding information.
> > > > >
> > > > > Signed-off-by: Jason Ekstrand
> > > > > ---
> > > > > drivers/gpu/drm/i915/gem/i915_gem_context.c | 18 +-
> > > > > .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 2 +-
> > > > > drivers/gpu/drm/i915/gt/intel_engine_types.h | 7 -
> > > > > .../drm/i915/gt/intel_execlists_submission.c | 100
> > > > > .../drm/i915/gt/intel_execlists_submission.h | 4 -
> > > > > drivers/gpu/drm/i915/gt/selftest_execlists.c | 229
> > > > > --
> > > > > 6 files changed, 7 insertions(+), 353 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > > > b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > > > index e8179918fa306..5f8d0faf783aa 100644
> > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > > > @@ -1553,6 +1553,12 @@ set_engines__bond(struct i915_user_extension
> > > > > __user *base, void *data)
> > > > > }
> > > > > virtual = set->engines->engines[idx]->engine;
> > > > >
> > > > > + if (intel_engine_is_virtual(virtual)) {
> > > > > + drm_dbg(>drm,
> > > > > + "Bonding with virtual engines not allowed\n");
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > err = check_user_mbz(>flags);
> > > > > if (err)
> > > > > return err;
> > > > > @@ -1593,18 +1599,6 @@ set_engines__bond(struct i915_user_extension
> > > > > __user *base, void *data)
> > > > > n, ci.engine_class,
> > > > > ci.engine_instance);
> > > > > return -EINVAL;
> > > > > }
> > > > > -
> > > > > - /*
> > > > > -* A non-virtual engine has no siblings to choose
> > > > > between; and
> > > > > -* a submit fence will always be directed to the one
> > > > > engine.
> > > > > -*/
> > > > > - if (intel_engine_is_virtual(virtual)) {
> > > > > - err =
> > > > > intel_virtual_engine_attach_bond(virtual,
> > > > > - master,
> > > > > - bond);
> > > > > - if (err)
> > > > > - return err;
> > > > > - }
> > > > > }
> > > > >
> > > > > return 0;
> > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > > > b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > > > index d640bba6ad9ab..efb2fa3522a42 100644
> > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > > > @@ -3474,7 +3474,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
> > > > > if (args->flags & I915_EXEC_FENCE_SUBMIT)
> > > > > err = i915_request_await_execution(eb.request,
> > > > >in_fence,
> > > > > -
> > > > > eb.engine->bond_execute);
> > > > > + NULL);
> > > > > else
> > > > > err = i915_request_await_dma_fence(eb.request,
> > > > >in_fence);
> > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > > > b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > > > index 883bafc449024..68cfe5080325c 100644
> > > > > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > > > @@ -446,13 +446,6 @@ struct intel_engine_cs {
> > > > > */
> > > > > void(*submit_request)(struct i915_request *rq);
> > > > >
> > > > > - /*
> > > > > -
Re: [Intel-gfx] [PATCH 08/21] drm/i915/gem: Disallow bonding of virtual engines
On Wed, Apr 28, 2021 at 12:18:29PM -0500, Jason Ekstrand wrote:
> On Wed, Apr 28, 2021 at 5:13 AM Daniel Vetter wrote:
> >
> > On Tue, Apr 27, 2021 at 08:51:08AM -0500, Jason Ekstrand wrote:
> > > On Fri, Apr 23, 2021 at 5:31 PM Jason Ekstrand
> > > wrote:
> > > >
> > > > This adds a bunch of complexity which the media driver has never
> > > > actually used. The media driver does technically bond a balanced engine
> > > > to another engine but the balanced engine only has one engine in the
> > > > sibling set. This doesn't actually result in a virtual engine.
> > > >
> > > > Unless some userspace badly wants it, there's no good reason to support
> > > > this case. This makes I915_CONTEXT_ENGINES_EXT_BOND a total no-op. We
> > > > leave the validation code in place in case we ever decide we want to do
> > > > something interesting with the bonding information.
> > > >
> > > > Signed-off-by: Jason Ekstrand
> > > > ---
> > > > drivers/gpu/drm/i915/gem/i915_gem_context.c | 18 +-
> > > > .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 2 +-
> > > > drivers/gpu/drm/i915/gt/intel_engine_types.h | 7 -
> > > > .../drm/i915/gt/intel_execlists_submission.c | 100
> > > > .../drm/i915/gt/intel_execlists_submission.h | 4 -
> > > > drivers/gpu/drm/i915/gt/selftest_execlists.c | 229 --
> > > > 6 files changed, 7 insertions(+), 353 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > > b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > > index e8179918fa306..5f8d0faf783aa 100644
> > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > > @@ -1553,6 +1553,12 @@ set_engines__bond(struct i915_user_extension
> > > > __user *base, void *data)
> > > > }
> > > > virtual = set->engines->engines[idx]->engine;
> > > >
> > > > + if (intel_engine_is_virtual(virtual)) {
> > > > + drm_dbg(>drm,
> > > > + "Bonding with virtual engines not allowed\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > err = check_user_mbz(>flags);
> > > > if (err)
> > > > return err;
> > > > @@ -1593,18 +1599,6 @@ set_engines__bond(struct i915_user_extension
> > > > __user *base, void *data)
> > > > n, ci.engine_class, ci.engine_instance);
> > > > return -EINVAL;
> > > > }
> > > > -
> > > > - /*
> > > > -* A non-virtual engine has no siblings to choose
> > > > between; and
> > > > -* a submit fence will always be directed to the one
> > > > engine.
> > > > -*/
> > > > - if (intel_engine_is_virtual(virtual)) {
> > > > - err = intel_virtual_engine_attach_bond(virtual,
> > > > - master,
> > > > - bond);
> > > > - if (err)
> > > > - return err;
> > > > - }
> > > > }
> > > >
> > > > return 0;
> > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > > b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > > index d640bba6ad9ab..efb2fa3522a42 100644
> > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > > @@ -3474,7 +3474,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
> > > > if (args->flags & I915_EXEC_FENCE_SUBMIT)
> > > > err = i915_request_await_execution(eb.request,
> > > >in_fence,
> > > > -
> > > > eb.engine->bond_execute);
> > > > + NULL);
> > > > else
> > > > err = i915_request_await_dma_fence(eb.request,
> > > >in_fence);
> > > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > > b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > > index 883bafc449024..68cfe5080325c 100644
> > > > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > > @@ -446,13 +446,6 @@ struct intel_engine_cs {
> > > > */
> > > > void(*submit_request)(struct i915_request *rq);
> > > >
> > > > - /*
> > > > -* Called on signaling of a SUBMIT_FENCE, passing along the
> > > > signaling
> > > > -* request down to the bonded pairs.
> > > > -*/
> > > > - void(*bond_execute)(struct i915_request *rq,
> > > > - struct dma_fence *signal);
> > > > -
> > > >
Re: [Intel-gfx] [PATCH 08/21] drm/i915/gem: Disallow bonding of virtual engines
On 23/04/2021 23:31, Jason Ekstrand wrote:
This adds a bunch of complexity which the media driver has never
actually used. The media driver does technically bond a balanced engine
to another engine but the balanced engine only has one engine in the
sibling set. This doesn't actually result in a virtual engine.
For historical reference, this is not because uapi was over-engineered
but because certain SKUs never materialized.
Regards,
Tvrtko
Unless some userspace badly wants it, there's no good reason to support
this case. This makes I915_CONTEXT_ENGINES_EXT_BOND a total no-op. We
leave the validation code in place in case we ever decide we want to do
something interesting with the bonding information.
Signed-off-by: Jason Ekstrand
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 18 +-
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 2 +-
drivers/gpu/drm/i915/gt/intel_engine_types.h | 7 -
.../drm/i915/gt/intel_execlists_submission.c | 100
.../drm/i915/gt/intel_execlists_submission.h | 4 -
drivers/gpu/drm/i915/gt/selftest_execlists.c | 229 --
6 files changed, 7 insertions(+), 353 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index e8179918fa306..5f8d0faf783aa 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1553,6 +1553,12 @@ set_engines__bond(struct i915_user_extension __user
*base, void *data)
}
virtual = set->engines->engines[idx]->engine;
+ if (intel_engine_is_virtual(virtual)) {
+ drm_dbg(>drm,
+ "Bonding with virtual engines not allowed\n");
+ return -EINVAL;
+ }
+
err = check_user_mbz(>flags);
if (err)
return err;
@@ -1593,18 +1599,6 @@ set_engines__bond(struct i915_user_extension __user
*base, void *data)
n, ci.engine_class, ci.engine_instance);
return -EINVAL;
}
-
- /*
-* A non-virtual engine has no siblings to choose between; and
-* a submit fence will always be directed to the one engine.
-*/
- if (intel_engine_is_virtual(virtual)) {
- err = intel_virtual_engine_attach_bond(virtual,
- master,
- bond);
- if (err)
- return err;
- }
}
return 0;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index d640bba6ad9ab..efb2fa3522a42 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -3474,7 +3474,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
if (args->flags & I915_EXEC_FENCE_SUBMIT)
err = i915_request_await_execution(eb.request,
in_fence,
-
eb.engine->bond_execute);
+ NULL);
else
err = i915_request_await_dma_fence(eb.request,
in_fence);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h
b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index 883bafc449024..68cfe5080325c 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -446,13 +446,6 @@ struct intel_engine_cs {
*/
void(*submit_request)(struct i915_request *rq);
- /*
-* Called on signaling of a SUBMIT_FENCE, passing along the signaling
-* request down to the bonded pairs.
-*/
- void(*bond_execute)(struct i915_request *rq,
- struct dma_fence *signal);
-
/*
* Call when the priority on a request has changed and it and its
* dependencies may need rescheduling. Note the request itself may
diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index de124870af44d..b6e2b59f133b7 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -181,18 +181,6 @@ struct virtual_engine {
int prio;
} nodes[I915_NUM_ENGINES];
- /*
-* Keep track of bonded pairs -- restrictions upon on our selection
-* of physical engines any particular request may be submitted to.
-* If we receive a submit-fence from a master engine, we will only
-* use one of sibling_mask physical
