On Tue, 24 Oct 2017, Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx> 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.
Do we *really* want to start on this path? We have the *_offsets in
device info for some groups of registers, but this is a whole another
ballgame. You could make the same argument for half the registers in
i915_reg.h!
If you don't want to use a function to get the correct
register... almost all of the rest of the driver uses if-else in code
for this...
The issue is not this single patch per se. The issue is the precedence
it sets, and the apparent lack of thought on where we'll end up with
this.
BR,
Jani.
>
> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 5 +++++
> drivers/gpu/drm/i915/i915_irq.c | 30 ++++++++----------------------
> drivers/gpu/drm/i915/intel_device_info.c | 11 +++++++++++
> 3 files changed, 24 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..68c6f44 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;
> -}
> -
> /**
> * snb_update_pm_irq - update GEN6_PMIMR
> * @dev_priv: driver private
> @@ -343,8 +328,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(dev_priv->info.pm_imr_offset, dev_priv->pm_imr);
> + POSTING_READ(dev_priv->info.pm_imr_offset);
> }
> }
>
> @@ -371,7 +356,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 = dev_priv->info.pm_iir_offset;
>
> lockdep_assert_held(&dev_priv->irq_lock);
>
> @@ -385,7 +370,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(dev_priv->info.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 +381,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(dev_priv->info.pm_ier_offset, dev_priv->pm_ier);
> /* though a barrier is missing here, but don't really need a one */
> }
>
> @@ -417,7 +402,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(dev_priv->info.pm_iir_offset) &
> + dev_priv->pm_rps_events);
> rps->interrupts_enabled = true;
> gen6_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
>
> @@ -461,7 +447,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(dev_priv->info.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;
> + }
> }
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
