From patchwork Fri Mar 1 10:39:03 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 208895 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7301:2097:b0:108:e6aa:91d0 with SMTP id gs23csp1154187dyb; Fri, 1 Mar 2024 07:26:52 -0800 (PST) X-Forwarded-Encrypted: i=3; AJvYcCXdF5YBmwKibVnZWQ/+SzMcJf4i+mBgnxf1181cTw/gaiNblt1EDIhEavche1Xkbv6hChDmSCLSx8A4/nSWdSQy3SEGOA== X-Google-Smtp-Source: AGHT+IENW53sCuGWXVF0aANx+SIXvcVLuEuNQgVI4h4rKERlRXpCBULOjlcFPq6OaC/mPqK8MAMP X-Received: by 2002:a05:6402:7d4:b0:564:f9d1:6b14 with SMTP id u20-20020a05640207d400b00564f9d16b14mr1787356edy.14.1709306812500; Fri, 01 Mar 2024 07:26:52 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1709306812; cv=pass; d=google.com; s=arc-20160816; b=mMLQWapwakCQCnmzXGYMpsr47cxUq8VZ+GKwTlaAqTvqy2egN30cjEwPAKd4GucM/k BCo1VYmXYYBMfa015PDKfeMMXhOqrW+6VeRkrjJoAS5uHLuQ762Nf5t+eWaX4R+cJAR5 Zyzd5ZoMSkt95Ff+3In2ZWsXhO5Gr84mw1bbRu/g9FvrzkCGub0e6AhqvwT2C5jkA5tE F0wNSN5PehpR8c8mvicXFwF3cqbpORob0s8VBB7FEHecoZFA7bZA0Ve+t1zFiXukUvsw /BzUMXf+m6G9CLvfc3QK/h4A585wKxEX4pFB4MCZHMAUobRcparx9W8gH63Vh318OCQz wKrw== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:references:in-reply-to:message-id :date:subject:cc:to:from:dkim-signature; bh=fePj1lO9tth4Z81r0wTp5zyq5wY/LQ0qjbQTJUa59fQ=; fh=HTXlE8wRLLSCxBDJcgLE+zXQ0rOCbmso+QRC6RPsEys=; b=ouOqcbBXQWaC7x41tTvzdtAB3zUmzzUWgmmJw83eIWZJQTXUe+hCZ2x5JV0jteuq4d leiIosBklD/Bv0gID86wLPY/rIy8mYpgy8hMFeRbteFBrg83iDZVt86ZpHu8Jafb9m0M u4RvbCH4ZEU//MRZfsa+wAp2c/b/2ghoP/WszCA/AWkwpwPR1hfxu6txY7TPZxMzAK18 lzPCzUTHZevLoTIn2oFhVUvHoeZ3n4uHtb0PYepCww69eNL0lOhKAygmS9JrZrcuVKJ5 ubWHg0uVJ9GAqD3t4k1Bhj/VZvMGgaWd25l9FrYGnbCOgswEG7xS1bjXiXAnn4V4ADA2 7rug==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@ffwll.ch header.s=google header.b="CgNYaF/Z"; arc=pass (i=1 dkim=pass dkdomain=ffwll.ch); spf=pass (google.com: domain of linux-kernel+bounces-88260-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) smtp.mailfrom="linux-kernel+bounces-88260-ouuuleilei=gmail.com@vger.kernel.org" Received: from am.mirrors.kernel.org (am.mirrors.kernel.org. [147.75.80.249]) by mx.google.com with ESMTPS id r8-20020a056402234800b00566d84b50b1si512771eda.327.2024.03.01.07.26.52 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 01 Mar 2024 07:26:52 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-88260-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249; Authentication-Results: mx.google.com; dkim=pass header.i=@ffwll.ch header.s=google header.b="CgNYaF/Z"; arc=pass (i=1 dkim=pass dkdomain=ffwll.ch); spf=pass (google.com: domain of linux-kernel+bounces-88260-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) smtp.mailfrom="linux-kernel+bounces-88260-ouuuleilei=gmail.com@vger.kernel.org" Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by am.mirrors.kernel.org (Postfix) with ESMTPS id 433E71F26F7C for ; Fri, 1 Mar 2024 10:39:25 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 5BC6C6BFA0; Fri, 1 Mar 2024 10:39:14 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b="CgNYaF/Z" Received: from mail-ej1-f52.google.com (mail-ej1-f52.google.com [209.85.218.52]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 81B6716FF46 for ; Fri, 1 Mar 2024 10:39:07 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.218.52 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709289550; cv=none; b=mZ68cppoc2iCrtw8VXj3Ev4TEqgpeKxK/I8Teexcb4mspF908/jt+pJbFPIC7KVA7/chNz+51xfHvMJJrtsJIfEDebakoNEFwWYLV6NXeTeXqi5Tlrsb+/30UIhgMYhvVmL4oyOHs0qbpUd2syaODPUyPwQ7Cn/aAkpz0sbIKPM= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709289550; c=relaxed/simple; bh=SmsNgOeXcYBeZVMj7SI3Q2vhFXai5DXhHbFFk4IH5JE=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=CwyR1WCxjQ1IsK9+loEG2STYMnRRXaJEfxKoAiXJ2Tge61Zz8fFX4RYLn5fLpsfwGX8EHxtNs0BQ7XD4JsgXWcd3cHOv/3Jlyv+Uz9lOQFG/zIrle9y4AWPYCpj5k10o3vudeCJ5yCkk23rZPWqL9tSMOS282VmldVu6HGsXCKc= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch; spf=none smtp.mailfrom=ffwll.ch; dkim=pass (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b=CgNYaF/Z; arc=none smtp.client-ip=209.85.218.52 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=ffwll.ch Received: by mail-ej1-f52.google.com with SMTP id a640c23a62f3a-a4462fbf098so37257366b.1 for ; Fri, 01 Mar 2024 02:39:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; t=1709289546; x=1709894346; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=fePj1lO9tth4Z81r0wTp5zyq5wY/LQ0qjbQTJUa59fQ=; b=CgNYaF/ZHWu4AMDyHNfTEgO6K0UrYH+cJOlgxyNTndIn7ALLLylIR6FWSXCYlzqe0l q66SgSj0mVxxIUeFikUWSWD2qVf3ucFmpjaVt0EJH/3zTIOQUeMbZXPXuPohwM+xo5Ak X5W+dDMTJviPKJJSgxpYyTu4ZEiiMdm9kp42w= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1709289546; x=1709894346; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=fePj1lO9tth4Z81r0wTp5zyq5wY/LQ0qjbQTJUa59fQ=; b=qYO1JMu4xa3rt44io8xB7eVvHXJ09QGsJqhaXkgAd7CUcUpuKNkn+65GB2RWVMJshL kVuSS2rerC6+mZ208bwleIvDknGrUgX91zZbYKhy6VJ2kuVhWzejgYD7JcmzL/jDxNJf HJD9/+NW/2KxL4m9ilVj8HVXvEPXu+HFR+LEIUq8C2MRS4CTqeMVYjhSH9w++01TE90z R64zgRBKFTXNL5BsKWUxgl8sfzQdQBLb0ewCS7vNpJ3fsdCJmy61ZaDw0Si8fDRh5HjP 8oLXmPKHASSzf/DslpwQA7JDCfJEIlIcN4Kxk6rFcoIlinZGsxXt0mAMQID9+6DXWMDV OCZA== X-Gm-Message-State: AOJu0YxqhxesplukLqi4PP2GYN1JwRTm8gtYm5Gb+8UDx5tP75exzZVz w0JwglHpXmpsLPbZ1aZtRlzyqhCbH7aUU7MfUr0kUvIktnzy0Vj0e9BNctNnZF0= X-Received: by 2002:a17:907:1608:b0:a44:205d:bb84 with SMTP id cw8-20020a170907160800b00a44205dbb84mr853956ejd.2.1709289545617; Fri, 01 Mar 2024 02:39:05 -0800 (PST) Received: from phenom.ffwll.local ([2a02:168:57f4:0:efd0:b9e5:5ae6:c2fa]) by smtp.gmail.com with ESMTPSA id j11-20020a5d604b000000b0033d9ee09b7asm4197063wrt.107.2024.03.01.02.39.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 01 Mar 2024 02:39:05 -0800 (PST) From: Daniel Vetter To: DRI Development Cc: LKML , Daniel Vetter , Daniel Vetter , Jocelyn Falempe , Andrew Morton , "Peter Zijlstra (Intel)" , Lukas Wunner , Petr Mladek , Steven Rostedt , John Ogness , Sergey Senozhatsky , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Daniel Vetter Subject: [RFC] drm/panic: Add drm panic locking Date: Fri, 1 Mar 2024 11:39:03 +0100 Message-ID: <20240301103903.2538083-1-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240301100516.2516297-1-daniel.vetter@ffwll.ch> References: <20240301100516.2516297-1-daniel.vetter@ffwll.ch> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1792322899631721683 X-GMAIL-MSGID: 1792338100243351795 Rough sketch for the locking of drm panic printing code. The upshot of this approach is that we can pretty much entirely rely on the atomic commit flow, with the pair of raw_spin_lock/unlock providing any barriers we need, without having to create really big critical sections in code. This also avoids the need that drivers must explicitly update the panic handler state, which they might forget to do, or not do consistently, and then we blow up in the worst possible times. It is somewhat racy against a concurrent atomic update, and we might write into a buffer which the hardware will never display. But there's fundamentally no way to avoid that - if we do the panic state update explicitly after writing to the hardware, we might instead write to an old buffer that the user will barely ever see. Note that an rcu protected deference of plane->state would give us the the same guarantees, but it has the downside that we then need to protect the plane state freeing functions with call_rcu too. Which would very widely impact a lot of code and therefore doesn't seem worth the complexity compared to a raw spinlock with very tiny critical sections. Plus rcu cannot be used to protect access to peek/poke registers anyway, so we'd still need it for those cases. Peek/poke registers for vram access (or a gart pte reserved just for panic code) are also the reason I've gone with a per-device and not per-plane spinlock, since usually these things are global for the entire display. Going with per-plane locks would mean drivers for such hardware would need additional locks, which we don't want, since it deviates from the per-console takeoverlocks design. Longer term it might be useful if the panic notifiers grow a bit more structure than just the absolute bare EXPORT_SYMBOL(panic_notifier_list) - somewhat aside, why is that not EXPORT_SYMBOL_GPL ... If panic notifiers would be more like console drivers with proper register/unregister interfaces we could perhaps reuse the very fancy console lock with all it's check and takeover semantics that John Ogness is developing to fix the console_lock mess. But for the initial cut of a drm panic printing support I don't think we need that, because the critical sections are extremely small and only happen once per display refresh. So generally just 60 tiny locked sections per second, which is nothing compared to a serial console running a 115kbaud doing really slow mmio writes for each byte. So for now the raw spintrylock in drm panic notifier callback should be good enough. Another benefit of making panic notifiers more like full blown consoles (that are used in panics only) would be that we get the two stage design, where first all the safe outputs are used. And then the dangerous takeover tricks are deployed (where for display drivers we also might try to intercept any in-flight display buffer flips, which if we race and misprogram fifos and watermarks can hang the memory controller on some hw). For context the actual implementation on the drm side is by Jocelyn and this patch is meant to be combined with the overall approach in v7 (v8 is a bit less flexible, which I think is the wrong direction): https://lore.kernel.org/dri-devel/20240104160301.185915-1-jfalempe@redhat.com/ Note that the locking is very much not correct there, hence this separate rfc. v2: - fix authorship, this was all my typing - some typo oopsies - link to the drm panic work by Jocelyn for context Signed-off-by: Daniel Vetter Cc: Jocelyn Falempe Cc: Andrew Morton Cc: "Peter Zijlstra (Intel)" Cc: Lukas Wunner Cc: Petr Mladek Cc: Steven Rostedt Cc: John Ogness Cc: Sergey Senozhatsky Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: Thomas Zimmermann Cc: David Airlie Cc: Daniel Vetter --- drivers/gpu/drm/drm_atomic_helper.c | 3 + include/drm/drm_mode_config.h | 10 +++ include/drm/drm_panic.h | 99 +++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+) create mode 100644 include/drm/drm_panic.h diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 40c2bd3e62e8..5a908c186037 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -38,6 +38,7 @@ #include #include #include +#include #include #include #include @@ -3086,6 +3087,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, } } + drm_panic_lock(state->dev); for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { WARN_ON(plane->state != old_plane_state); @@ -3095,6 +3097,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, state->planes[i].state = old_plane_state; plane->state = new_plane_state; } + drm_panic_unlock(state->dev); for_each_oldnew_private_obj_in_state(state, obj, old_obj_state, new_obj_state, i) { WARN_ON(obj->state != old_obj_state); diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index 973119a9176b..e79f1a557a22 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -505,6 +505,16 @@ struct drm_mode_config { */ struct list_head plane_list; + /** + * @panic_lock: + * + * Raw spinlock used to protect critical sections of code that access + * the display hardware or modeset software state, which the panic + * printing code must be protected against. See drm_panic_trylock(), + * drm_panic_lock() and drm_panic_unlock(). + */ + struct raw_spinlock panic_lock; + /** * @num_crtc: * diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h new file mode 100644 index 000000000000..f2135d03f1eb --- /dev/null +++ b/include/drm/drm_panic.h @@ -0,0 +1,99 @@ +/* SPDX-License-Identifier: GPL-2.0 or MIT */ +#ifndef __DRM_PANIC_H__ +#define __DRM_PANIC_H__ + +#include +/* + * Copyright (c) 2024 Intel + */ + +/** + * drm_panic_trylock - try to enter the panic printing critical section + * @dev: struct drm_device + * + * This function must be called by any panic printing code. The panic printing + * attempt must be aborted if the trylock fails. + * + * Panic printing code can make the following assumptions while holding the + * panic lock: + * + * - Anything protected by drm_panic_lock() and drm_panic_unlock() pairs is safe + * to access. + * + * - Furthermore the panic printing code only registers in drm_dev_unregister() + * and gets removed in drm_dev_unregister(). This allows the panic code to + * safely access any state which is invariant in between these two function + * calls, like the list of planes drm_mode_config.plane_list or most of the + * struct drm_plane structure. + * + * Specifically thanks to the protection around plane updates in + * drm_atomic_helper_swap_state() the following additional guarantees hold: + * + * - It is safe to deference the drm_plane.state pointer. + * + * - Anything in struct drm_plane_state or the driver's subclass thereof which + * stays invariant after the atomic check code has finished is safe to access. + * Specifically this includes the reference counted pointers to framebuffer + * and buffer objects. + * + * - Anything set up by drm_plane_helper_funcs.fb_prepare and cleaned up + * drm_plane_helper_funcs.fb_cleanup is safe to access, as long as it stays + * invariant between these two calls. This also means that for drivers using + * dynamic buffer management the framebuffer is pinned, and therefer all + * relevant datastructures can be accessed without taking any further locks + * (which would be impossible in panic context anyway). + * + * - Importantly, software and hardware state set up by + * drm_plane_helper_funcs.begin_fb_access and + * drm_plane_helper_funcs.end_fb_access is not safe to access. + * + * Drivers must not make any assumptions about the actual state of the hardware, + * unless they explicitly protected these hardware access with drm_panic_lock() + * and drm_panic_unlock(). + * + * Returns: + * + * 0 when failing to acquire the raw spinlock, nonzero on success. + */ +static inline int drm_panic_trylock(struct drm_device *dev) +{ + return raw_spin_trylock(&dev->mode_config.panic_lock); +} + +/** + * drm_panic_lock - protect panic printing relevant state + * @dev: struct drm_device + * + * This function must be called to protect software and hardware state that the + * panic printing code must be able to rely on. The protected sections must be + * as small as possible. Examples include: + * + * - Access to peek/poke or other similar registers, if that is the way the + * driver prints the pixels into the scanout buffer at panic time. + * + * - Updates to pointers like drm_plane.state, allowing the panic handler to + * safely deference these. This is done in drm_atomic_helper_swap_state(). + * + * - An state that isn't invariant and that the driver must be able to access + * during panic printing. + * + * Call drm_panic_unlock() to unlock the locked spinlock. + */ +static inline void drm_panic_lock(struct drm_device *dev) +{ + return raw_spin_lock(&dev->mode_config.panic_lock); +} + +/** + * drm_panic_unlock - end of the panic printing critical section + * @dev: struct drm_device + * + * Unlocks the raw spinlock acquired by either drm_panic_lock() or + * drm_panic_trylock(). + */ +static inline void drm_panic_unlock(struct drm_device *dev) +{ + raw_spin_unlock(&dev->mode_config.panic_lock); +} + +#endif /* __DRM_PANIC_H__ */