Re: [PATCH 7/7] drm/i915/guc: handle interrupts from media GuC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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(&gt->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(&gt->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



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux