Re: [PATCH 02/24] drm/i915/icl: GSE interrupt moves from DE_MISC to GU_MISC

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> writes:

> On Thu, 2018-05-24 at 12:22 +0300, Mika Kuoppala wrote:
>> Paulo Zanoni <paulo.r.zanoni@intel.com> writes:
>> 
>> > 
>> > From: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>> > 
>> > 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.
>> > 
>> > 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 | 38
>> > ++++++++++++++++++++++++++++++++++++--
>> >  drivers/gpu/drm/i915/i915_reg.h |  7 +++++++
>> >  2 files changed, 43 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
>> > b/drivers/gpu/drm/i915/i915_irq.c
>> > index 2fd92a886789..dde938bbfb0a 100644
>> > --- a/drivers/gpu/drm/i915/i915_irq.c
>> > +++ b/drivers/gpu/drm/i915/i915_irq.c
>> > @@ -2605,7 +2605,8 @@ gen8_de_irq_handler(struct drm_i915_private
>> > *dev_priv, u32 master_ctl)
>> >  			I915_WRITE(GEN8_DE_MISC_IIR, iir);
>> >  			ret = IRQ_HANDLED;
>> >  
>> > -			if (iir & GEN8_DE_MISC_GSE) {
>> > +			if (INTEL_GEN(dev_priv) <= 10 &&
>> > +			    (iir & GEN8_DE_MISC_GSE)) {
>> This bit should not be ever set with gen11 so no need to
>> add extra guards?
> The bit is reserved on gen-11, we can't be sure if some future platform
> is not going to reuse it for something else.The guard also adds clarity
> that the gen-11 handler is elsewhere.

It adds latency to interrupt handler too. We already mask that specific
interrupt so guarding against collision would be the responsibility
of that future platform enabling patchset.

>
>> 
>> > 
>> >  				intel_opregion_asle_intr(dev_priv)
>> > ;
>> >  				found = true;
>> >  			}
>> > @@ -2943,6 +2944,30 @@ gen11_gt_irq_handler(struct drm_i915_private
>> > * const i915,
>> >  	spin_unlock(&i915->irq_lock);
>> >  }
>> >  
>> > +static irqreturn_t
>> Return is never used for anything, just use void.
> Looks like the caller was reworked upstream, I'll change this.
>
>> 
>> > 
>> > +gen11_gu_misc_irq_handler(struct drm_i915_private *dev_priv, u32
>> > master_ctl)
>> > +{
>> > +	irqreturn_t ret = IRQ_NONE;
>> > +	u32 iir;
>> > +
>> > +	if (!(master_ctl & GEN11_GU_MISC_IRQ))
>> > +		return ret;
>> > +
>> > +	iir = I915_READ(GEN11_GU_MISC_IIR);
>> This reg seems to out of forcewake domain so
>> just use raw_reg_read() in here.
> How do you check that? And what exactly is the forcewake domain? Is it
> similar to a power domain?
>

from intel_uncore.c:
static const struct intel_forcewake_range __gen11_fw_ranges[] = {
..
	GEN_FW_RANGE(0x40000, 0x1bffff, 0),
...

So using I915_READ will work as it will that it doesn't need
forcewake dance. But the checks add latency, not much, but this
is interrupt handler after all so we do care.

forcewake domains are per engine power saving feature where you need
to do certain dance to wake the domain up and also to let it sleep.

For more details skim through intel_uncore.[hc].

-Mika

>> 
>> > 
>> > +	if (iir) {
>> just a note that likely(iir) if you want to add emphasis.
>> 
>> > 
>> > +		I915_WRITE(GEN11_GU_MISC_IIR, iir);
>> raw_reg_write()
>> -Mika
>> 
>> > 
>> > +		ret = IRQ_HANDLED;
>> > +		if (iir & GEN11_GU_MISC_GSE)
>> > +			intel_opregion_asle_intr(dev_priv);
>> > +		else
>> > +			DRM_ERROR("Unexpected GU Misc interrupt
>> > 0x%08x\n", iir);
>> > +	} else {
>> > +		DRM_ERROR("The master control interrupt lied (GU
>> > MISC)!\n");
>> > +	}
>> > +
>> > +	return ret;
>> > +}
>> > +
>> >  static irqreturn_t gen11_irq_handler(int irq, void *arg)
>> >  {
>> >  	struct drm_i915_private * const i915 = to_i915(arg);
>> > @@ -2976,6 +3001,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 +3492,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 +3936,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 +4035,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 196a0eb79272..ca474f6f523c 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -7016,9 +7016,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)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx