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.
+ 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx