On Tue, Feb 19, 2013 at 03:16:38PM +0200, ville.syrjala at linux.intel.com wrote:
> From: Ville Syrj?l? <ville.syrjala at 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 at chris-wilson.co.uk>
> Tested-by: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Ville Syrj?l? <ville.syrjala at 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 at 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
