Re: [PATCH v3 3/4] drm/i915/bdw: Ack interrupts before handling them (GEN8)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jun 16, 2014 at 04:10:59PM +0100, oscar.mateo@xxxxxxxxx wrote:
> From: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> 
> Otherwise, we might receive a new interrupt before we have time to
> ack the first one, eventually missing it.
> 
> The right order should be:
> 
> 1 - Disable Master Interrupt Control.
> 2 - Find the category of interrupt that is pending.
> 3 - Find the source(s) of the interrupt and clear the Interrupt Identity bits (IIR)
> 4 - Process the interrupt(s) that had bits set in the IIRs.
> 5 - Re-enable Master Interrupt Control.
> 
> Without an atomic XCHG operation with mmio space, the above merely reduces the window
> in which we can miss an interrupt (especially when you consider how heavyweight the
> I915_READ/I915_WRITE operations are).
> 
> Spotted by Bob Beckett <robert.beckett@xxxxxxxxx>.
> 
> v2: Add warning to commit message and comments to the code as per Chris Wilson's request.
> 
> v3: Improve the source code comment.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@xxxxxxxxx>

This one had a small conflict with Chris' stall detection patch that I
fixed up while applying. Entire series merged, thanks.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 91 ++++++++++++++++++++---------------------
>  1 file changed, 45 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 20f7c05..46fe3ad 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1462,6 +1462,7 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
>  	if (master_ctl & (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ)) {
>  		tmp = I915_READ(GEN8_GT_IIR(0));
>  		if (tmp) {
> +			I915_WRITE(GEN8_GT_IIR(0), tmp);
>  			ret = IRQ_HANDLED;
>  			rcs = tmp >> GEN8_RCS_IRQ_SHIFT;
>  			bcs = tmp >> GEN8_BCS_IRQ_SHIFT;
> @@ -1469,7 +1470,6 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
>  				notify_ring(dev, &dev_priv->ring[RCS]);
>  			if (bcs & GT_RENDER_USER_INTERRUPT)
>  				notify_ring(dev, &dev_priv->ring[BCS]);
> -			I915_WRITE(GEN8_GT_IIR(0), tmp);
>  		} else
>  			DRM_ERROR("The master control interrupt lied (GT0)!\n");
>  	}
> @@ -1477,6 +1477,7 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
>  	if (master_ctl & (GEN8_GT_VCS1_IRQ | GEN8_GT_VCS2_IRQ)) {
>  		tmp = I915_READ(GEN8_GT_IIR(1));
>  		if (tmp) {
> +			I915_WRITE(GEN8_GT_IIR(1), tmp);
>  			ret = IRQ_HANDLED;
>  			vcs = tmp >> GEN8_VCS1_IRQ_SHIFT;
>  			if (vcs & GT_RENDER_USER_INTERRUPT)
> @@ -1484,7 +1485,6 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
>  			vcs = tmp >> GEN8_VCS2_IRQ_SHIFT;
>  			if (vcs & GT_RENDER_USER_INTERRUPT)
>  				notify_ring(dev, &dev_priv->ring[VCS2]);
> -			I915_WRITE(GEN8_GT_IIR(1), tmp);
>  		} else
>  			DRM_ERROR("The master control interrupt lied (GT1)!\n");
>  	}
> @@ -1492,10 +1492,10 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
>  	if (master_ctl & GEN8_GT_PM_IRQ) {
>  		tmp = I915_READ(GEN8_GT_IIR(2));
>  		if (tmp & dev_priv->pm_rps_events) {
> -			ret = IRQ_HANDLED;
> -			gen8_rps_irq_handler(dev_priv, tmp);
>  			I915_WRITE(GEN8_GT_IIR(2),
>  				   tmp & dev_priv->pm_rps_events);
> +			ret = IRQ_HANDLED;
> +			gen8_rps_irq_handler(dev_priv, tmp);
>  		} else
>  			DRM_ERROR("The master control interrupt lied (PM)!\n");
>  	}
> @@ -1503,11 +1503,11 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
>  	if (master_ctl & GEN8_GT_VECS_IRQ) {
>  		tmp = I915_READ(GEN8_GT_IIR(3));
>  		if (tmp) {
> +			I915_WRITE(GEN8_GT_IIR(3), tmp);
>  			ret = IRQ_HANDLED;
>  			vcs = tmp >> GEN8_VECS_IRQ_SHIFT;
>  			if (vcs & GT_RENDER_USER_INTERRUPT)
>  				notify_ring(dev, &dev_priv->ring[VECS]);
> -			I915_WRITE(GEN8_GT_IIR(3), tmp);
>  		} else
>  			DRM_ERROR("The master control interrupt lied (GT3)!\n");
>  	}
> @@ -2238,36 +2238,36 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
>  	I915_WRITE(GEN8_MASTER_IRQ, 0);
>  	POSTING_READ(GEN8_MASTER_IRQ);
>  
> +	/* Find, clear, then process each source of interrupt */
> +
>  	ret = gen8_gt_irq_handler(dev, dev_priv, master_ctl);
>  
>  	if (master_ctl & GEN8_DE_MISC_IRQ) {
>  		tmp = I915_READ(GEN8_DE_MISC_IIR);
> -		if (tmp & GEN8_DE_MISC_GSE)
> -			intel_opregion_asle_intr(dev);
> -		else if (tmp)
> -			DRM_ERROR("Unexpected DE Misc interrupt\n");
> -		else
> -			DRM_ERROR("The master control interrupt lied (DE MISC)!\n");
> -
>  		if (tmp) {
>  			I915_WRITE(GEN8_DE_MISC_IIR, tmp);
>  			ret = IRQ_HANDLED;
> +			if (tmp & GEN8_DE_MISC_GSE)
> +				intel_opregion_asle_intr(dev);
> +			else
> +				DRM_ERROR("Unexpected DE Misc interrupt\n");
>  		}
> +		else
> +			DRM_ERROR("The master control interrupt lied (DE MISC)!\n");
>  	}
>  
>  	if (master_ctl & GEN8_DE_PORT_IRQ) {
>  		tmp = I915_READ(GEN8_DE_PORT_IIR);
> -		if (tmp & GEN8_AUX_CHANNEL_A)
> -			dp_aux_irq_handler(dev);
> -		else if (tmp)
> -			DRM_ERROR("Unexpected DE Port interrupt\n");
> -		else
> -			DRM_ERROR("The master control interrupt lied (DE PORT)!\n");
> -
>  		if (tmp) {
>  			I915_WRITE(GEN8_DE_PORT_IIR, tmp);
>  			ret = IRQ_HANDLED;
> +			if (tmp & GEN8_AUX_CHANNEL_A)
> +				dp_aux_irq_handler(dev);
> +			else
> +				DRM_ERROR("Unexpected DE Port interrupt\n");
>  		}
> +		else
> +			DRM_ERROR("The master control interrupt lied (DE PORT)!\n");
>  	}
>  
>  	for_each_pipe(pipe) {
> @@ -2277,33 +2277,32 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
>  			continue;
>  
>  		pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
> -		if (pipe_iir & GEN8_PIPE_VBLANK)
> -			intel_pipe_handle_vblank(dev, pipe);
> -
> -		if (pipe_iir & GEN8_PIPE_PRIMARY_FLIP_DONE) {
> -			intel_prepare_page_flip(dev, pipe);
> -			intel_finish_page_flip_plane(dev, pipe);
> -		}
> +		if (pipe_iir) {
> +			ret = IRQ_HANDLED;
> +			I915_WRITE(GEN8_DE_PIPE_IIR(pipe), pipe_iir);
> +			if (pipe_iir & GEN8_PIPE_VBLANK)
> +				intel_pipe_handle_vblank(dev, pipe);
>  
> -		if (pipe_iir & GEN8_PIPE_CDCLK_CRC_DONE)
> -			hsw_pipe_crc_irq_handler(dev, pipe);
> +			if (pipe_iir & GEN8_PIPE_PRIMARY_FLIP_DONE) {
> +				intel_prepare_page_flip(dev, pipe);
> +				intel_finish_page_flip_plane(dev, pipe);
> +			}
>  
> -		if (pipe_iir & GEN8_PIPE_FIFO_UNDERRUN) {
> -			if (intel_set_cpu_fifo_underrun_reporting(dev, pipe,
> -								  false))
> -				DRM_ERROR("Pipe %c FIFO underrun\n",
> -					  pipe_name(pipe));
> -		}
> +			if (pipe_iir & GEN8_PIPE_CDCLK_CRC_DONE)
> +				hsw_pipe_crc_irq_handler(dev, pipe);
>  
> -		if (pipe_iir & GEN8_DE_PIPE_IRQ_FAULT_ERRORS) {
> -			DRM_ERROR("Fault errors on pipe %c\n: 0x%08x",
> -				  pipe_name(pipe),
> -				  pipe_iir & GEN8_DE_PIPE_IRQ_FAULT_ERRORS);
> -		}
> +			if (pipe_iir & GEN8_PIPE_FIFO_UNDERRUN) {
> +				if (intel_set_cpu_fifo_underrun_reporting(dev, pipe,
> +									  false))
> +					DRM_ERROR("Pipe %c FIFO underrun\n",
> +						  pipe_name(pipe));
> +			}
>  
> -		if (pipe_iir) {
> -			ret = IRQ_HANDLED;
> -			I915_WRITE(GEN8_DE_PIPE_IIR(pipe), pipe_iir);
> +			if (pipe_iir & GEN8_DE_PIPE_IRQ_FAULT_ERRORS) {
> +				DRM_ERROR("Fault errors on pipe %c\n: 0x%08x",
> +					  pipe_name(pipe),
> +					  pipe_iir & GEN8_DE_PIPE_IRQ_FAULT_ERRORS);
> +			}
>  		} else
>  			DRM_ERROR("The master control interrupt lied (DE PIPE)!\n");
>  	}
> @@ -2315,13 +2314,13 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
>  		 * on older pch-split platforms. But this needs testing.
>  		 */
>  		u32 pch_iir = I915_READ(SDEIIR);
> -
> -		cpt_irq_handler(dev, pch_iir);
> -
>  		if (pch_iir) {
>  			I915_WRITE(SDEIIR, pch_iir);
>  			ret = IRQ_HANDLED;
> -		}
> +			cpt_irq_handler(dev, pch_iir);
> +		} else
> +			DRM_ERROR("The master control interrupt lied (SDE)!\n");
> +
>  	}
>  
>  	I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
> -- 
> 1.9.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux