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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@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.

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