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

[Tvrtko Ursulin]

On 27/06/16 17:35, Goel, Akash wrote:
> On 6/27/2016 9:16 PM, Tvrtko Ursulin wrote:
>>
>> On 27/06/16 13:16, 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.
>>>
>>> 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  | 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.

Thanks!

>>>   {
>>>       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.

Yes for mask/unmask, but I think the suffix really needs to be the same 
since it is the same functional family.

Regards,

Tvrtko