From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: Fix timeout with missed interrupts in __wait_seqno Date: Thu, 12 Dec 2013 15:28:19 +0100 Message-ID: <20131212142819.GI9804@phenom.ffwll.local> References: <20131210121429.GD10793@nuc-i3427.alporthouse.com> <1386687763-21777-1-git-send-email-mika.kuoppala@intel.com> <20131212124847.GY10036@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-ea0-f175.google.com (mail-ea0-f175.google.com [209.85.215.175]) by gabe.freedesktop.org (Postfix) with ESMTP id 5EFA5F9C95 for ; Thu, 12 Dec 2013 06:27:30 -0800 (PST) Received: by mail-ea0-f175.google.com with SMTP id z10so271113ead.34 for ; Thu, 12 Dec 2013 06:27:29 -0800 (PST) Content-Disposition: inline In-Reply-To: <20131212124847.GY10036@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Cc: intel-gfx@lists.freedesktop.org, miku@iki.fi List-Id: intel-gfx@lists.freedesktop.org On Thu, Dec 12, 2013 at 02:48:47PM +0200, Ville Syrj=E4l=E4 wrote: > On Tue, Dec 10, 2013 at 05:02:43PM +0200, Mika Kuoppala wrote: > > Commit 094f9a54e355 ("drm/i915: Fix __wait_seqno to use true infinite > > timeouts") added support for __wait_seqno to detect missing interrupts = and > > go around them by polling. As there is also timeout detection in > > __wait_seqno, the polling and timeout detection were done with the same > > timer. > > = > > When there has been missed interrupts and polling is needed, the timer = is > > set to trigger in (now + 1) jiffies in future, instead of the caller > > specified timeout. > > = > > Now when io_schedule() returns, we calculate the jiffies left to timeout > > using the timer expiration value. As the current jiffies is now bound t= o be > > always equal or greater than the expiration value, the timeout_jiffies = will > > become zero or negative and we return -ETIME to caller even tho the > > timeout was never reached. > > = > > Fix this by decoupling timeout calculation from timer expiration. > > = > > v2: Commit message with some sense in it (Chris Wilson) > > = > > v3: add parenthesis on timeout_expire calculation > > = > > v4: don't read jiffies without timeout (Chris Wilson) > > = > > Signed-off-by: Mika Kuoppala > > --- > > drivers/gpu/drm/i915/i915_gem.c | 14 ++++++-------- > > 1 file changed, 6 insertions(+), 8 deletions(-) > > = > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i91= 5_gem.c > > index 92149bc..6d2e786 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -1017,7 +1017,7 @@ static int __wait_seqno(struct intel_ring_buffer = *ring, u32 seqno, > > drm_i915_private_t *dev_priv =3D ring->dev->dev_private; > > struct timespec before, now; > > DEFINE_WAIT(wait); > > - long timeout_jiffies; > > + unsigned long timeout_expire; > > int ret; > > = > > WARN(dev_priv->pc8.irqs_disabled, "IRQs disabled\n"); > > @@ -1025,7 +1025,7 @@ static int __wait_seqno(struct intel_ring_buffer = *ring, u32 seqno, > > if (i915_seqno_passed(ring->get_seqno(ring, true), seqno)) > > return 0; > > = > > - timeout_jiffies =3D timeout ? timespec_to_jiffies_timeout(timeout) : = 1; > > + timeout_expire =3D timeout ? jiffies + timespec_to_jiffies_timeout(ti= meout) : 0; > > = > > if (dev_priv->info->gen >=3D 6 && can_wait_boost(file_priv)) { > > gen6_rps_boost(dev_priv); > > @@ -1044,7 +1044,6 @@ static int __wait_seqno(struct intel_ring_buffer = *ring, u32 seqno, > > getrawmonotonic(&before); > > for (;;) { > > struct timer_list timer; > > - unsigned long expire; > > = > > prepare_to_wait(&ring->irq_queue, &wait, > > interruptible ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE); > > @@ -1070,23 +1069,22 @@ static int __wait_seqno(struct intel_ring_buffe= r *ring, u32 seqno, > > break; > > } > > = > > - if (timeout_jiffies <=3D 0) { > > + if (timeout && time_after_eq(jiffies, timeout_expire)) { > > ret =3D -ETIME; > > break; > > } > > = > > timer.function =3D NULL; > > if (timeout || missed_irq(dev_priv, ring)) { > > + unsigned long expire; > > + > > setup_timer_on_stack(&timer, fake_irq, (unsigned long)current); > > - expire =3D jiffies + (missed_irq(dev_priv, ring) ? 1: timeout_jiffi= es); > > + expire =3D missed_irq(dev_priv, ring) ? jiffies + 1 : timeout_expir= e; > = > I guess we have very small race here if we get called w/ timeout=3D=3DNUL= L, and > missed_irq() was true above but is no longer true here. At that point we = would > set expire=3D0 and might end up waiting for quite a while. But that issue= was > present already in the code before this patch and otherwise it all > looks good to me, so: We shouldn't ever reset misseq_irq in normal operations, so this should be ok. > Reviewed-by: Ville Syrj=E4l=E4 Queued for -next, thanks for the patch. -Daniel -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch