On Thu, Sep 22, 2022 at 03:11:17PM -0700, Daniele Ceraolo Spurio wrote:
> The render and media GuCs share the same interrupt enable register, so
> we can no longer disable interrupts when we disable communication for
> one of the GuCs as this would impact the other GuC. Instead, we keep the
> interrupts always enabled in HW and use a variable in the GuC structure
> to determine if we want to service the received interrupts or not.
Even if they have a unified enable bit, can't we still just update the
per-GuC mask bit to get the same behavior (i.e., no interrupts
delivered to the host for that specific GuC)?
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> Cc: John Harrison <John.C.Harrison@xxxxxxxxx>
> Cc: Alan Previn <alan.previn.teres.alexis@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/gt/intel_gt_irq.c | 21 ++++++++++++++----
> drivers/gpu/drm/i915/gt/intel_gt_regs.h | 2 ++
> drivers/gpu/drm/i915/gt/uc/intel_guc.c | 29 ++++++++++++++-----------
> drivers/gpu/drm/i915/gt/uc/intel_guc.h | 5 ++++-
> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 8 +++++--
> 5 files changed, 45 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> index f26882fdc24c..e33ed9ae1439 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> @@ -17,6 +17,9 @@
>
> static void guc_irq_handler(struct intel_guc *guc, u16 iir)
> {
> + if (unlikely(!guc->interrupts.enabled))
> + return;
> +
> if (iir & GUC_INTR_GUC2HOST)
> intel_guc_to_host_event_handler(guc);
> }
> @@ -249,6 +252,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt)
> {
> struct intel_uncore *uncore = gt->uncore;
> u32 irqs = GT_RENDER_USER_INTERRUPT;
> + u32 guc_mask = intel_uc_wants_guc(>->uc) ? GUC_INTR_GUC2HOST : 0;
> const u32 gsc_mask = GSC_IRQ_INTF(0) | GSC_IRQ_INTF(1);
> u32 dmask;
> u32 smask;
> @@ -299,6 +303,19 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt)
> if (HAS_HECI_GSC(gt->i915))
> intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_MASK, ~gsc_mask);
>
> + if (guc_mask) {
> + /* the enable bit is common for both GTs but the masks are separate */
> + u32 mask = gt->type == GT_MEDIA ?
> + REG_FIELD_PREP(ENGINE0_MASK, guc_mask) :
> + REG_FIELD_PREP(ENGINE1_MASK, guc_mask);
> +
> + intel_uncore_write(uncore, GEN11_GUC_SG_INTR_ENABLE,
> + REG_FIELD_PREP(ENGINE1_MASK, guc_mask));
> +
> + /* we might not be the first GT to write this reg */
> + intel_uncore_rmw(uncore, GEN12_GUC_MGUC_INTR_MASK, mask, 0);
> + }
> +
> /*
> * RPS interrupts will get enabled/disabled on demand when RPS itself
> * is enabled/disabled.
> @@ -307,10 +324,6 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt)
> gt->pm_imr = ~gt->pm_ier;
> intel_uncore_write(uncore, GEN11_GPM_WGBOXPERF_INTR_ENABLE, 0);
> intel_uncore_write(uncore, GEN11_GPM_WGBOXPERF_INTR_MASK, ~0);
> -
> - /* Same thing for GuC interrupts */
> - intel_uncore_write(uncore, GEN11_GUC_SG_INTR_ENABLE, 0);
> - intel_uncore_write(uncore, GEN11_GUC_SG_INTR_MASK, ~0);
> }
>
> void gen5_gt_irq_handler(struct intel_gt *gt, u32 gt_iir)
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> index 1cbb7226400b..792809e49680 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> @@ -1519,6 +1519,7 @@
> #define GEN11_CSME (31)
> #define GEN11_GUNIT (28)
> #define GEN11_GUC (25)
> +#define GEN12_GUCM (24)
> #define GEN11_WDPERF (20)
> #define GEN11_KCR (19)
> #define GEN11_GTPM (16)
> @@ -1573,6 +1574,7 @@
> #define GEN11_VECS0_VECS1_INTR_MASK _MMIO(0x1900d0)
> #define GEN12_VECS2_VECS3_INTR_MASK _MMIO(0x1900d4)
> #define GEN11_GUC_SG_INTR_MASK _MMIO(0x1900e8)
> +#define GEN12_GUC_MGUC_INTR_MASK _MMIO(0x1900e8) /* MTL+ */
Technically we should probably give this a "MTL_" prefix or something
since we're not referring to new platforms as "gen12" anymore.
> #define GEN11_GPM_WGBOXPERF_INTR_MASK _MMIO(0x1900ec)
> #define GEN11_CRYPTO_RSVD_INTR_MASK _MMIO(0x1900f0)
> #define GEN11_GUNIT_CSME_INTR_MASK _MMIO(0x1900f4)
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> index b0beab44b34c..ab0263d8e1cf 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> @@ -98,6 +98,8 @@ static void gen9_enable_guc_interrupts(struct intel_guc *guc)
> gt->pm_guc_events);
> gen6_gt_pm_enable_irq(gt, gt->pm_guc_events);
> spin_unlock_irq(gt->irq_lock);
> +
> + guc->interrupts.enabled = true;
> }
>
> static void gen9_disable_guc_interrupts(struct intel_guc *guc)
> @@ -105,6 +107,7 @@ static void gen9_disable_guc_interrupts(struct intel_guc *guc)
> struct intel_gt *gt = guc_to_gt(guc);
>
> assert_rpm_wakelock_held(>->i915->runtime_pm);
> + guc->interrupts.enabled = false;
>
> spin_lock_irq(gt->irq_lock);
>
> @@ -116,39 +119,39 @@ static void gen9_disable_guc_interrupts(struct intel_guc *guc)
> gen9_reset_guc_interrupts(guc);
> }
>
> +static bool __gen11_reset_guc_interrupts(struct intel_gt *gt)
> +{
> + u32 irq = gt->type == GT_MEDIA ? GEN12_GUCM : GEN11_GUC;
> +
> + lockdep_assert_held(gt->irq_lock);
> + return gen11_gt_reset_one_iir(gt, 0, irq);
> +}
> +
> static void gen11_reset_guc_interrupts(struct intel_guc *guc)
> {
> struct intel_gt *gt = guc_to_gt(guc);
>
> spin_lock_irq(gt->irq_lock);
> - gen11_gt_reset_one_iir(gt, 0, GEN11_GUC);
> + __gen11_reset_guc_interrupts(gt);
> spin_unlock_irq(gt->irq_lock);
> }
>
> static void gen11_enable_guc_interrupts(struct intel_guc *guc)
> {
> struct intel_gt *gt = guc_to_gt(guc);
> - u32 events = REG_FIELD_PREP(ENGINE1_MASK, GUC_INTR_GUC2HOST);
>
> spin_lock_irq(gt->irq_lock);
> - WARN_ON_ONCE(gen11_gt_reset_one_iir(gt, 0, GEN11_GUC));
> - intel_uncore_write(gt->uncore,
> - GEN11_GUC_SG_INTR_ENABLE, events);
> - intel_uncore_write(gt->uncore,
> - GEN11_GUC_SG_INTR_MASK, ~events);
The modified postinstall left us with GUC2HOST enabled but masked.
Don't we still need to clear the mask so the interrupts will start being
delivered?
Matt
> + __gen11_reset_guc_interrupts(gt);
> spin_unlock_irq(gt->irq_lock);
> +
> + guc->interrupts.enabled = true;
> }
>
> static void gen11_disable_guc_interrupts(struct intel_guc *guc)
> {
> struct intel_gt *gt = guc_to_gt(guc);
>
> - spin_lock_irq(gt->irq_lock);
> -
> - intel_uncore_write(gt->uncore, GEN11_GUC_SG_INTR_MASK, ~0);
> - intel_uncore_write(gt->uncore, GEN11_GUC_SG_INTR_ENABLE, 0);
> -
> - spin_unlock_irq(gt->irq_lock);
> + guc->interrupts.enabled = false;
> intel_synchronize_irq(gt->i915);
>
> gen11_reset_guc_interrupts(guc);
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> index 804133df1ac9..061d55de3a94 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> @@ -78,6 +78,7 @@ struct intel_guc {
>
> /** @interrupts: pointers to GuC interrupt-managing functions. */
> struct {
> + bool enabled;
> void (*reset)(struct intel_guc *guc);
> void (*enable)(struct intel_guc *guc);
> void (*disable)(struct intel_guc *guc);
> @@ -316,9 +317,11 @@ static inline int intel_guc_send_busy_loop(struct intel_guc *guc,
> return err;
> }
>
> +/* Only call this from the interrupt handler code */
> static inline void intel_guc_to_host_event_handler(struct intel_guc *guc)
> {
> - intel_guc_ct_event_handler(&guc->ct);
> + if (guc->interrupts.enabled)
> + intel_guc_ct_event_handler(&guc->ct);
> }
>
> /* GuC addresses above GUC_GGTT_TOP also don't map through the GTT */
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index 4cd8a787f9e5..1d28286e6f06 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -636,8 +636,10 @@ void intel_uc_runtime_suspend(struct intel_uc *uc)
> {
> struct intel_guc *guc = &uc->guc;
>
> - if (!intel_guc_is_ready(guc))
> + if (!intel_guc_is_ready(guc)) {
> + guc->interrupts.enabled = false;
> return;
> + }
>
> /*
> * Wait for any outstanding CTB before tearing down communication /w the
> @@ -657,8 +659,10 @@ void intel_uc_suspend(struct intel_uc *uc)
> intel_wakeref_t wakeref;
> int err;
>
> - if (!intel_guc_is_ready(guc))
> + if (!intel_guc_is_ready(guc)) {
> + guc->interrupts.enabled = false;
> return;
> + }
>
> with_intel_runtime_pm(&uc_to_gt(uc)->i915->runtime_pm, wakeref) {
> err = intel_guc_suspend(guc);
> --
> 2.37.3
>
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
