[Intel-gfx] [PATCH] drm/i915: Eliminate race from gen2/3 page flip interrupt handling

2013-02-18 Thread ville . syrjala 
From: Ville Syrjälä ville.syrj...@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 complete too soon.

To eliminate this race check the live pending flip status from the ISR
register before finishing the page flip.

Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
 drivers/gpu/drm/i915/i915_irq.c | 21 +++--
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 9fde49a..3de570c 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,11 @@ 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];
+
+   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: [Intel-gfx] [PATCH] drm/i915: Eliminate race from gen2/3 page flip interrupt handling

2013-02-18 Thread Paul Menzel 
Am Montag, den 18.02.2013, 13:57 +0200 schrieb ville.syrj...@linux.intel.com:
 From: Ville Syrjälä ville.syrj...@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 complete too soon.

»would complete« or »would be complete*d*«

 To eliminate this race check the live pending flip status from the ISR
 register before finishing the page flip.

Could this be tested somehow? Could a test case be written for this?

 Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
 ---
  drivers/gpu/drm/i915/i915_irq.c | 21 +++--
  1 file changed, 15 insertions(+), 6 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
 index 9fde49a..3de570c 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,11 @@ 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];
 +
 + if ((I915_READ(ISR)  flip[plane]) == 
 0) {

Why not `I915_READ16`?

 + intel_finish_page_flip(dev, 
 pipe);
 + flip_mask = ~flip[plane];
 + }
   }
   }


Thanks,

Paul


signature.asc
Description: This is a digitally signed message part
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Eliminate race from gen2/3 page flip interrupt handling

2013-02-18 Thread Chris Wilson 
On Mon, Feb 18, 2013 at 01:57:51PM +0200, ville.syrj...@linux.intel.com wrote:
 From: Ville Syrjälä ville.syrj...@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 complete too soon.
 
 To eliminate this race check the live pending flip status from the ISR
 register before finishing the page flip.

Ok, that makes a lot of sense.

/* 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.
 */
 
 Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
Reviewed-by: Chris Wilson chris@chris-wils

Time to give it a quick test and make sure it doesn't break a ton of
assumptions... :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Eliminate race from gen2/3 page flip interrupt handling

2013-02-18 Thread Ville Syrjälä 
On Mon, Feb 18, 2013 at 01:07:53PM +0100, Paul Menzel wrote:
 Am Montag, den 18.02.2013, 13:57 +0200 schrieb ville.syrj...@linux.intel.com:
  From: Ville Syrjälä ville.syrj...@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 complete too soon.
 
 »would complete« or »would be complete*d*«

The second on is what I meant. Will fix.

 
  To eliminate this race check the live pending flip status from the ISR
  register before finishing the page flip.
 
 Could this be tested somehow? Could a test case be written for this?

It shouldn't be too difficult to force it from within the kernel. Just
turn off interrupts before vblank start, wait until vblank start is
passed, execute the MI_DISPLAY_FLIP, and finally turn the interrupts
back on again.

Given the timing constraints I'm not sure we can come up with anything
that would realiably hit it otherwise.

 
  Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
  ---
   drivers/gpu/drm/i915/i915_irq.c | 21 +++--
   1 file changed, 15 insertions(+), 6 deletions(-)
  
  diff --git a/drivers/gpu/drm/i915/i915_irq.c 
  b/drivers/gpu/drm/i915/i915_irq.c
  index 9fde49a..3de570c 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,11 @@ 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];
  +
  +   if ((I915_READ(ISR)  flip[plane]) == 
  0) {
 
 Why not `I915_READ16`?

It matches the rest if i915_irq_handler(). I suppose the interrupt
registers were 16 bit on gen2 and 32 bit on gen3+.

 
  +   intel_finish_page_flip(dev, 
  pipe);
  +   flip_mask = ~flip[plane];
  +   }
  }
  }
 
 
 Thanks,
 
 Paul



-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Eliminate race from gen2/3 page flip interrupt handling

2013-02-18 Thread Ville Syrjälä 
On Mon, Feb 18, 2013 at 12:16:09PM +, Chris Wilson wrote:
 On Mon, Feb 18, 2013 at 01:57:51PM +0200, ville.syrj...@linux.intel.com wrote:
  From: Ville Syrjälä ville.syrj...@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 complete too soon.
  
  To eliminate this race check the live pending flip status from the ISR
  register before finishing the page flip.
 
 Ok, that makes a lot of sense.
 
 /* 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.
  */

You want me to include that comment somewhere in the code?

  Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
 Reviewed-by: Chris Wilson chris@chris-wils
 
 Time to give it a quick test and make sure it doesn't break a ton of
 assumptions... :)
 -Chris
 
 -- 
 Chris Wilson, Intel Open Source Technology Centre

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Eliminate race from gen2/3 page flip interrupt handling

2013-02-18 Thread Chris Wilson 
On Mon, Feb 18, 2013 at 02:27:24PM +0200, Ville Syrjälä wrote:
 On Mon, Feb 18, 2013 at 12:16:09PM +, Chris Wilson wrote:
  /* 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.
   */
 
 You want me to include that comment somewhere in the code?

Please, would be useful for future archaeologists. I'd pick the gen3
location as it is more compact.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx