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(-)
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 + * 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;
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.
- 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:
- 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.
- 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.
- 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.
- 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.@@ -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.@@ -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.-- 2.24.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Dec 05, 2019 at 05:02:24PM +0100, Thierry Reding wrote:
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.
- 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:
- 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.
- 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.
- 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
All your comments applied and patch merged, thanks for your review. -Daniel
- 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.
- 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.@@ -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.@@ -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.-- 2.24.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Daniel Vetter -
Thierry Reding