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

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

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(&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

Looks reasonable, I'll give it a try.

Daniele

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx