| Message ID | 1467029818-3417-5-git-send-email-akash.goel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27/06/16 13:16, 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) > > 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 | 95 ++++++++++++++++++++++++++++-- > drivers/gpu/drm/i915/i915_reg.h | 11 ++++ > drivers/gpu/drm/i915/intel_drv.h | 3 + > drivers/gpu/drm/i915/intel_guc.h | 5 ++ > drivers/gpu/drm/i915/intel_guc_loader.c | 4 ++ > 7 files changed, 120 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 85a7103..20c701c 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1808,6 +1808,7 @@ struct drm_i915_private { > u32 pm_irq_mask; > u32 pm_ier_mask; > u32 pm_rps_events; > + u32 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 28a810f..8105ddd 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -1038,6 +1038,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; > @@ -1064,6 +1066,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 7316ab4..3043e45 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 > @@ -422,6 +423,42 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv) > synchronize_irq(dev_priv->dev->irq); > } > > +void gen9_reset_guc_interrupts(struct drm_i915_private *dev_priv) > +{ > + spin_lock_irq(&dev_priv->irq_lock); > + gen6_reset_pm_interrupts(dev_priv, dev_priv->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(I915_READ(gen6_pm_iir(dev_priv)) & > + dev_priv->guc_events); > + dev_priv->guc.interrupts_enabled = true; > + gen6_enable_pm_interrupts(dev_priv, dev_priv->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; > + spin_unlock_irq(&dev_priv->irq_lock); > + > + cancel_work_sync(&dev_priv->guc.events_work); > + > + spin_lock_irq(&dev_priv->irq_lock); > + > + gen6_disable_pm_interrupts(dev_priv, dev_priv->guc_events); > + > + spin_unlock_irq(&dev_priv->irq_lock); > + > + synchronize_irq(dev_priv->dev->irq); > +} > + > /** > * bdw_update_port_irq - update DE port interrupt > * @dev_priv: driver private > @@ -1196,6 +1233,33 @@ out: > ENABLE_RPM_WAKEREF_ASSERTS(dev_priv); > } > > +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 cancelation during disabling guc interrupts. */ > + if (!dev_priv->guc.interrupts_enabled) { > + spin_unlock_irq(&dev_priv->irq_lock); > + return; > + } > + > + /* Though this work item gets synced during rpm suspend, but still need > + * a rpm get/put to avoid the warning, as it could get executed in a > + * window, where rpm ref count has dropped to zero but rpm suspend has > + * not kicked in. Generally device is expected to be active only at this > + * time so get/put should be really quick. > + */ > + intel_runtime_pm_get(dev_priv); > + > + gen6_enable_pm_irq(dev_priv, GEN9_GUC_TO_HOST_INT_EVENT); > + spin_unlock_irq(&dev_priv->irq_lock); > + > + /* TODO: Handle the events for which GuC interrupted host */ > + > + intel_runtime_pm_put(dev_priv); > +} > > /** > * ivybridge_parity_work - Workqueue called when a parity error interrupt > @@ -1371,11 +1435,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->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->guc_events)); > ret = IRQ_HANDLED; > } else > DRM_ERROR("The master control interrupt lied (PM)!\n"); > @@ -1407,6 +1473,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->guc_events) > + gen9_guc_irq_handler(dev_priv, gt_iir[2]); > } > > static bool bxt_port_hotplug_long_detect(enum port port, u32 val) > @@ -1653,6 +1722,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) > +{ > + if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) { > + spin_lock(&dev_priv->irq_lock); > + if (dev_priv->guc.interrupts_enabled) { So it is expected interrupts will always be enabled when i915.guc_log_level is set, correct? Also do you need to check against dev_priv->guc.interrupts_enabled at all then? Or from an opposite angle, would you instead need to log the fact unexpected interrupt was received here? > + /* Process all the GuC to Host events in bottom half */ > + gen6_disable_pm_irq(dev_priv, > + GEN9_GUC_TO_HOST_INT_EVENT); Why it is important to disable the interrupt here? Not for the queue work I think. Also, is it safe with regards to potentially losing the interrupt? > + queue_work(dev_priv->wq, &dev_priv->guc.events_work); Because dev_priv->wq is a one a time in order wq so if something else is running on it and taking time, can that also be a cause of dropping an interrupt or being late with sending the flush signal to the guc and so losing some logs? > + } > + spin_unlock(&dev_priv->irq_lock); > + } > +} > + > static bool intel_pipe_handle_vblank(struct drm_i915_private *dev_priv, > enum pipe pipe) > { > @@ -3809,7 +3892,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_irq_mask, 0); > GEN8_IRQ_INIT_NDX(GT, 3, ~gt_interrupts[3], gt_interrupts[3]); > @@ -4594,6 +4677,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->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 c6bfbf8..4441918 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 2a013fc..6966ffe 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1083,6 +1083,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 3e3e743..ae787e2 100644 > --- a/drivers/gpu/drm/i915/intel_guc.h > +++ b/drivers/gpu/drm/i915/intel_guc.h > @@ -126,6 +126,11 @@ struct intel_guc { > struct intel_guc_fw guc_fw; > uint32_t log_flags; > struct drm_i915_gem_object *log_obj; > + /* > + * work, interrupts_enabled are protected by dev_priv->irq_lock > + */ > + struct work_struct events_work; > + bool interrupts_enabled; > > struct drm_i915_gem_object *ads_obj; > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index db3c897..fcf36a2 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -448,6 +448,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; > > @@ -494,6 +495,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
On 6/28/2016 3:33 PM, Tvrtko Ursulin wrote: > > On 27/06/16 13:16, 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) >> >> 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 | 95 >> ++++++++++++++++++++++++++++-- >> drivers/gpu/drm/i915/i915_reg.h | 11 ++++ >> drivers/gpu/drm/i915/intel_drv.h | 3 + >> drivers/gpu/drm/i915/intel_guc.h | 5 ++ >> drivers/gpu/drm/i915/intel_guc_loader.c | 4 ++ >> 7 files changed, 120 insertions(+), 4 deletions(-) >>>> >> +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 cancelation during disabling guc interrupts. */ >> + if (!dev_priv->guc.interrupts_enabled) { >> + spin_unlock_irq(&dev_priv->irq_lock); >> + return; >> + } >> + >> + /* Though this work item gets synced during rpm suspend, but >> still need >> + * a rpm get/put to avoid the warning, as it could get executed in a >> + * window, where rpm ref count has dropped to zero but rpm >> suspend has >> + * not kicked in. Generally device is expected to be active only >> at this >> + * time so get/put should be really quick. >> + */ >> + intel_runtime_pm_get(dev_priv); >> + >> + gen6_enable_pm_irq(dev_priv, GEN9_GUC_TO_HOST_INT_EVENT); >> + spin_unlock_irq(&dev_priv->irq_lock); >> + >> + /* TODO: Handle the events for which GuC interrupted host */ >> + >> + intel_runtime_pm_put(dev_priv); >> +} >> >> /** >> * ivybridge_parity_work - Workqueue called when a parity error >> interrupt >> @@ -1371,11 +1435,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->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->guc_events)); >> ret = IRQ_HANDLED; >> } else >> DRM_ERROR("The master control interrupt lied (PM)!\n"); >> @@ -1407,6 +1473,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->guc_events) >> + gen9_guc_irq_handler(dev_priv, gt_iir[2]); >> } >> >> static bool bxt_port_hotplug_long_detect(enum port port, u32 val) >> @@ -1653,6 +1722,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) >> +{ >> + if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) { >> + spin_lock(&dev_priv->irq_lock); >> + if (dev_priv->guc.interrupts_enabled) { > > So it is expected interrupts will always be enabled when > i915.guc_log_level is set, correct? > Yes currently only when guc_log_level > 0, interrupt should be enabled. But we need to disable/enable the interrupt upon suspend/resume and across GPU reset. So interrupt may not be always in a enabled state when guc_log_level>0. > Also do you need to check against dev_priv->guc.interrupts_enabled at > all then? Or from an opposite angle, would you instead need to log the > fact unexpected interrupt was received here? I think this check is needed, to avoid the race in disabling interrupt. Please refer the sequence in interrupt disabling function (same as rps disabling), there we first set the interrupts_enabled flag to false, then wait for the work item to finish execution and then program the IMR register. > >> + /* Process all the GuC to Host events in bottom half */ >> + gen6_disable_pm_irq(dev_priv, >> + GEN9_GUC_TO_HOST_INT_EVENT); > > Why it is important to disable the interrupt here? Not for the queue > work I think. We want to & can handle one interrupt at a time, unless the queued work item is executed we can't process the next interrupt, so better to keep the interrupt masked. Sorry this is what is my understanding. > > Also, is it safe with regards to potentially losing the interrupt? > Particularly for the FLUSH_LOG_BUFFER case, GuC won't send a new flush interrupt unless its gets an acknowledgement (flush signal) of the previous one from Host. >> + queue_work(dev_priv->wq, &dev_priv->guc.events_work); > > Because dev_priv->wq is a one a time in order wq so if something else is > running on it and taking time, can that also be a cause of dropping an > interrupt or being late with sending the flush signal to the guc and so > losing some logs? Its a Driver's private workqueue and Turbo work item is also queued inside this workqueue which too needs to be executed without much delay. But yes the flush work item can get substantially delayed in case if there are other work items queued before it, especially the mm.retire_work (but generally executes every ~1 second). Best would be if the log buffer (44KB data) can be sampled in IRQ context (or Tasklet context) itself. Best regards Akash > >> + } >> + spin_unlock(&dev_priv->irq_lock); >> + } >> +} >> + >> static bool intel_pipe_handle_vblank(struct drm_i915_private *dev_priv, >> enum pipe pipe) >> { >> @@ -3809,7 +3892,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_irq_mask, 0); >> GEN8_IRQ_INIT_NDX(GT, 3, ~gt_interrupts[3], gt_interrupts[3]); >> @@ -4594,6 +4677,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->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 c6bfbf8..4441918 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 2a013fc..6966ffe 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -1083,6 +1083,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 3e3e743..ae787e2 100644 >> --- a/drivers/gpu/drm/i915/intel_guc.h >> +++ b/drivers/gpu/drm/i915/intel_guc.h >> @@ -126,6 +126,11 @@ struct intel_guc { >> struct intel_guc_fw guc_fw; >> uint32_t log_flags; >> struct drm_i915_gem_object *log_obj; >> + /* >> + * work, interrupts_enabled are protected by dev_priv->irq_lock >> + */ >> + struct work_struct events_work; >> + bool interrupts_enabled; >> >> struct drm_i915_gem_object *ads_obj; >> >> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c >> b/drivers/gpu/drm/i915/intel_guc_loader.c >> index db3c897..fcf36a2 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_loader.c >> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c >> @@ -448,6 +448,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; >> >> @@ -494,6 +495,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 >
On 28/06/16 12:12, Goel, Akash wrote: > > > On 6/28/2016 3:33 PM, Tvrtko Ursulin wrote: >> >> On 27/06/16 13:16, 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) >>> >>> 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 | 95 >>> ++++++++++++++++++++++++++++-- >>> drivers/gpu/drm/i915/i915_reg.h | 11 ++++ >>> drivers/gpu/drm/i915/intel_drv.h | 3 + >>> drivers/gpu/drm/i915/intel_guc.h | 5 ++ >>> drivers/gpu/drm/i915/intel_guc_loader.c | 4 ++ >>> 7 files changed, 120 insertions(+), 4 deletions(-) >>>>> >>> +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 cancelation during disabling guc interrupts. */ >>> + if (!dev_priv->guc.interrupts_enabled) { >>> + spin_unlock_irq(&dev_priv->irq_lock); >>> + return; >>> + } >>> + >>> + /* Though this work item gets synced during rpm suspend, but >>> still need >>> + * a rpm get/put to avoid the warning, as it could get executed >>> in a >>> + * window, where rpm ref count has dropped to zero but rpm >>> suspend has >>> + * not kicked in. Generally device is expected to be active only >>> at this >>> + * time so get/put should be really quick. >>> + */ >>> + intel_runtime_pm_get(dev_priv); >>> + >>> + gen6_enable_pm_irq(dev_priv, GEN9_GUC_TO_HOST_INT_EVENT); >>> + spin_unlock_irq(&dev_priv->irq_lock); >>> + >>> + /* TODO: Handle the events for which GuC interrupted host */ >>> + >>> + intel_runtime_pm_put(dev_priv); >>> +} >>> >>> /** >>> * ivybridge_parity_work - Workqueue called when a parity error >>> interrupt >>> @@ -1371,11 +1435,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->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->guc_events)); >>> ret = IRQ_HANDLED; >>> } else >>> DRM_ERROR("The master control interrupt lied (PM)!\n"); >>> @@ -1407,6 +1473,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->guc_events) >>> + gen9_guc_irq_handler(dev_priv, gt_iir[2]); >>> } >>> >>> static bool bxt_port_hotplug_long_detect(enum port port, u32 val) >>> @@ -1653,6 +1722,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) >>> +{ >>> + if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) { >>> + spin_lock(&dev_priv->irq_lock); >>> + if (dev_priv->guc.interrupts_enabled) { >> >> So it is expected interrupts will always be enabled when >> i915.guc_log_level is set, correct? >> > Yes currently only when guc_log_level > 0, interrupt should be enabled. > > But we need to disable/enable the interrupt upon suspend/resume and > across GPU reset. > So interrupt may not be always in a enabled state when guc_log_level>0. > >> Also do you need to check against dev_priv->guc.interrupts_enabled at >> all then? Or from an opposite angle, would you instead need to log the >> fact unexpected interrupt was received here? > > I think this check is needed, to avoid the race in disabling interrupt. > Please refer the sequence in interrupt disabling function (same as rps > disabling), there we first set the interrupts_enabled flag to false, > then wait for the work item to finish execution and then program the IMR > register. Right I see now that it is copy-pasted existing sequence. In this case I won't question it further. :) >> >>> + /* Process all the GuC to Host events in bottom half */ >>> + gen6_disable_pm_irq(dev_priv, >>> + GEN9_GUC_TO_HOST_INT_EVENT); >> >> Why it is important to disable the interrupt here? Not for the queue >> work I think. > > We want to & can handle one interrupt at a time, unless the queued work > item is executed we can't process the next interrupt, so better to keep > the interrupt masked. > Sorry this is what is my understanding. So it is queued in hardware and will get asserted when unmasked? > >> >> Also, is it safe with regards to potentially losing the interrupt? >> > Particularly for the FLUSH_LOG_BUFFER case, GuC won't send a new flush > interrupt unless its gets an acknowledgement (flush signal) of the > previous one from Host. Ah so the previous comment is really impossible? I mean the need to mask? Possibly just put a comment up there explaining that. > >>> + queue_work(dev_priv->wq, &dev_priv->guc.events_work); >> >> Because dev_priv->wq is a one a time in order wq so if something else is >> running on it and taking time, can that also be a cause of dropping an >> interrupt or being late with sending the flush signal to the guc and so >> losing some logs? > > Its a Driver's private workqueue and Turbo work item is also queued > inside this workqueue which too needs to be executed without much delay. > But yes the flush work item can get substantially delayed in case if > there are other work items queued before it, especially the > mm.retire_work (but generally executes every ~1 second). > > Best would be if the log buffer (44KB data) can be sampled in IRQ > context (or Tasklet context) itself. I was just trying to understand if you perhaps need a dedicated wq. I don't have a feel at all on how much data guc logging generates per second. If the interrupt is low frequency even with a lot of cmd submission happening it could be fine like it is. Regards, Tvrtko
On 6/28/2016 7:14 PM, Tvrtko Ursulin wrote: > > On 28/06/16 12:12, Goel, Akash wrote: >> >> >> On 6/28/2016 3:33 PM, Tvrtko Ursulin wrote: >>> >>> On 27/06/16 13:16, 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) >>>> >>>> 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 | 95 >>>> ++++++++++++++++++++++++++++-- >>>> drivers/gpu/drm/i915/i915_reg.h | 11 ++++ >>>> drivers/gpu/drm/i915/intel_drv.h | 3 + >>>> drivers/gpu/drm/i915/intel_guc.h | 5 ++ >>>> drivers/gpu/drm/i915/intel_guc_loader.c | 4 ++ >>>> 7 files changed, 120 insertions(+), 4 deletions(-) >>>>>> >>>> +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 cancelation during disabling guc interrupts. */ >>>> + if (!dev_priv->guc.interrupts_enabled) { >>>> + spin_unlock_irq(&dev_priv->irq_lock); >>>> + return; >>>> + } >>>> + >>>> + /* Though this work item gets synced during rpm suspend, but >>>> still need >>>> + * a rpm get/put to avoid the warning, as it could get executed >>>> in a >>>> + * window, where rpm ref count has dropped to zero but rpm >>>> suspend has >>>> + * not kicked in. Generally device is expected to be active only >>>> at this >>>> + * time so get/put should be really quick. >>>> + */ >>>> + intel_runtime_pm_get(dev_priv); >>>> + >>>> + gen6_enable_pm_irq(dev_priv, GEN9_GUC_TO_HOST_INT_EVENT); >>>> + spin_unlock_irq(&dev_priv->irq_lock); >>>> + >>>> + /* TODO: Handle the events for which GuC interrupted host */ >>>> + >>>> + intel_runtime_pm_put(dev_priv); >>>> +} >>>> >>>> static bool bxt_port_hotplug_long_detect(enum port port, u32 val) >>>> @@ -1653,6 +1722,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) >>>> +{ >>>> + if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) { >>>> + spin_lock(&dev_priv->irq_lock); >>>> + if (dev_priv->guc.interrupts_enabled) { >>> >>> So it is expected interrupts will always be enabled when >>> i915.guc_log_level is set, correct? >>> >> Yes currently only when guc_log_level > 0, interrupt should be enabled. >> >> But we need to disable/enable the interrupt upon suspend/resume and >> across GPU reset. >> So interrupt may not be always in a enabled state when guc_log_level>0. >> >>> Also do you need to check against dev_priv->guc.interrupts_enabled at >>> all then? Or from an opposite angle, would you instead need to log the >>> fact unexpected interrupt was received here? >> >> I think this check is needed, to avoid the race in disabling interrupt. >> Please refer the sequence in interrupt disabling function (same as rps >> disabling), there we first set the interrupts_enabled flag to false, >> then wait for the work item to finish execution and then program the IMR >> register. > > Right I see now that it is copy-pasted existing sequence. In this case I > won't question it further. :) > >>> >>>> + /* Process all the GuC to Host events in bottom half */ >>>> + gen6_disable_pm_irq(dev_priv, >>>> + GEN9_GUC_TO_HOST_INT_EVENT); >>> >>> Why it is important to disable the interrupt here? Not for the queue >>> work I think. >> >> We want to & can handle one interrupt at a time, unless the queued work >> item is executed we can't process the next interrupt, so better to keep >> the interrupt masked. >> Sorry this is what is my understanding. > > So it is queued in hardware and will get asserted when unmasked? As per my understanding, if the interrupt is masked (IMR), it won't be queued, will be ignored & so will not be asserted on unmasking. If the interrupt wasn't masked, but was disabled (in IER) then it will be asserted (in IIR) when its enabled. > >> >>> >>> Also, is it safe with regards to potentially losing the interrupt? >>> >> Particularly for the FLUSH_LOG_BUFFER case, GuC won't send a new flush >> interrupt unless its gets an acknowledgement (flush signal) of the >> previous one from Host. > > Ah so the previous comment is really impossible? I mean the need to mask? Sorry my comments were not fully correct. GuC can send a new flush interrupt, even if the previous one is pending, but that will be for a different log buffer type (3 types of log buffer ISR, DPC, CRASH). For the same buffer type, GuC won't send a new flush interrupt unless its gets an acknowledgement of the previous one from Host. But as you said the workqueue is ordered and furthermore there is a single instance of work item, so the serialization will be provided implicitly and there is no real need to mask the interrupt. As mentioned above, a new flush interrupt can come while the previous one is being processed on Host but due to a single instance of work item either that new interrupt will not do anything effectively if work item was in a pending state or will re queue the work item if it was getting executed at that time. Also the state of all 3 log buffer types are being parsed irrespective for which one the interrupt actually came, and the whole buffer is being captured (this is how it has been recommended to handle the flush interrupts from Host side). So if a new interrupt comes while the work item was in a pending state, then effectively work of this new interrupt will also be done when work item is executed later. So will remove the masking then ? > > Possibly just put a comment up there explaining that. > >> >>>> + queue_work(dev_priv->wq, &dev_priv->guc.events_work); >>> >>> Because dev_priv->wq is a one a time in order wq so if something else is >>> running on it and taking time, can that also be a cause of dropping an >>> interrupt or being late with sending the flush signal to the guc and so >>> losing some logs? >> >> Its a Driver's private workqueue and Turbo work item is also queued >> inside this workqueue which too needs to be executed without much delay. >> But yes the flush work item can get substantially delayed in case if >> there are other work items queued before it, especially the >> mm.retire_work (but generally executes every ~1 second). >> >> Best would be if the log buffer (44KB data) can be sampled in IRQ >> context (or Tasklet context) itself. > > I was just trying to understand if you perhaps need a dedicated wq. I > don't have a feel at all on how much data guc logging generates per > second. If the interrupt is low frequency even with a lot of cmd > submission happening it could be fine like it is. > Actually with maximum verbosity level, I am seeing flush interrupt every ms, with 'gem_exec_nop' IGT, as there are lot of submissions being done. But such may not happen in real life scenario. I think, if needed, later on we can either have a dedicated high priority work queue for logging work or use the tasklet context to do the processing. Best regards Akash > Regards, > > Tvrtko
On 01/07/16 07:16, Goel, Akash wrote: [snip] >>>> >>>>> + /* Process all the GuC to Host events in bottom half */ >>>>> + gen6_disable_pm_irq(dev_priv, >>>>> + GEN9_GUC_TO_HOST_INT_EVENT); >>>> >>>> Why it is important to disable the interrupt here? Not for the queue >>>> work I think. >>> >>> We want to & can handle one interrupt at a time, unless the queued work >>> item is executed we can't process the next interrupt, so better to keep >>> the interrupt masked. >>> Sorry this is what is my understanding. >> >> So it is queued in hardware and will get asserted when unmasked? > As per my understanding, if the interrupt is masked (IMR), it won't be > queued, will be ignored & so will not be asserted on unmasking. > > If the interrupt wasn't masked, but was disabled (in IER) then it > will be asserted (in IIR) when its enabled. > >> >>> >>>> >>>> Also, is it safe with regards to potentially losing the interrupt? >>>> >>> Particularly for the FLUSH_LOG_BUFFER case, GuC won't send a new flush >>> interrupt unless its gets an acknowledgement (flush signal) of the >>> previous one from Host. >> >> Ah so the previous comment is really impossible? I mean the need to mask? > > Sorry my comments were not fully correct. GuC can send a new flush > interrupt, even if the previous one is pending, but that will be for a > different log buffer type (3 types of log buffer ISR, DPC, CRASH). > For the same buffer type, GuC won't send a new flush interrupt unless > its gets an acknowledgement of the previous one from Host. > > But as you said the workqueue is ordered and furthermore there is a > single instance of work item, so the serialization will be provided > implicitly and there is no real need to mask the interrupt. > > As mentioned above, a new flush interrupt can come while the previous > one is being processed on Host but due to a single instance of work item > either that new interrupt will not do anything effectively if work > item was in a pending state or will re queue the work item if it was > getting executed at that time. > > Also the state of all 3 log buffer types are being parsed irrespective > for which one the interrupt actually came, and the whole buffer is being > captured (this is how it has been recommended to handle the flush > interrupts from Host side). So if a new interrupt comes while the work > item was in a pending state, then effectively work of this new interrupt > will also be done when work item is executed later. > > So will remove the masking then ? I think so, because if I understood what you wrote, masking can lose us an interrupt. >> >> Possibly just put a comment up there explaining that. >> >>> >>>>> + queue_work(dev_priv->wq, &dev_priv->guc.events_work); >>>> >>>> Because dev_priv->wq is a one a time in order wq so if something >>>> else is >>>> running on it and taking time, can that also be a cause of dropping an >>>> interrupt or being late with sending the flush signal to the guc and so >>>> losing some logs? >>> >>> Its a Driver's private workqueue and Turbo work item is also queued >>> inside this workqueue which too needs to be executed without much delay. >>> But yes the flush work item can get substantially delayed in case if >>> there are other work items queued before it, especially the >>> mm.retire_work (but generally executes every ~1 second). >>> >>> Best would be if the log buffer (44KB data) can be sampled in IRQ >>> context (or Tasklet context) itself. >> >> I was just trying to understand if you perhaps need a dedicated wq. I >> don't have a feel at all on how much data guc logging generates per >> second. If the interrupt is low frequency even with a lot of cmd >> submission happening it could be fine like it is. >> > Actually with maximum verbosity level, I am seeing flush interrupt every > ms, with 'gem_exec_nop' IGT, as there are lot of submissions being done. > But such may not happen in real life scenario. > > I think, if needed, later on we can either have a dedicated high > priority work queue for logging work or use the tasklet context to do > the processing. Hm, do you need to add some DRM_ERROR or something if wq starts lagging behind the flush interrupts? How many missed flush interrupts can we afford before the logging buffer starts getting overwritten? Regards, Tvrtko
On 7/1/2016 2:17 PM, Tvrtko Ursulin wrote: > > > On 01/07/16 07:16, Goel, Akash wrote: > > [snip] > >>>>> >>>>>> + /* Process all the GuC to Host events in bottom half */ >>>>>> + gen6_disable_pm_irq(dev_priv, >>>>>> + GEN9_GUC_TO_HOST_INT_EVENT); >>>>> >>>>> Why it is important to disable the interrupt here? Not for the queue >>>>> work I think. >>>> >>>> We want to & can handle one interrupt at a time, unless the queued work >>>> item is executed we can't process the next interrupt, so better to keep >>>> the interrupt masked. >>>> Sorry this is what is my understanding. >>> >>> So it is queued in hardware and will get asserted when unmasked? >> As per my understanding, if the interrupt is masked (IMR), it won't be >> queued, will be ignored & so will not be asserted on unmasking. >> >> If the interrupt wasn't masked, but was disabled (in IER) then it >> will be asserted (in IIR) when its enabled. >> >>> >>>> >>>>> >>>>> Also, is it safe with regards to potentially losing the interrupt? >>>>> >>>> Particularly for the FLUSH_LOG_BUFFER case, GuC won't send a new flush >>>> interrupt unless its gets an acknowledgement (flush signal) of the >>>> previous one from Host. >>> >>> Ah so the previous comment is really impossible? I mean the need to >>> mask? >> >> Sorry my comments were not fully correct. GuC can send a new flush >> interrupt, even if the previous one is pending, but that will be for a >> different log buffer type (3 types of log buffer ISR, DPC, CRASH). >> For the same buffer type, GuC won't send a new flush interrupt unless >> its gets an acknowledgement of the previous one from Host. >> >> But as you said the workqueue is ordered and furthermore there is a >> single instance of work item, so the serialization will be provided >> implicitly and there is no real need to mask the interrupt. >> >> As mentioned above, a new flush interrupt can come while the previous >> one is being processed on Host but due to a single instance of work item >> either that new interrupt will not do anything effectively if work >> item was in a pending state or will re queue the work item if it was >> getting executed at that time. >> >> Also the state of all 3 log buffer types are being parsed irrespective >> for which one the interrupt actually came, and the whole buffer is being >> captured (this is how it has been recommended to handle the flush >> interrupts from Host side). So if a new interrupt comes while the work >> item was in a pending state, then effectively work of this new interrupt >> will also be done when work item is executed later. >> >> So will remove the masking then ? > > I think so, because if I understood what you wrote, masking can lose us > an interrupt. > If a new flush interrupt comes while the work item was getting executed then there is a potential of losing an opportunity to sample the log buffer. Will not mask the interrupt. Thanks for persisting on this. >>> >>> Possibly just put a comment up there explaining that. >>> >>>> >>>>>> + queue_work(dev_priv->wq, &dev_priv->guc.events_work); >>>>> >>>>> Because dev_priv->wq is a one a time in order wq so if something >>>>> else is >>>>> running on it and taking time, can that also be a cause of dropping an >>>>> interrupt or being late with sending the flush signal to the guc >>>>> and so >>>>> losing some logs? >>>> >>>> Its a Driver's private workqueue and Turbo work item is also queued >>>> inside this workqueue which too needs to be executed without much >>>> delay. >>>> But yes the flush work item can get substantially delayed in case if >>>> there are other work items queued before it, especially the >>>> mm.retire_work (but generally executes every ~1 second). >>>> >>>> Best would be if the log buffer (44KB data) can be sampled in IRQ >>>> context (or Tasklet context) itself. >>> >>> I was just trying to understand if you perhaps need a dedicated wq. I >>> don't have a feel at all on how much data guc logging generates per >>> second. If the interrupt is low frequency even with a lot of cmd >>> submission happening it could be fine like it is. >>> >> Actually with maximum verbosity level, I am seeing flush interrupt every >> ms, with 'gem_exec_nop' IGT, as there are lot of submissions being done. >> But such may not happen in real life scenario. >> >> I think, if needed, later on we can either have a dedicated high >> priority work queue for logging work or use the tasklet context to do >> the processing. > > Hm, do you need to add some DRM_ERROR or something if wq starts lagging > behind the flush interrupts? How many missed flush interrupts can we > afford before the logging buffer starts getting overwritten? > Actually if GuC is producing logs at such a fast rate then we can't afford to miss even a single interrupt, if we don't want to lose any logs. When the log buffer becomes half full, GuC sends a flush interrupt. GuC firmware expects that while it is writing to 2nd half of the buffer, first half would get consumed by Host and then get a flush completed acknowledgement from Host, so that it does not end up doing any overwrite causing loss of logs. There is a buffer_full_cnt field in the state structure which GuC firmware increments every time it detects a potential log buffer overflow. Probably this can be shown via debugfs ? The return value of queue_work() can be checked in the interrupt, if it is zero then it means the work item is still in the queue so the previous flush hasn't been processed yet, this can give an inkling of lag. I had initially thought that creating a dedicated workqueue for flush work might be considered an overkill. Will proceed as per your suggestion. Best regards Akash > Regards, > > Tvrtko > > >
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 85a7103..20c701c 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1808,6 +1808,7 @@ struct drm_i915_private { u32 pm_irq_mask; u32 pm_ier_mask; u32 pm_rps_events; + u32 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 28a810f..8105ddd 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -1038,6 +1038,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; @@ -1064,6 +1066,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 7316ab4..3043e45 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 @@ -422,6 +423,42 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv) synchronize_irq(dev_priv->dev->irq); } +void gen9_reset_guc_interrupts(struct drm_i915_private *dev_priv) +{ + spin_lock_irq(&dev_priv->irq_lock); + gen6_reset_pm_interrupts(dev_priv, dev_priv->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(I915_READ(gen6_pm_iir(dev_priv)) & + dev_priv->guc_events); + dev_priv->guc.interrupts_enabled = true; + gen6_enable_pm_interrupts(dev_priv, dev_priv->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; + spin_unlock_irq(&dev_priv->irq_lock); + + cancel_work_sync(&dev_priv->guc.events_work); + + spin_lock_irq(&dev_priv->irq_lock); + + gen6_disable_pm_interrupts(dev_priv, dev_priv->guc_events); + + spin_unlock_irq(&dev_priv->irq_lock); + + synchronize_irq(dev_priv->dev->irq); +} + /** * bdw_update_port_irq - update DE port interrupt * @dev_priv: driver private @@ -1196,6 +1233,33 @@ out: ENABLE_RPM_WAKEREF_ASSERTS(dev_priv); } +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 cancelation during disabling guc interrupts. */ + if (!dev_priv->guc.interrupts_enabled) { + spin_unlock_irq(&dev_priv->irq_lock); + return; + } + + /* Though this work item gets synced during rpm suspend, but still need + * a rpm get/put to avoid the warning, as it could get executed in a + * window, where rpm ref count has dropped to zero but rpm suspend has + * not kicked in. Generally device is expected to be active only at this + * time so get/put should be really quick. + */ + intel_runtime_pm_get(dev_priv); + + gen6_enable_pm_irq(dev_priv, GEN9_GUC_TO_HOST_INT_EVENT); + spin_unlock_irq(&dev_priv->irq_lock); + + /* TODO: Handle the events for which GuC interrupted host */ + + intel_runtime_pm_put(dev_priv); +} /** * ivybridge_parity_work - Workqueue called when a parity error interrupt @@ -1371,11 +1435,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->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->guc_events)); ret = IRQ_HANDLED; } else DRM_ERROR("The master control interrupt lied (PM)!\n"); @@ -1407,6 +1473,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->guc_events) + gen9_guc_irq_handler(dev_priv, gt_iir[2]); } static bool bxt_port_hotplug_long_detect(enum port port, u32 val) @@ -1653,6 +1722,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) +{ + if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) { + spin_lock(&dev_priv->irq_lock); + if (dev_priv->guc.interrupts_enabled) { + /* Process all the GuC to Host events in bottom half */ + gen6_disable_pm_irq(dev_priv, + GEN9_GUC_TO_HOST_INT_EVENT); + queue_work(dev_priv->wq, &dev_priv->guc.events_work); + } + spin_unlock(&dev_priv->irq_lock); + } +} + static bool intel_pipe_handle_vblank(struct drm_i915_private *dev_priv, enum pipe pipe) { @@ -3809,7 +3892,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_irq_mask, 0); GEN8_IRQ_INIT_NDX(GT, 3, ~gt_interrupts[3], gt_interrupts[3]); @@ -4594,6 +4677,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->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 c6bfbf8..4441918 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 2a013fc..6966ffe 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1083,6 +1083,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 3e3e743..ae787e2 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -126,6 +126,11 @@ struct intel_guc { struct intel_guc_fw guc_fw; uint32_t log_flags; struct drm_i915_gem_object *log_obj; + /* + * work, interrupts_enabled are protected by dev_priv->irq_lock + */ + struct work_struct events_work; + bool interrupts_enabled; struct drm_i915_gem_object *ads_obj; diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index db3c897..fcf36a2 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -448,6 +448,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; @@ -494,6 +495,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;