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