[Intel-gfx] [RFC 03/12] drm/i915: Support for GuC interrupts

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre