Re: [RFC 03/12] drm/i915: Support for GuC interrupts

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

 



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@xxxxxxxxx wrote:
> >>From: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
> >>
> >>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@xxxxxxxxx>
> >>Signed-off-by: Akash Goel <akash.goel@xxxxxxxxx>
> >>---
> >> 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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux