[Intel-gfx] [PATCH v2] drm/i915/icl: GSE interrupt moves from DE_MISC to GU_MISC

[Ville Syrjälä]

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 at linux.intel.com>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > [Paulo: bikesheds and rebases]
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel