From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: Eliminate race from gen2/3 page flip interrupt handling Date: Mon, 18 Feb 2013 14:20:54 +0200 Message-ID: <20130218122053.GM9135@intel.com> References: <1361188671-11639-1-git-send-email-ville.syrjala@linux.intel.com> <1361189273.5878.33.camel@mattotaupa> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga14.intel.com (mga14.intel.com [143.182.124.37]) by gabe.freedesktop.org (Postfix) with ESMTP id AD560E5C82 for ; Mon, 18 Feb 2013 04:20:57 -0800 (PST) Content-Disposition: inline In-Reply-To: <1361189273.5878.33.camel@mattotaupa> 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: Paul Menzel Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Mon, Feb 18, 2013 at 01:07:53PM +0100, Paul Menzel wrote: > Am Montag, den 18.02.2013, 13:57 +0200 schrieb ville.syrjala@linux.intel.= com: > > 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 complete too soon. > = > =BBwould complete=AB or =BBwould be complete*d*=AB 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=E4l=E4 > > --- > > 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/i91= 5_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, voi= d *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_INTERRUP= T) =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, voi= d *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_INTERRUP= T) =3D=3D 0) { > > + intel_finish_page_flip(dev, 1); > > + flip_mask &=3D ~I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT; > > + } > > } > > } > > = > > @@ -2491,8 +2497,11 @@ static irqreturn_t i915_irq_handler(int irq, voi= d *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]; > > + > > + if ((I915_READ(ISR) & flip[plane]) =3D=3D 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 &=3D ~flip[plane]; > > + } > > } > > } > = > = > Thanks, > = > Paul -- = Ville Syrj=E4l=E4 Intel OTC