[PATCH 0/2] drm/i915: gen2-4 page flip fixes

[PATCH 0/2] drm/i915: gen2-4 page flip fixes

[ville.syrjala]

These should hopefully fix up the races in gen2-4 page flip support.

Kudos to Chris for fixing up my mistakes in the gen4 patch, and for
providing a nice comment to explain the logic.

Disclaimer: I don't have hardware to test any of this. Currently it has
only been tested by Chris.

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

[ville.syrjala]

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];
+					}
 				}
 			}
 
-- 
1.7.12.4

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

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

[Daniel Vetter]

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

[PATCH v2 2/2] drm/i915: Fix races in gen4 page flip interrupt handling

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Use the gen3 logic for handling page flip interrupts on gen4.

Unfortuantely this kills the stall_check since that looks like it can
easily trigger too early. With the current logic the stall check would
kick in on the first vblank after the flip has been submitted to the
ring. If the CS takes longer than that to process the commands in the
ring, the stall check will cause the page flip to be complete too
early. That doesn't sound like a very good idea. Something better
should be deviced if we still need the stall check. For now, mark
i915_pageflip_stall_check() as unused.

v2: Fix irq enable_mask and add __always_unused (Chris Wilson)

References: https://bugs.launchpad.net/ubuntu/+source/xserver-xorg-video-intel/+bug/1116587
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 | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6488249..18de788 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1547,7 +1547,7 @@ void i915_handle_error(struct drm_device *dev, bool wedged)
 	queue_work(dev_priv->wq, &dev_priv->gpu_error.work);
 }
 
-static void i915_pageflip_stall_check(struct drm_device *dev, int pipe)
+static void __always_unused i915_pageflip_stall_check(struct drm_device *dev, int pipe)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
@@ -2598,6 +2598,8 @@ static int i965_irq_postinstall(struct drm_device *dev)
 			       I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT);
 
 	enable_mask = ~dev_priv->irq_mask;
+	enable_mask &= ~(I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT |
+			 I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT);
 	enable_mask |= I915_USER_INTERRUPT;
 
 	if (IS_G4X(dev))
@@ -2684,6 +2686,13 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
 	unsigned long irqflags;
 	int irq_received;
 	int ret = IRQ_NONE, pipe;
+	u32 flip[2] = {
+		I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT,
+		I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT
+	};
+	u32 flip_mask =
+		I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT |
+		I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
 
 	atomic_inc(&dev_priv->irq_received);
 
@@ -2692,7 +2701,7 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
 	for (;;) {
 		bool blc_event = false;
 
-		irq_received = iir != 0;
+		irq_received = (iir & ~flip_mask) != 0;
 
 		/* Can't rely on pipestat interrupt bit in iir as it might
 		 * have been cleared after the pipestat interrupt was received.
@@ -2739,7 +2748,7 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
 			I915_READ(PORT_HOTPLUG_STAT);
 		}
 
-		I915_WRITE(IIR, iir);
+		I915_WRITE(IIR, iir & ~flip_mask);
 		new_iir = I915_READ(IIR); /* Flush posted writes */
 
 		if (iir & I915_USER_INTERRUPT)
@@ -2747,17 +2756,17 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
 		if (iir & I915_BSD_USER_INTERRUPT)
 			notify_ring(dev, &dev_priv->ring[VCS]);
 
-		if (iir & I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT)
-			intel_prepare_page_flip(dev, 0);
-
-		if (iir & I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT)
-			intel_prepare_page_flip(dev, 1);
-
 		for_each_pipe(pipe) {
 			if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS &&
 			    drm_handle_vblank(dev, pipe)) {
-				i915_pageflip_stall_check(dev, pipe);
-				intel_finish_page_flip(dev, pipe);
+				if (iir & flip[pipe]) {
+					intel_prepare_page_flip(dev, pipe);
+
+					if ((I915_READ(ISR) & flip[pipe]) == 0) {
+						intel_finish_page_flip(dev, pipe);
+						flip_mask &= ~flip[pipe];
+					}
+				}
 			}
 
 			if (pipe_stats[pipe] & PIPE_LEGACY_BLC_EVENT_STATUS)
-- 
1.7.12.4

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

Re: [PATCH v2 2/2] drm/i915: Fix races in gen4 page flip interrupt handling

[Daniel Vetter]

On Tue, Feb 19, 2013 at 03:16:39PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Use the gen3 logic for handling page flip interrupts on gen4.
> 
> Unfortuantely this kills the stall_check since that looks like it can
> easily trigger too early. With the current logic the stall check would
> kick in on the first vblank after the flip has been submitted to the
> ring. If the CS takes longer than that to process the commands in the
> ring, the stall check will cause the page flip to be complete too
> early. That doesn't sound like a very good idea. Something better
> should be deviced if we still need the stall check. For now, mark
> i915_pageflip_stall_check() as unused.
> 
> v2: Fix irq enable_mask and add __always_unused (Chris Wilson)
> 
> References: https://bugs.launchpad.net/ubuntu/+source/xserver-xorg-video-intel/+bug/1116587
> 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>

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