[Intel-xe] [PATCH] drm/xe/irq: Clear GFX_MSTR_IRQ as part of IRQ reset

[Gustavo Sousa]

Quoting Lucas De Marchi (2023-09-20 01:08:04-03:00)
>On Tue, Sep 19, 2023 at 02:35:51PM -0300, Gustavo Sousa wrote:
>>Quoting Lucas De Marchi (2023-09-19 14:31:24-03:00)
>>>On Tue, Sep 19, 2023 at 11:41:10AM -0300, Gustavo Sousa wrote:
>>>>Starting with Xe_LP+, GFX_MSTR_IRQ contains status bits that have W1C
>>>>behavior. If we do not properly reset them, we would miss delivery of
>>>>interrupts if a pending bit is set when enabling IRQs.
>>>>
>>>>As an example, the display part of our probe routine contains paths
>>>>where we wait for vblank interrupts. If a display interrupt was already
>>>>pending when enabling IRQs, we would time out waiting for the vblank.
>>>>
>>>>That in fact happened recently when modprobing Xe on a Lunar Lake with a
>>>>specific configuration; and that's how we found out we were missing this
>>>>step in the IRQ enabling logic.
>>>>
>>>>Fix the issue by clearing GFX_MSTR_IRQ as part of the IRQ reset.
>>>>
>>>>BSpec: 50875, 54028, 62357
>>>>Signed-off-by: Gustavo Sousa <gustavo.sousa at intel.com>
>>>>---
>>>> drivers/gpu/drm/xe/xe_irq.c | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>>
>>>>diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
>>>>index ccb934f8fa34..3746e9204e48 100644
>>>>--- a/drivers/gpu/drm/xe/xe_irq.c
>>>>+++ b/drivers/gpu/drm/xe/xe_irq.c
>>>>@@ -456,6 +456,7 @@ static irqreturn_t dg1_irq_handler(int irq, void *arg)
>>>>
>>>> static void gt_irq_reset(struct xe_tile *tile)
>>>> {
>>>>+        struct xe_device *xe = tile_to_xe(tile);
>>>>         struct xe_gt *mmio = tile->primary_gt;
>>>>
>>>>         u32 ccs_mask = xe_hw_engine_mask_per_class(tile->primary_gt,
>>>>@@ -463,6 +464,9 @@ static void gt_irq_reset(struct xe_tile *tile)
>>>>         u32 bcs_mask = xe_hw_engine_mask_per_class(tile->primary_gt,
>>>>                                                    XE_ENGINE_CLASS_COPY);
>>>>
>>>>+        if (GRAPHICS_VERx100(xe) >= 1210)
>>>>+                xe_mmio_write32(mmio, GFX_MSTR_IRQ, ~0);
>>>
>>>shouldn´t you exclude bit 31 (MSTR_INT) since it'ss not a status bit
>>>and would rather enable the interrupts... ?
>>
>>I thought about that, but looking closer at the BSpec for that register, that
>>bit seems not to exist for the targeted graphics IPs.
>
>true, I missed that... but then this would be a weird place to reset
>it. We already forked the irq setup in xe_irq_reset(). Also, this
>is going to be called for each gt, which doesn't seem right.
>Better to move it to dg1_irq_reset().

Hm... Actually, gt_irq_reset() is being called for each tile. But I agree with
the suggestion that dg1_irq_reset() is indeed a better place for this. Thanks!

--
Gustavo Sousa

>
>Lucas De Marchi
>
>>
>>Gustavo Sousa
>>
>>>
>>>Lucas De Marchi
>>>
>>>>+
>>>>         /* Disable RCS, BCS, VCS and VECS class engines. */
>>>>         xe_mmio_write32(mmio, RENDER_COPY_INTR_ENABLE, 0);
>>>>         xe_mmio_write32(mmio, VCS_VECS_INTR_ENABLE, 0);
>>>>--
>>>>2.42.0
>>>>