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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 21 Sep 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 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@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
>> 
>> 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@linux.intel.com>

Should this have been Cc: stable?

BR,
Jani.


-- 
Jani Nikula, Intel