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
