From patchwork Fri Feb 12 19:30:28 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mario Kleiner X-Patchwork-Id: 8296651 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 3F11DC02AA for ; Fri, 12 Feb 2016 19:31:45 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 3041A20460 for ; Fri, 12 Feb 2016 19:31:44 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id DFC1120452 for ; Fri, 12 Feb 2016 19:31:42 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 38EF06EA54; Fri, 12 Feb 2016 11:31:42 -0800 (PST) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-wm0-f66.google.com (mail-wm0-f66.google.com [74.125.82.66]) by gabe.freedesktop.org (Postfix) with ESMTPS id DB5436EA54 for ; Fri, 12 Feb 2016 11:31:40 -0800 (PST) Received: by mail-wm0-f66.google.com with SMTP id p63so4400046wmp.1 for ; Fri, 12 Feb 2016 11:31:40 -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=6Am4iwndVblVuLxso3DWKtgXvAGhQAJ0ivndyRdUX0E=; b=jI3P6N816j2EzkaGAiGqiLZ0Zm3TE+digg+LpwKtp+17xgUAhRW8lyg5/Ara+PBIi2 90UmOuDBFAOvdaWa3ze8UYCGzrkE06stEjIv+dQYMEZ7ewaQJeiNp9TeooR4IS2HhUIy 6ulRDZfFCQ7OAo4paZgge+fbFSQCR8Npxc0sc1DBG2DOK2KrayZkZKvj6ZN4XZndU7JG 9bFh1Vsw7YZxvDk07xyqQKpbsHqskNZb4JyypwbV16bG4smgImahlHkhZg6T/NM/uOxD ZIL5+e4Y9xQSCKb6jnYQimuxnExANzByfkQ7B7mPwMzNMO1hnoB6sVYplVxa3UDZkHpx yJLA== 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=6Am4iwndVblVuLxso3DWKtgXvAGhQAJ0ivndyRdUX0E=; b=g5cfi2BcuN4+Tv6fYsm2PxQaCnXD+4lEoQRnyNGr7yECD+8SFc6jgi0+mQ/xsg6Kio kT/Gj7gEmAhJ/RdVf7/MIiS6vQS9EMRW/atqxlNQyU3IL42KEN46S/ENQfwnIB4ao6FJ iHH+dHMO2qArzHnxxwfZ8+Ad6prb9rxhdvE7loZtisw5ZaTkHAH8nYiMJ7aR+h22lE1B 0ffsGEwZmhuSK4drM/MeKcMBifaA5Bs4og4PUxS8vUZP38BC2SopqBlVtfIK8UFhXBLN E4K1xO0NRyNHMAHyE+1viM6fUFccwERapqpAUqm4QL/o7KmFoar4c0d/UZKsddWysMyc Gqtg== X-Gm-Message-State: AG10YOSeRmaDNlZK7F+GUuSTT1nxl2OZnadUYycYLFE8GP9Qp48Hk1CUi4Cc4aaiVS/fGg== X-Received: by 10.28.140.210 with SMTP id o201mr4920368wmd.28.1455305454692; Fri, 12 Feb 2016 11:30:54 -0800 (PST) Received: from twisty.cin.medizin.uni-tuebingen.de (cin-11.medizin.uni-tuebingen.de. [134.2.118.242]) by smtp.gmail.com with ESMTPSA id v78sm3637797wmv.23.2016.02.12.11.30.53 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Fri, 12 Feb 2016 11:30:53 -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. (v2) Date: Fri, 12 Feb 2016 20:30:28 +0100 Message-Id: <1455305432-28770-3-git-send-email-mario.kleiner.de@gmail.com> X-Mailer: git-send-email 1.9.1 In-Reply-To: <1455305432-28770-1-git-send-email-mario.kleiner.de@gmail.com> References: <1455305432-28770-1-git-send-email-mario.kleiner.de@gmail.com> Cc: daniel.vetter@ffwll.ch, michel@daenzer.net, 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.2 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. A better solution would be to rewrite the timestamp caching to use full seqlocks to allow concurrent writes and reads for arbitrary vblank counter increments. v2: Add code comment that this is essentially a hack and should be replaced by a full seqlock implementation for caching of timestamps. Signed-off-by: Mario Kleiner Reviewed-by: Daniel Vetter 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 --- drivers/gpu/drm/drm_irq.c | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 1a18033..92ad62f 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -221,6 +221,49 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, diff = (flags & DRM_CALLED_FROM_VBLIRQ) != 0; } + /* + * FIMXE: Need to replace this hack with proper seqlocks. + * + * 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);