| Message ID | 20190123023227.8117-1-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/icl: do a posting read after irq install | expand |
On 1/22/2019 6:32 PM, Daniele Ceraolo Spurio wrote: > When reading GEN11_GT_INTR_DWx closely after enabling the interrupts > in gen11_irq_postinstall, the returned value is garbage. This can To clarify, this only happens (or at least I've only seen it) during runtime_resume. Daniele > cause other parts of the setup code (e.g. gen11_reset_one_iir) to > think that there are interrupts to be cleared when there are none. > > The garbage value is only seen on the first read done after the enable, > so this looks like a posting issue. Adding a posting read after enabling > the interrupts does indeed fix the problem. > > Note that the posting read has been purposely added outside of > gen11_master_intr_enable since the issue has only been observed when the > full interrupt setup is performed. > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > --- > drivers/gpu/drm/i915/i915_irq.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 5fd5080c4ccb..7056ae2d1e0e 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -4089,6 +4089,7 @@ static int gen11_irq_postinstall(struct drm_device *dev) > I915_WRITE(GEN11_DISPLAY_INT_CTL, GEN11_DISPLAY_IRQ_ENABLE); > > gen11_master_intr_enable(dev_priv->regs); > + POSTING_READ(GEN11_GFX_MSTR_IRQ); > > return 0; > }
Quoting Daniele Ceraolo Spurio (2019-01-23 02:32:27) > When reading GEN11_GT_INTR_DWx closely after enabling the interrupts > in gen11_irq_postinstall, the returned value is garbage. This can > cause other parts of the setup code (e.g. gen11_reset_one_iir) to > think that there are interrupts to be cleared when there are none. > > The garbage value is only seen on the first read done after the enable, > so this looks like a posting issue. Adding a posting read after enabling > the interrupts does indeed fix the problem. > > Note that the posting read has been purposely added outside of > gen11_master_intr_enable since the issue has only been observed when the > full interrupt setup is performed. > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> irq regs and flushing stale results (side effect or double buffering? or maybe latching?), seem to go hand in hand. Acked-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> writes: > On 1/22/2019 6:32 PM, Daniele Ceraolo Spurio wrote: >> When reading GEN11_GT_INTR_DWx closely after enabling the interrupts >> in gen11_irq_postinstall, the returned value is garbage. This can > > To clarify, this only happens (or at least I've only seen it) during > runtime_resume. > How did you notice? > Daniele > >> cause other parts of the setup code (e.g. gen11_reset_one_iir) to >> think that there are interrupts to be cleared when there are none. >> >> The garbage value is only seen on the first read done after the enable, >> so this looks like a posting issue. Adding a posting read after enabling >> the interrupts does indeed fix the problem. >> >> Note that the posting read has been purposely added outside of >> gen11_master_intr_enable since the issue has only been observed when the >> full interrupt setup is performed. Scary enough that maybe it would have been warranted inside it. But well we know where to escalate if it shows up elsewhere. Acked-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> >> >> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> --- >> drivers/gpu/drm/i915/i915_irq.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c >> index 5fd5080c4ccb..7056ae2d1e0e 100644 >> --- a/drivers/gpu/drm/i915/i915_irq.c >> +++ b/drivers/gpu/drm/i915/i915_irq.c >> @@ -4089,6 +4089,7 @@ static int gen11_irq_postinstall(struct drm_device *dev) >> I915_WRITE(GEN11_DISPLAY_INT_CTL, GEN11_DISPLAY_IRQ_ENABLE); >> >> gen11_master_intr_enable(dev_priv->regs); >> + POSTING_READ(GEN11_GFX_MSTR_IRQ); >> >> return 0; >> }
Chris Wilson <chris@chris-wilson.co.uk> writes: > Quoting Daniele Ceraolo Spurio (2019-01-23 02:32:27) >> When reading GEN11_GT_INTR_DWx closely after enabling the interrupts >> in gen11_irq_postinstall, the returned value is garbage. This can >> cause other parts of the setup code (e.g. gen11_reset_one_iir) to >> think that there are interrupts to be cleared when there are none. >> >> The garbage value is only seen on the first read done after the enable, >> so this looks like a posting issue. Adding a posting read after enabling >> the interrupts does indeed fix the problem. >> >> Note that the posting read has been purposely added outside of >> gen11_master_intr_enable since the issue has only been observed when the >> full interrupt setup is performed. >> >> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > irq regs and flushing stale results (side effect or double buffering? or > maybe latching?), seem to go hand in hand. > > Acked-by: Chris Wilson <chris@chris-wilson.co.uk> Pushed. Thank you for patch and ack. -Mika
On 01/23/2019 03:40 AM, Mika Kuoppala wrote: > Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> writes: > >> On 1/22/2019 6:32 PM, Daniele Ceraolo Spurio wrote: >>> When reading GEN11_GT_INTR_DWx closely after enabling the interrupts >>> in gen11_irq_postinstall, the returned value is garbage. This can >> >> To clarify, this only happens (or at least I've only seen it) during >> runtime_resume. >> > > How did you notice? > The gen11 guc patches add WARN_ON_ONCE(gen11_reset_one_iir(dev_priv, 0, GEN11_GUC)); in the resume path and we saw the warning fire off in testing. A bit of extra logging showed that the whole register was in an invalid state after interrupts were re-enabled, not just the GuC bit. pm_rpm@basic-pci-d3-state hits this consistently. Daniele >> Daniele >> >>> cause other parts of the setup code (e.g. gen11_reset_one_iir) to >>> think that there are interrupts to be cleared when there are none. >>> >>> The garbage value is only seen on the first read done after the enable, >>> so this looks like a posting issue. Adding a posting read after enabling >>> the interrupts does indeed fix the problem. >>> >>> Note that the posting read has been purposely added outside of >>> gen11_master_intr_enable since the issue has only been observed when the >>> full interrupt setup is performed. > > Scary enough that maybe it would have been warranted inside it. But > well we know where to escalate if it shows up elsewhere. > > Acked-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > >>> >>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> >>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_irq.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c >>> index 5fd5080c4ccb..7056ae2d1e0e 100644 >>> --- a/drivers/gpu/drm/i915/i915_irq.c >>> +++ b/drivers/gpu/drm/i915/i915_irq.c >>> @@ -4089,6 +4089,7 @@ static int gen11_irq_postinstall(struct drm_device *dev) >>> I915_WRITE(GEN11_DISPLAY_INT_CTL, GEN11_DISPLAY_IRQ_ENABLE); >>> >>> gen11_master_intr_enable(dev_priv->regs); >>> + POSTING_READ(GEN11_GFX_MSTR_IRQ); >>> >>> return 0; >>> }
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 5fd5080c4ccb..7056ae2d1e0e 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -4089,6 +4089,7 @@ static int gen11_irq_postinstall(struct drm_device *dev) I915_WRITE(GEN11_DISPLAY_INT_CTL, GEN11_DISPLAY_IRQ_ENABLE); gen11_master_intr_enable(dev_priv->regs); + POSTING_READ(GEN11_GFX_MSTR_IRQ); return 0; }
When reading GEN11_GT_INTR_DWx closely after enabling the interrupts in gen11_irq_postinstall, the returned value is garbage. This can cause other parts of the setup code (e.g. gen11_reset_one_iir) to think that there are interrupts to be cleared when there are none. The garbage value is only seen on the first read done after the enable, so this looks like a posting issue. Adding a posting read after enabling the interrupts does indeed fix the problem. Note that the posting read has been purposely added outside of gen11_master_intr_enable since the issue has only been observed when the full interrupt setup is performed. Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> --- drivers/gpu/drm/i915/i915_irq.c | 1 + 1 file changed, 1 insertion(+)