| Message ID | 20230920195351.59421-2-gustavo.sousa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915/irq: Clear GFX_MSTR_IRQ as part of IRQ reset | expand |
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? > } > > static void cherryview_irq_reset(struct drm_i915_private *dev_priv) > -- > 2.42.0
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? -- Gustavo Sousa > >> } >> >> static void cherryview_irq_reset(struct drm_i915_private *dev_priv) >> -- >> 2.42.0 > >-- >Ville Syrjälä >Intel
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
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? -- Gustavo Sousa By the way, here is the content of confirm-gfx-mstr-irq-is-single-buffered.sh: #!/bin/bash declare -A REGISTERS=( [GEN8_DE_PIPE_IMR__PIPE_A]=0x44404 [GEN8_DE_PIPE_IIR__PIPE_A]=0x44408 [GEN8_DE_PIPE_IER__PIPE_A]=0x4440c [GEN11_DISPLAY_INT_CTL]=0x44200 [DG1_MSTR_TILE_INTR]=0x190008 [GEN11_GFX_MSTR_IRQ]=0x190010 ) function reg_write() { # Redirect stderr to silence "register spec not found" warning intel_reg write ${REGISTERS[$1]} $2 2>/dev/null printf " write %s\t0x%08x\n" $1 $2 } function reg_read() { # Redirect stderr to silence "register spec not found" warning REG_READ_VAL=$(intel_reg read read ${REGISTERS[$1]} 2>/dev/null \ | sed 's,^.*: ,,') printf " read %s\t0x%08x\n" $1 $REG_READ_VAL } echo "Reset stuff" reg_write GEN8_DE_PIPE_IMR__PIPE_A 0xffffffff reg_write GEN8_DE_PIPE_IER__PIPE_A 0x00000000 reg_write GEN8_DE_PIPE_IIR__PIPE_A 0xffffffff reg_write GEN8_DE_PIPE_IIR__PIPE_A 0xffffffff reg_write GEN11_DISPLAY_INT_CTL 0x00000000 reg_write GEN11_GFX_MSTR_IRQ 0xffffffff reg_write GEN11_GFX_MSTR_IRQ 0xffffffff reg_read GEN8_DE_PIPE_IIR__PIPE_A reg_read GEN11_DISPLAY_INT_CTL reg_read GEN11_GFX_MSTR_IRQ echo echo "Enable vblank on PIPE A" reg_write GEN8_DE_PIPE_IMR__PIPE_A 0xfffffffe reg_write GEN8_DE_PIPE_IER__PIPE_A 0x00000001 echo echo "Check that display interrupt was generated" reg_read GEN11_DISPLAY_INT_CTL echo echo "Enable display interrupt" reg_write GEN11_DISPLAY_INT_CTL 0x80000000 echo echo "Check that display bit in GFX_MSTR_IRQ was set" reg_read GEN11_GFX_MSTR_IRQ echo echo "Toggle the enable bit to generate edges" reg_write GEN11_DISPLAY_INT_CTL 0x00000000 reg_write GEN11_DISPLAY_INT_CTL 0x80000000 reg_write GEN11_DISPLAY_INT_CTL 0x00000000 reg_write GEN11_DISPLAY_INT_CTL 0x80000000 echo echo "Clear and read GFX_MSTR_IRQ again" reg_write GEN11_GFX_MSTR_IRQ 0xffffffff reg_read GEN11_GFX_MSTR_IRQ > >> >> -- >> Gustavo Sousa >> >> > >> >> } >> >> >> >> static void cherryview_irq_reset(struct drm_i915_private *dev_priv) >> >> -- >> >> 2.42.0 >> > >> >-- >> >Ville Syrjälä >> >Intel > >-- >Ville Syrjälä >Intel
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>
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.
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); } static void cherryview_irq_reset(struct drm_i915_private *dev_priv)
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(+)