[PATCH 7/7] drm/i915/guc: handle interrupts from media GuC
Ceraolo Spurio, Daniele
daniele.ceraolospurio at intel.com
Wed Sep 28 00:22:41 UTC 2022
On 9/27/2022 5:10 PM, Matt Roper wrote:
> 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)?
We could yes, but we've avoided dynamically using masks for gen11+
because it can mess with rc6 (e.g., see
https://patchwork.freedesktop.org/patch/207829/).
>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Matt Roper <matthew.d.roper at intel.com>
>> Cc: John Harrison <John.C.Harrison at Intel.com>
>> Cc: Alan Previn <alan.previn.teres.alexis at intel.com>
>> ---
>> 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.
ok. Should I change GEN12_GUCM as well?
>
>> #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?
The postinstall does:
intel_uncore_rmw(uncore, GEN12_GUC_MGUC_INTR_MASK, mask, 0);
which clears the "mask" (i.e. the G2H interrupt shifted based on which
GuC it is) in the mask register, so the G2H is allowed.
Daniele
>
>
> 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
>>
More information about the dri-devel
mailing list