Re: [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info

[Sagar Arun Kamble <sagar.a.kamble@intel.com>]

On 10/24/2017 4:16 PM, Michal Wajdeczko wrote:
> On Tue, 24 Oct 2017 12:41:13 +0200, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>> PM interrupt register offsets are constant per platforms and saving 
>> those
>> in device info is more appropriate than getting those through functions.
>> This patch removes functions gen6_pm_iir/imr/ier and saves those offsets
>> in device info.
>>
>> v2: Use INTEL_INFO() to access device info. (Chris)
>>
>> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h          |  5 +++++
>>  drivers/gpu/drm/i915/i915_irq.c          | 31 
>> +++++++++----------------------
>>  drivers/gpu/drm/i915/intel_device_info.c | 11 +++++++++++
>>  3 files changed, 25 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 54b5d4c..2f77d26 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -888,6 +888,11 @@ struct intel_device_info {
>>          u16 degamma_lut_size;
>>          u16 gamma_lut_size;
>>      } color;
>> +
>> +    /* PM interrupt register offsets */
>> +    i915_reg_t pm_iir_offset;
>> +    i915_reg_t pm_imr_offset;
>> +    i915_reg_t pm_ier_offset;
>>  };
>> struct intel_display_error_state;
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c 
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index b1296a5..5d448af 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -306,21 +306,6 @@ void gen5_disable_gt_irq(struct drm_i915_private 
>> *dev_priv, uint32_t mask)
>>      ilk_update_gt_irq(dev_priv, mask, 0);
>>  }
>> -static i915_reg_t gen6_pm_iir(struct drm_i915_private *dev_priv)
>> -{
>> -    return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IIR(2) : GEN6_PMIIR;
>> -}
>> -
>> -static i915_reg_t gen6_pm_imr(struct drm_i915_private *dev_priv)
>> -{
>> -    return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IMR(2) : GEN6_PMIMR;
>> -}
>> -
>> -static i915_reg_t gen6_pm_ier(struct drm_i915_private *dev_priv)
>> -{
>> -    return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IER(2) : GEN6_PMIER;
>> -}
>> -
>
> btw, if you keep these functions but modify them into:
>
>     return INTEL_INFO(dev_priv)->pm_xxx_offset;
>
> then most of below changes will not be needed
Yes. Will keep these functions.
>
>>  /**
>>   * snb_update_pm_irq - update GEN6_PMIMR
>>   * @dev_priv: driver private
>> @@ -332,6 +317,7 @@ static void snb_update_pm_irq(struct 
>> drm_i915_private *dev_priv,
>>                    uint32_t enabled_irq_mask)
>>  {
>>      uint32_t new_val;
>> +    i915_reg_t reg = INTEL_INFO(dev_priv)->pm_imr_offset;
>
> s/reg/imr ?
will remove this change since we are keeping gen6_pm* functions.
>
>>     WARN_ON(enabled_irq_mask & ~interrupt_mask);
>> @@ -343,8 +329,8 @@ static void snb_update_pm_irq(struct 
>> drm_i915_private *dev_priv,
>>     if (new_val != dev_priv->pm_imr) {
>>          dev_priv->pm_imr = new_val;
>> -        I915_WRITE(gen6_pm_imr(dev_priv), dev_priv->pm_imr);
>> -        POSTING_READ(gen6_pm_imr(dev_priv));
>> +        I915_WRITE(reg, dev_priv->pm_imr);
>> +        POSTING_READ(reg);
>>      }
>>  }
>> @@ -371,7 +357,7 @@ void gen6_mask_pm_irq(struct drm_i915_private 
>> *dev_priv, u32 mask)
>> static void gen6_reset_pm_iir(struct drm_i915_private *dev_priv, u32 
>> reset_mask)
>>  {
>> -    i915_reg_t reg = gen6_pm_iir(dev_priv);
>> +    i915_reg_t reg = INTEL_INFO(dev_priv)->pm_iir_offset;
>
> s/reg/iir ?
will remove this as well.
>
>>     lockdep_assert_held(&dev_priv->irq_lock);
>> @@ -385,7 +371,7 @@ static void gen6_enable_pm_irq(struct 
>> drm_i915_private *dev_priv, u32 enable_mas
>>      lockdep_assert_held(&dev_priv->irq_lock);
>>     dev_priv->pm_ier |= enable_mask;
>> -    I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier);
>> +    I915_WRITE(INTEL_INFO(dev_priv)->pm_ier_offset, dev_priv->pm_ier);
>>      gen6_unmask_pm_irq(dev_priv, enable_mask);
>>      /* unmask_pm_irq provides an implicit barrier (POSTING_READ) */
>>  }
>> @@ -396,7 +382,7 @@ static void gen6_disable_pm_irq(struct 
>> drm_i915_private *dev_priv, u32 disable_m
>>     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);
>> +    I915_WRITE(INTEL_INFO(dev_priv)->pm_ier_offset, dev_priv->pm_ier);
>>      /* though a barrier is missing here, but don't really need a one */
>>  }
>> @@ -417,7 +403,8 @@ void gen6_enable_rps_interrupts(struct 
>> drm_i915_private *dev_priv)
>>     spin_lock_irq(&dev_priv->irq_lock);
>>      WARN_ON_ONCE(rps->pm_iir);
>> -    WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & 
>> dev_priv->pm_rps_events);
>> + WARN_ON_ONCE(I915_READ(INTEL_INFO(dev_priv)->pm_iir_offset) &
>> +                   dev_priv->pm_rps_events);
>
> Can you define separate iir_reg variable as in above functions to 
> simplify
> this nested statement ?
and this as well.
>
>>      rps->interrupts_enabled = true;
>>      gen6_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
>> @@ -461,7 +448,7 @@ void gen9_enable_guc_interrupts(struct 
>> drm_i915_private *dev_priv)
>>  {
>>      spin_lock_irq(&dev_priv->irq_lock);
>>      if (!dev_priv->guc.interrupts_enabled) {
>> -        WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
>> + WARN_ON_ONCE(I915_READ(INTEL_INFO(dev_priv)->pm_iir_offset) &
>>                         dev_priv->pm_guc_events);
>>          dev_priv->guc.interrupts_enabled = true;
>>          gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c 
>> b/drivers/gpu/drm/i915/intel_device_info.c
>> index 875d428..d1a4911 100644
>> --- a/drivers/gpu/drm/i915/intel_device_info.c
>> +++ b/drivers/gpu/drm/i915/intel_device_info.c
>> @@ -462,4 +462,15 @@ void intel_device_info_runtime_init(struct 
>> drm_i915_private *dev_priv)
>>               info->sseu.has_subslice_pg ? "y" : "n");
>>      DRM_DEBUG_DRIVER("has EU power gating: %s\n",
>>               info->sseu.has_eu_pg ? "y" : "n");
>> +
>> +    /* Initialize PM interrupt register offsets */
>> +    if (INTEL_GEN(dev_priv) >= 8) {
>> +        info->pm_iir_offset = GEN8_GT_IIR(2);
>> +        info->pm_imr_offset = GEN8_GT_IMR(2);
>> +        info->pm_ier_offset = GEN8_GT_IER(2);
>> +    } else {
>> +        info->pm_iir_offset = GEN6_PMIIR;
>> +        info->pm_imr_offset = GEN6_PMIMR;
>> +        info->pm_ier_offset = GEN6_PMIER;
>> +    }
>>  }

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx