Re: [PATCH 01/11] drm/i915: Export low level PM IRQ functions to control from GuC functions

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 18/10/2017 07:46, Sagar Arun Kamble wrote:
> In order to separate GuC IRQ handling functions from i915_irq.c we need
> to export the low level pm irq handlers. Export pm_iir, reset_pm_iir and
> enable/disable_pm_irq functions.
> 
> Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_irq.c  | 8 ++++----
>   drivers/gpu/drm/i915/intel_drv.h | 4 ++++
>   2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b1296a5..caa6283 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -306,7 +306,7 @@ 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)
> +i915_reg_t gen6_pm_iir(struct drm_i915_private *dev_priv)
>   {
>   	return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IIR(2) : GEN6_PMIIR;
>   }

Can we pencil in for some future work to move this into the device 
private/info/... somewhere? It would be smaller on the aggregate, and 
more importantly more elegant that way I think.

> @@ -369,7 +369,7 @@ void gen6_mask_pm_irq(struct drm_i915_private *dev_priv, u32 mask)
>   	__gen6_mask_pm_irq(dev_priv, mask);
>   }
>   
> -static void gen6_reset_pm_iir(struct drm_i915_private *dev_priv, u32 reset_mask)
> +void gen6_reset_pm_iir(struct drm_i915_private *dev_priv, u32 reset_mask)
>   {
>   	i915_reg_t reg = gen6_pm_iir(dev_priv);
>   
> @@ -380,7 +380,7 @@ static void gen6_reset_pm_iir(struct drm_i915_private *dev_priv, u32 reset_mask)
>   	POSTING_READ(reg);
>   }
>   
> -static void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, u32 enable_mask)
> +void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, u32 enable_mask)
>   {
>   	lockdep_assert_held(&dev_priv->irq_lock);
>   
> @@ -390,7 +390,7 @@ static void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, u32 enable_mas
>   	/* unmask_pm_irq provides an implicit barrier (POSTING_READ) */
>   }
>   
> -static void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, u32 disable_mask)
> +void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, u32 disable_mask)
>   {
>   	lockdep_assert_held(&dev_priv->irq_lock);

I briefly considered, if now that these functions are exported, we would 
benefit from expressing the lock requirement in the functions name 
somehow, either with a double underscore prefix, or locked suffix. But 
since they have the asserts already I think it is fine.

>   
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d61985f..792d8ea 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1237,8 +1237,12 @@ void intel_pch_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
>   /* i915_irq.c */
>   void gen5_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
>   void gen5_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> +i915_reg_t gen6_pm_iir(struct drm_i915_private *dev_priv);
>   void gen6_mask_pm_irq(struct drm_i915_private *dev_priv, u32 mask);
>   void gen6_unmask_pm_irq(struct drm_i915_private *dev_priv, u32 mask);
> +void gen6_reset_pm_iir(struct drm_i915_private *dev_priv, u32 reset_mask);
> +void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, u32 enable_mask);
> +void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, u32 disable_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);
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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