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

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

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@intel.com>
> >> Signed-off-by: Gustavo Sousa <gustavo.sousa@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

> 
> --
> Gustavo Sousa
> 
> >
> >>  }
> >>  
> >>  static void cherryview_irq_reset(struct drm_i915_private *dev_priv)
> >> -- 
> >> 2.42.0
> >
> >-- 
> >Ville Syrjälä
> >Intel

-- 
Ville Syrjälä
Intel