Re: [PATCH v3 1/2] drm/i915: Eliminate race from gen2/3 page flip interrupt handling

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Feb 19, 2013 at 03:16:38PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> If the interrupt handler were to process a previous vblank interrupt and
> the following flip pending interrupt at the same time, the page flip
> would be completed too soon.
> 
> To eliminate this race, check the live pending flip status from the ISR
> register before finishing the page flip.
> 
> v2: Added a comment explaining the logic (by Chris Wilson)
> v3: Fix a typo in the comment
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Tested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 9fde49a..6488249 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2284,8 +2284,11 @@ static irqreturn_t i8xx_irq_handler(int irq, void *arg)
>  		    drm_handle_vblank(dev, 0)) {
>  			if (iir & I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT) {
>  				intel_prepare_page_flip(dev, 0);
> -				intel_finish_page_flip(dev, 0);
> -				flip_mask &= ~I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT;
> +
> +				if ((I915_READ16(ISR) & I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT) == 0) {
> +					intel_finish_page_flip(dev, 0);
> +					flip_mask &= ~I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT;
> +				}
>  			}
>  		}
>  
> @@ -2293,8 +2296,11 @@ static irqreturn_t i8xx_irq_handler(int irq, void *arg)
>  		    drm_handle_vblank(dev, 1)) {
>  			if (iir & I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT) {
>  				intel_prepare_page_flip(dev, 1);
> -				intel_finish_page_flip(dev, 1);
> -				flip_mask &= ~I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
> +
> +				if ((I915_READ16(ISR) & I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT) == 0) {
> +					intel_finish_page_flip(dev, 1);
> +					flip_mask &= ~I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
> +				}
>  			}
>  		}
>  
> @@ -2491,8 +2497,17 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
>  			    drm_handle_vblank(dev, pipe)) {
>  				if (iir & flip[plane]) {
>  					intel_prepare_page_flip(dev, plane);
> -					intel_finish_page_flip(dev, pipe);
> -					flip_mask &= ~flip[plane];
> +
> +					/* We detect FlipDone by looking for the change in PendingFlip from '1'
> +					 * to '0' on the following vblank, i.e. IIR has the Pendingflip
> +					 * asserted following the MI_DISPLAY_FLIP, but ISR is deasserted, hence
> +					 * the flip is completed (no longer pending). Since this doesn't raise an
> +					 * interrupt per se, we watch for the change at vblank.
> +					 */
> +					if ((I915_READ(ISR) & flip[plane]) == 0) {
> +						intel_finish_page_flip(dev, pipe);
> +						flip_mask &= ~flip[plane];

I think 6 levels of indentation is a wee bit too much ;-) Can I volunteer
you to extract the vblank stuff here into little helpers? In helper
functions we can easily switch from

if (foo) {
	...
}

to

if (!foo)
	return;

...

which should further clarify the code. And you could also split up the
comment and put it right to the different conditions without leading to
confusion.
- Daniel

> +					}
>  				}
>  			}
>  
> -- 
> 1.7.12.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch