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

[Gustavo Sousa]

Quoting Ville Syrjälä (2023-09-20 07:56:04-03:00)
>On Tue, Sep 19, 2023 at 11:08:04PM -0500, Lucas De Marchi wrote:
>> 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().
>
>It should also be cleared after disabling/masking off all the lower
>level interrupts, otherwise you could just get some bit relatching
>immediately after clearing it.

Meaning to move this to be done after the call to
xe_display_irq_reset()?

The BSpec says:

  "For any new interrupts set after the 190010h read, new interrupts are
  re-generated after setting 31 of 190008h."

Considering that we disable interrupts with dg1_intr_disable(), doesn't
that mean that relatching won't happen until dg1_intr_enable() gets
called?

Btw, from the sentence above, I believe the proper way of resetting this
register would be to read its value and then write it back instead of
simply writing 1's as I did in this version.

--
Gustavo Sousa

>
>Also, are the latched bits single or double buffered? If double
>then we need the double clear that standard IIR registers use.
>
>-- 
>Ville Syrjälä
>Intel