From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH v3 1/2] drm/i915: Eliminate race from gen2/3 page flip interrupt handling Date: Tue, 19 Feb 2013 20:19:41 +0100 Message-ID: <20130219191941.GZ5813@phenom.ffwll.local> References: <1361279799-16582-1-git-send-email-ville.syrjala@linux.intel.com> <1361279799-16582-2-git-send-email-ville.syrjala@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-wg0-f52.google.com (mail-wg0-f52.google.com [74.125.82.52]) by gabe.freedesktop.org (Postfix) with ESMTP id E5032E5DCF for ; Tue, 19 Feb 2013 11:17:23 -0800 (PST) Received: by mail-wg0-f52.google.com with SMTP id 12so5886394wgh.7 for ; Tue, 19 Feb 2013 11:17:23 -0800 (PST) Content-Disposition: inline In-Reply-To: <1361279799-16582-2-git-send-email-ville.syrjala@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: ville.syrjala@linux.intel.com Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Tue, Feb 19, 2013 at 03:16:38PM +0200, ville.syrjala@linux.intel.com wro= te: > From: Ville Syrj=E4l=E4 > = > 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 > Tested-by: Chris Wilson > Signed-off-by: Ville Syrj=E4l=E4 > --- > 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 &=3D ~I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT; > + > + if ((I915_READ16(ISR) & I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT)= =3D=3D 0) { > + intel_finish_page_flip(dev, 0); > + flip_mask &=3D ~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 &=3D ~I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT; > + > + if ((I915_READ16(ISR) & I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT)= =3D=3D 0) { > + intel_finish_page_flip(dev, 1); > + flip_mask &=3D ~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 &=3D ~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, h= ence > + * the flip is completed (no longer pending). Since this doesn't ra= ise an > + * interrupt per se, we watch for the change at vblank. > + */ > + if ((I915_READ(ISR) & flip[plane]) =3D=3D 0) { > + intel_finish_page_flip(dev, pipe); > + flip_mask &=3D ~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