| Message ID | 20210105231947.31235-6-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Split GuC submission from execlists submission | expand |
Quoting Daniele Ceraolo Spurio (2021-01-05 23:19:47) > In GuC submission mode the CS is owned by the GuC FW, so all CS status > interrupts are handled by it. We only need the user interrupt as that > signals request completion. > > Since we're now starting the engines directly in GuC submission mode > when selected, we can stop switching back and forth between the > execlists and the GuC programming and select directly the correct > interrupt mask. > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Matthew Brost <matthew.brost@intel.com> > Cc: John Harrison <john.c.harrison@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_gt_irq.c | 18 ++++++----- > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 31 ------------------- > 2 files changed, 11 insertions(+), 38 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c > index 9830342aa6f4..7b2b8cb2d2be 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c > @@ -237,14 +237,18 @@ void gen11_gt_irq_reset(struct intel_gt *gt) > > void gen11_gt_irq_postinstall(struct intel_gt *gt) > { > - const u32 irqs = > - GT_CS_MASTER_ERROR_INTERRUPT | > - GT_RENDER_USER_INTERRUPT | > - GT_CONTEXT_SWITCH_INTERRUPT | > - GT_WAIT_SEMAPHORE_INTERRUPT; > struct intel_uncore *uncore = gt->uncore; > - const u32 dmask = irqs << 16 | irqs; > - const u32 smask = irqs << 16; > + u32 irqs = GT_RENDER_USER_INTERRUPT; > + u32 dmask; > + u32 smask; > + > + if (!intel_uc_wants_guc_submission(>->uc)) > + irqs |= GT_CS_MASTER_ERROR_INTERRUPT | > + GT_CONTEXT_SWITCH_INTERRUPT | > + GT_WAIT_SEMAPHORE_INTERRUPT; Hmm, we should stop performing this by default then, and make the execlists setup request the interrupt vector it desires. That's certainly a bit more fiddly to untangle the packed iir across multiple gen. :| -Chris
On 1/5/2021 3:38 PM, Chris Wilson wrote: > Quoting Daniele Ceraolo Spurio (2021-01-05 23:19:47) >> In GuC submission mode the CS is owned by the GuC FW, so all CS status >> interrupts are handled by it. We only need the user interrupt as that >> signals request completion. >> >> Since we're now starting the engines directly in GuC submission mode >> when selected, we can stop switching back and forth between the >> execlists and the GuC programming and select directly the correct >> interrupt mask. >> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Matthew Brost <matthew.brost@intel.com> >> Cc: John Harrison <john.c.harrison@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> --- >> drivers/gpu/drm/i915/gt/intel_gt_irq.c | 18 ++++++----- >> .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 31 ------------------- >> 2 files changed, 11 insertions(+), 38 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c >> index 9830342aa6f4..7b2b8cb2d2be 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c >> +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c >> @@ -237,14 +237,18 @@ void gen11_gt_irq_reset(struct intel_gt *gt) >> >> void gen11_gt_irq_postinstall(struct intel_gt *gt) >> { >> - const u32 irqs = >> - GT_CS_MASTER_ERROR_INTERRUPT | >> - GT_RENDER_USER_INTERRUPT | >> - GT_CONTEXT_SWITCH_INTERRUPT | >> - GT_WAIT_SEMAPHORE_INTERRUPT; >> struct intel_uncore *uncore = gt->uncore; >> - const u32 dmask = irqs << 16 | irqs; >> - const u32 smask = irqs << 16; >> + u32 irqs = GT_RENDER_USER_INTERRUPT; >> + u32 dmask; >> + u32 smask; >> + >> + if (!intel_uc_wants_guc_submission(>->uc)) >> + irqs |= GT_CS_MASTER_ERROR_INTERRUPT | >> + GT_CONTEXT_SWITCH_INTERRUPT | >> + GT_WAIT_SEMAPHORE_INTERRUPT; > Hmm, we should stop performing this by default then, and make the > execlists setup request the interrupt vector it desires. > > That's certainly a bit more fiddly to untangle the packed iir across > multiple gen. :| I had considered that, but this is a gt-level setup while the execlists submission code is currently all engine-based, so I couldn't find a good place to move this to and I didn't want to add a new function just for it. Any preference? Daniele > -Chris
Quoting Daniele Ceraolo Spurio (2021-01-05 23:56:52) > > > On 1/5/2021 3:38 PM, Chris Wilson wrote: > > Quoting Daniele Ceraolo Spurio (2021-01-05 23:19:47) > >> In GuC submission mode the CS is owned by the GuC FW, so all CS status > >> interrupts are handled by it. We only need the user interrupt as that > >> signals request completion. > >> > >> Since we're now starting the engines directly in GuC submission mode > >> when selected, we can stop switching back and forth between the > >> execlists and the GuC programming and select directly the correct > >> interrupt mask. > >> > >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >> Cc: Matthew Brost <matthew.brost@intel.com> > >> Cc: John Harrison <john.c.harrison@intel.com> > >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > >> --- > >> drivers/gpu/drm/i915/gt/intel_gt_irq.c | 18 ++++++----- > >> .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 31 ------------------- > >> 2 files changed, 11 insertions(+), 38 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c > >> index 9830342aa6f4..7b2b8cb2d2be 100644 > >> --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c > >> +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c > >> @@ -237,14 +237,18 @@ void gen11_gt_irq_reset(struct intel_gt *gt) > >> > >> void gen11_gt_irq_postinstall(struct intel_gt *gt) > >> { > >> - const u32 irqs = > >> - GT_CS_MASTER_ERROR_INTERRUPT | > >> - GT_RENDER_USER_INTERRUPT | > >> - GT_CONTEXT_SWITCH_INTERRUPT | > >> - GT_WAIT_SEMAPHORE_INTERRUPT; > >> struct intel_uncore *uncore = gt->uncore; > >> - const u32 dmask = irqs << 16 | irqs; > >> - const u32 smask = irqs << 16; > >> + u32 irqs = GT_RENDER_USER_INTERRUPT; > >> + u32 dmask; > >> + u32 smask; > >> + > >> + if (!intel_uc_wants_guc_submission(>->uc)) > >> + irqs |= GT_CS_MASTER_ERROR_INTERRUPT | > >> + GT_CONTEXT_SWITCH_INTERRUPT | > >> + GT_WAIT_SEMAPHORE_INTERRUPT; > > Hmm, we should stop performing this by default then, and make the > > execlists setup request the interrupt vector it desires. > > > > That's certainly a bit more fiddly to untangle the packed iir across > > multiple gen. :| > > I had considered that, but this is a gt-level setup while the execlists > submission code is currently all engine-based, so I couldn't find a good > place to move this to and I didn't want to add a new function just for > it. Any preference? I think we should definitely strive to avoid having too many backend- conditional paths in the common routines, and we can certainly adjust the interrupts as we enable each engine (it certainly would help when debugging by removing engines by removing spurious iir). I guess we may end up with something like gen8_gt_unmask_engine_irq(struct intel_gt *gt, struct intel_engine_cs *engine, u16 iir) { if (INTEL_GEN(gt->i915) >= 11) __gen11... else __gen8... } [Not sure if gen8_gt or intel_gt; I chose gen8_gt for the iir definition] Then that would fit reasonably into logical_ring_default_irqs(). Hindsight says that if we had done that earlier, we could have avoided maintaining the same engine-iir in two different sites. So maybe worth a bit of effort. -Chris
Quoting Daniele Ceraolo Spurio (2021-01-05 23:19:47) > In GuC submission mode the CS is owned by the GuC FW, so all CS status > interrupts are handled by it. We only need the user interrupt as that > signals request completion. > > Since we're now starting the engines directly in GuC submission mode > when selected, we can stop switching back and forth between the > execlists and the GuC programming and select directly the correct > interrupt mask. > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Matthew Brost <matthew.brost@intel.com> > Cc: John Harrison <john.c.harrison@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_gt_irq.c | 18 ++++++----- > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 31 ------------------- > 2 files changed, 11 insertions(+), 38 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c > index 9830342aa6f4..7b2b8cb2d2be 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c > @@ -237,14 +237,18 @@ void gen11_gt_irq_reset(struct intel_gt *gt) > > void gen11_gt_irq_postinstall(struct intel_gt *gt) > { > - const u32 irqs = > - GT_CS_MASTER_ERROR_INTERRUPT | > - GT_RENDER_USER_INTERRUPT | > - GT_CONTEXT_SWITCH_INTERRUPT | > - GT_WAIT_SEMAPHORE_INTERRUPT; > struct intel_uncore *uncore = gt->uncore; > - const u32 dmask = irqs << 16 | irqs; > - const u32 smask = irqs << 16; > + u32 irqs = GT_RENDER_USER_INTERRUPT; > + u32 dmask; > + u32 smask; > + > + if (!intel_uc_wants_guc_submission(>->uc)) > + irqs |= GT_CS_MASTER_ERROR_INTERRUPT | > + GT_CONTEXT_SWITCH_INTERRUPT | > + GT_WAIT_SEMAPHORE_INTERRUPT; Since these are all inside the engine mask as well, we can set the GT iir with minimal concern. Is there any advantage in having a patch with if (!intel_uc_wants_guc_submission(>->uc)) here at this point in time? -Chris
On 1/5/2021 4:15 PM, Chris Wilson wrote: > Quoting Daniele Ceraolo Spurio (2021-01-05 23:56:52) >> >> On 1/5/2021 3:38 PM, Chris Wilson wrote: >>> Quoting Daniele Ceraolo Spurio (2021-01-05 23:19:47) >>>> In GuC submission mode the CS is owned by the GuC FW, so all CS status >>>> interrupts are handled by it. We only need the user interrupt as that >>>> signals request completion. >>>> >>>> Since we're now starting the engines directly in GuC submission mode >>>> when selected, we can stop switching back and forth between the >>>> execlists and the GuC programming and select directly the correct >>>> interrupt mask. >>>> >>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>>> Cc: Matthew Brost <matthew.brost@intel.com> >>>> Cc: John Harrison <john.c.harrison@intel.com> >>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/gt/intel_gt_irq.c | 18 ++++++----- >>>> .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 31 ------------------- >>>> 2 files changed, 11 insertions(+), 38 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c >>>> index 9830342aa6f4..7b2b8cb2d2be 100644 >>>> --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c >>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c >>>> @@ -237,14 +237,18 @@ void gen11_gt_irq_reset(struct intel_gt *gt) >>>> >>>> void gen11_gt_irq_postinstall(struct intel_gt *gt) >>>> { >>>> - const u32 irqs = >>>> - GT_CS_MASTER_ERROR_INTERRUPT | >>>> - GT_RENDER_USER_INTERRUPT | >>>> - GT_CONTEXT_SWITCH_INTERRUPT | >>>> - GT_WAIT_SEMAPHORE_INTERRUPT; >>>> struct intel_uncore *uncore = gt->uncore; >>>> - const u32 dmask = irqs << 16 | irqs; >>>> - const u32 smask = irqs << 16; >>>> + u32 irqs = GT_RENDER_USER_INTERRUPT; >>>> + u32 dmask; >>>> + u32 smask; >>>> + >>>> + if (!intel_uc_wants_guc_submission(>->uc)) >>>> + irqs |= GT_CS_MASTER_ERROR_INTERRUPT | >>>> + GT_CONTEXT_SWITCH_INTERRUPT | >>>> + GT_WAIT_SEMAPHORE_INTERRUPT; >>> Hmm, we should stop performing this by default then, and make the >>> execlists setup request the interrupt vector it desires. >>> >>> That's certainly a bit more fiddly to untangle the packed iir across >>> multiple gen. :| >> I had considered that, but this is a gt-level setup while the execlists >> submission code is currently all engine-based, so I couldn't find a good >> place to move this to and I didn't want to add a new function just for >> it. Any preference? > I think we should definitely strive to avoid having too many backend- > conditional paths in the common routines, and we can certainly adjust > the interrupts as we enable each engine (it certainly would help when > debugging by removing engines by removing spurious iir). > > I guess we may end up with something like > gen8_gt_unmask_engine_irq(struct intel_gt *gt, > struct intel_engine_cs *engine, > u16 iir) > { > if (INTEL_GEN(gt->i915) >= 11) > __gen11... > else > __gen8... > } > [Not sure if gen8_gt or intel_gt; I chose gen8_gt for the iir definition] > > Then that would fit reasonably into logical_ring_default_irqs(). > Hindsight says that if we had done that earlier, we could have avoided > maintaining the same engine-iir in two different sites. So maybe worth > a bit of effort. > -Chris Looks reasonable, I'll give it a try. Daniele
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c index 9830342aa6f4..7b2b8cb2d2be 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c @@ -237,14 +237,18 @@ void gen11_gt_irq_reset(struct intel_gt *gt) void gen11_gt_irq_postinstall(struct intel_gt *gt) { - const u32 irqs = - GT_CS_MASTER_ERROR_INTERRUPT | - GT_RENDER_USER_INTERRUPT | - GT_CONTEXT_SWITCH_INTERRUPT | - GT_WAIT_SEMAPHORE_INTERRUPT; struct intel_uncore *uncore = gt->uncore; - const u32 dmask = irqs << 16 | irqs; - const u32 smask = irqs << 16; + u32 irqs = GT_RENDER_USER_INTERRUPT; + u32 dmask; + u32 smask; + + if (!intel_uc_wants_guc_submission(>->uc)) + irqs |= GT_CS_MASTER_ERROR_INTERRUPT | + GT_CONTEXT_SWITCH_INTERRUPT | + GT_WAIT_SEMAPHORE_INTERRUPT; + + dmask = irqs << 16 | irqs; + smask = irqs << 16; BUILD_BUG_ON(irqs & 0xffff0000); diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index 3993f1d75e87..84ec118f270d 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -424,32 +424,6 @@ void intel_guc_submission_fini(struct intel_guc *guc) } } -static void guc_interrupts_capture(struct intel_gt *gt) -{ - struct intel_uncore *uncore = gt->uncore; - u32 irqs = GT_CONTEXT_SWITCH_INTERRUPT; - u32 dmask = irqs << 16 | irqs; - - GEM_BUG_ON(INTEL_GEN(gt->i915) < 11); - - /* Don't handle the ctx switch interrupt in GuC submission mode */ - intel_uncore_rmw(uncore, GEN11_RENDER_COPY_INTR_ENABLE, dmask, 0); - intel_uncore_rmw(uncore, GEN11_VCS_VECS_INTR_ENABLE, dmask, 0); -} - -static void guc_interrupts_release(struct intel_gt *gt) -{ - struct intel_uncore *uncore = gt->uncore; - u32 irqs = GT_CONTEXT_SWITCH_INTERRUPT; - u32 dmask = irqs << 16 | irqs; - - GEM_BUG_ON(INTEL_GEN(gt->i915) < 11); - - /* Handle ctx switch interrupts again */ - intel_uncore_rmw(uncore, GEN11_RENDER_COPY_INTR_ENABLE, 0, dmask); - intel_uncore_rmw(uncore, GEN11_VCS_VECS_INTR_ENABLE, 0, dmask); -} - static int guc_context_alloc(struct intel_context *ce) { return lrc_alloc(ce, ce->engine); @@ -751,9 +725,6 @@ int intel_guc_submission_setup(struct intel_engine_cs *engine) void intel_guc_submission_enable(struct intel_guc *guc) { guc_stage_desc_init(guc); - - /* Take over from manual control of ELSP (execlists) */ - guc_interrupts_capture(guc_to_gt(guc)); } void intel_guc_submission_disable(struct intel_guc *guc) @@ -764,8 +735,6 @@ void intel_guc_submission_disable(struct intel_guc *guc) /* Note: By the time we're here, GuC may have already been reset */ - guc_interrupts_release(gt); - guc_stage_desc_fini(guc); }
In GuC submission mode the CS is owned by the GuC FW, so all CS status interrupts are handled by it. We only need the user interrupt as that signals request completion. Since we're now starting the engines directly in GuC submission mode when selected, we can stop switching back and forth between the execlists and the GuC programming and select directly the correct interrupt mask. Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Matthew Brost <matthew.brost@intel.com> Cc: John Harrison <john.c.harrison@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> --- drivers/gpu/drm/i915/gt/intel_gt_irq.c | 18 ++++++----- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 31 ------------------- 2 files changed, 11 insertions(+), 38 deletions(-)