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

[Ville Syrjälä]

On Thu, Sep 21, 2023 at 11:40:22AM -0300, Gustavo Sousa wrote:
> Quoting Ville Syrjälä (2023-09-20 17:29:42-03:00)
> >On Wed, Sep 20, 2023 at 05:13:36PM -0300, Gustavo Sousa wrote:
> >> Quoting Ville Syrjälä (2023-09-20 17:00:07-03:00)
> >> >On Wed, Sep 20, 2023 at 04:53:52PM -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.
> >> >> 
> >> >> Avoid the potential issue by clearing GFX_MSTR_IRQ as part of the IRQ
> >> >> reset.
> >> >> 
> >> >> v2:
> >> >>   - Move logic from gen11_gt_irq_reset() to dg1_irq_reset(). (Matt)
> >> >> 
> >> >> BSpec: 50875, 54028
> >> >> Cc: Matt Roper <matthew.d.roper at intel.com>
> >> >> Signed-off-by: Gustavo Sousa <gustavo.sousa at intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/i915/i915_irq.c | 2 ++
> >> >>  1 file changed, 2 insertions(+)
> >> >> 
> >> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >> >> index 1bfcfbe6e30b..8130f043693b 100644
> >> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> >> @@ -751,6 +751,8 @@ static void dg1_irq_reset(struct drm_i915_private *dev_priv)
> >> >>  
> >> >>          GEN3_IRQ_RESET(uncore, GEN11_GU_MISC_);
> >> >>          GEN3_IRQ_RESET(uncore, GEN8_PCU_);
> >> >> +
> >> >> +        intel_uncore_write(uncore, GEN11_GFX_MSTR_IRQ, ~0);
> >> >
> >> >Did you confirm that it's not double buffered?
> >> 
> >> Ah, sorry... I forgot to mention on the original thread where you asked:
> >> 
> >>   - BSpec 50875 and 66434 (for Xe2) does not say that GFX_MSTR_IRQ is
> >>     double buffered. In fact they mention the fact that display IIR
> >>     registers are double buffered and require multiple writes to fully
> >>     clear, but does not say the same about GFX_MSTR_IRQ.
> >> 
> >>   - Also, BSpec 54028 does not mention that the register is double
> >>     buffered.
> >> 
> >> Based on the above, I'm assuming it is not double buffered.
> >> 
> >> Should I check other sources? Is there some sort of runtime check that
> >> can be done?
> >
> >I'd probably just poke at it with intel_reg. Eg:
> >1. boot w/o driver
> >2. unmask+enable eg. pipe vblank interrupt in GEN8_DE_PIPE_IMR/IER
> >3. make sure GEN11_DISPLAY_INT_CTL sees it
> >4. enable GEN11_DISPLAY_IRQ_ENABLE
> >5. make sure GEN11_GFX_MSTR_IRQ see it
> >6. toggle GEN11_DISPLAY_IRQ_ENABLE a few more times to generate extra edges
> >7. try to clear the bit in GEN11_GFX_MSTR_IRQ
> >8. did the bit clear?
> >   yes -> single buffered
> >   no -> goto 7 and check again to make sure it clears on the second time around -> double buffered
> 
> Thanks for the detailed sequence above.
> 
> I wrote a small bash script based on the above and ran it on a Lunar
> Lake machine. The output is presented below.
> 
> I belive the output is self explanatory, but just to be sure: "write
> <REG_NAME> <VAL>" tells that we are writing <VAL> to <REG_NAME>; and
> "read <REG_NAME> <VAL>" tells that we got <VAL> after reading
> <REG_NAME>.
> 
>     $ sudo ./confirm-gfx-mstr-irq-is-single-buffered.sh
>     Reset stuff
>       write GEN8_DE_PIPE_IMR__PIPE_A        0xffffffff
>       write GEN8_DE_PIPE_IER__PIPE_A        0x00000000
>       write GEN8_DE_PIPE_IIR__PIPE_A        0xffffffff
>       write GEN8_DE_PIPE_IIR__PIPE_A        0xffffffff
>       write GEN11_DISPLAY_INT_CTL   0x00000000
>       write GEN11_GFX_MSTR_IRQ      0xffffffff
>       write GEN11_GFX_MSTR_IRQ      0xffffffff
>       read  GEN8_DE_PIPE_IIR__PIPE_A        0x00000000
>       read  GEN11_DISPLAY_INT_CTL   0x00000000
>       read  GEN11_GFX_MSTR_IRQ      0x00000000
> 
>     Enable vblank on PIPE A
>       write GEN8_DE_PIPE_IMR__PIPE_A        0xfffffffe
>       write GEN8_DE_PIPE_IER__PIPE_A        0x00000001
> 
>     Check that display interrupt was generated
>       read  GEN11_DISPLAY_INT_CTL   0x00010000
> 
>     Enable display interrupt
>       write GEN11_DISPLAY_INT_CTL   0x80000000
> 
>     Check that display bit in GFX_MSTR_IRQ was set
>       read  GEN11_GFX_MSTR_IRQ      0x00010000
> 
>     Toggle the enable bit to generate edges
>       write GEN11_DISPLAY_INT_CTL   0x00000000
>       write GEN11_DISPLAY_INT_CTL   0x80000000
>       write GEN11_DISPLAY_INT_CTL   0x00000000
>       write GEN11_DISPLAY_INT_CTL   0x80000000
> 
>     Clear and read GFX_MSTR_IRQ again
>       write GEN11_GFX_MSTR_IRQ      0xffffffff
>       read  GEN11_GFX_MSTR_IRQ      0x00000000
> 
> By the output above, I belive we can conclude that GFX_MSTR_IRQ is
> single buffered, right?

Aye. Looks conclusive.

Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

-- 
Ville Syrjälä
Intel