| Message ID | 20180109232336.11029-8-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2018-01-09 at 21:23 -0200, Paulo Zanoni wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > 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@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> <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
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@intel.com> > > > > 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@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel. > > com> > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > <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
On 10/01/2018 10:16, Joonas Lahtinen wrote: > On Tue, 2018-01-09 at 21:23 -0200, Paulo Zanoni wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> 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@intel.com> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > <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. That would be a bit unfriendly when you are bringing up the codebase and not all pices have been implemented yet. It gets removed later in the series, or in a following series, whatever. >> 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 :) Name or body? >> +{ >> + 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. To early to know. TBD. >> + tmp = dw; >> + for_each_set_bit(bit, &tmp, 32) { >> + I915_WRITE_FW(GEN11_IIR_REG_SELECTOR(bank), 1 << bit); > > BIT(bit) > > + newline here Yes sir! > >> + 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. Too early to know. TBD. Spec wants us to busy poll and doesn't specify the timeout. Chris also complained and we chatted about this on IRC, guess you missed this particular stream of conversation. >> + >> + 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. Precedent in "master control interrupt lied" messages. Can be revisited when it is known how the real silicon behaves. > > + 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? Chris also had some ideas on how to modify the looping, maybe the same as yours. Best we chat about it when we are all present. > > <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. Not sure on the states of patches which bring up these bits. Maybe it can be re-arranged somehow but not sure. > >> +} > > <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. Yes, as above. > >> } >> >> 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. It might be possible in some cases, or it might be quite challenging in others. Need to look into it but no promises. We might have to live with having place holders like this in the code which get removed by later patches/series. It's quite complex logistically to organise multiple series, written by multiple authors, at different times, and make it look 100% pretty. (And not just squash and butcher everything up at merge time.) Regards, Tvrtko
Em Sex, 2018-01-19 às 17:30 +0000, Tvrtko Ursulin escreveu: > On 10/01/2018 10:16, Joonas Lahtinen wrote: > > On Tue, 2018-01-09 at 21:23 -0200, Paulo Zanoni wrote: > > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > > > 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@intel.com> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@inte > > > l.com> > > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > <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. > > That would be a bit unfriendly when you are bringing up the codebase > and > not all pices have been implemented yet. It gets removed later in > the > series, or in a following series, whatever. > > > > 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 :) > > Name or body? > > > > +{ > > > + 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. > > To early to know. TBD. > > > > + tmp = dw; > > > + for_each_set_bit(bit, &tmp, 32) { > > > + I915_WRITE_FW(GEN11_IIR_REG_SELE > > > CTOR(bank), 1 << bit); > > > > BIT(bit) > > > > + newline here > > Yes sir! > > > > > > + 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. > > Too early to know. TBD. Spec wants us to busy poll and doesn't > specify > the timeout. Chris also complained and we chatted about this on IRC, > guess you missed this particular stream of conversation. And so did I. What was the summary of the conversation? What did you two conclude should be done in this case? Is the current code acceptable? > > > > + > > > + 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. > > Precedent in "master control interrupt lied" messages. Can be > revisited > when it is known how the real silicon behaves. > > > > > + newline here > > > > > + I915_WRITE_FW(GEN11_INTR_IDENTIT > > > Y_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(en > > > gine)]); > > > + 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(e > > > ngine)]); > > > + ret = IRQ_HANDLED; > > > + } > > > + } > > > > Doesn't the above code look like a table to be iterated over? > > Chris also had some ideas on how to modify the looping, maybe the > same > as yours. Best we chat about it when we are all present. Please make sure conclusions are posted to this list so the public review is properly documented. > > > > > <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. > > Not sure on the states of patches which bring up these bits. Maybe > it > can be re-arranged somehow but not sure. > > > > > > +} > > > > <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. > > Yes, as above. > > > > > > } > > > > > > 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. > > It might be possible in some cases, or it might be quite challenging > in > others. Need to look into it but no promises. We might have to live > with > having place holders like this in the code which get removed by > later > patches/series. It's quite complex logistically to organise multiple > series, written by multiple authors, at different times, and make it > look 100% pretty. (And not just squash and butcher everything up at > merge time.) I agree with Tvrtko here and in the other points above. If we take Joonas's point to the extreme, ICL enabling would be a single giant patch. We have to accept that some things are going to be incomplete in the series that enable a platform. We also have the alpha_support option to protect us here, and CI to make sure ICL's incompleteness doesn't affect the other platforms. > > Regards, > > Tvrtko
Quoting Paulo Zanoni (2018-01-19 18:10:51) > Em Sex, 2018-01-19 às 17:30 +0000, Tvrtko Ursulin escreveu: > > On 10/01/2018 10:16, Joonas Lahtinen wrote: > > > If these are in a later patch, should be squashed here. > > > > It might be possible in some cases, or it might be quite challenging > > in > > others. Need to look into it but no promises. We might have to live > > with > > having place holders like this in the code which get removed by > > later > > patches/series. It's quite complex logistically to organise multiple > > series, written by multiple authors, at different times, and make it > > look 100% pretty. (And not just squash and butcher everything up at > > merge time.) > > I agree with Tvrtko here and in the other points above. If we take > Joonas's point to the extreme, ICL enabling would be a single giant > patch. We have to accept that some things are going to be incomplete in > the series that enable a platform. We also have the alpha_support > option to protect us here, and CI to make sure ICL's incompleteness > doesn't affect the other platforms. Later in this series is a patch which fixes a bug in this patch. That certainly needs to be addressed. ;) -Chris
On Fri, 19 Jan 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Paulo Zanoni (2018-01-19 18:10:51) >> Em Sex, 2018-01-19 às 17:30 +0000, Tvrtko Ursulin escreveu: >> > On 10/01/2018 10:16, Joonas Lahtinen wrote: >> > > If these are in a later patch, should be squashed here. >> > >> > It might be possible in some cases, or it might be quite >> > challenging in others. Need to look into it but no promises. We >> > might have to live with having place holders like this in the code >> > which get removed by later patches/series. It's quite complex >> > logistically to organise multiple series, written by multiple >> > authors, at different times, and make it look 100% pretty. (And not >> > just squash and butcher everything up at merge time.) >> >> I agree with Tvrtko here and in the other points above. If we take >> Joonas's point to the extreme, ICL enabling would be a single giant >> patch. We have to accept that some things are going to be incomplete >> in the series that enable a platform. We also have the alpha_support >> option to protect us here, and CI to make sure ICL's incompleteness >> doesn't affect the other platforms. > > Later in this series is a patch which fixes a bug in this patch. That > certainly needs to be addressed. ;) Might be helpful to point that out... As to the larger point of squashing stuff, it's hard to make the division into patches for large enabling series just right. Sometimes it's just an arbitrary choice that's been made at some point to not bloat the patches too much and to not make the rebasing unnecessarily hard. All other things being equal, I'd err toward whatever gets us closer to merging the patches. BR, Jani. > -Chris > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
<snip> > + > +static irqreturn_t > +gen11_gt_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl) > +{ > + 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)) { > + dw = I915_READ_FW(GEN11_GT_INTR_DW(bank)); > + if (!dw) > + DRM_ERROR("GT_INTR_DW%u blank!\n", bank); > + tmp = dw; > + for_each_set_bit(bit, &tmp, 32) { > + I915_WRITE_FW(GEN11_IIR_REG_SELECTOR(bank), 1 << bit); > + 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)); > + > + 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); I think having the identity reg valid but empty is a possible case and we shouldn't error on it. The scenario I'm thinking of is: - Interrupt on engine, we read master IRQ and disable interrupts - we read GEN11_GT_INTR_DW. the register is now locked until a write occurs - a second interrupt arrives on the same engine, gets double buffered behind the current one in GEN11_GT_INTR_DW - we read the irq vector (which will now contain both interrupts), process it and clear it - we write to GEN11_GT_INTR_DW to clear it and the buffered value is now propagated to it - GEN11_GT_INTR_DW now has the bit set, but we already serviced the interrupt so the identity register will be zero. Just dropping the error message should be enough since we have correctly serviced everything. Daniele
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 3517c6548e2c..49fb8d60f770 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; @@ -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) +{ + 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)) { + dw = I915_READ_FW(GEN11_GT_INTR_DW(bank)); + if (!dw) + DRM_ERROR("GT_INTR_DW%u blank!\n", bank); + tmp = dw; + for_each_set_bit(bit, &tmp, 32) { + I915_WRITE_FW(GEN11_IIR_REG_SELECTOR(bank), 1 << bit); + 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)); + + 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); + I915_WRITE_FW(GEN11_INTR_IDENTITY_REG(bank), ident); + } + 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; + } + } + + if (irq[0][GEN11_GTPM] & dev_priv->pm_rps_events) { + ret = IRQ_HANDLED; + gen6_rps_irq_handler(dev_priv, tmp); + } + + return ret; +} + +static irqreturn_t gen11_irq_handler(int irq, void *arg) +{ + struct drm_device *dev = arg; + struct drm_i915_private *dev_priv = dev->dev_private; + u32 master_ctl; + u32 disp_ctl; + irqreturn_t ret; + + if (!intel_irqs_enabled(dev_priv)) + return IRQ_NONE; + + master_ctl = I915_READ_FW(GEN11_GFX_MSTR_IRQ); + + master_ctl &= ~GEN11_MASTER_IRQ; + if (!master_ctl) + return IRQ_NONE; + + /* Disable interrupts. */ + I915_WRITE_FW(GEN11_GFX_MSTR_IRQ, 0); + + /* IRQs are synced during runtime_suspend, we don't require a wakeref */ + disable_rpm_wakeref_asserts(dev_priv); + + /* Find, clear, then process each source of interrupt. */ + ret = gen11_gt_irq_handler(dev_priv, master_ctl); + + if (master_ctl & GEN11_DISPLAY_IRQ) { + disp_ctl = I915_READ_FW(GEN11_DISPLAY_INT_CTL); + ret |= gen8_de_irq_handler(dev_priv, disp_ctl); + } + + /* Acknowledge and enable interrupts. */ + I915_WRITE_FW(GEN11_GFX_MSTR_IRQ, GEN11_MASTER_IRQ | master_ctl); + POSTING_READ_FW(GEN11_GFX_MSTR_IRQ); + + enable_rpm_wakeref_asserts(dev_priv); + + return ret; +} + /** * i915_reset_device - do process context error handling work * @dev_priv: i915 device private @@ -3164,6 +3295,42 @@ static void gen8_irq_reset(struct drm_device *dev) ibx_irq_reset(dev_priv); } +static void gen11_gt_irq_reset(struct drm_i915_private *dev_priv) +{ + /* Disable RCS, BCS, VCS and VECS class engines. */ + I915_WRITE(GEN11_RENDER_COPY_INTR_ENABLE, 0); + I915_WRITE(GEN11_VCS_VECS_INTR_ENABLE, 0); + + /* Restore masks irqs on RCS, BCS, VCS and VECS engines. */ + I915_WRITE(GEN11_RCS0_RSVD_INTR_MASK, ~0); + I915_WRITE(GEN11_BCS_RSVD_INTR_MASK, ~0); + I915_WRITE(GEN11_VCS0_VCS1_INTR_MASK, ~0); + I915_WRITE(GEN11_VCS2_VCS3_INTR_MASK, ~0); + I915_WRITE(GEN11_VECS0_VECS1_INTR_MASK, ~0); +} + +static void gen11_irq_reset(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + int pipe; + + I915_WRITE(GEN11_GFX_MSTR_IRQ, 0); + POSTING_READ(GEN11_GFX_MSTR_IRQ); + + gen11_gt_irq_reset(dev_priv); + + I915_WRITE(GEN11_DISPLAY_INT_CTL, 0); + + for_each_pipe(dev_priv, pipe) + if (intel_display_power_is_enabled(dev_priv, + POWER_DOMAIN_PIPE(pipe))) + GEN8_IRQ_RESET_NDX(DE_PIPE, pipe); + + GEN3_IRQ_RESET(GEN8_DE_PORT_); + GEN3_IRQ_RESET(GEN8_DE_MISC_); + GEN3_IRQ_RESET(GEN8_PCU_); +} + void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv, u8 pipe_mask) { @@ -3658,6 +3825,41 @@ static int gen8_irq_postinstall(struct drm_device *dev) return 0; } +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); + + /* 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)); + + dev_priv->pm_imr = 0xffffffff; /* TODO */ +} + +static int gen11_irq_postinstall(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + + gen11_gt_irq_postinstall(dev_priv); + gen8_de_irq_postinstall(dev_priv); + + I915_WRITE(GEN11_DISPLAY_INT_CTL, GEN11_DISPLAY_IRQ_ENABLE); + + I915_WRITE(GEN11_GFX_MSTR_IRQ, GEN11_MASTER_IRQ); + POSTING_READ(GEN11_GFX_MSTR_IRQ); + + return 0; +} + static int cherryview_irq_postinstall(struct drm_device *dev) { struct drm_i915_private *dev_priv = to_i915(dev); @@ -4106,6 +4308,14 @@ void intel_irq_init(struct drm_i915_private *dev_priv) dev->driver->enable_vblank = i965_enable_vblank; dev->driver->disable_vblank = i965_disable_vblank; dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup; + } else if (INTEL_GEN(dev_priv) >= 11) { + dev->driver->irq_handler = gen11_irq_handler; + dev->driver->irq_preinstall = gen11_irq_reset; + dev->driver->irq_postinstall = gen11_irq_postinstall; + dev->driver->irq_uninstall = gen11_irq_reset; + dev->driver->enable_vblank = gen8_enable_vblank; + dev->driver->disable_vblank = gen8_disable_vblank; + dev_priv->display.hpd_irq_setup = spt_hpd_irq_setup; } else if (INTEL_GEN(dev_priv) >= 8) { dev->driver->irq_handler = gen8_irq_handler; dev->driver->irq_preinstall = gen8_irq_reset; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 66f70316ca5b..8d02d8abeea3 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ 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); } 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 */ } else if (INTEL_GEN(dev_priv) >= 9) { gen9_enable_rps(dev_priv); } else if (IS_BROADWELL(dev_priv)) {