From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: Fix __wait_seqno to use true infinite timeouts Date: Fri, 23 Aug 2013 10:37:29 +0200 Message-ID: References: <1377219916-3943-1-git-send-email-chris@chris-wilson.co.uk> <20130823080625.GA21757@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ie0-f171.google.com (mail-ie0-f171.google.com [209.85.223.171]) by gabe.freedesktop.org (Postfix) with ESMTP id 49A0BE5D01 for ; Fri, 23 Aug 2013 01:37:30 -0700 (PDT) Received: by mail-ie0-f171.google.com with SMTP id 9so392725iec.2 for ; Fri, 23 Aug 2013 01:37:29 -0700 (PDT) In-Reply-To: <20130823080625.GA21757@nuc-i3427.alporthouse.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: Chris Wilson , Daniel Vetter , intel-gfx List-Id: intel-gfx@lists.freedesktop.org On Fri, Aug 23, 2013 at 10:06 AM, Chris Wilson wrote: >> Yeah, I think this approach should work. A few comments: >> - I think we need a debugfs file to shut the safety quirk off - when >> testing on a machine where we actually miss interrupts it might be >> usful to get the warning output every time. > > I did consider a modparam. Exposing it would indeed necessitate some > protection against concurrent modificatin. > >> - I'd go for a new bool dev_priv->unreliable_seqno_signalling or so to >> avoid any races due to the rmw cycle you now do on dev_priv->quirks. > > There isn't a race during writing, as hangcheck should never be run > concurrently (or at least any concurrent calls filtered out at the start > of the function). The read side is inherently racey. It's more that thus far we've used dev_priv->quirks only for stuff which never changes, now we have a quirk which gets only set at runtime. It just feels conceptually wrong ;-) And if we add a 2nd such quirk it'll break a bit, hence why I'd prefer a distinct piece of state tracking >> - I'd have opted for a faster timeout of the fake irq, but one that rearms. > > Whoops, that was a mistake. The intention was to run at 100Hz, do you > want even faster? We could switch to a hrtimer and kill two birds with > one stone (as timer is singleshot only). I'd simply go with a 1 jiffies rearming timer. >> Also I'd love to be able to test all this (both the missed irq >> detection stuff and the fake irq) but I don't have a good idea right >> now ... So I guess we need to again hope that it won't break too >> quickly (since it eventually will break again). > > Disable the call to ring->get_irq. Perhaps the high word of > dev_priv->stop_rings? Yeah, something like stop_ring_irqs or so could work, maybe by wrapping the wake_up_all calls into a little helper like wake_up_ring which noops if the respective bit is set in stop_ring_irqs. But I'm not sure whether this is worth the fuzz. Otoh we've just proven that for untested code the question isn't if it breaks, but when ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch