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

[Dhinakaran Pandiyan]

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 at linux.intel.com>
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.c
> > > > om>
> > > > [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.

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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx