Re: [PATCH] drm/atomc: Update docs around locking and commit sequencing

On Wed, Dec 04, 2019 at 11:00:11AM +0100, Daniel Vetter wrote:

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 treding@nvidia.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

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(-)

Hi Daniel,

"drm/atomic" in the subject. A couple more minor things I noticed below.

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 <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<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 <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 <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 <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

"affected _by_ the new state update"?

• • 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

I think "therefor" has a slightly different meaning than "therefore", and I think you actually want the latter in this case.

• • 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

"code"

• • multiple CRTCs in parallel. In general, adding additional state structures
• • should be avoided as much as possible, because this reduces parallism in

"parallelism"

• • (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

I stumbled across this a couple of times when reading it. I think it becomes clearer when you replace the comma by a colon:

Note that commit workers do not hold any locks: their access is only coordinated through ordering.

Or perhaps replace the comma with "and"?

Other than that, looks good to me:

Reviewed-by: Thierry Reding treding@nvidia.com

• • 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;

-- 2.24.0

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel