[Intel-gfx] [PATCH v3 3/5] drm/i915/mtl: make IRQ reset and postinstall multi-gt aware

[Ceraolo Spurio, Daniele]

On 3/6/2023 4:24 PM, Matt Roper wrote:
> On Mon, Mar 06, 2023 at 04:14:49PM -0800, Sripada, Radhakrishna wrote:
>> +Daniele,
>>
>> Hi Matt,
>>
>>> -----Original Message-----
>>> From: Roper, Matthew D <matthew.d.roper at intel.com>
>>> Sent: Monday, March 6, 2023 2:55 PM
>>> To: Sripada, Radhakrishna <radhakrishna.sripada at intel.com>
>>> Cc: intel-gfx at lists.freedesktop.org; De Marchi, Lucas
>>> <lucas.demarchi at intel.com>; Zanoni, Paulo R <paulo.r.zanoni at intel.com>
>>> Subject: Re: [Intel-gfx] [PATCH v3 3/5] drm/i915/mtl: make IRQ reset and
>>> postinstall multi-gt aware
>>>
>>> On Wed, Mar 01, 2023 at 12:10:51PM -0800, Radhakrishna Sripada wrote:
>>>> Irq reset and post install are to be made multi-gt aware for the
>>>> interrupts to work for the media tile on Meteorlake. Iterate through
>>>> all the gts to process irq reset for each gt.
>>> I think I mentioned on the previous version, but this isn't right.  MTL
>>> does not have separate interrupt registers for each GT the way
>>> multi-tile platforms like PVC do.  The GT interrupt registers you're
>>> handling here are in the sgunit so there's only a single copy of them;
>>> iterating over them multiple times in a row doesn't accomplish anything.
>>>
>>> The media engine bits are still on the same registers as the primary GT
>>> and the GSC and media GuC are new additional bits that need to be
>>> handled.  The necessary handling for the GSC and media GuC should have
>>> already landed in a187f13d51fa ("drm/i915/guc: handle interrupts from
>>> media GuC") and c07ee636901d ("drm/i915/mtl: add GSC CS interrupt
>>> support"), but if there's another bit that was missed somewhere (or if
>>> we were doing something like looking at the wrong intel_gt's engine mask
>>> somewhere), that would need to be addressed directly rather than just
>>> looping over the same IRQ registers multiple times.
>> This patch is needed to handle media interrupts. Without this patch we observed
>> GuC not loading/communication errors on media gt.
>>
>> My understanding is that Sgunit is embedded into the SAMedia block and hence need
>> To be iterated over separately.
> No, the sgunit is not replicated.  You can confirm by just going to the
> various IRQ register pages in the bspec...there's only a single register
> offset rather than (offset) and (offset+0x380000) like there are for GT
> GSI registers.  The i915 code also only adds the GSI offset to register
> operations in the 0x0 - 0x40000 range.

I agree with Matt that this is how the HW works. The issue here is that 
the gen11_gt_irq_* functions only program the interrupts for GuC and 
engines on the given GT, so when calling them for the root GT they will 
only program the interrupts for RCS, BCS, CCS and the root GuC. We could 
modify the functions to program all registers from the root GT, but IMO 
that doesn't work very well with how other parts of the driver implement 
the MTL multi-gt flow.

Daniele


>
> Matt
>
>> Daniele,
>> Can you confirm if that is accurate.
>>
>> Thanks,
>> RK
>>>
>>> Matt
>>>
>>>> Based on original version by Paulo and Tvrtko
>>>>
>>>> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>> Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>
>>>> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada at intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_irq.c | 30 ++++++++++++++++++------------
>>>>   1 file changed, 18 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>>>> index 417c981e4968..9377f59c1ac2 100644
>>>> --- a/drivers/gpu/drm/i915/i915_irq.c
>>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>>>> @@ -2759,16 +2759,19 @@ static void gen11_irq_reset(struct
>>> drm_i915_private *dev_priv)
>>>>   static void dg1_irq_reset(struct drm_i915_private *dev_priv)
>>>>   {
>>>> -	struct intel_gt *gt = to_gt(dev_priv);
>>>> -	struct intel_uncore *uncore = gt->uncore;
>>>> +	struct intel_gt *gt;
>>>> +	unsigned int i;
>>>>
>>>>   	dg1_master_intr_disable(dev_priv->uncore.regs);
>>>>
>>>> -	gen11_gt_irq_reset(gt);
>>>> -	gen11_display_irq_reset(dev_priv);
>>>> +	for_each_gt(gt, dev_priv, i) {
>>>> +		gen11_gt_irq_reset(gt);
>>>>
>>>> -	GEN3_IRQ_RESET(uncore, GEN11_GU_MISC_);
>>>> -	GEN3_IRQ_RESET(uncore, GEN8_PCU_);
>>>> +		GEN3_IRQ_RESET(gt->uncore, GEN11_GU_MISC_);
>>>> +		GEN3_IRQ_RESET(gt->uncore, GEN8_PCU_);
>>>> +	}
>>>> +
>>>> +	gen11_display_irq_reset(dev_priv);
>>>>   }
>>>>
>>>>   void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
>>>> @@ -3422,13 +3425,16 @@ static void gen11_irq_postinstall(struct
>>> drm_i915_private *dev_priv)
>>>>   static void dg1_irq_postinstall(struct drm_i915_private *dev_priv)
>>>>   {
>>>> -	struct intel_gt *gt = to_gt(dev_priv);
>>>> -	struct intel_uncore *uncore = gt->uncore;
>>>>   	u32 gu_misc_masked = GEN11_GU_MISC_GSE;
>>>> +	struct intel_gt *gt;
>>>> +	unsigned int i;
>>>>
>>>> -	gen11_gt_irq_postinstall(gt);
>>>> +	for_each_gt(gt, dev_priv, i) {
>>>> +		gen11_gt_irq_postinstall(gt);
>>>>
>>>> -	GEN3_IRQ_INIT(uncore, GEN11_GU_MISC_, ~gu_misc_masked,
>>> gu_misc_masked);
>>>> +		GEN3_IRQ_INIT(gt->uncore, GEN11_GU_MISC_,
>>> ~gu_misc_masked,
>>>> +			      gu_misc_masked);
>>>> +	}
>>>>
>>>>   	if (HAS_DISPLAY(dev_priv)) {
>>>>   		icp_irq_postinstall(dev_priv);
>>>> @@ -3437,8 +3443,8 @@ static void dg1_irq_postinstall(struct
>>> drm_i915_private *dev_priv)
>>>>   				   GEN11_DISPLAY_IRQ_ENABLE);
>>>>   	}
>>>>
>>>> -	dg1_master_intr_enable(uncore->regs);
>>>> -	intel_uncore_posting_read(uncore, DG1_MSTR_TILE_INTR);
>>>> +	dg1_master_intr_enable(to_gt(dev_priv)->uncore->regs);
>>>> +	intel_uncore_posting_read(to_gt(dev_priv)->uncore,
>>> DG1_MSTR_TILE_INTR);
>>>>   }
>>>>
>>>>   static void cherryview_irq_postinstall(struct drm_i915_private *dev_priv)
>>>> --
>>>> 2.34.1
>>>>
>>> --
>>> Matt Roper
>>> Graphics Software Engineer
>>> Linux GPU Platform Enablement
>>> Intel Corporation