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
