From patchwork Fri Mar 17 10:19:51 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Chris Wilson X-Patchwork-Id: 9630265 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 27D5060132 for ; Fri, 17 Mar 2017 10:20:01 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0ABCF28697 for ; Fri, 17 Mar 2017 10:20:01 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id F1C47286A2; Fri, 17 Mar 2017 10:20:00 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id ABDD428697 for ; Fri, 17 Mar 2017 10:20:00 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 311D46ECE6; Fri, 17 Mar 2017 10:20:00 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from fireflyinternet.com (mail.fireflyinternet.com [109.228.58.192]) by gabe.freedesktop.org (Postfix) with ESMTPS id D7B006ECE6; Fri, 17 Mar 2017 10:19:58 +0000 (UTC) X-Default-Received-SPF: pass (skip=forwardok (res=PASS)) x-ip-name=78.156.65.138; Received: from nuc-i3427.alporthouse.com (unverified [78.156.65.138]) by fireflyinternet.com (Firefly Internet (M1)) with ESMTP id 5915708-1500050 for multiple; Fri, 17 Mar 2017 10:19:52 +0000 Received: by nuc-i3427.alporthouse.com (sSMTP sendmail emulation); Fri, 17 Mar 2017 10:19:51 +0000 Date: Fri, 17 Mar 2017 10:19:51 +0000 From: Chris Wilson To: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 1/2] drm: Mark up accesses of vblank->enabled outside of its spinlock Message-ID: <20170317101951.GO2118@nuc-i3427.alporthouse.com> Mail-Followup-To: Chris Wilson , Ville =?iso-8859-1?Q?Syrj=E4l=E4?= , dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, Daniel Vetter References: <20170316234749.1297-1-chris@chris-wilson.co.uk> <20170317094751.GL31595@intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170317094751.GL31595@intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: Daniel Vetter , intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Fri, Mar 17, 2017 at 11:47:51AM +0200, Ville Syrjälä wrote: > On Thu, Mar 16, 2017 at 11:47:48PM +0000, Chris Wilson wrote: > > @@ -360,7 +358,7 @@ static void vblank_disable_fn(unsigned long arg) > > unsigned long irqflags; > > > > spin_lock_irqsave(&dev->vbl_lock, irqflags); > > - if (atomic_read(&vblank->refcount) == 0 && vblank->enabled) { > > + if (atomic_read(&vblank->refcount) == 0 && READ_ONCE(vblank->enabled)) { > > Hmm. Aren't most of these accesses inside the lock? Looks like you're > marking everything READ/WRITE_ONCE()? There's like 3 different locks here. Afaict, the correct one for serialising vblank->enabled was dev->vblank_time_lock. Every access outside of that lock, I marked up as READ_ONCE. Oh, you are using vbl_lock as the barrier? That's not as clear for disable: > > @@ -1714,6 +1717,9 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) > > if (WARN_ON(pipe >= dev->num_crtcs)) > > return false; > > > > + if (!READ_ONCE(vblank->enabled)) > > + return false; > > This to me looks like it could theoretically cause us to > miss an interrupt. > > 1. enable_irq() > 2. drm_update_vblank_count() > 3. irq fires > 4. drm_handle_vblank() doesn't do anything > 5. enabled=true Sure. There's a danger you miss the irq anyway, and so the last action after enabling the interrupt should be to process any completed events - that's implicitly handled by enabling the interrupt in advance of adding the first event. In the scenario above, there should be nothing to do. -Chris diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 53a526c7b24d..f447ed07ef95 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -325,6 +325,8 @@ static void vblank_disable_and_save(struct drm_device *dev, unsigned int pipe) struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; unsigned long irqflags; + assert_spin_locked(&dev->vbl_lock); + /* Prevent vblank irq processing while disabling vblank irqs, * so no updates of timestamps or count can happen after we've * disabled. Needed to prevent races in case of delayed irq's.