| Message ID | 20180525194313.28218-1-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Dhinakaran Pandiyan (2018-05-25 20:43:13) > The Graphics System Event(GSE) interrupt bit has a new location in the > GU_MISC_INTERRUPT_{IIR, ISR, IMR, IER} registers. Since GSE was the only > DE_MISC interrupt that was enabled, with this change we don't enable/handle > any of DE_MISC interrupts for gen11. Credits to Paulo for pointing out > the register change. > > v2: from DK > raw_reg_[read/write], branch prediction hint and drop platform check (Mika) > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > [Paulo: bikesheds and rebases] > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/i915_irq.c | 31 ++++++++++++++++++++++++++++++- > drivers/gpu/drm/i915/i915_reg.h | 7 +++++++ > 2 files changed, 37 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 2fd92a886789..cdbc23b21df6 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2943,6 +2943,26 @@ gen11_gt_irq_handler(struct drm_i915_private * const i915, > spin_unlock(&i915->irq_lock); > } > > +static void > +gen11_gu_misc_irq_handler(struct drm_i915_private *dev_priv, > + const u32 master_ctl) > +{ > + void __iomem * const regs = dev_priv->regs; > + u32 iir; > + > + if (!(master_ctl & GEN11_GU_MISC_IRQ)) > + return; > + > + iir = raw_reg_read(regs, GEN11_GU_MISC_IIR); > + if (likely(iir)) { > + raw_reg_write(regs, GEN11_GU_MISC_IIR, iir); > + if (iir & GEN11_GU_MISC_GSE) > + intel_opregion_asle_intr(dev_priv); > + else > + DRM_ERROR("Unexpected GU Misc interrupt 0x%08x\n", iir); You should be re-enabling the master interrupt *before* doing any work. No? Keeping the master interrupt disabled stops all other CPUs from processing our interrupts; e.g. basically stopping us feeding the GPU with work while we wait for you. -Chris
On Fri, 2018-05-25 at 20:56 +0100, Chris Wilson wrote: > Quoting Dhinakaran Pandiyan (2018-05-25 20:43:13) > > > > The Graphics System Event(GSE) interrupt bit has a new location in > > the > > GU_MISC_INTERRUPT_{IIR, ISR, IMR, IER} registers. Since GSE was the > > only > > DE_MISC interrupt that was enabled, with this change we don't > > enable/handle > > any of DE_MISC interrupts for gen11. Credits to Paulo for pointing > > out > > the register change. > > > > v2: from DK > > raw_reg_[read/write], branch prediction hint and drop platform > > check (Mika) > > > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > [Paulo: bikesheds and rebases] > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/i915_irq.c | 31 > > ++++++++++++++++++++++++++++++- > > drivers/gpu/drm/i915/i915_reg.h | 7 +++++++ > > 2 files changed, 37 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c > > b/drivers/gpu/drm/i915/i915_irq.c > > index 2fd92a886789..cdbc23b21df6 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -2943,6 +2943,26 @@ gen11_gt_irq_handler(struct drm_i915_private > > * const i915, > > spin_unlock(&i915->irq_lock); > > } > > > > +static void > > +gen11_gu_misc_irq_handler(struct drm_i915_private *dev_priv, > > + const u32 master_ctl) > > +{ > > + void __iomem * const regs = dev_priv->regs; > > + u32 iir; > > + > > + if (!(master_ctl & GEN11_GU_MISC_IRQ)) > > + return; > > + > > + iir = raw_reg_read(regs, GEN11_GU_MISC_IIR); > > + if (likely(iir)) { > > + raw_reg_write(regs, GEN11_GU_MISC_IIR, iir); > > + if (iir & GEN11_GU_MISC_GSE) > > + intel_opregion_asle_intr(dev_priv); > > + else > > + DRM_ERROR("Unexpected GU Misc interrupt > > 0x%08x\n", iir); > You should be re-enabling the master interrupt *before* doing any > work. > No? intel_opregion_asle_intr() doesn't do much other than scheduling work and this is similar to how interrupts are handled for other platforms. Are you suggesting we optimize our interrupt handling logic to read all the required IIR's first, re-enable the master interrupt and then call the specific handlers based on the set IIR's? This would be a widespread change. > > Keeping the master interrupt disabled stops all other CPUs from > processing our interrupts; e.g. basically stopping us feeding the GPU > with work while we wait for you. > -Chris
On Wed, Jun 13, 2018 at 06:51:37PM -0700, Dhinakaran Pandiyan wrote: > On Fri, 2018-05-25 at 20:56 +0100, Chris Wilson wrote: > > Quoting Dhinakaran Pandiyan (2018-05-25 20:43:13) > > > > > > The Graphics System Event(GSE) interrupt bit has a new location in > > > the > > > GU_MISC_INTERRUPT_{IIR, ISR, IMR, IER} registers. Since GSE was the > > > only > > > DE_MISC interrupt that was enabled, with this change we don't > > > enable/handle > > > any of DE_MISC interrupts for gen11. Credits to Paulo for pointing > > > out > > > the register change. > > > > > > v2: from DK > > > raw_reg_[read/write], branch prediction hint and drop platform > > > check (Mika) > > > > > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > [Paulo: bikesheds and rebases] > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_irq.c | 31 > > > ++++++++++++++++++++++++++++++- > > > drivers/gpu/drm/i915/i915_reg.h | 7 +++++++ > > > 2 files changed, 37 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c > > > b/drivers/gpu/drm/i915/i915_irq.c > > > index 2fd92a886789..cdbc23b21df6 100644 > > > --- a/drivers/gpu/drm/i915/i915_irq.c > > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > > @@ -2943,6 +2943,26 @@ gen11_gt_irq_handler(struct drm_i915_private > > > * const i915, > > > spin_unlock(&i915->irq_lock); > > > } > > > > > > +static void > > > +gen11_gu_misc_irq_handler(struct drm_i915_private *dev_priv, > > > + const u32 master_ctl) > > > +{ > > > + void __iomem * const regs = dev_priv->regs; > > > + u32 iir; > > > + > > > + if (!(master_ctl & GEN11_GU_MISC_IRQ)) > > > + return; > > > + > > > + iir = raw_reg_read(regs, GEN11_GU_MISC_IIR); > > > + if (likely(iir)) { > > > + raw_reg_write(regs, GEN11_GU_MISC_IIR, iir); > > > + if (iir & GEN11_GU_MISC_GSE) > > > + intel_opregion_asle_intr(dev_priv); > > > + else > > > + DRM_ERROR("Unexpected GU Misc interrupt > > > 0x%08x\n", iir); > > You should be re-enabling the master interrupt *before* doing any > > work. > > No? > intel_opregion_asle_intr() doesn't do much other than scheduling work > and this is similar to how interrupts are handled for other platforms. > > Are you suggesting we optimize our interrupt handling logic to read all > the required IIR's first, re-enable the master interrupt and then call > the specific handlers based on the set IIR's? This would be a > widespread change. That is what I have done for all the gmch platforms. The gen8+ gt irq handler is already split up as well because chv needed it. I think it's the right way to do things. I just didn't have the energy at the time to convert all the more moden platforms as well. It would also open up the possibility of using threaded irqs and keeping some of the super latency sensitive stuff in the hard irq handler while moving everthing else into the thread. > > > > > Keeping the master interrupt disabled stops all other CPUs from > > processing our interrupts; e.g. basically stopping us feeding the GPU > > with work while we wait for you. > > -Chris > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, 2018-06-14 at 13:32 +0300, Ville Syrjälä wrote: > On Wed, Jun 13, 2018 at 06:51:37PM -0700, Dhinakaran Pandiyan wrote: > > > > On Fri, 2018-05-25 at 20:56 +0100, Chris Wilson wrote: > > > > > > Quoting Dhinakaran Pandiyan (2018-05-25 20:43:13) > > > > > > > > > > > > The Graphics System Event(GSE) interrupt bit has a new location > > > > in > > > > the > > > > GU_MISC_INTERRUPT_{IIR, ISR, IMR, IER} registers. Since GSE was > > > > the > > > > only > > > > DE_MISC interrupt that was enabled, with this change we don't > > > > enable/handle > > > > any of DE_MISC interrupts for gen11. Credits to Paulo for > > > > pointing > > > > out > > > > the register change. > > > > > > > > v2: from DK > > > > raw_reg_[read/write], branch prediction hint and drop platform > > > > check (Mika) > > > > > > > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.c > > > > om> > > > > [Paulo: bikesheds and rebases] > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_irq.c | 31 > > > > ++++++++++++++++++++++++++++++- > > > > drivers/gpu/drm/i915/i915_reg.h | 7 +++++++ > > > > 2 files changed, 37 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c > > > > b/drivers/gpu/drm/i915/i915_irq.c > > > > index 2fd92a886789..cdbc23b21df6 100644 > > > > --- a/drivers/gpu/drm/i915/i915_irq.c > > > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > > > @@ -2943,6 +2943,26 @@ gen11_gt_irq_handler(struct > > > > drm_i915_private > > > > * const i915, > > > > spin_unlock(&i915->irq_lock); > > > > } > > > > > > > > +static void > > > > +gen11_gu_misc_irq_handler(struct drm_i915_private *dev_priv, > > > > + const u32 master_ctl) > > > > +{ > > > > + void __iomem * const regs = dev_priv->regs; > > > > + u32 iir; > > > > + > > > > + if (!(master_ctl & GEN11_GU_MISC_IRQ)) > > > > + return; > > > > + > > > > + iir = raw_reg_read(regs, GEN11_GU_MISC_IIR); > > > > + if (likely(iir)) { > > > > + raw_reg_write(regs, GEN11_GU_MISC_IIR, iir); > > > > + if (iir & GEN11_GU_MISC_GSE) > > > > + intel_opregion_asle_intr(dev_priv); > > > > + else > > > > + DRM_ERROR("Unexpected GU Misc interrupt > > > > 0x%08x\n", iir); > > > You should be re-enabling the master interrupt *before* doing any > > > work. > > > No? > > intel_opregion_asle_intr() doesn't do much other than scheduling > > work > > and this is similar to how interrupts are handled for other > > platforms. > > > > Are you suggesting we optimize our interrupt handling logic to read > > all > > the required IIR's first, re-enable the master interrupt and then > > call > > the specific handlers based on the set IIR's? This would be a > > widespread change. > That is what I have done for all the gmch platforms. The gen8+ gt > irq handler is already split up as well because chv needed it. > I think it's the right way to do things. I just didn't have the > energy at the time to convert all the more moden platforms as well. Makes a lot of sense.I have converted this particular patch for now. -DK > > It would also open up the possibility of using threaded irqs and > keeping some of the super latency sensitive stuff in the hard irq > handler while moving everthing else into the thread. > > > > > > > > > > > > > > Keeping the master interrupt disabled stops all other CPUs from > > > processing our interrupts; e.g. basically stopping us feeding the > > > GPU > > > with work while we wait for you. > > > -Chris > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 2fd92a886789..cdbc23b21df6 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2943,6 +2943,26 @@ gen11_gt_irq_handler(struct drm_i915_private * const i915, spin_unlock(&i915->irq_lock); } +static void +gen11_gu_misc_irq_handler(struct drm_i915_private *dev_priv, + const u32 master_ctl) +{ + void __iomem * const regs = dev_priv->regs; + u32 iir; + + if (!(master_ctl & GEN11_GU_MISC_IRQ)) + return; + + iir = raw_reg_read(regs, GEN11_GU_MISC_IIR); + if (likely(iir)) { + raw_reg_write(regs, GEN11_GU_MISC_IIR, iir); + if (iir & GEN11_GU_MISC_GSE) + intel_opregion_asle_intr(dev_priv); + else + DRM_ERROR("Unexpected GU Misc interrupt 0x%08x\n", iir); + } +} + static irqreturn_t gen11_irq_handler(int irq, void *arg) { struct drm_i915_private * const i915 = to_i915(arg); @@ -2976,6 +2996,8 @@ static irqreturn_t gen11_irq_handler(int irq, void *arg) enable_rpm_wakeref_asserts(i915); } + gen11_gu_misc_irq_handler(i915, master_ctl); + /* Acknowledge and enable interrupts. */ raw_reg_write(regs, GEN11_GFX_MSTR_IRQ, GEN11_MASTER_IRQ | master_ctl); @@ -3465,6 +3487,7 @@ static void gen11_irq_reset(struct drm_device *dev) GEN3_IRQ_RESET(GEN8_DE_PORT_); GEN3_IRQ_RESET(GEN8_DE_MISC_); + GEN3_IRQ_RESET(GEN11_GU_MISC_); GEN3_IRQ_RESET(GEN8_PCU_); } @@ -3908,9 +3931,12 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv) uint32_t de_pipe_enables; u32 de_port_masked = GEN8_AUX_CHANNEL_A; u32 de_port_enables; - u32 de_misc_masked = GEN8_DE_MISC_GSE | GEN8_DE_EDP_PSR; + u32 de_misc_masked = GEN8_DE_EDP_PSR; enum pipe pipe; + if (INTEL_GEN(dev_priv) <= 10) + de_misc_masked |= GEN8_DE_MISC_GSE; + if (INTEL_GEN(dev_priv) >= 9) { de_pipe_masked |= GEN9_DE_PIPE_IRQ_FAULT_ERRORS; de_port_masked |= GEN9_AUX_CHANNEL_B | GEN9_AUX_CHANNEL_C | @@ -4004,10 +4030,13 @@ static void gen11_gt_irq_postinstall(struct drm_i915_private *dev_priv) static int gen11_irq_postinstall(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + u32 gu_misc_masked = GEN11_GU_MISC_GSE; gen11_gt_irq_postinstall(dev_priv); gen8_de_irq_postinstall(dev_priv); + GEN3_IRQ_INIT(GEN11_GU_MISC_, ~gu_misc_masked, gu_misc_masked); + I915_WRITE(GEN11_DISPLAY_INT_CTL, GEN11_DISPLAY_IRQ_ENABLE); I915_WRITE(GEN11_GFX_MSTR_IRQ, GEN11_MASTER_IRQ); diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 6953419881c4..67d392c3ca2c 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7025,9 +7025,16 @@ enum { #define GEN8_PCU_IIR _MMIO(0x444e8) #define GEN8_PCU_IER _MMIO(0x444ec) +#define GEN11_GU_MISC_ISR _MMIO(0x444f0) +#define GEN11_GU_MISC_IMR _MMIO(0x444f4) +#define GEN11_GU_MISC_IIR _MMIO(0x444f8) +#define GEN11_GU_MISC_IER _MMIO(0x444fc) +#define GEN11_GU_MISC_GSE (1 << 27) + #define GEN11_GFX_MSTR_IRQ _MMIO(0x190010) #define GEN11_MASTER_IRQ (1 << 31) #define GEN11_PCU_IRQ (1 << 30) +#define GEN11_GU_MISC_IRQ (1 << 29) #define GEN11_DISPLAY_IRQ (1 << 16) #define GEN11_GT_DW_IRQ(x) (1 << (x)) #define GEN11_GT_DW1_IRQ (1 << 1)
The Graphics System Event(GSE) interrupt bit has a new location in the GU_MISC_INTERRUPT_{IIR, ISR, IMR, IER} registers. Since GSE was the only DE_MISC interrupt that was enabled, with this change we don't enable/handle any of DE_MISC interrupts for gen11. Credits to Paulo for pointing out the register change. v2: from DK raw_reg_[read/write], branch prediction hint and drop platform check (Mika) Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> [Paulo: bikesheds and rebases] Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/i915_irq.c | 31 ++++++++++++++++++++++++++++++- drivers/gpu/drm/i915/i915_reg.h | 7 +++++++ 2 files changed, 37 insertions(+), 1 deletion(-)