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

[Gustavo Sousa]

Quoting Lucas De Marchi (2023-09-25 14:09:56-03:00)
>On Wed, Sep 20, 2023 at 04:45:35PM -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.
>>
>>v2:
>>  - Make resetting GFX_MSTR_IRQ be the last step to avoid bit
>>    re-latching. (Ville)
>>
>>BSpec: 50875, 54028, 62357
>>Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>>Cc: Matt Roper <matthew.d.roper at intel.com>
>>Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>Signed-off-by: Gustavo Sousa <gustavo.sousa at intel.com>
>>---
>> drivers/gpu/drm/xe/xe_irq.c | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>>diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
>>index ccb934f8fa34..cb03f40df272 100644
>>--- a/drivers/gpu/drm/xe/xe_irq.c
>>+++ b/drivers/gpu/drm/xe/xe_irq.c
>>@@ -519,6 +519,13 @@ static void dg1_irq_reset(struct xe_tile *tile)
>>         mask_and_disable(tile, PCU_IRQ_OFFSET);
>> }
>>
>>+static void dg1_irq_reset_mstr(struct xe_tile *tile)
>>+{
>>+        struct xe_gt *mmio = tile->primary_gt;
>>+
>>+        xe_mmio_write32(mmio, GFX_MSTR_IRQ, ~0);
>>+}
>>+
>> static void xe_irq_reset(struct xe_device *xe)
>> {
>>         struct xe_tile *tile;
>>@@ -534,6 +541,15 @@ static void xe_irq_reset(struct xe_device *xe)
>>         tile = xe_device_get_root_tile(xe);
>>         mask_and_disable(tile, GU_MISC_IRQ_OFFSET);
>>         xe_display_irq_reset(xe);
>>+
>>+        /*
>>+         * The tile's top-level status register should be the last one
>>+         * to be reset to avoid possible bit re-latching from lower
>>+         * level interrupts.
>>+         */
>>+        for_each_tile(tile, xe, id)
>>+                if (GRAPHICS_VERx100(xe) >= 1210)
>
>Better with the version checked swapped:
>        
>        if (GRAPHICS_VERx100(xe) >= 1210)
>                for_each_tile(tile, xe, id)
>                        dg1_irq_reset_mstr(tile);
>
>The code in the beginning of this function is about choosing between 2
>versions of the function, which is not the case here

Hah! That was my first approach, but it looked inconsistent with the above loop.
:-)

Thanks, I just sent a v3 swapping the order.

--
Gustavo Sousa

>
>Lucas De Marchi
>
>>+                        dg1_irq_reset_mstr(tile);
>> }
>>
>> static void xe_irq_postinstall(struct xe_device *xe)
>>-- 
>>2.42.0
>>