Re: [PATCH 4/4] drm/i915/icl: Interrupt handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> writes:

> Em Ter, 2018-02-27 às 11:51 -0800, Daniele Ceraolo Spurio escreveu:
>> 
>> On 20/02/18 07:37, Mika Kuoppala wrote:
>> > v2: Rebase.
>> > 
>> > v3:
>> >    * Remove DPF, it has been removed from SKL+.
>> >    * Fix -internal rebase wrt. execlists interrupt handling.
>> > 
>> > v4: Rebase.
>> > 
>> > v5:
>> >    * Updated for POR changes. (Daniele Ceraolo Spurio)
>> >    * Merged with irq handling fixes by Daniele Ceraolo Spurio:
>> >        * Simplify the code by using gen8_cs_irq_handler.
>> >        * Fix interrupt handling for the upstream kernel.
>> > 
>> > v6:
>> >    * Remove early bringup debug messages (Tvrtko)
>> >    * Add NB about arbitrary spin wait timeout (Tvrtko)
>> > 
>> > v7 (from Paulo):
>> >    * Don't try to write RO bits to registers.
>> >    * Don't check for PCH types that don't exist. PCH interrupts are
>> > not
>> >      here yet.
>> > 
>> > v9:
>> >    * squashed in selector and shared register handling (Daniele)
>> >    * skip writing of irq if data is not valid (Daniele)
>> >    * use time_after32 (Chris)
>> >    * use I915_MAX_VCS and I915_MAX_VECS (Daniele)
>> >    * remove fake pm interrupt handling for later patch (Mika)
>> > 
>> > v10:
>> >    * Direct processing of banks. clear banks early (Chris)
>> >    * remove poll on valid bit, only clear valid bit (Mika)
>> >    * use raw accessors, better naming (Chris)
>> > 
>> > v11:
>> >    * adapt to raw_reg_[read|write]
>> >    * bring back polling the valid bit (Daniele)
>> > 
>> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
>> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>> > Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx>
>> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.
>> > com>
>> > Signed-off-by: Oscar Mateo <oscar.mateo@xxxxxxxxx>
>> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>> > Signed-off-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
>> > ---
>> >   drivers/gpu/drm/i915/i915_irq.c | 229
>> > ++++++++++++++++++++++++++++++++++++++++
>> >   drivers/gpu/drm/i915/intel_pm.c |   7 +-
>> >   2 files changed, 235 insertions(+), 1 deletion(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
>> > b/drivers/gpu/drm/i915/i915_irq.c
>> > index 17de6cef2a30..a79f47ac742a 100644
>> > --- a/drivers/gpu/drm/i915/i915_irq.c
>> > +++ b/drivers/gpu/drm/i915/i915_irq.c
>> > @@ -415,6 +415,9 @@ void gen6_enable_rps_interrupts(struct
>> > drm_i915_private *dev_priv)
>> >   	if (READ_ONCE(rps->interrupts_enabled))
>> >   		return;
>> >   
>> > +	if (WARN_ON_ONCE(IS_GEN11(dev_priv)))
>> > +		return;
>> > +
>> >   	spin_lock_irq(&dev_priv->irq_lock);
>> >   	WARN_ON_ONCE(rps->pm_iir);
>> >   	WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & dev_priv-
>> > >pm_rps_events);
>> > @@ -431,6 +434,9 @@ void gen6_disable_rps_interrupts(struct
>> > drm_i915_private *dev_priv)
>> >   	if (!READ_ONCE(rps->interrupts_enabled))
>> >   		return;
>> >   
>> > +	if (WARN_ON_ONCE(IS_GEN11(dev_priv)))
>> > +		return;
>> > +
>> >   	spin_lock_irq(&dev_priv->irq_lock);
>> >   	rps->interrupts_enabled = false;
>> >   
>> > @@ -2755,6 +2761,150 @@ static void __fini_wedge(struct wedge_me
>> > *w)
>> >   	     (W)->i915;						
>> > 	\
>> >   	     __fini_wedge((W)))
>> >   
>> > +static __always_inline void
>> > +gen11_cs_irq_handler(struct intel_engine_cs * const engine, const
>> > u32 iir)
>> > +{
>> > +	gen8_cs_irq_handler(engine, iir, 0);
>> > +}
>> > +
>> > +static void
>> > +gen11_gt_engine_irq_handler(struct drm_i915_private * const i915,
>> > +			    const unsigned int bank,
>> > +			    const unsigned int engine_n,
>> > +			    const u16 iir)
>> > +{
>> > +	struct intel_engine_cs ** const engine = i915->engine;
>> > +
>> > +	switch (bank) {
>> > +	case 0:
>> > +		switch (engine_n) {
>> > +
>> > +		case GEN11_RCS0:
>> > +			return gen11_cs_irq_handler(engine[RCS],
>> > iir);
>> > +
>> > +		case GEN11_BCS:
>> > +			return gen11_cs_irq_handler(engine[BCS],
>> > iir);
>> > +		}
>> > +	case 1:
>> > +		switch (engine_n) {
>> > +
>> > +		case GEN11_VCS(0):
>> > +			return
>> > gen11_cs_irq_handler(engine[_VCS(0)], iir);
>> > +		case GEN11_VCS(1):
>> > +			return
>> > gen11_cs_irq_handler(engine[_VCS(1)], iir);
>> > +		case GEN11_VCS(2):
>> > +			return
>> > gen11_cs_irq_handler(engine[_VCS(2)], iir);
>> > +		case GEN11_VCS(3):
>> > +			return
>> > gen11_cs_irq_handler(engine[_VCS(3)], iir);
>> > +
>> > +		case GEN11_VECS(0):
>> > +			return
>> > gen11_cs_irq_handler(engine[_VECS(0)], iir);
>> > +		case GEN11_VECS(1):
>> > +			return
>> > gen11_cs_irq_handler(engine[_VECS(1)], iir);
>> > +		}
>> > +	}
>> > +}
>> > +
>> > +static u32
>> > +gen11_gt_engine_intr(struct drm_i915_private * const i915,
>> > +		     const unsigned int bank, const unsigned int
>> > bit)
>> > +{
>> > +	void __iomem * const regs = i915->regs;
>> > +	u32 timeout_ts;
>> > +	u32 ident;
>> > +
>> > +	raw_reg_write(regs, GEN11_IIR_REG_SELECTOR(bank),
>> > BIT(bit));
>> > +
>> > +	/*
>> > +	 * NB: Specs do not specify how long to spin wait,
>> > +	 * so we do ~100us as an educated guess.
>> > +	 */
>> > +	timeout_ts = (local_clock() >> 10) + 100;
>> > +	do {
>> > +		ident = raw_reg_read(regs,
>> > GEN11_INTR_IDENTITY_REG(bank));
>> > +	} while (!(ident & GEN11_INTR_DATA_VALID) &&
>> > +		 !time_after32(local_clock() >> 10, timeout_ts));
>> > +
>> > +	if (unlikely(!(ident & GEN11_INTR_DATA_VALID))) {
>> > +		DRM_ERROR("INTR_IDENTITY_REG%u:%u 0x%08x not
>> > valid!\n",
>> > +			  bank, bit, ident);
>> > +		return 0;
>> > +	}
>> > +
>> > +	raw_reg_write(regs, GEN11_INTR_IDENTITY_REG(bank),
>> > +		      GEN11_INTR_DATA_VALID);
>> > +
>> > +	return ident & GEN11_INTR_ENGINE_MASK;
>> > +}
>> > +
>> > +static void
>> > +gen11_gt_irq_handler(struct drm_i915_private * const i915,
>> > +		     const u32 master_ctl)
>> > +{
>> > +	void __iomem * const regs = i915->regs;
>> > +	unsigned int bank;
>> > +
>> > +	for (bank = 0; bank < 2; bank++) {
>> > +		unsigned long intr_dw;
>> > +		unsigned int bit;
>> > +
>> > +		if (!(master_ctl & GEN11_GT_DW_IRQ(bank)))
>> > +			continue;
>> > +
>> > +		intr_dw = raw_reg_read(regs,
>> > GEN11_GT_INTR_DW(bank));
>> > +
>> > +		if (unlikely(!intr_dw))
>> > +			DRM_ERROR("GT_INTR_DW%u blank!\n", bank);
>> 
>> Could continue here, since the other operations in the loop won't be 
>> meaningful if intr_dw=0
>> 
>> > +
>> > +		for_each_set_bit(bit, &intr_dw, 32) {
>> > +			const u16 iir = gen11_gt_engine_intr(i915,
>> > bank, bit);
>> > +
>> > +			if (unlikely(!iir))
>> > +				continue;
>> > +
>> > +			gen11_gt_engine_irq_handler(i915, bank,
>> > bit, iir);
>> > +		}
>> > +
>> > +		/* Clear must be after shared has been served for
>> > engine */
>> > +		raw_reg_write(regs, GEN11_GT_INTR_DW(bank),
>> > intr_dw);
>> > +	}
>> > +}
>> > +
>> > +static irqreturn_t gen11_irq_handler(int irq, void *arg)
>> > +{
>> > +	struct drm_i915_private * const i915 = to_i915(arg);
>> > +	void __iomem * const regs = i915->regs;
>> > +	u32 master_ctl;
>> > +
>> > +	if (!intel_irqs_enabled(i915))
>> > +		return IRQ_NONE;
>> > +
>> > +	master_ctl = raw_reg_read(regs, GEN11_GFX_MSTR_IRQ);
>> > +	master_ctl &= ~GEN11_MASTER_IRQ;
>> > +	if (!master_ctl)
>> > +		return IRQ_NONE;
>> > +
>> > +	/* Disable interrupts. */
>> > +	raw_reg_write(regs, GEN11_GFX_MSTR_IRQ, 0);
>> > +
>> > +	/* Find, clear, then process each source of interrupt. */
>> > +	gen11_gt_irq_handler(i915, master_ctl);
>> > +
>> > +	/* IRQs are synced during runtime_suspend, we don't
>> > require a wakeref */
>> > +	if (master_ctl & GEN11_DISPLAY_IRQ) {
>> > +		const u32 disp_ctl = raw_reg_read(regs,
>> > GEN11_DISPLAY_INT_CTL);
>> > +
>> > +		disable_rpm_wakeref_asserts(i915);
>> > +		gen8_de_irq_handler(i915, disp_ctl);
>> 
>> gen8_de_irq_handler refers to the provided value as "master_ctl". 
>> GEN11_DISPLAY_INT_CTL has the same format as GEN8_MASTER_IRQ for the 
>> display-related bits (16-30) so it is ok to provide disp_ctl, but we 
>> could use a comment to explain that. Also, gen8_de_irq_handler logs 
>> errors blaming the master when things go wrong, so we could update
>> that 
>> to make things clearer, but that can come as a follow up.
>> 
>> I've checked the bit definitions of the registers used in 
>> gen8_de_irq_handler and some of the bits have moved, but AFAICS
>> among 
>> the ones we use the only one that is impacted is GEN8_DE_MISC_GSE,
>> which 
>> is now gone (bit is now reserved). This shouldn't impact 
>> gen8_de_irq_handler because the interrupt shouldn't trigger at all,
>> but 
>> we could avoid enabling it from gen8_de_irq_postinstall.
>
> We have a patch for the GSE bit. Also, a few other display-related
> interrupts have changed, and we have patches for them too.
>
> All the display-related interrupt patches depend on this patch here, I
> didn't send them yet to the list since I realized this patch here would
> still get a few new versions, generating more and conflicts on each
> version.
>
> Once this one is merged we can send the display interrupt ones.
>

This is now pushed with the suggested changes by Daniele.
Thanks for review.
-Mika
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux