From patchwork Mon Feb 8 01:13:25 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mario Kleiner X-Patchwork-Id: 8246121 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 153459F38B for ; Mon, 8 Feb 2016 01:14:47 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 1B015201ED for ; Mon, 8 Feb 2016 01:14:46 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 90D6D201C8 for ; Mon, 8 Feb 2016 01:14:44 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id C3DB96E328; Sun, 7 Feb 2016 17:14:43 -0800 (PST) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-wm0-f65.google.com (mail-wm0-f65.google.com [74.125.82.65]) by gabe.freedesktop.org (Postfix) with ESMTPS id DAE5E6E328 for ; Sun, 7 Feb 2016 17:14:42 -0800 (PST) Received: by mail-wm0-f65.google.com with SMTP id r129so13228212wmr.0 for ; Sun, 07 Feb 2016 17:14:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=Ndp9Z75PIMNV6xCsU1cfkSYEi6GGcVFm9dr1AmRAICQ=; b=Dpp1iYtXaYD29Gz27euT/EniTuahj2PWho3KdoVZ6dKPfwJkyoW8dWx2bX4CW54EXC Pl249XT0E7D150cX65pzBnoBpYbEUBJpjpP/Z1/XP+u+LLFI3QWe3jrMigW1RDhRj69G R+4Kx40SbNq676fvVntERBVVZq5KvA6h7zBhbcFVBiD4QzOEVsmhog5EGxLnV/7JJouT Yw/J7sjnckcZS67yoRZOeZy0WNUb5+O2AKXAaUhgX0hzaFIa2yoaD2a54zCSzTCICxuw ZL0ER6TM1brikTyub/eo29BNht7KXYew9zZ6eSui1fksll4/v8Hi4d1p9bqznNYuAKEL GZug== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=Ndp9Z75PIMNV6xCsU1cfkSYEi6GGcVFm9dr1AmRAICQ=; b=hnpgIeDxK61wEANAecpUP5pGix6EAGKr8ylNO3axavuVDHWG3zBvk9R3n9r/SDeXcv SdRw4kDTbJpOtk8FMoKG5ETiCv/7WhbOrFfiWVJBGP76GZqds9vRndy0Oda8Pn4eto/R u7JMhqJ7G9e/GllCCyb1BrGTN/32N/F+ZUHxncuJ+eiZLBx+PQ4ekX+tygFHChic60WO Z5GQnD3C+AZHgi6BMUktYJtLpQ3aDKyp43x3SE3IPouTph7on26PePZzTwWzXEL0GSSw ha6jX6QJjtLqg79WPm1lIJtEHKmXxCM5Jk0595j6df93XRqiT5OUeEIZrZ69Jz8fJRX1 Osxg== X-Gm-Message-State: AG10YOSeMgJfZMw5gnNY4WK2uTh4Ha802ARz3jSn8vBLuf1M9jO3bGK6i0t4LxCe5CGA2g== X-Received: by 10.28.238.144 with SMTP id j16mr26304348wmi.43.1454894081569; Sun, 07 Feb 2016 17:14:41 -0800 (PST) Received: from twisty.fritz.box (x4d02db5e.dyn.telefonica.de. [77.2.219.94]) by smtp.gmail.com with ESMTPSA id b5sm9486380wmh.15.2016.02.07.17.14.39 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Sun, 07 Feb 2016 17:14:40 -0800 (PST) From: Mario Kleiner To: dri-devel@lists.freedesktop.org Subject: [PATCH 2/6] drm: Prevent vblank counter bumps > 1 with active vblank clients. Date: Mon, 8 Feb 2016 02:13:25 +0100 Message-Id: <1454894009-15466-3-git-send-email-mario.kleiner.de@gmail.com> X-Mailer: git-send-email 1.9.1 In-Reply-To: <1454894009-15466-1-git-send-email-mario.kleiner.de@gmail.com> References: <1454894009-15466-1-git-send-email-mario.kleiner.de@gmail.com> Cc: daniel.vetter@ffwll.ch, michel@daenzer.net, linux@bernd-steinhauser.de, stable@vger.kernel.org, alexander.deucher@amd.com, christian.koenig@amd.com, vbabka@suse.cz 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: , MIME-Version: 1.0 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP This fixes a regression introduced by the new drm_update_vblank_count() implementation in Linux 4.4: Restrict the bump of the software vblank counter in drm_update_vblank_count() to a safe maximum value of +1 whenever there is the possibility that concurrent readers of vblank timestamps could be active at the moment, as the current implementation of the timestamp caching and updating is not safe against concurrent readers for calls to store_vblank() with a bump of anything but +1. A bump != 1 would very likely return corrupted timestamps to userspace, because the same slot in the cache could be concurrently written by store_vblank() and read by one of those readers in a non-atomic fashion and without the read-retry logic detecting this collision. Concurrent readers can exist while drm_update_vblank_count() is called from the drm_vblank_off() or drm_vblank_on() functions or other non-vblank- irq callers. However, all those calls are happening with the vbl_lock locked thereby preventing a drm_vblank_get(), so the vblank refcount can't increase while drm_update_vblank_count() is executing. Therefore a zero vblank refcount during execution of that function signals that is safe for arbitrary counter bumps if called from outside vblank irq, whereas a non-zero count is not safe. Whenever the function is called from vblank irq, we have to assume concurrent readers could show up any time during its execution, even if the refcount is currently zero, as vblank irqs are usually only enabled due to the presence of readers, and because when it is called from vblank irq it can't hold the vbl_lock to protect it from sudden bumps in vblank refcount. Therefore also restrict bumps to +1 when the function is called from vblank irq. Such bumps of more than +1 can happen at other times than reenabling vblank irqs, e.g., when regular vblank interrupts get delayed by more than 1 frame due to long held locks, long irq off periods, realtime preemption on RT kernels, or system management interrupts. Signed-off-by: Mario Kleiner Cc: # 4.4+ Cc: michel@daenzer.net Cc: vbabka@suse.cz Cc: ville.syrjala@linux.intel.com Cc: daniel.vetter@ffwll.ch Cc: dri-devel@lists.freedesktop.org Cc: alexander.deucher@amd.com Cc: christian.koenig@amd.com Reviewed-by: Daniel Vetter --- drivers/gpu/drm/drm_irq.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index bcb8528..aa2c74b 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -221,6 +221,47 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, diff = (flags & DRM_CALLED_FROM_VBLIRQ) != 0; } + /* + * Restrict the bump of the software vblank counter to a safe maximum + * value of +1 whenever there is the possibility that concurrent readers + * of vblank timestamps could be active at the moment, as the current + * implementation of the timestamp caching and updating is not safe + * against concurrent readers for calls to store_vblank() with a bump + * of anything but +1. A bump != 1 would very likely return corrupted + * timestamps to userspace, because the same slot in the cache could + * be concurrently written by store_vblank() and read by one of those + * readers without the read-retry logic detecting the collision. + * + * Concurrent readers can exist when we are called from the + * drm_vblank_off() or drm_vblank_on() functions and other non-vblank- + * irq callers. However, all those calls to us are happening with the + * vbl_lock locked to prevent drm_vblank_get(), so the vblank refcount + * can't increase while we are executing. Therefore a zero refcount at + * this point is safe for arbitrary counter bumps if we are called + * outside vblank irq, a non-zero count is not 100% safe. Unfortunately + * we must also accept a refcount of 1, as whenever we are called from + * drm_vblank_get() -> drm_vblank_enable() the refcount will be 1 and + * we must let that one pass through in order to not lose vblank counts + * during vblank irq off - which would completely defeat the whole + * point of this routine. + * + * Whenever we are called from vblank irq, we have to assume concurrent + * readers exist or can show up any time during our execution, even if + * the refcount is currently zero, as vblank irqs are usually only + * enabled due to the presence of readers, and because when we are called + * from vblank irq we can't hold the vbl_lock to protect us from sudden + * bumps in vblank refcount. Therefore also restrict bumps to +1 when + * called from vblank irq. + */ + if ((diff > 1) && (atomic_read(&vblank->refcount) > 1 || + (flags & DRM_CALLED_FROM_VBLIRQ))) { + DRM_DEBUG_VBL("clamping vblank bump to 1 on crtc %u: diffr=%u " + "refcount %u, vblirq %u\n", pipe, diff, + atomic_read(&vblank->refcount), + (flags & DRM_CALLED_FROM_VBLIRQ) != 0); + diff = 1; + } + DRM_DEBUG_VBL("updating vblank count on crtc %u:" " current=%u, diff=%u, hw=%u hw_last=%u\n", pipe, vblank->count, diff, cur_vblank, vblank->last);