[Intel-gfx] [PATCH 5/5] drm/i915/guc: enable only the user interrupt when using GuC submission

[Chris Wilson]

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 at intel.com>
> >> Cc: Matthew Brost <matthew.brost at intel.com>
> >> Cc: John Harrison <john.c.harrison at intel.com>
> >> Cc: Michal Wajdeczko <michal.wajdeczko at 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(&gt->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