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@xxxxxxxxx>
> 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_SELECTOR(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_REG%u:%u timed out!\n",
> + bank, bit);
> +
> + irq[bank][bit] = ident & GEN11_INTR_ENGINE_MASK;
> + if (!irq[bank][bit])
> + DRM_ERROR("INTR_IDENTITY_REG%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(engine)]);
> + 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(engine)]);
> + 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.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
