Re: [PATCH 05/17] drm/i915: Support for GuC interrupts

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 10/07/16 14:41, akash.goel@intel.com wrote:
> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>
> There are certain types of interrupts which Host can recieve from GuC.
> GuC ukernel sends an interrupt to Host for certain events, like for
> example retrieve/consume the logs generated by ukernel.
> This patch adds support to receive interrupts from GuC but currently
> enables & partially handles only the interrupt sent by GuC ukernel.
> Future patches will add support for handling other interrupt types.
>
> v2:
> - Use common low level routines for PM IER/IIR programming (Chris)
> - Rename interrupt functions to gen9_xxx from gen8_xxx (Chris)
> - Replace disabling of wake ref asserts with rpm get/put (Chris)
>
> v3:
> - Update comments for more clarity. (Tvrtko)
> - Remove the masking of GuC interrupt, which was kept masked till the
>    start of bottom half, its not really needed as there is only a
>    single instance of work item & wq is ordered. (Tvrtko)
>
> v4:
> - Rebase.
> - Rename guc_events to pm_guc_events so as to be indicative of the
>    register/control block it is associated with. (Chris)
> - Add handling for back to back log buffer flush interrupts.
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h            |  1 +
>   drivers/gpu/drm/i915/i915_guc_submission.c |  5 ++
>   drivers/gpu/drm/i915/i915_irq.c            | 98 ++++++++++++++++++++++++++++--
>   drivers/gpu/drm/i915/i915_reg.h            | 11 ++++
>   drivers/gpu/drm/i915/intel_drv.h           |  3 +
>   drivers/gpu/drm/i915/intel_guc.h           |  4 ++
>   drivers/gpu/drm/i915/intel_guc_loader.c    |  4 ++
>   7 files changed, 122 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c3a579f..6e2ddfa 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1794,6 +1794,7 @@ struct drm_i915_private {
>   	u32 pm_imr;
>   	u32 pm_ier;
>   	u32 pm_rps_events;
> +	u32 pm_guc_events;
>   	u32 pipestat_irq_mask[I915_MAX_PIPES];
>
>   	struct i915_hotplug hotplug;
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 0fb00ab..0bac172 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -1044,6 +1044,8 @@ int intel_guc_suspend(struct drm_device *dev)
>   	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
>   		return 0;
>
> +	gen9_disable_guc_interrupts(dev_priv);
> +
>   	ctx = dev_priv->kernel_context;
>
>   	data[0] = HOST2GUC_ACTION_ENTER_S_STATE;
> @@ -1070,6 +1072,9 @@ int intel_guc_resume(struct drm_device *dev)
>   	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
>   		return 0;
>
> +	if (i915.guc_log_level >= 0)
> +		gen9_enable_guc_interrupts(dev_priv);
> +
>   	ctx = dev_priv->kernel_context;
>
>   	data[0] = HOST2GUC_ACTION_EXIT_S_STATE;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 24bbaf7..fd73c94 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -170,6 +170,7 @@ static void gen5_assert_iir_is_zero(struct drm_i915_private *dev_priv,
>   } while (0)
>
>   static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
> +static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
>
>   /* For display hotplug interrupt */
>   static inline void
> @@ -411,6 +412,39 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
>   	gen6_reset_rps_interrupts(dev_priv);
>   }
>
> +void gen9_reset_guc_interrupts(struct drm_i915_private *dev_priv)
> +{
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	gen6_reset_pm_iir(dev_priv, dev_priv->pm_guc_events);
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +}
> +
> +void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv)
> +{
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	if (!dev_priv->guc.interrupts_enabled) {
> +		WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
> +						dev_priv->pm_guc_events);
> +		dev_priv->guc.interrupts_enabled = true;
> +		gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
> +	}
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +}
> +
> +void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv)
> +{
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	dev_priv->guc.interrupts_enabled = false;
> +
> +	gen6_disable_pm_irq(dev_priv, dev_priv->pm_guc_events);
> +
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +	synchronize_irq(dev_priv->drm.irq);
> +
> +	cancel_work_sync(&dev_priv->guc.events_work);
> +	gen9_reset_guc_interrupts(dev_priv);
> +}
> +
>   /**
>    * bdw_update_port_irq - update DE port interrupt
>    * @dev_priv: driver private
> @@ -1174,6 +1208,21 @@ static void gen6_pm_rps_work(struct work_struct *work)
>   	mutex_unlock(&dev_priv->rps.hw_lock);
>   }
>
> +static void gen9_guc2host_events_work(struct work_struct *work)
> +{
> +	struct drm_i915_private *dev_priv =
> +		container_of(work, struct drm_i915_private, guc.events_work);
> +
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	/* Speed up work cancellation during disabling guc interrupts. */
> +	if (!dev_priv->guc.interrupts_enabled) {
> +		spin_unlock_irq(&dev_priv->irq_lock);
> +		return;
> +	}
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +
> +	/* TODO: Handle the events for which GuC interrupted host */
> +}
>
>   /**
>    * ivybridge_parity_work - Workqueue called when a parity error interrupt
> @@ -1346,11 +1395,13 @@ static irqreturn_t gen8_gt_irq_ack(struct drm_i915_private *dev_priv,
>   			DRM_ERROR("The master control interrupt lied (GT3)!\n");
>   	}
>
> -	if (master_ctl & GEN8_GT_PM_IRQ) {
> +	if (master_ctl & (GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)) {
>   		gt_iir[2] = I915_READ_FW(GEN8_GT_IIR(2));
> -		if (gt_iir[2] & dev_priv->pm_rps_events) {
> +		if (gt_iir[2] & (dev_priv->pm_rps_events |
> +				 dev_priv->pm_guc_events)) {
>   			I915_WRITE_FW(GEN8_GT_IIR(2),
> -				      gt_iir[2] & dev_priv->pm_rps_events);
> +				      gt_iir[2] & (dev_priv->pm_rps_events |
> +						   dev_priv->pm_guc_events));
>   			ret = IRQ_HANDLED;
>   		} else
>   			DRM_ERROR("The master control interrupt lied (PM)!\n");
> @@ -1382,6 +1433,9 @@ static void gen8_gt_irq_handler(struct drm_i915_private *dev_priv,
>
>   	if (gt_iir[2] & dev_priv->pm_rps_events)
>   		gen6_rps_irq_handler(dev_priv, gt_iir[2]);
> +
> +	if (gt_iir[2] & dev_priv->pm_guc_events)
> +		gen9_guc_irq_handler(dev_priv, gt_iir[2]);
>   }
>
>   static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
> @@ -1628,6 +1682,38 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
>   	}
>   }
>
> +static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir)
> +{
> +	if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
> +		spin_lock(&dev_priv->irq_lock);
> +		if (dev_priv->guc.interrupts_enabled) {
> +			/* Sample the log buffer flush related bits & clear them
> +			 * out now itself from the message identity register to
> +			 * minimize the probability of losing a flush interrupt,
> +			 * when there are back to back flush interrupts.
> +			 * There can be a new flush interrupt, for different log
> +			 * buffer type (like for ISR), whilst Host is handling
> +			 * one (for DPC). Since same bit is used in message
> +			 * register for ISR & DPC, it could happen that GuC
> +			 * sets the bit for 2nd interrupt but Host clears out
> +			 * the bit on handling the 1st interrupt.
> +			 */
> +			u32 msg = I915_READ(SOFT_SCRATCH(15)) &
> +					(GUC2HOST_MSG_CRASH_DUMP_POSTED |
> +					 GUC2HOST_MSG_FLUSH_LOG_BUFFER);
> +			if (msg) {
> +				/* Clear the message bits that are handled */
> +				I915_WRITE(SOFT_SCRATCH(15),
> +					I915_READ(SOFT_SCRATCH(15)) & ~msg);
> +
> +				/* Handle flush interrupt event in bottom half */
> +				queue_work(dev_priv->wq, &dev_priv->guc.events_work);

Since the later patch is changing this to use a thread, since you have 
established worker is too slow - especially the shared one - I would 
really recommend you start with the kthread straight away. Not have the 
worker for a while in the same series and then later change it to a thread.

> +			}
> +		}
> +		spin_unlock(&dev_priv->irq_lock);

Why does the above needs to be done under the irq_lock ?

> +	}
> +}
> +
>   static bool intel_pipe_handle_vblank(struct drm_i915_private *dev_priv,
>   				     enum pipe pipe)
>   {
> @@ -3754,7 +3840,7 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
>   	GEN8_IRQ_INIT_NDX(GT, 1, ~gt_interrupts[1], gt_interrupts[1]);
>   	/*
>   	 * RPS interrupts will get enabled/disabled on demand when RPS itself
> -	 * is enabled/disabled.
> +	 * is enabled/disabled. Same wil be the case for GuC interrupts.
>   	 */
>   	GEN8_IRQ_INIT_NDX(GT, 2, dev_priv->pm_imr, dev_priv->pm_ier);
>   	GEN8_IRQ_INIT_NDX(GT, 3, ~gt_interrupts[3], gt_interrupts[3]);
> @@ -4539,6 +4625,10 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>
>   	INIT_WORK(&dev_priv->rps.work, gen6_pm_rps_work);
>   	INIT_WORK(&dev_priv->l3_parity.error_work, ivybridge_parity_work);
> +	INIT_WORK(&dev_priv->guc.events_work, gen9_guc2host_events_work);
> +
> +	if (HAS_GUC_UCODE(dev))
> +		dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT;
>
>   	/* Let's track the enabled rps events */
>   	if (IS_VALLEYVIEW(dev_priv))
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8bfde75..d0d13a2 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5963,6 +5963,7 @@ enum {
>   #define  GEN8_DE_PIPE_A_IRQ		(1<<16)
>   #define  GEN8_DE_PIPE_IRQ(pipe)		(1<<(16+(pipe)))
>   #define  GEN8_GT_VECS_IRQ		(1<<6)
> +#define  GEN8_GT_GUC_IRQ		(1<<5)
>   #define  GEN8_GT_PM_IRQ			(1<<4)
>   #define  GEN8_GT_VCS2_IRQ		(1<<3)
>   #define  GEN8_GT_VCS1_IRQ		(1<<2)
> @@ -5974,6 +5975,16 @@ enum {
>   #define GEN8_GT_IIR(which) _MMIO(0x44308 + (0x10 * (which)))
>   #define GEN8_GT_IER(which) _MMIO(0x4430c + (0x10 * (which)))
>
> +#define GEN9_GUC_TO_HOST_INT_EVENT	(1<<31)
> +#define GEN9_GUC_EXEC_ERROR_EVENT	(1<<30)
> +#define GEN9_GUC_DISPLAY_EVENT		(1<<29)
> +#define GEN9_GUC_SEMA_SIGNAL_EVENT	(1<<28)
> +#define GEN9_GUC_IOMMU_MSG_EVENT	(1<<27)
> +#define GEN9_GUC_DB_RING_EVENT		(1<<26)
> +#define GEN9_GUC_DMA_DONE_EVENT		(1<<25)
> +#define GEN9_GUC_FATAL_ERROR_EVENT	(1<<24)
> +#define GEN9_GUC_NOTIFICATION_EVENT	(1<<23)
> +
>   #define GEN8_RCS_IRQ_SHIFT 0
>   #define GEN8_BCS_IRQ_SHIFT 16
>   #define GEN8_VCS1_IRQ_SHIFT 0
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ae6c535..6868f31 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1102,6 +1102,9 @@ void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
>   				     unsigned int pipe_mask);
>   void gen8_irq_power_well_pre_disable(struct drm_i915_private *dev_priv,
>   				     unsigned int pipe_mask);
> +void gen9_reset_guc_interrupts(struct drm_i915_private *dev_priv);
> +void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv);
> +void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv);
>
>   /* intel_crt.c */
>   void intel_crt_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index d52eca3..2663b41 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -131,6 +131,10 @@ struct intel_guc {
>   	struct intel_guc_fw guc_fw;
>   	struct intel_guc_log log;
>
> +	/* GuC2Host interrupt related state */
> +	struct work_struct events_work;
> +	bool interrupts_enabled;
> +
>   	struct drm_i915_gem_object *ads_obj;
>
>   	struct drm_i915_gem_object *ctx_pool_obj;
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 4882f17..d74d7a5 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -450,6 +450,7 @@ int intel_guc_setup(struct drm_device *dev)
>   	}
>
>   	direct_interrupts_to_host(dev_priv);
> +	gen9_reset_guc_interrupts(dev_priv);
>
>   	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
>
> @@ -496,6 +497,9 @@ int intel_guc_setup(struct drm_device *dev)
>   		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>
>   	if (i915.enable_guc_submission) {
> +		if (i915.guc_log_level >= 0)
> +			gen9_enable_guc_interrupts(dev_priv);
> +
>   		err = i915_guc_submission_enable(dev_priv);
>   		if (err)
>   			goto fail;
>

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx