Em Qua, 2018-01-10 às 12:16 +0200, Joonas Lahtinen escreveu:
> On Tue, 2018-01-09 at 21:23 -0200, Paulo Zanoni wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >
> > 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.
> >
> > 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>
>
> <SNIP>
>
> > +++ 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;
> > +
>
> These should be GEM_BUG_ON at most and should be the first thing in
> function.
>
> > spin_lock_irq(&dev_priv->irq_lock);
> > rps->interrupts_enabled = false;
> >
> > @@ -2751,6 +2757,131 @@ 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 *engine, u32 iir)
> > +{
> > + gen8_cs_irq_handler(engine, iir, 0);
> > +}
> > +
> > +static irqreturn_t
> > +gen11_gt_irq_handler(struct drm_i915_private *dev_priv, u32
> > master_ctl)
>
> This function could use some readability :)
>
> > +{
> > + irqreturn_t ret = IRQ_NONE;
> > + u16 irq[2][32];
> > + u32 dw, ident;
> > + unsigned long tmp;
> > + unsigned int bank, bit, engine;
> > + unsigned long wait_start, wait_end;
> > +
> > + memset(irq, 0, sizeof(irq));
> > +
> > + for (bank = 0; bank < 2; bank++) {
> > + if (master_ctl & GEN11_GT_DW_IRQ(bank)) {
>
> Invert condition and use continue;
>
> > + dw = I915_READ_FW(GEN11_GT_INTR_DW(bank));
> > + if (!dw)
> > + DRM_ERROR("GT_INTR_DW%u blank!\n",
> > bank);
>
> Probably needs a more appropriate action, GEM_BUG_ON if we've never
> seen this on HW.
>
> > + tmp = dw;
> > + for_each_set_bit(bit, &tmp, 32) {
> > + I915_WRITE_FW(GEN11_IIR_REG_SELECT
> > OR(bank), 1 << bit);
>
> BIT(bit)
>
> + newline here
>
> > + wait_start = local_clock() >> 10;
> > + /* NB: Specs do not specify how
> > long to spin wait.
> > + * Taking 100us as an educated
> > guess */
> > + wait_end = wait_start + 100;
> > + do {
> > + ident =
> > I915_READ_FW(GEN11_INTR_IDENTITY_REG(bank));
> > + } while (!(ident &
> > GEN11_INTR_DATA_VALID) &&
> > + !time_after((unsigned
> > long)local_clock() >> 10, wait_end));
>
> Uh oh, not a really nice thing to do in IRQ handler :( We should
> probably look at some actual delay numbers and not do this.
>
> > +
> > + if (!(ident &
> > GEN11_INTR_DATA_VALID))
> > + DRM_ERROR("INTR_IDENTITY_R
> > EG%u:%u timed out!\n",
> > + bank, bit);
> > +
> > + irq[bank][bit] = ident &
> > GEN11_INTR_ENGINE_MASK;
> > + if (!irq[bank][bit])
> > + DRM_ERROR("INTR_IDENTITY_R
> > EG%u:%u blank!\n",
> > + bank, bit);
>
> DRM_ERROR again seems like bit of an understatement for interrupt.
>
> + newline here
>
> > + I915_WRITE_FW(GEN11_INTR_IDENTITY_
> > REG(bank), ident);
> > + }
>
> + newline here
>
> > + I915_WRITE_FW(GEN11_GT_INTR_DW(bank), dw);
> > + }
> > + }
> > +
> > + if (irq[0][GEN11_RCS0]) {
> > + gen11_cs_irq_handler(dev_priv->engine[RCS],
> > + irq[0][GEN11_RCS0]);
> > + ret = IRQ_HANDLED;
> > + }
> > +
> > + if (irq[0][GEN11_BCS]) {
> > + gen11_cs_irq_handler(dev_priv->engine[BCS],
> > + irq[0][GEN11_BCS]);
> > + ret = IRQ_HANDLED;
> > + }
> > +
> > + for (engine = 0; engine < 4; engine++) {
> > + if (irq[1][GEN11_VCS(engine)]) {
> > + gen11_cs_irq_handler(dev_priv-
> > >engine[_VCS(engine)],
> > + irq[1][GEN11_VCS(engi
> > ne)]);
> > + ret = IRQ_HANDLED;
> > + }
> > + }
> > +
> > + for (engine = 0; engine < 2; engine++) {
> > + if (irq[1][GEN11_VECS(engine)]) {
> > + gen11_cs_irq_handler(dev_priv-
> > >engine[_VECS(engine)],
> > + irq[1][GEN11_VECS(eng
> > ine)]);
> > + ret = IRQ_HANDLED;
> > + }
> > + }
>
> Doesn't the above code look like a table to be iterated over?
>
> <SNIP>
>
> > +static void gen11_gt_irq_postinstall(struct drm_i915_private
> > *dev_priv)
> > +{
> > + const u32 irqs = GT_RENDER_USER_INTERRUPT |
> > GT_CONTEXT_SWITCH_INTERRUPT;
> > +
> > + BUILD_BUG_ON(irqs & 0xffff0000);
>
> #define for the mask.
>
> > +
> > + /* Enable RCS, BCS, VCS and VECS class interrupts. */
> > + I915_WRITE(GEN11_RENDER_COPY_INTR_ENABLE, irqs << 16 |
> > irqs);
> > + I915_WRITE(GEN11_VCS_VECS_INTR_ENABLE, irqs << 16
> > | irqs);
> > +
> > + /* Unmask irqs on RCS, BCS, VCS and VECS engines. */
> > + I915_WRITE(GEN11_RCS0_RSVD_INTR_MASK, ~(irqs <<
> > 16));
> > + I915_WRITE(GEN11_BCS_RSVD_INTR_MASK, ~(irqs <<
> > 16));
> > + I915_WRITE(GEN11_VCS0_VCS1_INTR_MASK, ~(irqs | irqs
> > << 16));
> > + I915_WRITE(GEN11_VCS2_VCS3_INTR_MASK, ~(irqs | irqs
> > << 16));
> > + I915_WRITE(GEN11_VECS0_VECS1_INTR_MASK, ~(irqs |
> > irqs << 16));
>
> We should also have an actual shift and mask #defines for these, now
> it's all hardcoded here.
>
> > +
> > + dev_priv->pm_imr = 0xffffffff; /* TODO */
>
> Some code missing it seems.
>
> > +}
>
> <SNIP>
>
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -7957,7 +7957,10 @@ void intel_sanitize_gt_powersave(struct
> > drm_i915_private *dev_priv)
> > dev_priv->gt_pm.rc6.enabled = true; /* force RC6 disabling
> > */
> > intel_disable_gt_powersave(dev_priv);
> >
> > - gen6_reset_rps_interrupts(dev_priv);
> > + if (INTEL_GEN(dev_priv) < 11)
> > + gen6_reset_rps_interrupts(dev_priv);
> > + else
> > + WARN_ON_ONCE(1);
>
> Some code is obviously missing here.
>
> > }
> >
> > static inline void intel_disable_llc_pstate(struct
> > drm_i915_private *i915)
> > @@ -8070,6 +8073,8 @@ static void intel_enable_rps(struct
> > drm_i915_private *dev_priv)
> > cherryview_enable_rps(dev_priv);
> > } else if (IS_VALLEYVIEW(dev_priv)) {
> > valleyview_enable_rps(dev_priv);
> > + } else if (WARN_ON_ONCE(INTEL_GEN(dev_priv) >= 11)) {
> > + /* TODO */
>
> Ditto.
>
> If these are in a later patch, should be squashed here.
Some of the TODOs and WARNs added by this patch are implemented later
in the series. I'm not in favor of squashing everything into a single
patch: this patch is more like "add the foundation for interrupts" and
later patches implement specific features that are missing here (rps,
rc6, etc).
In addition to that, they are done by different authors, so squashing
makes authorship even more complicated. If these were features of an
existing and already-working platform then squashing would be needed,
but not here in this case where we know nothing works for ICL yet.
>
> Regards, Joonas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
