Re: [PATCH 04/20] drm/i915: Add low level set of routines for programming PM IER/IIR/IMR register set

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

On 12/08/16 07:25, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@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.
>
> v2:
> - For appropriateness & avoid any ambiguity, rename old functions
>    enable/disable pm_irq to mask/unmask pm_irq and rename new functions
>    enable/disable pm_interrupts to enable/disable pm_irq. (Tvrtko)
> - Use u32 in place of uint32_t. (Tvrtko)
>
> v3:
> - Rename the fields pm_irq_mask & pm_ier_mask and do some cleanup. (Chris)
> - Rebase.
>
> v4: Fix the inadvertent disabling of User interrupt for VECS ring causing
>      failure for certain IGTs.
>
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h         |  3 +-
>   drivers/gpu/drm/i915/i915_irq.c         | 75 ++++++++++++++++++++++-----------
>   drivers/gpu/drm/i915/intel_drv.h        |  3 ++
>   drivers/gpu/drm/i915/intel_ringbuffer.c |  4 +-
>   4 files changed, 57 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7971c76..a608a5c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1776,7 +1776,8 @@ struct drm_i915_private {
>   		u32 de_irq_mask[I915_MAX_PIPES];
>   	};
>   	u32 gt_irq_mask;
> -	u32 pm_irq_mask;
> +	u32 pm_imr;
> +	u32 pm_ier;
>   	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 ebb83d5..5f93309 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -303,18 +303,18 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
>
>   	assert_spin_locked(&dev_priv->irq_lock);
>
> -	new_val = dev_priv->pm_irq_mask;
> +	new_val = dev_priv->pm_imr;
>   	new_val &= ~interrupt_mask;
>   	new_val |= (~enabled_irq_mask & interrupt_mask);
>
> -	if (new_val != dev_priv->pm_irq_mask) {
> -		dev_priv->pm_irq_mask = new_val;
> -		I915_WRITE(gen6_pm_imr(dev_priv), dev_priv->pm_irq_mask);
> +	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));
>   	}
>   }
>
> -void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> +void gen6_unmask_pm_irq(struct drm_i915_private *dev_priv, u32 mask)
>   {
>   	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
>   		return;
> @@ -322,28 +322,54 @@ void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
>   	snb_update_pm_irq(dev_priv, mask, mask);
>   }
>
> -static void __gen6_disable_pm_irq(struct drm_i915_private *dev_priv,
> -				  uint32_t mask)
> +static void __gen6_mask_pm_irq(struct drm_i915_private *dev_priv, u32 mask)
>   {
>   	snb_update_pm_irq(dev_priv, mask, 0);
>   }
>
> -void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> +void gen6_mask_pm_irq(struct drm_i915_private *dev_priv, u32 mask)
>   {
>   	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
>   		return;
>
> -	__gen6_disable_pm_irq(dev_priv, mask);
> +	__gen6_mask_pm_irq(dev_priv, mask);
>   }
>
> -void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv)
> +void gen6_reset_pm_iir(struct drm_i915_private *dev_priv, u32 reset_mask)
>   {
>   	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_irq(struct drm_i915_private *dev_priv, u32 enable_mask)
> +{
> +	assert_spin_locked(&dev_priv->irq_lock);
> +
> +	dev_priv->pm_ier |= enable_mask;
> +	I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier);
> +	gen6_unmask_pm_irq(dev_priv, enable_mask);
> +	/* unmask_pm_irq provides an implicit barrier (POSTING_READ) */
> +}
> +
> +void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, u32 disable_mask)
> +{
> +	assert_spin_locked(&dev_priv->irq_lock);
> +
> +	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);
> +	/* though a barrier is missing here, but don't really need a one */
> +}
> +
> +void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv)
> +{
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	gen6_reset_pm_iir(dev_priv, dev_priv->pm_rps_events);
>   	dev_priv->rps.pm_iir = 0;
>   	spin_unlock_irq(&dev_priv->irq_lock);
>   }
> @@ -354,8 +380,6 @@ void gen6_enable_rps_interrupts(struct drm_i915_private *dev_priv)
>   	WARN_ON_ONCE(dev_priv->rps.pm_iir);
>   	WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & dev_priv->pm_rps_events);
>   	dev_priv->rps.interrupts_enabled = true;
> -	I915_WRITE(gen6_pm_ier(dev_priv), I915_READ(gen6_pm_ier(dev_priv)) |
> -				dev_priv->pm_rps_events);
>   	gen6_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
>
>   	spin_unlock_irq(&dev_priv->irq_lock);
> @@ -373,9 +397,7 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
>
>   	I915_WRITE(GEN6_PMINTRMSK, gen6_sanitize_rps_pm_mask(dev_priv, ~0));
>
> -	__gen6_disable_pm_irq(dev_priv, dev_priv->pm_rps_events);
> -	I915_WRITE(gen6_pm_ier(dev_priv), I915_READ(gen6_pm_ier(dev_priv)) &
> -				~dev_priv->pm_rps_events);
> +	gen6_disable_pm_irq(dev_priv, dev_priv->pm_rps_events);
>
>   	spin_unlock_irq(&dev_priv->irq_lock);
>   	synchronize_irq(dev_priv->drm.irq);
> @@ -1078,7 +1100,7 @@ static void gen6_pm_rps_work(struct work_struct *work)
>   	pm_iir = dev_priv->rps.pm_iir;
>   	dev_priv->rps.pm_iir = 0;
>   	/* Make sure not to corrupt PMIMR state used by ringbuffer on GEN6 */
> -	gen6_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
> +	gen6_unmask_pm_irq(dev_priv, dev_priv->pm_rps_events);
>   	client_boost = dev_priv->rps.client_boost;
>   	dev_priv->rps.client_boost = false;
>   	spin_unlock_irq(&dev_priv->irq_lock);
> @@ -1579,7 +1601,7 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
>   {
>   	if (pm_iir & dev_priv->pm_rps_events) {
>   		spin_lock(&dev_priv->irq_lock);
> -		gen6_disable_pm_irq(dev_priv, pm_iir & dev_priv->pm_rps_events);
> +		gen6_mask_pm_irq(dev_priv, pm_iir & dev_priv->pm_rps_events);
>   		if (dev_priv->rps.interrupts_enabled) {
>   			dev_priv->rps.pm_iir |= pm_iir & dev_priv->pm_rps_events;
>   			schedule_work(&dev_priv->rps.work);
> @@ -3568,11 +3590,13 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
>   		 * RPS interrupts will get enabled/disabled on demand when RPS
>   		 * itself is enabled/disabled.
>   		 */
> -		if (HAS_VEBOX(dev))
> +		if (HAS_VEBOX(dev)) {

dev_priv should be used for all these macros in new code.

>   			pm_irqs |= PM_VEBOX_USER_INTERRUPT;
> +			dev_priv->pm_ier |= PM_VEBOX_USER_INTERRUPT;
> +		}
>
> -		dev_priv->pm_irq_mask = 0xffffffff;
> -		GEN5_IRQ_INIT(GEN6_PM, dev_priv->pm_irq_mask, pm_irqs);
> +		dev_priv->pm_imr = 0xffffffff;
> +		GEN5_IRQ_INIT(GEN6_PM, dev_priv->pm_imr, pm_irqs);
>   	}
>   }
>
> @@ -3692,14 +3716,15 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
>   	if (HAS_L3_DPF(dev_priv))
>   		gt_interrupts[0] |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
>
> -	dev_priv->pm_irq_mask = 0xffffffff;
> +	dev_priv->pm_ier = 0x0;
> +	dev_priv->pm_imr = ~dev_priv->pm_ier;
>   	GEN8_IRQ_INIT_NDX(GT, 0, ~gt_interrupts[0], gt_interrupts[0]);
>   	GEN8_IRQ_INIT_NDX(GT, 1, ~gt_interrupts[1], gt_interrupts[1]);
>   	/*
>   	 * RPS interrupts will get enabled/disabled on demand when RPS itself
>   	 * is enabled/disabled.
>   	 */
> -	GEN8_IRQ_INIT_NDX(GT, 2, dev_priv->pm_irq_mask, 0);
> +	GEN8_IRQ_INIT_NDX(GT, 2, dev_priv->pm_imr, dev_priv->pm_ier);
>   	GEN8_IRQ_INIT_NDX(GT, 3, ~gt_interrupts[3], gt_interrupts[3]);
>   }
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9539f0e..80cd05f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1094,6 +1094,9 @@ void intel_check_pch_fifo_underruns(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);
> +void gen6_reset_pm_iir(struct drm_i915_private *dev_priv, u32 mask);
> +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_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
>   void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
>   void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index ed19868..e8fa26c 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1700,7 +1700,7 @@ hsw_vebox_irq_enable(struct intel_engine_cs *engine)
>   	struct drm_i915_private *dev_priv = engine->i915;
>
>   	I915_WRITE_IMR(engine, ~engine->irq_enable_mask);
> -	gen6_enable_pm_irq(dev_priv, engine->irq_enable_mask);
> +	gen6_unmask_pm_irq(dev_priv, engine->irq_enable_mask);
>   }
>
>   static void
> @@ -1709,7 +1709,7 @@ hsw_vebox_irq_disable(struct intel_engine_cs *engine)
>   	struct drm_i915_private *dev_priv = engine->i915;
>
>   	I915_WRITE_IMR(engine, ~0);
> -	gen6_disable_pm_irq(dev_priv, engine->irq_enable_mask);
> +	gen6_mask_pm_irq(dev_priv, engine->irq_enable_mask);
>   }
>
>   static void
>

Looks like only a single line change since v3, so with the above small 
detail fixed;

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