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

[Goel, Akash]

On 6/28/2016 2:05 PM, Tvrtko Ursulin wrote:
>
> 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.
Fine, so will rename gen6_enable_pm_interrupts to gen6_enable_pm_irq,
and gen6_enable_pm_irq to gen6_unmask_pm_irq

Best regards
Akash
>
> Regards,
>
> Tvrtko