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

Tvrtko Ursulin
2016-08-15T14:55:00Z

On 15/08/16 15:49, akash.goel@intel.com wrote:
> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>
> There are certain types of interrupts which Host can receive 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.
>
> v5:
> - Move the read & clearing of register, containing Guc2Host message
> bits, outside the irq spinlock. (Tvrtko)
>
> v6:
> - Move the log buffer flush interrupt related stuff to the following
> patch so as to do only generic bits in this patch. (Tvrtko)
> - Rebase.
>
> 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 | 63 ++++++++++++++++++++++++++++--
> drivers/gpu/drm/i915/i915_reg.h | 11 ++++++
> drivers/gpu/drm/i915/intel_drv.h | 3 ++
> drivers/gpu/drm/i915/intel_guc.h | 3 ++
> drivers/gpu/drm/i915/intel_guc_loader.c | 4 ++
> 7 files changed, 86 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a187ea7..08553b9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1783,6 +1783,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 34fb893..b062da6 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -1055,6 +1055,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;
> @@ -1081,6 +1083,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 83d708a..eb4774f 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,38 @@ 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);
> +
> + gen9_reset_guc_interrupts(dev_priv);
> +}
> +
> /**
> * bdw_update_port_irq - update DE port interrupt
> * @dev_priv: driver private
> @@ -1339,11 +1372,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");
> @@ -1375,6 +1410,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)
> @@ -1621,6 +1659,20 @@ 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)
> +{
> + bool interrupts_enabled;
> +
> + if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
> + spin_lock(&dev_priv->irq_lock);
> + interrupts_enabled = dev_priv->guc.interrupts_enabled;
> + spin_unlock(&dev_priv->irq_lock);
> + if (interrupts_enabled) {
> + /* TODO: Handle events for which GuC interrupted host */
> + }
> + }
> +}
> +
> static bool intel_pipe_handle_vblank(struct drm_i915_private *dev_priv,
> enum pipe pipe)
> {
> @@ -3722,7 +3774,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]);
> @@ -4508,6 +4560,9 @@ 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);
>
> + if (HAS_GUC_SCHED(dev))
> + dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT;
> +
> /* Let's track the enabled rps events */
> if (IS_VALLEYVIEW(dev_priv))
> /* WaGsvRC0ResidencyMethod:vlv */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index d4adf28..cab089a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6011,6 +6011,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)
> @@ -6022,6 +6023,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 9b99a84..f7f16ac 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1120,6 +1120,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 d32023c..1fc63fe 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -130,6 +130,9 @@ struct intel_guc {
> struct intel_guc_fw guc_fw;
> struct intel_guc_log log;
>
> + /* GuC2Host interrupt related state */
> + bool interrupts_enabled;
> +
> struct i915_vma *ads_vma;
> struct i915_vma *ctx_pool_vma;
> struct ida ctx_ids;
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 6fd24d9..ef35e99 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -464,6 +464,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;
>
> @@ -510,6 +511,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;
>

I still don't see that sampling interrupts_enabled under a spinlock does
anything useful. But it is not wrong so on the basis of that:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko