| Message ID | 1464378183-9433-4-git-send-email-akash.goel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, May 28, 2016 at 01:12:54AM +0530, 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. gen8 doesn't have a GuC. How can these functions be applicable to gen8? -Chris
On Sat, May 28, 2016 at 01:12:54AM +0530, 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. > > 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 | 2 + > drivers/gpu/drm/i915/i915_irq.c | 100 ++++++++++++++++++++++++++++- > 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 | 1 + > 7 files changed, 120 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index e4c8e34..7aae033 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1790,6 +1790,7 @@ struct drm_i915_private { > u32 gt_irq_mask; > u32 pm_irq_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 da7c242..c2f3a67 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -990,6 +990,8 @@ int intel_guc_suspend(struct drm_device *dev) > if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS) > return 0; > > + gen8_disable_guc_interrupts(dev); > + > ctx = dev_priv->kernel_context; > > data[0] = HOST2GUC_ACTION_ENTER_S_STATE; > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index caaf1e2..b4294a8 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 gen8_guc_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir); > > /* For display hotplug interrupt */ > static inline void > @@ -400,6 +401,55 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv) > synchronize_irq(dev_priv->dev->irq); > } > > +void gen8_reset_guc_interrupts(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + i915_reg_t reg = gen6_pm_iir(dev_priv); From the looks of this we have multiple shadows for the same register. That's very bad. Now the platforms might be mutually exclusive, but it is still a mistake that will catch us out. > + spin_lock_irq(&dev_priv->irq_lock); > + I915_WRITE(reg, dev_priv->guc_events); > + I915_WRITE(reg, dev_priv->guc_events); What? Not even the tiniest of comments to explain? > + POSTING_READ(reg); Again. Not even the tiniest of comments to explain? > + spin_unlock_irq(&dev_priv->irq_lock); > +} > + > +void gen8_enable_guc_interrupts(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + 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; > + I915_WRITE(gen6_pm_ier(dev_priv), > + I915_READ(gen6_pm_ier(dev_priv)) | dev_priv->guc_events); ier should be known, rmw on the reg should not be required. > + gen6_enable_pm_irq(dev_priv, dev_priv->guc_events); > + } > + spin_unlock_irq(&dev_priv->irq_lock); > +} > + > +void gen8_disable_guc_interrupts(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + 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_irq(dev_priv, dev_priv->guc_events); > + I915_WRITE(gen6_pm_ier(dev_priv), I915_READ(gen6_pm_ier(dev_priv)) & > + ~dev_priv->guc_events); > + > + spin_unlock_irq(&dev_priv->irq_lock); > + > + synchronize_irq(dev->irq); > +} > + > /** > * bdw_update_port_irq - update DE port interrupt > * @dev_priv: driver private > @@ -1174,6 +1224,27 @@ out: > ENABLE_RPM_WAKEREF_ASSERTS(dev_priv); > } > > +static void gen8_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; > + } > + > + DISABLE_RPM_WAKEREF_ASSERTS(dev_priv); This just shouts that the code is broken. > 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 41601c7..e20792d 100644 > --- a/drivers/gpu/drm/i915/intel_guc.h > +++ b/drivers/gpu/drm/i915/intel_guc.h > @@ -125,6 +125,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; The work gets added here, yet bugs are fixed for the worker in later patches. Squash in the bug fixes. -Chris
On 5/28/2016 1:13 AM, Chris Wilson wrote: > On Sat, May 28, 2016 at 01:12:54AM +0530, 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. > > gen8 doesn't have a GuC. How can these functions be applicable to gen8? Sorry I missed that in the Driver GuC is used only from Gen9 onwards. But Gen8 (both BDW & CHV) does have a GuC. I will rename the functions. Best regards Akash > -Chris >
On 5/28/2016 1:26 AM, Chris Wilson wrote: > On Sat, May 28, 2016 at 01:12:54AM +0530, 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. >> >> 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 | 2 + >> drivers/gpu/drm/i915/i915_irq.c | 100 ++++++++++++++++++++++++++++- >> 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 | 1 + >> 7 files changed, 120 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index e4c8e34..7aae033 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1790,6 +1790,7 @@ struct drm_i915_private { >> u32 gt_irq_mask; >> u32 pm_irq_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 da7c242..c2f3a67 100644 >> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c >> @@ -990,6 +990,8 @@ int intel_guc_suspend(struct drm_device *dev) >> if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS) >> return 0; >> >> + gen8_disable_guc_interrupts(dev); >> + >> ctx = dev_priv->kernel_context; >> >> data[0] = HOST2GUC_ACTION_ENTER_S_STATE; >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c >> index caaf1e2..b4294a8 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 gen8_guc_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir); >> >> /* For display hotplug interrupt */ >> static inline void >> @@ -400,6 +401,55 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv) >> synchronize_irq(dev_priv->dev->irq); >> } >> >> +void gen8_reset_guc_interrupts(struct drm_device *dev) >> +{ >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + i915_reg_t reg = gen6_pm_iir(dev_priv); > > From the looks of this we have multiple shadows for the same register. > That's very bad. > > Now the platforms might be mutually exclusive, but it is still a mistake > that will catch us out. > Will check how it is in newer platforms. >> + spin_lock_irq(&dev_priv->irq_lock); >> + I915_WRITE(reg, dev_priv->guc_events); >> + I915_WRITE(reg, dev_priv->guc_events); > > What? Not even the tiniest of comments to explain? > Sorry actually just copied these steps as is from the gen6_reset_rps_interrupts(), considering that the same set of registers (IIR, IER, IMR) are involved here. So the double clearing of IIR followed by posting read could be needed here also. >> + POSTING_READ(reg); > > Again. Not even the tiniest of comments to explain? > >> + spin_unlock_irq(&dev_priv->irq_lock); >> +} >> + >> +void gen8_enable_guc_interrupts(struct drm_device *dev) >> +{ >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + >> + 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; >> + I915_WRITE(gen6_pm_ier(dev_priv), >> + I915_READ(gen6_pm_ier(dev_priv)) | dev_priv->guc_events); > > ier should be known, rmw on the reg should not be required. > Sorry same as above, copy paste from gen6_enable_rps_interrupts(). Without rmw, would this be fine ? if (dev_priv->rps.interrupts_enabled) I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_rps_events | dev_priv->guc_events); else I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->guc_events); >> + gen6_enable_pm_irq(dev_priv, dev_priv->guc_events); >> + } >> + spin_unlock_irq(&dev_priv->irq_lock); >> +} >> + >> +void gen8_disable_guc_interrupts(struct drm_device *dev) >> +{ >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + >> + 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_irq(dev_priv, dev_priv->guc_events); >> + I915_WRITE(gen6_pm_ier(dev_priv), I915_READ(gen6_pm_ier(dev_priv)) & >> + ~dev_priv->guc_events); >> + >> + spin_unlock_irq(&dev_priv->irq_lock); >> + >> + synchronize_irq(dev->irq); >> +} >> + >> /** >> * bdw_update_port_irq - update DE port interrupt >> * @dev_priv: driver private >> @@ -1174,6 +1224,27 @@ out: >> ENABLE_RPM_WAKEREF_ASSERTS(dev_priv); >> } >> >> +static void gen8_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; >> + } >> + >> + DISABLE_RPM_WAKEREF_ASSERTS(dev_priv); > > This just shouts that the code is broken. > You mean to say that ideally the wakeref_count (& power.usage_count) should already be non zero here. > >> 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 41601c7..e20792d 100644 >> --- a/drivers/gpu/drm/i915/intel_guc.h >> +++ b/drivers/gpu/drm/i915/intel_guc.h >> @@ -125,6 +125,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; > > The work gets added here, yet bugs are fixed for the worker in later > patches. Squash in the bug fixes. Sorry didn't get this. Are you alluding to cancellation of this work_item Or flushing of work item from the error handling path ? Cancellation is being done as a part of disabling interrupt and the call to disable interrupt is there in intel_guc_suspend(), part of this patch only. Best regards Akash > -Chris >
On Sat, May 28, 2016 at 02:52:16PM +0530, Goel, Akash wrote: > > > On 5/28/2016 1:26 AM, Chris Wilson wrote: > >On Sat, May 28, 2016 at 01:12:54AM +0530, 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. > >> > >>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 | 2 + > >> drivers/gpu/drm/i915/i915_irq.c | 100 ++++++++++++++++++++++++++++- > >> 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 | 1 + > >> 7 files changed, 120 insertions(+), 3 deletions(-) > >> > >>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >>index e4c8e34..7aae033 100644 > >>--- a/drivers/gpu/drm/i915/i915_drv.h > >>+++ b/drivers/gpu/drm/i915/i915_drv.h > >>@@ -1790,6 +1790,7 @@ struct drm_i915_private { > >> u32 gt_irq_mask; > >> u32 pm_irq_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 da7c242..c2f3a67 100644 > >>--- a/drivers/gpu/drm/i915/i915_guc_submission.c > >>+++ b/drivers/gpu/drm/i915/i915_guc_submission.c > >>@@ -990,6 +990,8 @@ int intel_guc_suspend(struct drm_device *dev) > >> if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS) > >> return 0; > >> > >>+ gen8_disable_guc_interrupts(dev); > >>+ > >> ctx = dev_priv->kernel_context; > >> > >> data[0] = HOST2GUC_ACTION_ENTER_S_STATE; > >>diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > >>index caaf1e2..b4294a8 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 gen8_guc_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir); > >> > >> /* For display hotplug interrupt */ > >> static inline void > >>@@ -400,6 +401,55 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv) > >> synchronize_irq(dev_priv->dev->irq); > >> } > >> > >>+void gen8_reset_guc_interrupts(struct drm_device *dev) > >>+{ > >>+ struct drm_i915_private *dev_priv = dev->dev_private; > >>+ i915_reg_t reg = gen6_pm_iir(dev_priv); > > > >From the looks of this we have multiple shadows for the same register. > >That's very bad. > > > >Now the platforms might be mutually exclusive, but it is still a mistake > >that will catch us out. > > > Will check how it is in newer platforms. > > >>+ spin_lock_irq(&dev_priv->irq_lock); > >>+ I915_WRITE(reg, dev_priv->guc_events); > >>+ I915_WRITE(reg, dev_priv->guc_events); > > > >What? Not even the tiniest of comments to explain? > > > Sorry actually just copied these steps as is from the > gen6_reset_rps_interrupts(), considering that the same set of > registers (IIR, IER, IMR) are involved here. > So the double clearing of IIR followed by posting read could be > needed here also. Move it all to i915_irq.c and export routines to manipulate pm_iir such that multiple users do not conflict. > >>+ POSTING_READ(reg); > > > >Again. Not even the tiniest of comments to explain? > > > >>+ spin_unlock_irq(&dev_priv->irq_lock); > >>+} > >>+ > >>+void gen8_enable_guc_interrupts(struct drm_device *dev) > >>+{ > >>+ struct drm_i915_private *dev_priv = dev->dev_private; > >>+ > >>+ 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; > >>+ I915_WRITE(gen6_pm_ier(dev_priv), > >>+ I915_READ(gen6_pm_ier(dev_priv)) | dev_priv->guc_events); > > > >ier should be known, rmw on the reg should not be required. > > > Sorry same as above, copy paste from gen6_enable_rps_interrupts(). > Without rmw, would this be fine ? > > if (dev_priv->rps.interrupts_enabled) > I915_WRITE(gen6_pm_ier(dev_priv), > dev_priv->pm_rps_events | dev_priv->guc_events); > else > I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->guc_events); Still has the presumption of owning a register that is ostensibly used by others. > >>+static void gen8_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; > >>+ } > >>+ > >>+ DISABLE_RPM_WAKEREF_ASSERTS(dev_priv); > > > >This just shouts that the code is broken. > > > You mean to say that ideally the wakeref_count (& power.usage_count) > should already be non zero here. Yes. If it is not under your control, then you have a bug in your code. Existing DISABLE_RPM_WAKEREF_ASSERTS tell us where we know we have a bug (and hacks in place whilst we wait for patch review). > >> 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 41601c7..e20792d 100644 > >>--- a/drivers/gpu/drm/i915/intel_guc.h > >>+++ b/drivers/gpu/drm/i915/intel_guc.h > >>@@ -125,6 +125,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; > > > >The work gets added here, yet bugs are fixed for the worker in later > >patches. Squash in the bug fixes. > > Sorry didn't get this. Are you alluding to cancellation of this > work_item Or flushing of work item from the error handling path ? > > Cancellation is being done as a part of disabling interrupt and the > call to disable interrupt is there in intel_guc_suspend(), part of > this patch only. You add a flush later, which seems unrelated to that patch. -Chris
On 5/28/2016 5:43 PM, Chris Wilson wrote: > On Sat, May 28, 2016 at 02:52:16PM +0530, Goel, Akash wrote: >> >> >> On 5/28/2016 1:26 AM, Chris Wilson wrote: >>> On Sat, May 28, 2016 at 01:12:54AM +0530, 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. >>>> >>>> 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 | 2 + >>>> drivers/gpu/drm/i915/i915_irq.c | 100 ++++++++++++++++++++++++++++- >>>> 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 | 1 + >>>> 7 files changed, 120 insertions(+), 3 deletions(-) >>>>>>>> >>>> static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir); >>>> +static void gen8_guc_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir); >>>> >>>> /* For display hotplug interrupt */ >>>> static inline void >>>> @@ -400,6 +401,55 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv) >>>> synchronize_irq(dev_priv->dev->irq); >>>> } >>>> >>>> +void gen8_reset_guc_interrupts(struct drm_device *dev) >>>> +{ >>>> + struct drm_i915_private *dev_priv = dev->dev_private; >>>> + i915_reg_t reg = gen6_pm_iir(dev_priv); >>> >> >From the looks of this we have multiple shadows for the same register. >>> That's very bad. >>> >>> Now the platforms might be mutually exclusive, but it is still a mistake >>> that will catch us out. >>> >> Will check how it is in newer platforms. >> >>>> + spin_lock_irq(&dev_priv->irq_lock); >>>> + I915_WRITE(reg, dev_priv->guc_events); >>>> + I915_WRITE(reg, dev_priv->guc_events); >>> >>> What? Not even the tiniest of comments to explain? >>> >> Sorry actually just copied these steps as is from the >> gen6_reset_rps_interrupts(), considering that the same set of >> registers (IIR, IER, IMR) are involved here. >> So the double clearing of IIR followed by posting read could be >> needed here also. > > Move it all to i915_irq.c and export routines to manipulate pm_iir such > that multiple users do not conflict. > Sorry but all interrupt related stuff for rps & GuC is already inside i915_irq.c file. Also the IER, IMR, IIR registers are being updated in a non conflicting manner, no overlap between the PM bits & GuC events bits. You mean to say need to have single set of routines only for interrupt reset/enable/disable operations for rps & GuC. >>>> + POSTING_READ(reg); >>> >>> Again. Not even the tiniest of comments to explain? >>> >>>> + spin_unlock_irq(&dev_priv->irq_lock); >>>> +} >>>> + >>>> +void gen8_enable_guc_interrupts(struct drm_device *dev) >>>> +{ >>>> + struct drm_i915_private *dev_priv = dev->dev_private; >>>> + >>>> + 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; >>>> + I915_WRITE(gen6_pm_ier(dev_priv), >>>> + I915_READ(gen6_pm_ier(dev_priv)) | dev_priv->guc_events); >>> >>> ier should be known, rmw on the reg should not be required. >>> >> Sorry same as above, copy paste from gen6_enable_rps_interrupts(). >> Without rmw, would this be fine ? >> >> if (dev_priv->rps.interrupts_enabled) >> I915_WRITE(gen6_pm_ier(dev_priv), >> dev_priv->pm_rps_events | dev_priv->guc_events); >> else >> I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->guc_events); > > Still has the presumption of owning a register that is ostensibly used > by others. > Since pm_ier is a shared register and being used by others also, rmw seem to be more suited here. Otherwise need to be aware of who all is sharing it so as to update it without disturbing the bits owned by others. >>>> +static void gen8_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; >>>> + } >>>> + >>>> + DISABLE_RPM_WAKEREF_ASSERTS(dev_priv); >>> >>> This just shouts that the code is broken. >>> >> You mean to say that ideally the wakeref_count (& power.usage_count) >> should already be non zero here. > > Yes. If it is not under your control, then you have a bug in your code. > Existing DISABLE_RPM_WAKEREF_ASSERTS tell us where we know we have a bug > (and hacks in place whilst we wait for patch review). > This work item can also execute in a window where wakeref_count (& power.usage_count) have become zero but runtime suspend has not yet kicked in (due to auto-suspend delay), so "RPM wakelock ref not held during HW access" warning would come. >>>> 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 41601c7..e20792d 100644 >>>> --- a/drivers/gpu/drm/i915/intel_guc.h >>>> +++ b/drivers/gpu/drm/i915/intel_guc.h >>>> @@ -125,6 +125,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; >>> >>> The work gets added here, yet bugs are fixed for the worker in later >>> patches. Squash in the bug fixes. >> >> Sorry didn't get this. Are you alluding to cancellation of this >> work_item Or flushing of work item from the error handling path ? >> >> Cancellation is being done as a part of disabling interrupt and the >> call to disable interrupt is there in intel_guc_suspend(), part of >> this patch only. > > You add a flush later, which seems unrelated to that patch. The flush of work item is there in the last patch, inside the error handling path for forcefully getting a new flush interrupt from GuC and so that is not tightly coupled with this patch. Best regards Akash > -Chris >
On Sat, May 28, 2016 at 07:15:52PM +0530, Goel, Akash wrote: > > > On 5/28/2016 5:43 PM, Chris Wilson wrote: > >On Sat, May 28, 2016 at 02:52:16PM +0530, Goel, Akash wrote: > >> > >> > >>On 5/28/2016 1:26 AM, Chris Wilson wrote: > >>>On Sat, May 28, 2016 at 01:12:54AM +0530, 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. > >>>> > >>>>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 | 2 + > >>>>drivers/gpu/drm/i915/i915_irq.c | 100 ++++++++++++++++++++++++++++- > >>>>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 | 1 + > >>>>7 files changed, 120 insertions(+), 3 deletions(-) > >>>>>>>> > >>>>static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir); > >>>>+static void gen8_guc_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir); > >>>> > >>>>/* For display hotplug interrupt */ > >>>>static inline void > >>>>@@ -400,6 +401,55 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv) > >>>> synchronize_irq(dev_priv->dev->irq); > >>>>} > >>>> > >>>>+void gen8_reset_guc_interrupts(struct drm_device *dev) > >>>>+{ > >>>>+ struct drm_i915_private *dev_priv = dev->dev_private; > >>>>+ i915_reg_t reg = gen6_pm_iir(dev_priv); > >>> > >>>From the looks of this we have multiple shadows for the same register. > >>>That's very bad. > >>> > >>>Now the platforms might be mutually exclusive, but it is still a mistake > >>>that will catch us out. > >>> > >>Will check how it is in newer platforms. > >> > >>>>+ spin_lock_irq(&dev_priv->irq_lock); > >>>>+ I915_WRITE(reg, dev_priv->guc_events); > >>>>+ I915_WRITE(reg, dev_priv->guc_events); > >>> > >>>What? Not even the tiniest of comments to explain? > >>> > >>Sorry actually just copied these steps as is from the > >>gen6_reset_rps_interrupts(), considering that the same set of > >>registers (IIR, IER, IMR) are involved here. > >>So the double clearing of IIR followed by posting read could be > >>needed here also. > > > >Move it all to i915_irq.c and export routines to manipulate pm_iir such > >that multiple users do not conflict. > > > Sorry but all interrupt related stuff for rps & GuC is already > inside i915_irq.c file. Didn't notice, because this code didn't match my expectations for an interface exported from i915_irq.c > Also the IER, IMR, IIR registers are being updated in a non > conflicting manner, no overlap between the PM bits & GuC events > bits. They share a register, that mandates arbitration. > You mean to say need to have single set of routines only for interrupt > reset/enable/disable operations for rps & GuC. Yes. > >>>>+ POSTING_READ(reg); > >>> > >>>Again. Not even the tiniest of comments to explain? > >>> > >>>>+ spin_unlock_irq(&dev_priv->irq_lock); > >>>>+} > >>>>+ > >>>>+void gen8_enable_guc_interrupts(struct drm_device *dev) > >>>>+{ > >>>>+ struct drm_i915_private *dev_priv = dev->dev_private; > >>>>+ > >>>>+ 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; > >>>>+ I915_WRITE(gen6_pm_ier(dev_priv), > >>>>+ I915_READ(gen6_pm_ier(dev_priv)) | dev_priv->guc_events); > >>> > >>>ier should be known, rmw on the reg should not be required. > >>> > >>Sorry same as above, copy paste from gen6_enable_rps_interrupts(). > >>Without rmw, would this be fine ? > >> > >> if (dev_priv->rps.interrupts_enabled) > >> I915_WRITE(gen6_pm_ier(dev_priv), > >> dev_priv->pm_rps_events | dev_priv->guc_events); > >> else > >> I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->guc_events); > > > >Still has the presumption of owning a register that is ostensibly used > >by others. > > > Since pm_ier is a shared register and being used by others also, rmw > seem to be more suited here. Otherwise need to be aware of who all is > sharing it so as to update it without disturbing the bits owned by > others. Exactly, see above. The best interfaces from i915_irq.c do not use rmw on the register values. > >>>>+static void gen8_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; > >>>>+ } > >>>>+ > >>>>+ DISABLE_RPM_WAKEREF_ASSERTS(dev_priv); > >>> > >>>This just shouts that the code is broken. > >>> > >>You mean to say that ideally the wakeref_count (& power.usage_count) > >>should already be non zero here. > > > >Yes. If it is not under your control, then you have a bug in your code. > >Existing DISABLE_RPM_WAKEREF_ASSERTS tell us where we know we have a bug > >(and hacks in place whilst we wait for patch review). > > > > This work item can also execute in a window where wakeref_count (& > power.usage_count) have become zero but runtime suspend has not yet > kicked in (due to auto-suspend delay), so "RPM wakelock ref not held > during HW access" warning would come. i.e. your code is buggy, as DISABLE_RPM_WAKEREF_ASSERTS implied. > >>>>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 41601c7..e20792d 100644 > >>>>--- a/drivers/gpu/drm/i915/intel_guc.h > >>>>+++ b/drivers/gpu/drm/i915/intel_guc.h > >>>>@@ -125,6 +125,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; > >>> > >>>The work gets added here, yet bugs are fixed for the worker in later > >>>patches. Squash in the bug fixes. > >> > >>Sorry didn't get this. Are you alluding to cancellation of this > >>work_item Or flushing of work item from the error handling path ? > >> > >>Cancellation is being done as a part of disabling interrupt and the > >>call to disable interrupt is there in intel_guc_suspend(), part of > >>this patch only. > > > >You add a flush later, which seems unrelated to that patch. > > The flush of work item is there in the last patch, inside the error > handling path for forcefully getting a new flush interrupt from GuC > and so that is not tightly coupled with this patch. Reseting the work would seem be part of the log infrastructure. -Chris
On 5/28/2016 8:05 PM, Chris Wilson wrote: > On Sat, May 28, 2016 at 07:15:52PM +0530, Goel, Akash wrote: >> >> >> On 5/28/2016 5:43 PM, Chris Wilson wrote: >>> On Sat, May 28, 2016 at 02:52:16PM +0530, Goel, Akash wrote: >>>> >>>> >>>> On 5/28/2016 1:26 AM, Chris Wilson wrote: >>>>> On Sat, May 28, 2016 at 01:12:54AM +0530, akash.goel@intel.com wrote: >>>>>> +void gen8_reset_guc_interrupts(struct drm_device *dev) >>>>>> +{ >>>>>> + struct drm_i915_private *dev_priv = dev->dev_private; >>>>>> + i915_reg_t reg = gen6_pm_iir(dev_priv); >>>>> >>>> >From the looks of this we have multiple shadows for the same register. >>>>> That's very bad. >>>>> >>>>> Now the platforms might be mutually exclusive, but it is still a mistake >>>>> that will catch us out. >>>>> >>>> Will check how it is in newer platforms. >>>> >>>>>> + spin_lock_irq(&dev_priv->irq_lock); >>>>>> + I915_WRITE(reg, dev_priv->guc_events); >>>>>> + I915_WRITE(reg, dev_priv->guc_events); >>>>> >>>>> What? Not even the tiniest of comments to explain? >>>>> >>>> Sorry actually just copied these steps as is from the >>>> gen6_reset_rps_interrupts(), considering that the same set of >>>> registers (IIR, IER, IMR) are involved here. >>>> So the double clearing of IIR followed by posting read could be >>>> needed here also. >>> >>> Move it all to i915_irq.c and export routines to manipulate pm_iir such >>> that multiple users do not conflict. >>> >> Sorry but all interrupt related stuff for rps & GuC is already >> inside i915_irq.c file. > > Didn't notice, because this code didn't match my expectations for an > interface exported from i915_irq.c > >> Also the IER, IMR, IIR registers are being updated in a non >> conflicting manner, no overlap between the PM bits & GuC events >> bits. > > They share a register, that mandates arbitration. > I think the arbitration (& serialization) is already being provided by irq_lock. >> You mean to say need to have single set of routines only for interrupt >> reset/enable/disable operations for rps & GuC. > > Yes. > Fine will make them to use a single set of low level routines. >>>>>> + POSTING_READ(reg); >>>>> >>>>> Again. Not even the tiniest of comments to explain? >>>>> >>>>>> + spin_unlock_irq(&dev_priv->irq_lock); >>>>>> +} >>>>>> + >>>>>> +void gen8_enable_guc_interrupts(struct drm_device *dev) >>>>>> +{ >>>>>> + struct drm_i915_private *dev_priv = dev->dev_private; >>>>>> + >>>>>> + 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; >>>>>> + I915_WRITE(gen6_pm_ier(dev_priv), >>>>>> + I915_READ(gen6_pm_ier(dev_priv)) | dev_priv->guc_events); >>>>> >>>>> ier should be known, rmw on the reg should not be required. >>>>> >>>> Sorry same as above, copy paste from gen6_enable_rps_interrupts(). >>>> Without rmw, would this be fine ? >>>> >>>> if (dev_priv->rps.interrupts_enabled) >>>> I915_WRITE(gen6_pm_ier(dev_priv), >>>> dev_priv->pm_rps_events | dev_priv->guc_events); >>>> else >>>> I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->guc_events); >>> >>> Still has the presumption of owning a register that is ostensibly used >>> by others. >>> >> Since pm_ier is a shared register and being used by others also, rmw >> seem to be more suited here. Otherwise need to be aware of who all is >> sharing it so as to update it without disturbing the bits owned by >> others. > > Exactly, see above. The best interfaces from i915_irq.c do not use rmw > on the register values. Fine will try to do away with use rmw operation for pm_ier by maintaining a bit mask of enabled interrupts (just like pm_irq_mask). > >>>>>> +static void gen8_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; >>>>>> + } >>>>>> + >>>>>> + DISABLE_RPM_WAKEREF_ASSERTS(dev_priv); >>>>> >>>>> This just shouts that the code is broken. >>>>> >>>> You mean to say that ideally the wakeref_count (& power.usage_count) >>>> should already be non zero here. >>> >>> Yes. If it is not under your control, then you have a bug in your code. >>> Existing DISABLE_RPM_WAKEREF_ASSERTS tell us where we know we have a bug >>> (and hacks in place whilst we wait for patch review). >>> >> >> This work item can also execute in a window where wakeref_count (& >> power.usage_count) have become zero but runtime suspend has not yet >> kicked in (due to auto-suspend delay), so "RPM wakelock ref not held >> during HW access" warning would come. > > i.e. your code is buggy, as DISABLE_RPM_WAKEREF_ASSERTS implied. > But isn't this applicable to rps work item also ?. If there is a way found to circumvent this, then same can be applied to GuC work item also. DISABLE_RPM_WAKEREF_ASSERTS is a stopgap solution. >>>>>> 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 41601c7..e20792d 100644 >>>>>> --- a/drivers/gpu/drm/i915/intel_guc.h >>>>>> +++ b/drivers/gpu/drm/i915/intel_guc.h >>>>>> @@ -125,6 +125,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; >>>>> >>>>> The work gets added here, yet bugs are fixed for the worker in later >>>>> patches. Squash in the bug fixes. >>>> >>>> Sorry didn't get this. Are you alluding to cancellation of this >>>> work_item Or flushing of work item from the error handling path ? >>>> >>>> Cancellation is being done as a part of disabling interrupt and the >>>> call to disable interrupt is there in intel_guc_suspend(), part of >>>> this patch only. >>> >>> You add a flush later, which seems unrelated to that patch. >> >> The flush of work item is there in the last patch, inside the error >> handling path for forcefully getting a new flush interrupt from GuC >> and so that is not tightly coupled with this patch. > > Reseting the work would seem be part of the log infrastructure. Fine will add the work item reset part in this patch only. Best regards Akash > -Chris >
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index e4c8e34..7aae033 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1790,6 +1790,7 @@ struct drm_i915_private { u32 gt_irq_mask; u32 pm_irq_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 da7c242..c2f3a67 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -990,6 +990,8 @@ int intel_guc_suspend(struct drm_device *dev) if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS) return 0; + gen8_disable_guc_interrupts(dev); + ctx = dev_priv->kernel_context; data[0] = HOST2GUC_ACTION_ENTER_S_STATE; diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index caaf1e2..b4294a8 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 gen8_guc_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir); /* For display hotplug interrupt */ static inline void @@ -400,6 +401,55 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv) synchronize_irq(dev_priv->dev->irq); } +void gen8_reset_guc_interrupts(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + i915_reg_t reg = gen6_pm_iir(dev_priv); + + spin_lock_irq(&dev_priv->irq_lock); + I915_WRITE(reg, dev_priv->guc_events); + I915_WRITE(reg, dev_priv->guc_events); + POSTING_READ(reg); + spin_unlock_irq(&dev_priv->irq_lock); +} + +void gen8_enable_guc_interrupts(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + + 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; + I915_WRITE(gen6_pm_ier(dev_priv), + I915_READ(gen6_pm_ier(dev_priv)) | dev_priv->guc_events); + gen6_enable_pm_irq(dev_priv, dev_priv->guc_events); + } + spin_unlock_irq(&dev_priv->irq_lock); +} + +void gen8_disable_guc_interrupts(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + + 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_irq(dev_priv, dev_priv->guc_events); + I915_WRITE(gen6_pm_ier(dev_priv), I915_READ(gen6_pm_ier(dev_priv)) & + ~dev_priv->guc_events); + + spin_unlock_irq(&dev_priv->irq_lock); + + synchronize_irq(dev->irq); +} + /** * bdw_update_port_irq - update DE port interrupt * @dev_priv: driver private @@ -1174,6 +1224,27 @@ out: ENABLE_RPM_WAKEREF_ASSERTS(dev_priv); } +static void gen8_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; + } + + DISABLE_RPM_WAKEREF_ASSERTS(dev_priv); + + gen6_enable_pm_irq(dev_priv, GEN8_GUC_TO_HOST_INT_EVENT); + spin_unlock_irq(&dev_priv->irq_lock); + + /* TODO: Handle the events for which GuC interrupted host */ + + ENABLE_RPM_WAKEREF_ASSERTS(dev_priv); +} /** * ivybridge_parity_work - Workqueue called when a parity error interrupt @@ -1349,11 +1420,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"); @@ -1385,6 +1458,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) + gen8_guc_irq_handler(dev_priv, gt_iir[2]); } static bool bxt_port_hotplug_long_detect(enum port port, u32 val) @@ -1631,6 +1707,20 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir) } } +static void gen8_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir) +{ + if (gt_iir & GEN8_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, + GEN8_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) { @@ -4570,6 +4660,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, gen8_guc2host_events_work); + + if (HAS_GUC_UCODE(dev)) + dev_priv->guc_events = GEN8_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 e307725..54e1477 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5922,6 +5922,7 @@ enum skl_disp_power_wells { #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) @@ -5933,6 +5934,16 @@ enum skl_disp_power_wells { #define GEN8_GT_IIR(which) _MMIO(0x44308 + (0x10 * (which))) #define GEN8_GT_IER(which) _MMIO(0x4430c + (0x10 * (which))) +#define GEN8_GUC_TO_HOST_INT_EVENT (1<<31) +#define GEN8_GUC_EXEC_ERROR_EVENT (1<<30) +#define GEN8_GUC_DISPLAY_EVENT (1<<29) +#define GEN8_GUC_SEMA_SIGNAL_EVENT (1<<28) +#define GEN8_GUC_IOMMU_MSG_EVENT (1<<27) +#define GEN8_GUC_DB_RING_EVENT (1<<26) +#define GEN8_GUC_DMA_DONE_EVENT (1<<25) +#define GEN8_GUC_FATAL_ERROR_EVENT (1<<24) +#define GEN8_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 9b5f663..f6dd3370 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1075,6 +1075,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 gen8_reset_guc_interrupts(struct drm_device *dev); +void gen8_enable_guc_interrupts(struct drm_device *dev); +void gen8_disable_guc_interrupts(struct drm_device *dev); /* 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 41601c7..e20792d 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -125,6 +125,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 fdaeed8..9c2eb79 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -433,6 +433,7 @@ int intel_guc_setup(struct drm_device *dev) } direct_interrupts_to_host(dev_priv); + gen8_reset_guc_interrupts(dev); guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;