Re: [Intel-gfx] [PATCH 03/14] drm/i915: Add low level set of routines for programming PM IER/IIR/IMR register set
On 7/3/2016 3:08 PM, Chris Wilson wrote:
On Sun, Jul 03, 2016 at 12:21:20AM +0530, akash.g...@intel.com wrote:
From: Akash Goel
So far PM IER/IIR/IMR registers were being used only for Turbo related
interrupts. But interrupts coming from GuC also use the same set.
As a precursor to supporting GuC interrupts, added new low level routines
so as to allow sharing the programming of PM IER/IIR/IMR registers between
Turbo & GuC.
Also similar to PM IMR, maintaining a bitmask for PM IER register, to allow
easy sharing of it between Turbo & GuC without involving a rmw operation.
v2:
- For appropriateness & avoid any ambiguity, rename old functions
enable/disable pm_irq to mask/unmask pm_irq and rename new functions
enable/disable pm_interrupts to enable/disable pm_irq. (Tvrtko)
- Use u32 in place of uint32_t. (Tvrtko)
Suggested-by: Chris Wilson
Signed-off-by: Akash Goel
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/i915_irq.c | 63 -
drivers/gpu/drm/i915/intel_drv.h| 3 ++
drivers/gpu/drm/i915/intel_ringbuffer.c | 4 +--
4 files changed, 53 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9ef4919..85a7103 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1806,6 +1806,7 @@ struct drm_i915_private {
};
u32 gt_irq_mask;
u32 pm_irq_mask;
+ u32 pm_ier_mask;
Oops. u32 pm_imr; and u32 pm_ier;
Fine, will rename.
u32 pm_rps_events;
u32 pipestat_irq_mask[I915_MAX_PIPES];
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4378a65..dd5ae6d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -314,7 +314,7 @@ static void snb_update_pm_irq(struct drm_i915_private
*dev_priv,
}
}
-void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
+void gen6_unmask_pm_irq(struct drm_i915_private *dev_priv, u32 mask)
{
if (WARN_ON(!intel_irqs_enabled(dev_priv)))
return;
@@ -322,28 +322,62 @@ void gen6_enable_pm_irq(struct drm_i915_private
*dev_priv, uint32_t mask)
snb_update_pm_irq(dev_priv, mask, mask);
}
-static void __gen6_disable_pm_irq(struct drm_i915_private *dev_priv,
- uint32_t mask)
+static void __gen6_mask_pm_irq(struct drm_i915_private *dev_priv, u32 mask)
{
snb_update_pm_irq(dev_priv, mask, 0);
}
-void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
+void gen6_mask_pm_irq(struct drm_i915_private *dev_priv, u32 mask)
{
if (WARN_ON(!intel_irqs_enabled(dev_priv)))
return;
- __gen6_disable_pm_irq(dev_priv, mask);
+ __gen6_mask_pm_irq(dev_priv, mask);
}
-void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv)
+void gen6_reset_pm_irq(struct drm_i915_private *dev_priv, u32 reset_mask)
reset_pm_iir
Thanks, will update.
{
i915_reg_t reg = gen6_pm_iir(dev_priv);
- spin_lock_irq(&dev_priv->irq_lock);
- I915_WRITE(reg, dev_priv->pm_rps_events);
- I915_WRITE(reg, dev_priv->pm_rps_events);
+ assert_spin_locked(&dev_priv->irq_lock);
+
+ I915_WRITE(reg, reset_mask);
+ I915_WRITE(reg, reset_mask);
POSTING_READ(reg);
+}
+
+void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, u32 enable_mask)
+{
+ u32 new_val;
+
+ assert_spin_locked(&dev_priv->irq_lock);
+
+ new_val = dev_priv->pm_ier_mask;
+ new_val |= enable_mask;
+
+ dev_priv->pm_ier_mask = new_val;
dev_priv->pm_ier |= new_val;
Sorry, my bad.
+ I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier_mask);
+ gen6_unmask_pm_irq(dev_priv, enable_mask);
What barrier do you need between the hw and the caller? I presume there
is a POSTING_READ in this callchain, would be nice to document it.
/* unmask_pm_irq provides a POSTING_READ */
Thanks, will add the comment.
So will assume that POSTING_READ is good enough here.
+}
+
+void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, u32 disable_mask)
+{
+ u32 new_val;
+
+ assert_spin_locked(&dev_priv->irq_lock);
+
+ new_val = dev_priv->pm_ier_mask;
+ new_val &= ~disable_mask;
+
+ dev_priv->pm_ier_mask = new_val;
dev_priv->pm_ier &= ~disable_mask;
+ __gen6_mask_pm_irq(dev_priv, disable_mask);
+ I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier_mask);
Do we need a barrier upon disabling? (Usually we need a stronger barrier
on enabling to ensure we don't miss an interrupt when enabling, but for
disabling we don't care.)
So no modification needed here, as you mentioned that we don't need to
care about the register update getting completed in the disabling case.
+}
+
+void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv)
+{
+ spin_lock_irq(&dev_priv->irq_lock);
+ gen6_reset_pm_i
Re: [Intel-gfx] [PATCH 03/14] drm/i915: Add low level set of routines for programming PM IER/IIR/IMR register set
On Sun, Jul 03, 2016 at 12:21:20AM +0530, akash.g...@intel.com wrote:
> From: Akash Goel
>
> So far PM IER/IIR/IMR registers were being used only for Turbo related
> interrupts. But interrupts coming from GuC also use the same set.
> As a precursor to supporting GuC interrupts, added new low level routines
> so as to allow sharing the programming of PM IER/IIR/IMR registers between
> Turbo & GuC.
> Also similar to PM IMR, maintaining a bitmask for PM IER register, to allow
> easy sharing of it between Turbo & GuC without involving a rmw operation.
>
> v2:
> - For appropriateness & avoid any ambiguity, rename old functions
> enable/disable pm_irq to mask/unmask pm_irq and rename new functions
> enable/disable pm_interrupts to enable/disable pm_irq. (Tvrtko)
> - Use u32 in place of uint32_t. (Tvrtko)
>
> Suggested-by: Chris Wilson
> Signed-off-by: Akash Goel
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_irq.c | 63
> -
> drivers/gpu/drm/i915/intel_drv.h| 3 ++
> drivers/gpu/drm/i915/intel_ringbuffer.c | 4 +--
> 4 files changed, 53 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9ef4919..85a7103 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1806,6 +1806,7 @@ struct drm_i915_private {
> };
> u32 gt_irq_mask;
> u32 pm_irq_mask;
> + u32 pm_ier_mask;
Oops. u32 pm_imr; and u32 pm_ier;
> u32 pm_rps_events;
> u32 pipestat_irq_mask[I915_MAX_PIPES];
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 4378a65..dd5ae6d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -314,7 +314,7 @@ static void snb_update_pm_irq(struct drm_i915_private
> *dev_priv,
> }
> }
>
> -void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> +void gen6_unmask_pm_irq(struct drm_i915_private *dev_priv, u32 mask)
> {
> if (WARN_ON(!intel_irqs_enabled(dev_priv)))
> return;
> @@ -322,28 +322,62 @@ void gen6_enable_pm_irq(struct drm_i915_private
> *dev_priv, uint32_t mask)
> snb_update_pm_irq(dev_priv, mask, mask);
> }
>
> -static void __gen6_disable_pm_irq(struct drm_i915_private *dev_priv,
> - uint32_t mask)
> +static void __gen6_mask_pm_irq(struct drm_i915_private *dev_priv, u32 mask)
> {
> snb_update_pm_irq(dev_priv, mask, 0);
> }
>
> -void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> +void gen6_mask_pm_irq(struct drm_i915_private *dev_priv, u32 mask)
> {
> if (WARN_ON(!intel_irqs_enabled(dev_priv)))
> return;
>
> - __gen6_disable_pm_irq(dev_priv, mask);
> + __gen6_mask_pm_irq(dev_priv, mask);
> }
>
> -void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv)
> +void gen6_reset_pm_irq(struct drm_i915_private *dev_priv, u32 reset_mask)
reset_pm_iir
> {
> i915_reg_t reg = gen6_pm_iir(dev_priv);
>
> - spin_lock_irq(&dev_priv->irq_lock);
> - I915_WRITE(reg, dev_priv->pm_rps_events);
> - I915_WRITE(reg, dev_priv->pm_rps_events);
> + assert_spin_locked(&dev_priv->irq_lock);
> +
> + I915_WRITE(reg, reset_mask);
> + I915_WRITE(reg, reset_mask);
> POSTING_READ(reg);
> +}
> +
> +void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, u32 enable_mask)
> +{
> + u32 new_val;
> +
> + assert_spin_locked(&dev_priv->irq_lock);
> +
> + new_val = dev_priv->pm_ier_mask;
> + new_val |= enable_mask;
> +
> + dev_priv->pm_ier_mask = new_val;
dev_priv->pm_ier |= new_val;
> + I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier_mask);
> + gen6_unmask_pm_irq(dev_priv, enable_mask);
What barrier do you need between the hw and the caller? I presume there
is a POSTING_READ in this callchain, would be nice to document it.
/* unmask_pm_irq provides a POSTING_READ */
> +}
> +
> +void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, u32 disable_mask)
> +{
> + u32 new_val;
> +
> + assert_spin_locked(&dev_priv->irq_lock);
> +
> + new_val = dev_priv->pm_ier_mask;
> + new_val &= ~disable_mask;
> +
> + dev_priv->pm_ier_mask = new_val;
dev_priv->pm_ier &= ~disable_mask;
> + __gen6_mask_pm_irq(dev_priv, disable_mask);
> + I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier_mask);
Do we need a barrier upon disabling? (Usually we need a stronger barrier
on enabling to ensure we don't miss an interrupt when enabling, but for
disabling we don't care.)
> +}
> +
> +void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv)
> +{
> + spin_lock_irq(&dev_priv->irq_lock);
> + gen6_reset_pm_irq(dev_priv, dev_priv->pm_rps_events);
> dev_priv->rps.pm_iir = 0;
Hmm. That's slightly confusing, but passable since it is really the set
of bits in pm_iir for rps.
>
[Intel-gfx] [PATCH 03/14] drm/i915: Add low level set of routines for programming PM IER/IIR/IMR register set
From: Akash Goel
So far PM IER/IIR/IMR registers were being used only for Turbo related
interrupts. But interrupts coming from GuC also use the same set.
As a precursor to supporting GuC interrupts, added new low level routines
so as to allow sharing the programming of PM IER/IIR/IMR registers between
Turbo & GuC.
Also similar to PM IMR, maintaining a bitmask for PM IER register, to allow
easy sharing of it between Turbo & GuC without involving a rmw operation.
v2:
- For appropriateness & avoid any ambiguity, rename old functions
enable/disable pm_irq to mask/unmask pm_irq and rename new functions
enable/disable pm_interrupts to enable/disable pm_irq. (Tvrtko)
- Use u32 in place of uint32_t. (Tvrtko)
Suggested-by: Chris Wilson
Signed-off-by: Akash Goel
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/i915_irq.c | 63 -
drivers/gpu/drm/i915/intel_drv.h| 3 ++
drivers/gpu/drm/i915/intel_ringbuffer.c | 4 +--
4 files changed, 53 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9ef4919..85a7103 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1806,6 +1806,7 @@ struct drm_i915_private {
};
u32 gt_irq_mask;
u32 pm_irq_mask;
+ u32 pm_ier_mask;
u32 pm_rps_events;
u32 pipestat_irq_mask[I915_MAX_PIPES];
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4378a65..dd5ae6d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -314,7 +314,7 @@ static void snb_update_pm_irq(struct drm_i915_private
*dev_priv,
}
}
-void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
+void gen6_unmask_pm_irq(struct drm_i915_private *dev_priv, u32 mask)
{
if (WARN_ON(!intel_irqs_enabled(dev_priv)))
return;
@@ -322,28 +322,62 @@ void gen6_enable_pm_irq(struct drm_i915_private
*dev_priv, uint32_t mask)
snb_update_pm_irq(dev_priv, mask, mask);
}
-static void __gen6_disable_pm_irq(struct drm_i915_private *dev_priv,
- uint32_t mask)
+static void __gen6_mask_pm_irq(struct drm_i915_private *dev_priv, u32 mask)
{
snb_update_pm_irq(dev_priv, mask, 0);
}
-void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
+void gen6_mask_pm_irq(struct drm_i915_private *dev_priv, u32 mask)
{
if (WARN_ON(!intel_irqs_enabled(dev_priv)))
return;
- __gen6_disable_pm_irq(dev_priv, mask);
+ __gen6_mask_pm_irq(dev_priv, mask);
}
-void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv)
+void gen6_reset_pm_irq(struct drm_i915_private *dev_priv, u32 reset_mask)
{
i915_reg_t reg = gen6_pm_iir(dev_priv);
- spin_lock_irq(&dev_priv->irq_lock);
- I915_WRITE(reg, dev_priv->pm_rps_events);
- I915_WRITE(reg, dev_priv->pm_rps_events);
+ assert_spin_locked(&dev_priv->irq_lock);
+
+ I915_WRITE(reg, reset_mask);
+ I915_WRITE(reg, reset_mask);
POSTING_READ(reg);
+}
+
+void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, u32 enable_mask)
+{
+ u32 new_val;
+
+ assert_spin_locked(&dev_priv->irq_lock);
+
+ new_val = dev_priv->pm_ier_mask;
+ new_val |= enable_mask;
+
+ dev_priv->pm_ier_mask = new_val;
+ I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier_mask);
+ gen6_unmask_pm_irq(dev_priv, enable_mask);
+}
+
+void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, u32 disable_mask)
+{
+ u32 new_val;
+
+ assert_spin_locked(&dev_priv->irq_lock);
+
+ new_val = dev_priv->pm_ier_mask;
+ new_val &= ~disable_mask;
+
+ dev_priv->pm_ier_mask = new_val;
+ __gen6_mask_pm_irq(dev_priv, disable_mask);
+ I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier_mask);
+}
+
+void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv)
+{
+ spin_lock_irq(&dev_priv->irq_lock);
+ gen6_reset_pm_irq(dev_priv, dev_priv->pm_rps_events);
dev_priv->rps.pm_iir = 0;
spin_unlock_irq(&dev_priv->irq_lock);
}
@@ -355,8 +389,6 @@ void gen6_enable_rps_interrupts(struct drm_i915_private
*dev_priv)
WARN_ON(dev_priv->rps.pm_iir);
WARN_ON(I915_READ(gen6_pm_iir(dev_priv)) & dev_priv->pm_rps_events);
dev_priv->rps.interrupts_enabled = true;
- I915_WRITE(gen6_pm_ier(dev_priv), I915_READ(gen6_pm_ier(dev_priv)) |
- dev_priv->pm_rps_events);
gen6_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
spin_unlock_irq(&dev_priv->irq_lock);
@@ -379,9 +411,7 @@ void gen6_disable_rps_interrupts(struct drm_i915_private
*dev_priv)
I915_WRITE(GEN6_PMINTRMSK, gen6_sanitize_rps_pm_mask(dev_priv, ~0));
- __gen6_disable_pm_irq(dev_priv, dev_priv->pm_rps_events);
- I915_WRITE(gen6_pm_ier(dev_p
