From patchwork Tue Jul 23 13:13:37 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 11054521 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id D25F7746 for ; Tue, 23 Jul 2019 13:13:49 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C258B285ED for ; Tue, 23 Jul 2019 13:13:49 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id B6868285C9; Tue, 23 Jul 2019 13:13:49 +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=-5.2 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=unavailable 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 45170285ED for ; Tue, 23 Jul 2019 13:13:49 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 5BFAB89956; Tue, 23 Jul 2019 13:13:47 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mail-ed1-x542.google.com (mail-ed1-x542.google.com [IPv6:2a00:1450:4864:20::542]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2544D88401 for ; Tue, 23 Jul 2019 13:13:45 +0000 (UTC) Received: by mail-ed1-x542.google.com with SMTP id p15so43832250eds.8 for ; Tue, 23 Jul 2019 06:13:45 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=5TBdMSSuyBtgzU2NQMKecYmuqGDZGAfU1Y3/FNahfQo=; b=MEMEXIFD7ASfhaNE1obufZF6ZrdrTK+DfvTAlTX7DrlSW3CL5L5R9zUy0yQsbapzxJ nJ0MpZ8O3an0PSWkBfjN1hSJnKP5rqvQRr5Tg8KZIsIGIt3dUMs4YC2DccFLLPZyabJ6 ITowZ249pRDmS26/5m1z+SpifsvlHHkyewpTv24/U3PahYndlqLNy6LzfbO0cqpT+A1s XiDVMSpaUq0UuF6Qw5kGt3CT3Fjvhw1CuxsB2qQ4aULgVTKzEwKFOA7lr2DTkBB79KBe cx9rt0RD82ar1OwZWYRWTIz6PEQhucFZM0SsPn8b3PobALlNKUbHtesowwZ2mZSO3Q9j F9bw== X-Gm-Message-State: APjAAAXEux0RsptrG6oHUdlu5PGJKP25ORs4o5momqMmCKRgcE8ud0XM qPDI/yit8PHBcDgdNVFG2KQ= X-Google-Smtp-Source: APXvYqxdCsqo9y9WofhiCumgn+AjhJHjopxnb2yHspkYmRkyQLaxwPyv9xjnrjVm5qpmth3OKI5Shg== X-Received: by 2002:a50:a56b:: with SMTP id z40mr64371795edb.99.1563887623753; Tue, 23 Jul 2019 06:13:43 -0700 (PDT) Received: from phenom.ffwll.local ([2a02:168:569e:0:3106:d637:d723:e855]) by smtp.gmail.com with ESMTPSA id p23sm8669584ejl.43.2019.07.23.06.13.42 (version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256); Tue, 23 Jul 2019 06:13:42 -0700 (PDT) From: Daniel Vetter To: DRI Development Date: Tue, 23 Jul 2019 15:13:37 +0200 Message-Id: <20190723131337.22031-1-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 2.22.0 In-Reply-To: <20190719170654.GQ5942@intel.com> References: <20190719170654.GQ5942@intel.com> MIME-Version: 1.0 X-Mailman-Original-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=5TBdMSSuyBtgzU2NQMKecYmuqGDZGAfU1Y3/FNahfQo=; b=MTsI8aijzAVQnV1iMMjDvwGH3bmQ9G2Ah/pf1T8zCNlWE9hpCwimCr+K2dJE+JrQqc 7ticzvyBKjbnTnU30YV/61pN4IPcsp+9+eC5V8udHb0zGiPQC5ALklgEj7mwGbQ3cXLH lQq+b48nKTb7435DOqtcbQXYY5+m9VofvGl6Q= Subject: [Intel-gfx] [PATCH] drm/vblank: Document and fix vblank count barrier semantics X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Rodrigo Siqueira , Daniel Vetter , Intel Graphics Development , Daniel Vetter Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Virus-Scanned: ClamAV using ClamSMTP Noticed while reviewing code. I'm not sure whether this might or might not explain some of the missed vblank hilarity we've been seeing. I think those all go through the vblank completion event, which has unconditional barriers - it always takes the spinlock. Therefore no cc stable. v2: - Barrriers are hard, put them in in the right order (Chris). - Improve the comments a bit. v3: Ville noticed that on 32bit we might be breaking up the load/stores, now that the vblank counter has been switched over to be 64 bit. Fix that up by switching to atomic64_t. This this happens so rarely in practice I figured no need to cc: stable ... Cc: Ville Syrjälä Cc: Keith Packard References: 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") Cc: Rodrigo Siqueira Cc: Chris Wilson Signed-off-by: Daniel Vetter Reviewed-by: Ville Syrjälä Reviewed-by: Rodrigo Siqueira --- drivers/gpu/drm/drm_vblank.c | 45 ++++++++++++++++++++++++++++++++---- include/drm/drm_vblank.h | 15 ++++++++++-- 2 files changed, 54 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 603ab105125d..03e37bceac9c 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -107,7 +107,7 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe, write_seqlock(&vblank->seqlock); vblank->time = t_vblank; - vblank->count += vblank_count_inc; + atomic64_add(vblank_count_inc, &vblank->count); write_sequnlock(&vblank->seqlock); } @@ -273,7 +273,8 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, DRM_DEBUG_VBL("updating vblank count on crtc %u:" " current=%llu, diff=%u, hw=%u hw_last=%u\n", - pipe, vblank->count, diff, cur_vblank, vblank->last); + pipe, atomic64_read(&vblank->count), diff, + cur_vblank, vblank->last); if (diff == 0) { WARN_ON_ONCE(cur_vblank != vblank->last); @@ -295,11 +296,23 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; + u64 count; if (WARN_ON(pipe >= dev->num_crtcs)) return 0; - return vblank->count; + count = atomic64_read(&vblank->count); + + /* + * This read barrier corresponds to the implicit write barrier of the + * write seqlock in store_vblank(). Note that this is the only place + * where we need an explicit barrier, since all other access goes + * through drm_vblank_count_and_time(), which already has the required + * read barrier curtesy of the read seqlock. + */ + smp_rmb(); + + return count; } /** @@ -764,6 +777,14 @@ drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe, * vblank interrupt (since it only reports the software vblank counter), see * drm_crtc_accurate_vblank_count() for such use-cases. * + * Note that for a given vblank counter value drm_crtc_handle_vblank() + * and drm_crtc_vblank_count() or drm_crtc_vblank_count_and_time() + * provide a barrier: Any writes done before calling + * drm_crtc_handle_vblank() will be visible to callers of the later + * functions, iff the vblank count is the same or a later one. + * + * See also &drm_vblank_crtc.count. + * * Returns: * The software vblank counter. */ @@ -801,7 +822,7 @@ static u64 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe, do { seq = read_seqbegin(&vblank->seqlock); - vblank_count = vblank->count; + vblank_count = atomic64_read(&vblank->count); *vblanktime = vblank->time; } while (read_seqretry(&vblank->seqlock, seq)); @@ -818,6 +839,14 @@ static u64 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe, * vblank events since the system was booted, including lost events due to * modesetting activity. Returns corresponding system timestamp of the time * of the vblank interval that corresponds to the current vblank counter value. + * + * Note that for a given vblank counter value drm_crtc_handle_vblank() + * and drm_crtc_vblank_count() or drm_crtc_vblank_count_and_time() + * provide a barrier: Any writes done before calling + * drm_crtc_handle_vblank() will be visible to callers of the later + * functions, iff the vblank count is the same or a later one. + * + * See also &drm_vblank_crtc.count. */ u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc, ktime_t *vblanktime) @@ -1791,6 +1820,14 @@ EXPORT_SYMBOL(drm_handle_vblank); * * This is the native KMS version of drm_handle_vblank(). * + * Note that for a given vblank counter value drm_crtc_handle_vblank() + * and drm_crtc_vblank_count() or drm_crtc_vblank_count_and_time() + * provide a barrier: Any writes done before calling + * drm_crtc_handle_vblank() will be visible to callers of the later + * functions, iff the vblank count is the same or a later one. + * + * See also &drm_vblank_crtc.count. + * * Returns: * True if the event was successfully handled, false on failure. */ diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h index 9fe4ba8bc622..c16c44052b3d 100644 --- a/include/drm/drm_vblank.h +++ b/include/drm/drm_vblank.h @@ -109,9 +109,20 @@ struct drm_vblank_crtc { seqlock_t seqlock; /** - * @count: Current software vblank counter. + * @count: + * + * Current software vblank counter. + * + * Note that for a given vblank counter value drm_crtc_handle_vblank() + * and drm_crtc_vblank_count() or drm_crtc_vblank_count_and_time() + * provide a barrier: Any writes done before calling + * drm_crtc_handle_vblank() will be visible to callers of the later + * functions, iff the vblank count is the same or a later one. + * + * IMPORTANT: This guarantee requires barriers, therefor never access + * this field directly. Use drm_crtc_vblank_count() instead. */ - u64 count; + atomic64_t count; /** * @time: Vblank timestamp corresponding to @count. */