[Intel-gfx] [PATCH 03/14] drm/i915: Add low level set of routines for programming PM IER/IIR/IMR register set
Goel, Akash
akash.goel at intel.com
Sun Jul 3 13:07:10 UTC 2016
On 7/3/2016 3:08 PM, Chris Wilson wrote:
> On Sun, Jul 03, 2016 at 12:21:20AM +0530, akash.goel at intel.com wrote:
>> From: Akash Goel <akash.goel at intel.com>
>>
>> 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 <chris at chris-wilson.co.uk>
>> Signed-off-by: Akash Goel <akash.goel at intel.com>
>> ---
>> 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_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.
>
>> spin_unlock_irq(&dev_priv->irq_lock);
>> }
>> @@ -1599,7 +1629,7 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
>> {
>> if (pm_iir & dev_priv->pm_rps_events) {
>> spin_lock(&dev_priv->irq_lock);
>> - gen6_disable_pm_irq(dev_priv, pm_iir & dev_priv->pm_rps_events);
>> + gen6_mask_pm_irq(dev_priv, pm_iir & dev_priv->pm_rps_events);
>> if (dev_priv->rps.interrupts_enabled) {
>> dev_priv->rps.pm_iir |= pm_iir & dev_priv->pm_rps_events;
>> queue_work(dev_priv->wq, &dev_priv->rps.work);
>> @@ -3770,6 +3800,7 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
>> gt_interrupts[0] |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
>>
>> dev_priv->pm_irq_mask = 0xffffffff;
>> + dev_priv->pm_ier_mask = 0x0;
>
> dev_priv->pm_ier = 0;
> dev_priv->pm_imr = ~dev_priv->pm_ier;
>
> Make the initial relationship explicit.
Thanks, will modify.
>
>> GEN8_IRQ_INIT_NDX(GT, 0, ~gt_interrupts[0], gt_interrupts[0]);
>> GEN8_IRQ_INIT_NDX(GT, 1, ~gt_interrupts[1], gt_interrupts[1]);
>
> Missed changing
> GEN8_IRQ_INIT_NDX(GT, 2, dev_priv->pm_imr, dev_priv->pm_ier);
Thanks, will modify.
Best regards
Akash
> -Chris
>
More information about the Intel-gfx
mailing list