Re: [PATCH 03/11] drm/i915: Add low level set of routines for programming PM IER/IIR/IMR register set

["Goel, Akash" <akash.goel@intel.com>]

On 6/27/2016 9:16 PM, Tvrtko Ursulin wrote:
>
> On 27/06/16 13:16, akash.goel@intel.com wrote:
>> From: Akash Goel <akash.goel@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.
>>
>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Akash Goel <akash.goel@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h  |  1 +
>>   drivers/gpu/drm/i915/i915_irq.c  | 55
>> ++++++++++++++++++++++++++++++++--------
>>   drivers/gpu/drm/i915/intel_drv.h |  6 +++++
>>   3 files changed, 52 insertions(+), 10 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..7316ab4 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -336,14 +336,52 @@ void gen6_disable_pm_irq(struct drm_i915_private
>> *dev_priv, uint32_t mask)
>>       __gen6_disable_pm_irq(dev_priv, mask);
>>   }
>>
>> -void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv)
>> +void gen6_reset_pm_interrupts(struct drm_i915_private *dev_priv,
>> +                   uint32_t reset_mask)
>
> Kernel prefers u32. It is not that overall i915 is clean in that
> respect, but every time maintainers merge patches checkpatch shouts
> about it, and more noise tougher it is to spot more important issues. I
> would appreciate if u32 was used throughout.

Fine, will use u32.

>
>>   {
>>       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_interrupts(struct drm_i915_private *dev_priv,
>> +                   uint32_t enable_mask)
>> +{
>> +    uint32_t 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_enable_pm_irq(dev_priv, enable_mask);
>
> Hm, will this be confusing that we will have gen6_enable_pm_interrupts
> and gen6_enable_pm_irq, so extremely similar names and same parameters,
> but for different use?
Sorry for using confusing, ambiguous names.
>
> Maybe rename the old one to gen6_unmask_pm_irq and name this one
> gen6_enable_pm_irq ? If there is really need to have both. Or add some
> kerneldoc explaining which one is used for what?

Can I do like this, keep gen6_enable_pm_interrupts as is and rename 
gen6_enable_pm_irq to gen6_unmask_pm_irq.
Similarly also rename gen6_disable_pm_irq to gen6_mask_pm_irq.

Best regards
Akash
>
>> +}
>> +
>> +void gen6_disable_pm_interrupts(struct drm_i915_private *dev_priv,
>> +                uint32_t disable_mask)
>> +{
>> +    uint32_t 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_disable_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_interrupts(dev_priv, dev_priv->pm_rps_events);
>>       dev_priv->rps.pm_iir = 0;
>>       spin_unlock_irq(&dev_priv->irq_lock);
>>   }
>> @@ -355,9 +393,7 @@ 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);
>> +    gen6_enable_pm_interrupts(dev_priv, dev_priv->pm_rps_events);
>>
>>       spin_unlock_irq(&dev_priv->irq_lock);
>>   }
>> @@ -379,9 +415,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_priv), I915_READ(gen6_pm_ier(dev_priv)) &
>> -                ~dev_priv->pm_rps_events);
>> +    gen6_disable_pm_interrupts(dev_priv, dev_priv->pm_rps_events);
>>
>>       spin_unlock_irq(&dev_priv->irq_lock);
>>
>> @@ -3770,6 +3804,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;
>>       GEN8_IRQ_INIT_NDX(GT, 0, ~gt_interrupts[0], gt_interrupts[0]);
>>       GEN8_IRQ_INIT_NDX(GT, 1, ~gt_interrupts[1], gt_interrupts[1]);
>>       /*
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 3156d8d..2a013fc 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1060,6 +1060,12 @@ void gen6_disable_pm_irq(struct
>> drm_i915_private *dev_priv, uint32_t mask);
>>   void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv);
>>   void gen6_enable_rps_interrupts(struct drm_i915_private *dev_priv);
>>   void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv);
>> +void gen6_reset_pm_interrupts(struct drm_i915_private *dev_priv,
>> +                  uint32_t reset_mask);
>> +void gen6_enable_pm_interrupts(struct drm_i915_private *dev_priv,
>> +                   uint32_t enable_mask);
>> +void gen6_disable_pm_interrupts(struct drm_i915_private *dev_priv,
>> +                uint32_t disable_mask);
>>   u32 gen6_sanitize_rps_pm_mask(struct drm_i915_private *dev_priv, u32
>> mask);
>>   void intel_runtime_pm_disable_interrupts(struct drm_i915_private
>> *dev_priv);
>>   void intel_runtime_pm_enable_interrupts(struct drm_i915_private
>> *dev_priv);
>>
>
> Regards,
>
> Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx