Re: [Intel-gfx] [PATCH 1/1] drm/i915: Introduce i915_sched_engine object
On Thu, Jun 03, 2021 at 11:05:15AM +0200, Daniel Vetter wrote: > On Thu, Jun 3, 2021 at 5:30 AM Matthew Brost wrote: > > > > On Mon, May 31, 2021 at 10:31:53AM +0200, Daniel Vetter wrote: > > > On Thu, May 27, 2021 at 4:19 PM Matthew Brost > > > wrote: > > > > > > > > On Thu, May 27, 2021 at 10:41:10AM +0100, Tvrtko Ursulin wrote: > > > > > > > > > > On 26/05/2021 21:03, Matthew Brost wrote: > > > > > > Introduce i915_sched_engine object which is lower level data > > > > > > structure > > > > > > that i915_scheduler / generic code can operate on without touching > > > > > > execlist specific structures. This allows additional submission > > > > > > backends > > > > > > to be added without breaking the layering. > > > > > > > > > > > > This is a bit of detour to integrating the i915 with the DRM > > > > > > scheduler > > > > > > but this object will still exist when the DRM scheduler lands in the > > > > > > i915. It will however look a bit different. It will encapsulate the > > > > > > drm_gpu_scheduler object plus and common variables (to the backends) > > > > > > related to scheduling. Regardless this is a step in the right > > > > > > direction. > > > > > > > > > > It looks like pretty much the same concept Chris implemented in > > > > > upstream(*) before they got removed. Both series move the scheduling > > > > > lists into a new object, be it i915_sched, or i915_sched_engine. > > > > > There probably are some differences but without looking much deeper > > > > > it is hard to say if they are important. > > > > > > > > > > Were you aware of this series and were there any technical objections > > > > > to it? > > > > > > > > > > > > > I haven't dug to deep into the series but yes, I am aware of this > > > > series. I > > > > merged my series to internal while Chris had this inflight upstream. > > > > They do > > > > have similar concepts of i915_sched_engine object. > > > > > > Imo both of them are a detour since it's not drm/scheduler, but also > > > we need one and this one here is the one we have so I'm not seeing the > > > point of repainting that bikeshed in different colors. Imo much better > > > if we just land this and then head as straight as possible towards > > > drm/scheduler as the interface. > > > > > > > Agree. > > > > > Now a bit of polish here might be good, but in entirely different ways: > > > - I think splitting the patch into the purely mechanical code movement > > > and addition of the new callbacks would be good. Should slice really > > > well I hope, so not much work. > > > > > > > This patch is basically find / replace in the sense it moves existing > > variables + vfuncs from one structure to another. It does add a few > > wrappers so not to touch the new structures variables but nothing else > > beyond that. IMO there really isn't a logical split point. > > The thing is, it's big, and in big patches like this it's easy to hide > an accidental change. And then it's really tricky to found out what > exactly happened. > > The other thing is, this is supposed to be entirely mechanical > refactors, piled up into a single patch. This should split easily, and > every step should be fully functional, if it doesn't, there's a > problem in the patch. > Ah, I see your point. We discussed this today in sync on upstreaming the GuC code and I will split this patch into a series of smaller ones which should result in the same full tree diff in the end + make the review quite a bit easier. Matt > Pretty much the only reason I don't care much here is that we're going > to redo this all again anyway, but in generally more splitting and > less smashing everything into one patch is better. > -Daniel > > > > > > - Proper kerneldoc for at least anything new around datastructures. > > > That means a) check the header is pulled in somewhere suitable in > > > i915.rst b) minimal fixes for any warnings it throws c) then do right > > > in anything new. Especially with datastructure stuff like > > > locking/lifetime rules or rules around callbacks and these big ticket > > > items are important to document and cross reference, and I think the > > > pain for doing a)) for these is worth it. > > > > > > > I'll add some kerenl doc in my next post of this patch. > > > > Matt > > > > > > > Because there do appear to be some extra advantages in the dropped > > > > > work, like consolidating finding of active request and moving some > > > > > other bits to be backend agnostic. > > > > > > > > > > > > > After briefly looking at this series most of Chris's changes are not > > > > relevent if > > > > we move to DRM scheduler. All of the the below series [1] and internal > > > > is based > > > > this code not Chris's. I don't see the point revisiting Chris's old > > > > patches when > > > > the goal is to merge GuC submission quickly, rework it is place as > > > > needed, and > > > > the long term goal is to move to the DRM scheduler. > > > > > > Agreed. > > > > > > Cheers, Daniel > > >
Re: [Intel-gfx] [PATCH 1/1] drm/i915: Introduce i915_sched_engine object
On Thu, Jun 3, 2021 at 5:30 AM Matthew Brost wrote: > > On Mon, May 31, 2021 at 10:31:53AM +0200, Daniel Vetter wrote: > > On Thu, May 27, 2021 at 4:19 PM Matthew Brost > > wrote: > > > > > > On Thu, May 27, 2021 at 10:41:10AM +0100, Tvrtko Ursulin wrote: > > > > > > > > On 26/05/2021 21:03, Matthew Brost wrote: > > > > > Introduce i915_sched_engine object which is lower level data structure > > > > > that i915_scheduler / generic code can operate on without touching > > > > > execlist specific structures. This allows additional submission > > > > > backends > > > > > to be added without breaking the layering. > > > > > > > > > > This is a bit of detour to integrating the i915 with the DRM scheduler > > > > > but this object will still exist when the DRM scheduler lands in the > > > > > i915. It will however look a bit different. It will encapsulate the > > > > > drm_gpu_scheduler object plus and common variables (to the backends) > > > > > related to scheduling. Regardless this is a step in the right > > > > > direction. > > > > > > > > It looks like pretty much the same concept Chris implemented in > > > > upstream(*) before they got removed. Both series move the scheduling > > > > lists into a new object, be it i915_sched, or i915_sched_engine. There > > > > probably are some differences but without looking much deeper it is > > > > hard to say if they are important. > > > > > > > > Were you aware of this series and were there any technical objections > > > > to it? > > > > > > > > > > I haven't dug to deep into the series but yes, I am aware of this series. > > > I > > > merged my series to internal while Chris had this inflight upstream. They > > > do > > > have similar concepts of i915_sched_engine object. > > > > Imo both of them are a detour since it's not drm/scheduler, but also > > we need one and this one here is the one we have so I'm not seeing the > > point of repainting that bikeshed in different colors. Imo much better > > if we just land this and then head as straight as possible towards > > drm/scheduler as the interface. > > > > Agree. > > > Now a bit of polish here might be good, but in entirely different ways: > > - I think splitting the patch into the purely mechanical code movement > > and addition of the new callbacks would be good. Should slice really > > well I hope, so not much work. > > > > This patch is basically find / replace in the sense it moves existing > variables + vfuncs from one structure to another. It does add a few > wrappers so not to touch the new structures variables but nothing else > beyond that. IMO there really isn't a logical split point. The thing is, it's big, and in big patches like this it's easy to hide an accidental change. And then it's really tricky to found out what exactly happened. The other thing is, this is supposed to be entirely mechanical refactors, piled up into a single patch. This should split easily, and every step should be fully functional, if it doesn't, there's a problem in the patch. Pretty much the only reason I don't care much here is that we're going to redo this all again anyway, but in generally more splitting and less smashing everything into one patch is better. -Daniel > > > - Proper kerneldoc for at least anything new around datastructures. > > That means a) check the header is pulled in somewhere suitable in > > i915.rst b) minimal fixes for any warnings it throws c) then do right > > in anything new. Especially with datastructure stuff like > > locking/lifetime rules or rules around callbacks and these big ticket > > items are important to document and cross reference, and I think the > > pain for doing a)) for these is worth it. > > > > I'll add some kerenl doc in my next post of this patch. > > Matt > > > > > Because there do appear to be some extra advantages in the dropped > > > > work, like consolidating finding of active request and moving some > > > > other bits to be backend agnostic. > > > > > > > > > > After briefly looking at this series most of Chris's changes are not > > > relevent if > > > we move to DRM scheduler. All of the the below series [1] and internal is > > > based > > > this code not Chris's. I don't see the point revisiting Chris's old > > > patches when > > > the goal is to merge GuC submission quickly, rework it is place as > > > needed, and > > > the long term goal is to move to the DRM scheduler. > > > > Agreed. > > > > Cheers, Daniel > > > > > > > > Matt > > > > > > [1] https://patchwork.freedesktop.org/series/89844/ > > > > > > > Regards, > > > > > > > > Tvrtko > > > > > > > > > > > > *) Approx list: > > > > > > > > 564c84ac5dee drm/i915: Move finding the current active request to the > > > > scheduler > > > > f06f5161eba3 drm/i915: Move submit_request to i915_sched_engine > > > > 38a40d211075 drm/i915/gt: Only kick the scheduler on > > > > timeslice/preemption change > > > > 4f08e41b51e2 drm/i915: Move tasklet from execlists to sched > > > > 5d814ae56fdd drm/i915:
Re: [Intel-gfx] [PATCH 1/1] drm/i915: Introduce i915_sched_engine object
On Mon, May 31, 2021 at 10:31:53AM +0200, Daniel Vetter wrote: > On Thu, May 27, 2021 at 4:19 PM Matthew Brost wrote: > > > > On Thu, May 27, 2021 at 10:41:10AM +0100, Tvrtko Ursulin wrote: > > > > > > On 26/05/2021 21:03, Matthew Brost wrote: > > > > Introduce i915_sched_engine object which is lower level data structure > > > > that i915_scheduler / generic code can operate on without touching > > > > execlist specific structures. This allows additional submission backends > > > > to be added without breaking the layering. > > > > > > > > This is a bit of detour to integrating the i915 with the DRM scheduler > > > > but this object will still exist when the DRM scheduler lands in the > > > > i915. It will however look a bit different. It will encapsulate the > > > > drm_gpu_scheduler object plus and common variables (to the backends) > > > > related to scheduling. Regardless this is a step in the right direction. > > > > > > It looks like pretty much the same concept Chris implemented in > > > upstream(*) before they got removed. Both series move the scheduling > > > lists into a new object, be it i915_sched, or i915_sched_engine. There > > > probably are some differences but without looking much deeper it is hard > > > to say if they are important. > > > > > > Were you aware of this series and were there any technical objections to > > > it? > > > > > > > I haven't dug to deep into the series but yes, I am aware of this series. I > > merged my series to internal while Chris had this inflight upstream. They do > > have similar concepts of i915_sched_engine object. > > Imo both of them are a detour since it's not drm/scheduler, but also > we need one and this one here is the one we have so I'm not seeing the > point of repainting that bikeshed in different colors. Imo much better > if we just land this and then head as straight as possible towards > drm/scheduler as the interface. > Agree. > Now a bit of polish here might be good, but in entirely different ways: > - I think splitting the patch into the purely mechanical code movement > and addition of the new callbacks would be good. Should slice really > well I hope, so not much work. > This patch is basically find / replace in the sense it moves existing variables + vfuncs from one structure to another. It does add a few wrappers so not to touch the new structures variables but nothing else beyond that. IMO there really isn't a logical split point. > - Proper kerneldoc for at least anything new around datastructures. > That means a) check the header is pulled in somewhere suitable in > i915.rst b) minimal fixes for any warnings it throws c) then do right > in anything new. Especially with datastructure stuff like > locking/lifetime rules or rules around callbacks and these big ticket > items are important to document and cross reference, and I think the > pain for doing a)) for these is worth it. > I'll add some kerenl doc in my next post of this patch. Matt > > > Because there do appear to be some extra advantages in the dropped work, > > > like consolidating finding of active request and moving some other bits > > > to be backend agnostic. > > > > > > > After briefly looking at this series most of Chris's changes are not > > relevent if > > we move to DRM scheduler. All of the the below series [1] and internal is > > based > > this code not Chris's. I don't see the point revisiting Chris's old patches > > when > > the goal is to merge GuC submission quickly, rework it is place as needed, > > and > > the long term goal is to move to the DRM scheduler. > > Agreed. > > Cheers, Daniel > > > > > Matt > > > > [1] https://patchwork.freedesktop.org/series/89844/ > > > > > Regards, > > > > > > Tvrtko > > > > > > > > > *) Approx list: > > > > > > 564c84ac5dee drm/i915: Move finding the current active request to the > > > scheduler > > > f06f5161eba3 drm/i915: Move submit_request to i915_sched_engine > > > 38a40d211075 drm/i915/gt: Only kick the scheduler on timeslice/preemption > > > change > > > 4f08e41b51e2 drm/i915: Move tasklet from execlists to sched > > > 5d814ae56fdd drm/i915: Move scheduler queue > > > 4d4da5ab8b3c drm/i915: Move common active lists from engine to > > > i915_scheduler > > > 4a5c90b6f7a8 drm/i915: Wrap access to intel_engine.active > > > 34cab8ee986f drm/i915/gt: Pull all execlists scheduler initialisation > > > together > > > c968d4d87cf4 drm/i915: Extract the ability to defer and rerun a request > > > later > > > 746cafc44205 drm/i915: Extract request suspension from the execlists > > > 3d473f1476d8 drm/i915: Extract request rewinding from execlists > > > d353a704a6db drm/i915: Extract request submission from execlists > > > ea848ef93075 drm/i915: Replace engine->schedule() with a known request > > > operation > > > > > > > > > > > Cc: Daniel Vetter > > > > Cc: Daniele Ceraolo Spurio > > > > Signed-off-by: Matthew Brost > > > > --- > > > > drivers/gpu/drm/i915/gem/i915_gem_wait.c | 4 +- > > >
Re: [Intel-gfx] [PATCH 1/1] drm/i915: Introduce i915_sched_engine object
On Thu, May 27, 2021 at 4:19 PM Matthew Brost wrote: > > On Thu, May 27, 2021 at 10:41:10AM +0100, Tvrtko Ursulin wrote: > > > > On 26/05/2021 21:03, Matthew Brost wrote: > > > Introduce i915_sched_engine object which is lower level data structure > > > that i915_scheduler / generic code can operate on without touching > > > execlist specific structures. This allows additional submission backends > > > to be added without breaking the layering. > > > > > > This is a bit of detour to integrating the i915 with the DRM scheduler > > > but this object will still exist when the DRM scheduler lands in the > > > i915. It will however look a bit different. It will encapsulate the > > > drm_gpu_scheduler object plus and common variables (to the backends) > > > related to scheduling. Regardless this is a step in the right direction. > > > > It looks like pretty much the same concept Chris implemented in upstream(*) > > before they got removed. Both series move the scheduling lists into a new > > object, be it i915_sched, or i915_sched_engine. There probably are some > > differences but without looking much deeper it is hard to say if they are > > important. > > > > Were you aware of this series and were there any technical objections to it? > > > > I haven't dug to deep into the series but yes, I am aware of this series. I > merged my series to internal while Chris had this inflight upstream. They do > have similar concepts of i915_sched_engine object. Imo both of them are a detour since it's not drm/scheduler, but also we need one and this one here is the one we have so I'm not seeing the point of repainting that bikeshed in different colors. Imo much better if we just land this and then head as straight as possible towards drm/scheduler as the interface. Now a bit of polish here might be good, but in entirely different ways: - I think splitting the patch into the purely mechanical code movement and addition of the new callbacks would be good. Should slice really well I hope, so not much work. - Proper kerneldoc for at least anything new around datastructures. That means a) check the header is pulled in somewhere suitable in i915.rst b) minimal fixes for any warnings it throws c) then do right in anything new. Especially with datastructure stuff like locking/lifetime rules or rules around callbacks and these big ticket items are important to document and cross reference, and I think the pain for doing a)) for these is worth it. > > Because there do appear to be some extra advantages in the dropped work, > > like consolidating finding of active request and moving some other bits to > > be backend agnostic. > > > > After briefly looking at this series most of Chris's changes are not relevent > if > we move to DRM scheduler. All of the the below series [1] and internal is > based > this code not Chris's. I don't see the point revisiting Chris's old patches > when > the goal is to merge GuC submission quickly, rework it is place as needed, and > the long term goal is to move to the DRM scheduler. Agreed. Cheers, Daniel > > Matt > > [1] https://patchwork.freedesktop.org/series/89844/ > > > Regards, > > > > Tvrtko > > > > > > *) Approx list: > > > > 564c84ac5dee drm/i915: Move finding the current active request to the > > scheduler > > f06f5161eba3 drm/i915: Move submit_request to i915_sched_engine > > 38a40d211075 drm/i915/gt: Only kick the scheduler on timeslice/preemption > > change > > 4f08e41b51e2 drm/i915: Move tasklet from execlists to sched > > 5d814ae56fdd drm/i915: Move scheduler queue > > 4d4da5ab8b3c drm/i915: Move common active lists from engine to > > i915_scheduler > > 4a5c90b6f7a8 drm/i915: Wrap access to intel_engine.active > > 34cab8ee986f drm/i915/gt: Pull all execlists scheduler initialisation > > together > > c968d4d87cf4 drm/i915: Extract the ability to defer and rerun a request > > later > > 746cafc44205 drm/i915: Extract request suspension from the execlists > > 3d473f1476d8 drm/i915: Extract request rewinding from execlists > > d353a704a6db drm/i915: Extract request submission from execlists > > ea848ef93075 drm/i915: Replace engine->schedule() with a known request > > operation > > > > > > > > Cc: Daniel Vetter > > > Cc: Daniele Ceraolo Spurio > > > Signed-off-by: Matthew Brost > > > --- > > > drivers/gpu/drm/i915/gem/i915_gem_wait.c | 4 +- > > > drivers/gpu/drm/i915/gt/intel_engine.h| 16 - > > > drivers/gpu/drm/i915/gt/intel_engine_cs.c | 77 ++-- > > > .../gpu/drm/i915/gt/intel_engine_heartbeat.c | 4 +- > > > drivers/gpu/drm/i915/gt/intel_engine_pm.c | 10 +- > > > drivers/gpu/drm/i915/gt/intel_engine_types.h | 42 +-- > > > drivers/gpu/drm/i915/gt/intel_engine_user.c | 2 +- > > > .../drm/i915/gt/intel_execlists_submission.c | 350 +++--- > > > .../gpu/drm/i915/gt/intel_ring_submission.c | 13 +- > > > drivers/gpu/drm/i915/gt/mock_engine.c | 17 +- > > >
Re: [Intel-gfx] [PATCH 1/1] drm/i915: Introduce i915_sched_engine object
On Thu, May 27, 2021 at 10:41:10AM +0100, Tvrtko Ursulin wrote: > > On 26/05/2021 21:03, Matthew Brost wrote: > > Introduce i915_sched_engine object which is lower level data structure > > that i915_scheduler / generic code can operate on without touching > > execlist specific structures. This allows additional submission backends > > to be added without breaking the layering. > > > > This is a bit of detour to integrating the i915 with the DRM scheduler > > but this object will still exist when the DRM scheduler lands in the > > i915. It will however look a bit different. It will encapsulate the > > drm_gpu_scheduler object plus and common variables (to the backends) > > related to scheduling. Regardless this is a step in the right direction. > > It looks like pretty much the same concept Chris implemented in upstream(*) > before they got removed. Both series move the scheduling lists into a new > object, be it i915_sched, or i915_sched_engine. There probably are some > differences but without looking much deeper it is hard to say if they are > important. > > Were you aware of this series and were there any technical objections to it? > I haven't dug to deep into the series but yes, I am aware of this series. I merged my series to internal while Chris had this inflight upstream. They do have similar concepts of i915_sched_engine object. > Because there do appear to be some extra advantages in the dropped work, like > consolidating finding of active request and moving some other bits to be > backend agnostic. > After briefly looking at this series most of Chris's changes are not relevent if we move to DRM scheduler. All of the the below series [1] and internal is based this code not Chris's. I don't see the point revisiting Chris's old patches when the goal is to merge GuC submission quickly, rework it is place as needed, and the long term goal is to move to the DRM scheduler. Matt [1] https://patchwork.freedesktop.org/series/89844/ > Regards, > > Tvrtko > > > *) Approx list: > > 564c84ac5dee drm/i915: Move finding the current active request to the > scheduler > f06f5161eba3 drm/i915: Move submit_request to i915_sched_engine > 38a40d211075 drm/i915/gt: Only kick the scheduler on timeslice/preemption > change > 4f08e41b51e2 drm/i915: Move tasklet from execlists to sched > 5d814ae56fdd drm/i915: Move scheduler queue > 4d4da5ab8b3c drm/i915: Move common active lists from engine to i915_scheduler > 4a5c90b6f7a8 drm/i915: Wrap access to intel_engine.active > 34cab8ee986f drm/i915/gt: Pull all execlists scheduler initialisation together > c968d4d87cf4 drm/i915: Extract the ability to defer and rerun a request later > 746cafc44205 drm/i915: Extract request suspension from the execlists > 3d473f1476d8 drm/i915: Extract request rewinding from execlists > d353a704a6db drm/i915: Extract request submission from execlists > ea848ef93075 drm/i915: Replace engine->schedule() with a known request > operation > > > > > Cc: Daniel Vetter > > Cc: Daniele Ceraolo Spurio > > Signed-off-by: Matthew Brost > > --- > > drivers/gpu/drm/i915/gem/i915_gem_wait.c | 4 +- > > drivers/gpu/drm/i915/gt/intel_engine.h| 16 - > > drivers/gpu/drm/i915/gt/intel_engine_cs.c | 77 ++-- > > .../gpu/drm/i915/gt/intel_engine_heartbeat.c | 4 +- > > drivers/gpu/drm/i915/gt/intel_engine_pm.c | 10 +- > > drivers/gpu/drm/i915/gt/intel_engine_types.h | 42 +-- > > drivers/gpu/drm/i915/gt/intel_engine_user.c | 2 +- > > .../drm/i915/gt/intel_execlists_submission.c | 350 +++--- > > .../gpu/drm/i915/gt/intel_ring_submission.c | 13 +- > > drivers/gpu/drm/i915/gt/mock_engine.c | 17 +- > > drivers/gpu/drm/i915/gt/selftest_execlists.c | 36 +- > > drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 6 +- > > drivers/gpu/drm/i915/gt/selftest_lrc.c| 6 +- > > drivers/gpu/drm/i915/gt/selftest_reset.c | 2 +- > > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 75 ++-- > > drivers/gpu/drm/i915/i915_gpu_error.c | 7 +- > > drivers/gpu/drm/i915/i915_request.c | 50 +-- > > drivers/gpu/drm/i915/i915_request.h | 2 +- > > drivers/gpu/drm/i915/i915_scheduler.c | 168 - > > drivers/gpu/drm/i915/i915_scheduler.h | 65 +++- > > drivers/gpu/drm/i915/i915_scheduler_types.h | 63 > > 21 files changed, 575 insertions(+), 440 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c > > b/drivers/gpu/drm/i915/gem/i915_gem_wait.c > > index 4b9856d5ba14..af1fbf8e2a9a 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c > > @@ -104,8 +104,8 @@ static void fence_set_priority(struct dma_fence *fence, > > engine = rq->engine; > > rcu_read_lock(); /* RCU serialisation for set-wedged protection */ > > - if (engine->schedule) > > - engine->schedule(rq, attr); > > + if
Re: [Intel-gfx] [PATCH 1/1] drm/i915: Introduce i915_sched_engine object
On 26/05/2021 21:03, Matthew Brost wrote:
Introduce i915_sched_engine object which is lower level data structure
that i915_scheduler / generic code can operate on without touching
execlist specific structures. This allows additional submission backends
to be added without breaking the layering.
This is a bit of detour to integrating the i915 with the DRM scheduler
but this object will still exist when the DRM scheduler lands in the
i915. It will however look a bit different. It will encapsulate the
drm_gpu_scheduler object plus and common variables (to the backends)
related to scheduling. Regardless this is a step in the right direction.
It looks like pretty much the same concept Chris implemented in upstream(*)
before they got removed. Both series move the scheduling lists into a new
object, be it i915_sched, or i915_sched_engine. There probably are some
differences but without looking much deeper it is hard to say if they are
important.
Were you aware of this series and were there any technical objections to it?
Because there do appear to be some extra advantages in the dropped work, like
consolidating finding of active request and moving some other bits to be
backend agnostic.
Regards,
Tvrtko
*) Approx list:
564c84ac5dee drm/i915: Move finding the current active request to the scheduler
f06f5161eba3 drm/i915: Move submit_request to i915_sched_engine
38a40d211075 drm/i915/gt: Only kick the scheduler on timeslice/preemption change
4f08e41b51e2 drm/i915: Move tasklet from execlists to sched
5d814ae56fdd drm/i915: Move scheduler queue
4d4da5ab8b3c drm/i915: Move common active lists from engine to i915_scheduler
4a5c90b6f7a8 drm/i915: Wrap access to intel_engine.active
34cab8ee986f drm/i915/gt: Pull all execlists scheduler initialisation together
c968d4d87cf4 drm/i915: Extract the ability to defer and rerun a request later
746cafc44205 drm/i915: Extract request suspension from the execlists
3d473f1476d8 drm/i915: Extract request rewinding from execlists
d353a704a6db drm/i915: Extract request submission from execlists
ea848ef93075 drm/i915: Replace engine->schedule() with a known request operation
Cc: Daniel Vetter
Cc: Daniele Ceraolo Spurio
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gem/i915_gem_wait.c | 4 +-
drivers/gpu/drm/i915/gt/intel_engine.h| 16 -
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 77 ++--
.../gpu/drm/i915/gt/intel_engine_heartbeat.c | 4 +-
drivers/gpu/drm/i915/gt/intel_engine_pm.c | 10 +-
drivers/gpu/drm/i915/gt/intel_engine_types.h | 42 +--
drivers/gpu/drm/i915/gt/intel_engine_user.c | 2 +-
.../drm/i915/gt/intel_execlists_submission.c | 350 +++---
.../gpu/drm/i915/gt/intel_ring_submission.c | 13 +-
drivers/gpu/drm/i915/gt/mock_engine.c | 17 +-
drivers/gpu/drm/i915/gt/selftest_execlists.c | 36 +-
drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 6 +-
drivers/gpu/drm/i915/gt/selftest_lrc.c| 6 +-
drivers/gpu/drm/i915/gt/selftest_reset.c | 2 +-
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 75 ++--
drivers/gpu/drm/i915/i915_gpu_error.c | 7 +-
drivers/gpu/drm/i915/i915_request.c | 50 +--
drivers/gpu/drm/i915/i915_request.h | 2 +-
drivers/gpu/drm/i915/i915_scheduler.c | 168 -
drivers/gpu/drm/i915/i915_scheduler.h | 65 +++-
drivers/gpu/drm/i915/i915_scheduler_types.h | 63
21 files changed, 575 insertions(+), 440 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
index 4b9856d5ba14..af1fbf8e2a9a 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
@@ -104,8 +104,8 @@ static void fence_set_priority(struct dma_fence *fence,
engine = rq->engine;
rcu_read_lock(); /* RCU serialisation for set-wedged protection */
- if (engine->schedule)
- engine->schedule(rq, attr);
+ if (engine->sched_engine->schedule)
+ engine->sched_engine->schedule(rq, attr);
rcu_read_unlock();
}
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
index 8d9184920c51..988d9688ae4d 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -123,20 +123,6 @@ execlists_active(const struct intel_engine_execlists
*execlists)
return active;
}
-static inline void
-execlists_active_lock_bh(struct intel_engine_execlists *execlists)
-{
- local_bh_disable(); /* prevent local softirq and lock recursion */
- tasklet_lock(>tasklet);
-}
-
-static inline void
-execlists_active_unlock_bh(struct intel_engine_execlists *execlists)
-{
- tasklet_unlock(>tasklet);
- local_bh_enable(); /* restore softirq, and kick ksoftirqd! */
-}
-
struct i915_request *
execlists_unwind_incomplete_requests(struct intel_engine_execlists
*execlists);
[Intel-gfx] [PATCH 1/1] drm/i915: Introduce i915_sched_engine object
Introduce i915_sched_engine object which is lower level data structure
that i915_scheduler / generic code can operate on without touching
execlist specific structures. This allows additional submission backends
to be added without breaking the layering.
This is a bit of detour to integrating the i915 with the DRM scheduler
but this object will still exist when the DRM scheduler lands in the
i915. It will however look a bit different. It will encapsulate the
drm_gpu_scheduler object plus and common variables (to the backends)
related to scheduling. Regardless this is a step in the right direction.
Cc: Daniel Vetter
Cc: Daniele Ceraolo Spurio
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gem/i915_gem_wait.c | 4 +-
drivers/gpu/drm/i915/gt/intel_engine.h| 16 -
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 77 ++--
.../gpu/drm/i915/gt/intel_engine_heartbeat.c | 4 +-
drivers/gpu/drm/i915/gt/intel_engine_pm.c | 10 +-
drivers/gpu/drm/i915/gt/intel_engine_types.h | 42 +--
drivers/gpu/drm/i915/gt/intel_engine_user.c | 2 +-
.../drm/i915/gt/intel_execlists_submission.c | 350 +++---
.../gpu/drm/i915/gt/intel_ring_submission.c | 13 +-
drivers/gpu/drm/i915/gt/mock_engine.c | 17 +-
drivers/gpu/drm/i915/gt/selftest_execlists.c | 36 +-
drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 6 +-
drivers/gpu/drm/i915/gt/selftest_lrc.c| 6 +-
drivers/gpu/drm/i915/gt/selftest_reset.c | 2 +-
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 75 ++--
drivers/gpu/drm/i915/i915_gpu_error.c | 7 +-
drivers/gpu/drm/i915/i915_request.c | 50 +--
drivers/gpu/drm/i915/i915_request.h | 2 +-
drivers/gpu/drm/i915/i915_scheduler.c | 168 -
drivers/gpu/drm/i915/i915_scheduler.h | 65 +++-
drivers/gpu/drm/i915/i915_scheduler_types.h | 63
21 files changed, 575 insertions(+), 440 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
index 4b9856d5ba14..af1fbf8e2a9a 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
@@ -104,8 +104,8 @@ static void fence_set_priority(struct dma_fence *fence,
engine = rq->engine;
rcu_read_lock(); /* RCU serialisation for set-wedged protection */
- if (engine->schedule)
- engine->schedule(rq, attr);
+ if (engine->sched_engine->schedule)
+ engine->sched_engine->schedule(rq, attr);
rcu_read_unlock();
}
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h
b/drivers/gpu/drm/i915/gt/intel_engine.h
index 8d9184920c51..988d9688ae4d 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -123,20 +123,6 @@ execlists_active(const struct intel_engine_execlists
*execlists)
return active;
}
-static inline void
-execlists_active_lock_bh(struct intel_engine_execlists *execlists)
-{
- local_bh_disable(); /* prevent local softirq and lock recursion */
- tasklet_lock(>tasklet);
-}
-
-static inline void
-execlists_active_unlock_bh(struct intel_engine_execlists *execlists)
-{
- tasklet_unlock(>tasklet);
- local_bh_enable(); /* restore softirq, and kick ksoftirqd! */
-}
-
struct i915_request *
execlists_unwind_incomplete_requests(struct intel_engine_execlists *execlists);
@@ -257,8 +243,6 @@ intel_engine_find_active_request(struct intel_engine_cs
*engine);
u32 intel_engine_context_size(struct intel_gt *gt, u8 class);
-void intel_engine_init_active(struct intel_engine_cs *engine,
- unsigned int subclass);
#define ENGINE_PHYSICAL0
#define ENGINE_MOCK1
#define ENGINE_VIRTUAL 2
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 3f9a811eb02b..dc939c8ef288 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -8,6 +8,7 @@
#include "gem/i915_gem_context.h"
#include "i915_drv.h"
+#include "i915_scheduler.h"
#include "intel_breadcrumbs.h"
#include "intel_context.h"
@@ -326,9 +327,6 @@ static int intel_engine_setup(struct intel_gt *gt, enum
intel_engine_id id)
if (engine->context_size)
DRIVER_CAPS(i915)->has_logical_contexts = true;
- /* Nothing to do here, execute in order of dependencies */
- engine->schedule = NULL;
-
ewma__engine_latency_init(>latency);
seqcount_init(>stats.lock);
@@ -583,9 +581,6 @@ void intel_engine_init_execlists(struct intel_engine_cs
*engine)
memset(execlists->pending, 0, sizeof(execlists->pending));
execlists->active =
memset(execlists->inflight, 0, sizeof(execlists->inflight));
-
- execlists->queue_priority_hint = INT_MIN;
- execlists->queue = RB_ROOT_CACHED;
}
static void cleanup_status_page(struct intel_engine_cs *engine)
@@ -712,11
