From patchwork Wed Dec 4 10:00:11 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 11272679 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id F1C36138C for ; Wed, 4 Dec 2019 10:00:22 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id D812B206DB for ; Wed, 4 Dec 2019 10:00:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D812B206DB Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 0B75D72F52; Wed, 4 Dec 2019 10:00:22 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-wm1-x342.google.com (mail-wm1-x342.google.com [IPv6:2a00:1450:4864:20::342]) by gabe.freedesktop.org (Postfix) with ESMTPS id DFB7272F45 for ; Wed, 4 Dec 2019 10:00:19 +0000 (UTC) Received: by mail-wm1-x342.google.com with SMTP id g206so6326868wme.1 for ; Wed, 04 Dec 2019 02:00:19 -0800 (PST) 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:mime-version :content-transfer-encoding; bh=rrfBcEHBnhRJhBOtjy/7OJOEpgyByBGNF7k8q48remw=; b=hVas+BC4NxE9TWth4+WAossDAqvT/NBL+1CPf3DQ+thN6uzzOvTgWlzbKmPy1jvrDg 5CroKup2AHYUXmiVO563RaY0UNfC8lxvBEzv3QuuSTeeM7+2OH8XwlCUT3CFC80NKxfN ie4qKHcC22qIg8qXhA/xu+N1GhNrL/HlW+vSEiaMIo3ITiW+tG+3Y9wQXFCsvwAnzAXT loI6AESBn7kjm8A1fgjq7iCo4/Xlb/rCWzWBjMdvsWwsC9igju7O4mFlqYO9kkVinCA7 o6UoeL3rnghvbBQ66isIDLaIFGIA/aX97DWO8kLRXdnZWgMoosucaPiQ9pUr8UdyL7BJ OASQ== X-Gm-Message-State: APjAAAVsxEmcT6b/VdnnHAgpnL3rPy5Bjt3KxQRfYeNf7tyxEIK1Q0gp qFE0c9eVjmoXQ4L0NfwFNRG/6T8VYes= X-Google-Smtp-Source: APXvYqzas/zrVpAK3UID7BuHrh5lYRJp8QEfk95C1d48vNcLaNFdatPF7Y+M9SedvpIk7Ua96YJVOA== X-Received: by 2002:a7b:cd8b:: with SMTP id y11mr31955883wmj.95.1575453617943; Wed, 04 Dec 2019 02:00:17 -0800 (PST) Received: from phenom.ffwll.local (212-51-149-96.fiber7.init7.net. [212.51.149.96]) by smtp.gmail.com with ESMTPSA id p5sm7268009wrt.79.2019.12.04.02.00.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 04 Dec 2019 02:00:17 -0800 (PST) From: Daniel Vetter To: DRI Development Subject: [PATCH] drm/atomc: Update docs around locking and commit sequencing Date: Wed, 4 Dec 2019 11:00:11 +0100 Message-Id: <20191204100011.859468-1-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 2.24.0 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:mime-version :content-transfer-encoding; bh=rrfBcEHBnhRJhBOtjy/7OJOEpgyByBGNF7k8q48remw=; b=WR4W5iHpksSpgKLjNrOvoLEz4tFNY7sc4JVNGeFInVZIUNqdcu3ngHzk1tCBt21DPo dDWfV+1sP/zNbgGldsXX6F3lEsMrC7SjJ/GTUCtcPUSUWegLq3VlgFk24bwgUstxhZKk BzRmhFS2NGceqWbmARZe+DIAY14SIZ7+tvIBE= X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Daniel Vetter , Thierry Reding , Daniel Vetter Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Both locking and especially sequencing of nonblocking commits have evolved a lot. The details are all there, but I noticed that the big picture and connections have fallen behind a bit. Apply polish. Motivated by some review discussions with Thierry. Cc: Thierry Reding Signed-off-by: Daniel Vetter Reviewed-by: Thierry Reding --- Documentation/gpu/drm-kms.rst | 11 ++++++- drivers/gpu/drm/drm_atomic.c | 10 ++++--- drivers/gpu/drm/drm_atomic_helper.c | 46 ++++++++++++++++++----------- include/drm/drm_atomic.h | 13 ++++++-- 4 files changed, 56 insertions(+), 24 deletions(-) diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst index c68588ce4090..b9330343d1bc 100644 --- a/Documentation/gpu/drm-kms.rst +++ b/Documentation/gpu/drm-kms.rst @@ -260,7 +260,8 @@ Taken all together there's two consequences for the atomic design: drm_connector_state ` for connectors. These are the only objects with userspace-visible and settable state. For internal state drivers can subclass these structures through embeddeding, or add entirely new state - structures for their globally shared hardware functions. + structures for their globally shared hardware functions, see :c:type:`struct + drm_private_state`. - An atomic update is assembled and validated as an entirely free-standing pile of structures within the :c:type:`drm_atomic_state ` @@ -269,6 +270,14 @@ Taken all together there's two consequences for the atomic design: to the driver and modeset objects. This way rolling back an update boils down to releasing memory and unreferencing objects like framebuffers. +Locking of atomic state structures is internally using :c:type:`struct +drm_modeset_lock `. As a general rule the locking shouldn't be +exposed to drivers, instead the right locks should be automatically acquired by +any function that duplicates or peeks into a state, like e.g. +:c:func:`drm_atomic_get_crtc_state()`. Locking only protects the software data +structure, ordering of committing state changes to hardware is sequenced using +:c:type:`struct drm_crtc_commit `. + Read on in this chapter, and also in :ref:`drm_atomic_helper` for more detailed coverage of specific topics. diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index a351a9a39530..5b4787e33f0d 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -688,10 +688,12 @@ static void drm_atomic_plane_print_state(struct drm_printer *p, * associated state struct &drm_private_state. * * Similar to userspace-exposed objects, private state structures can be - * acquired by calling drm_atomic_get_private_obj_state(). Since this function - * does not take care of locking, drivers should wrap it for each type of - * private state object they have with the required call to drm_modeset_lock() - * for the corresponding &drm_modeset_lock. + * acquired by calling drm_atomic_get_private_obj_state(). This also takes care + * of locking, hence drivers should not have a need to call drm_modeset_lock() + * directly. Sequence of the actual hardware state commit is not handled, + * drivers might need to keep track of struct drm_crtc_commit within subclassed + * structure of &drm_private_state as necessary, e.g. similar to + * &drm_plane_state.commit. See also &drm_atomic_state.fake_commit. * * All private state structures contained in a &drm_atomic_state update can be * iterated using for_each_oldnew_private_obj_in_state(), diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 711801b9d4f1..10d62f726b22 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1827,17 +1827,21 @@ EXPORT_SYMBOL(drm_atomic_helper_commit); /** * DOC: implementing nonblocking commit * - * Nonblocking atomic commits have to be implemented in the following sequence: + * Nonblocking atomic commits should use struct &drm_crtc_commit to sequence + * different operations against each another. Locks, especially struct + * &drm_modeset_lock, should not be held in worker threads or any other + * asynchronous context used to commit the hardware state. * - * 1. Run drm_atomic_helper_prepare_planes() first. This is the only function - * which commit needs to call which can fail, so we want to run it first and + * drm_atomic_helper_commit() implements the recommended sequence for + * nonblocking commits, using drm_atomic_helper_setup_commit() internally: + * + * 1. Run drm_atomic_helper_prepare_planes(). Since this can fail and we + * need to propagate out of memory/VRAM errors to userspace, it must be called * synchronously. * * 2. Synchronize with any outstanding nonblocking commit worker threads which - * might be affected the new state update. This can be done by either cancelling - * or flushing the work items, depending upon whether the driver can deal with - * cancelled updates. Note that it is important to ensure that the framebuffer - * cleanup is still done when cancelling. + * might be affected the new state update. This is handled by + * drm_atomic_helper_setup_commit(). * * Asynchronous workers need to have sufficient parallelism to be able to run * different atomic commits on different CRTCs in parallel. The simplest way to @@ -1848,21 +1852,29 @@ EXPORT_SYMBOL(drm_atomic_helper_commit); * must be done as one global operation, and enabling or disabling a CRTC can * take a long time. But even that is not required. * + * IMPORTANT: A &drm_atomic_state update for multiple CRTCs is sequenced + * against all CRTCs therein. Therefor for atomic state updates which only flip + * planes the driver must not get the struct &drm_crtc_state of unrelated CRTCs + * in its atomic check codee: This would prevent committing of atomic updates to + * multiple CRTCs in parallel. In general, adding additional state structures + * should be avoided as much as possible, because this reduces parallism in + * (nonblocking) commits, both due to locking and due to commit sequencing + * requirements. + * * 3. The software state is updated synchronously with * drm_atomic_helper_swap_state(). Doing this under the protection of all modeset - * locks means concurrent callers never see inconsistent state. And doing this - * while it's guaranteed that no relevant nonblocking worker runs means that - * nonblocking workers do not need grab any locks. Actually they must not grab - * locks, for otherwise the work flushing will deadlock. + * locks means concurrent callers never see inconsistent state. Note that commit + * workers do not hold any locks, their access is only coordinated through + * ordering. If workers would access state only through the pointers in the + * free-standing state objects (currently not the case for any driver) then even + * multiple pending commits could be in-flight at the same time. * * 4. Schedule a work item to do all subsequent steps, using the split-out * commit helpers: a) pre-plane commit b) plane commit c) post-plane commit and * then cleaning up the framebuffers after the old framebuffer is no longer - * being displayed. - * - * The above scheme is implemented in the atomic helper libraries in - * drm_atomic_helper_commit() using a bunch of helper functions. See - * drm_atomic_helper_setup_commit() for a starting point. + * being displayed. The scheduled work should synchronize against other workers + * using the &drm_crtc_commit infrastructure as needed. See + * drm_atomic_helper_setup_commit() for more details. */ static int stall_checks(struct drm_crtc *crtc, bool nonblock) @@ -2085,7 +2097,7 @@ EXPORT_SYMBOL(drm_atomic_helper_setup_commit); * * This function waits for all preceeding commits that touch the same CRTC as * @old_state to both be committed to the hardware (as signalled by - * drm_atomic_helper_commit_hw_done) and executed by the hardware (as signalled + * drm_atomic_helper_commit_hw_done()) and executed by the hardware (as signalled * by calling drm_crtc_send_vblank_event() on the &drm_crtc_state.event). * * This is part of the atomic helper support for nonblocking commits, see diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index b6c73fd9f55a..5923819dcd68 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -60,8 +60,8 @@ * wait for flip_done <---- * clean up atomic state * - * The important bit to know is that cleanup_done is the terminal event, but the - * ordering between flip_done and hw_done is entirely up to the specific driver + * The important bit to know is that &cleanup_done is the terminal event, but the + * ordering between &flip_done and &hw_done is entirely up to the specific driver * and modeset state change. * * For an implementation of how to use this look at @@ -92,6 +92,9 @@ struct drm_crtc_commit { * commit is sent to userspace, or when an out-fence is singalled. Note * that for most hardware, in most cases this happens after @hw_done is * signalled. + * + * Completion of this stage is signalled implicitly by calling + * drm_crtc_send_vblank_event() on &drm_crtc_state.event. */ struct completion flip_done; @@ -107,6 +110,9 @@ struct drm_crtc_commit { * Note that this does not need to include separately reference-counted * resources like backing storage buffer pinning, or runtime pm * management. + * + * Drivers should call drm_atomic_helper_commit_hw_done() to signal + * completion of this stage. */ struct completion hw_done; @@ -118,6 +124,9 @@ struct drm_crtc_commit { * a vblank wait completed it might be a bit later. This completion is * useful to throttle updates and avoid hardware updates getting ahead * of the buffer cleanup too much. + * + * Drivers should call drm_atomic_helper_commit_cleanup_done() to signal + * completion of this stage. */ struct completion cleanup_done;