[PATCH] drm/atomic: Unconfuse the old_state mess in commmit_tail

[PATCH] drm/atomic: Unconfuse the old_state mess in commmit_tail

[Daniel Vetter]

I totally butcherd the job on typing the kernel-doc for these, and no
one realized. Noticed by Russell. Maarten has a more complete approach
to this confusion, by making it more explicit what the new/old state
is, instead of this magic switching behaviour.

Cc: Liviu Dudau <liviu.dudau@arm.com>
Reported-by: Russell King - ARM Linux <linux@armlinux.org.uk>
Cc: Russell King - ARM Linux <linux@armlinux.org.uk>
Fixes: 9f2a7950e77a ("drm/atomic-helper: nonblocking commit support")
Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Tomeu Vizoso <tomeu.vizoso@gmail.com>
Cc: Daniel Stone <daniels@collabora.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c      | 66 ++++++++++++++++----------------
 include/drm/drm_modeset_helper_vtables.h | 12 ++++--
 2 files changed, 41 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 0b16587cdc62..94cde2d3a2ce 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1005,7 +1005,7 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
 /**
  * drm_atomic_helper_wait_for_fences - wait for fences stashed in plane state
  * @dev: DRM device
- * @state: atomic state object with old state structures
+ * @old_state: atomic state object with old state structures
  * @pre_swap: if true, do an interruptible wait
  *
  * For implicit sync, driver should fish the exclusive fence out from the
@@ -1016,14 +1016,14 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
  * Returns zero if success or < 0 if dma_fence_wait() fails.
  */
 int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
-				      struct drm_atomic_state *state,
+				      struct drm_atomic_state *old_state,
 				      bool pre_swap)
 {
 	struct drm_plane *plane;
 	struct drm_plane_state *plane_state;
 	int i, ret;
 
-	for_each_plane_in_state(state, plane, plane_state, i) {
+	for_each_plane_in_state(old_state, plane, plane_state, i) {
 		if (!pre_swap)
 			plane_state = plane->state;
 
@@ -1147,7 +1147,7 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
 
 /**
  * drm_atomic_helper_commit_tail - commit atomic update to hardware
- * @state: new modeset state to be committed
+ * @old_state: atomic state object with old state structures
  *
  * This is the default implemenation for the ->atomic_commit_tail() hook of the
  * &drm_mode_config_helper_funcs vtable.
@@ -1158,53 +1158,53 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
  *
  * For drivers supporting runtime PM the recommended sequence is instead ::
  *
- *     drm_atomic_helper_commit_modeset_disables(dev, state);
+ *     drm_atomic_helper_commit_modeset_disables(dev, old_state);
  *
- *     drm_atomic_helper_commit_modeset_enables(dev, state);
+ *     drm_atomic_helper_commit_modeset_enables(dev, old_state);
  *
- *     drm_atomic_helper_commit_planes(dev, state,
+ *     drm_atomic_helper_commit_planes(dev, old_state,
  *                                     DRM_PLANE_COMMIT_ACTIVE_ONLY);
  *
  * for committing the atomic update to hardware.  See the kerneldoc entries for
  * these three functions for more details.
  */
-void drm_atomic_helper_commit_tail(struct drm_atomic_state *state)
+void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
 {
-	struct drm_device *dev = state->dev;
+	struct drm_device *dev = old_state->dev;
 
-	drm_atomic_helper_commit_modeset_disables(dev, state);
+	drm_atomic_helper_commit_modeset_disables(dev, old_state);
 
-	drm_atomic_helper_commit_planes(dev, state, 0);
+	drm_atomic_helper_commit_planes(dev, old_state, 0);
 
-	drm_atomic_helper_commit_modeset_enables(dev, state);
+	drm_atomic_helper_commit_modeset_enables(dev, old_state);
 
-	drm_atomic_helper_commit_hw_done(state);
+	drm_atomic_helper_commit_hw_done(old_state);
 
-	drm_atomic_helper_wait_for_vblanks(dev, state);
+	drm_atomic_helper_wait_for_vblanks(dev, old_state);
 
-	drm_atomic_helper_cleanup_planes(dev, state);
+	drm_atomic_helper_cleanup_planes(dev, old_state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_tail);
 
-static void commit_tail(struct drm_atomic_state *state)
+static void commit_tail(struct drm_atomic_state *old_state)
 {
-	struct drm_device *dev = state->dev;
+	struct drm_device *dev = old_state->dev;
 	struct drm_mode_config_helper_funcs *funcs;
 
 	funcs = dev->mode_config.helper_private;
 
-	drm_atomic_helper_wait_for_fences(dev, state, false);
+	drm_atomic_helper_wait_for_fences(dev, old_state, false);
 
-	drm_atomic_helper_wait_for_dependencies(state);
+	drm_atomic_helper_wait_for_dependencies(old_state);
 
 	if (funcs && funcs->atomic_commit_tail)
-		funcs->atomic_commit_tail(state);
+		funcs->atomic_commit_tail(old_state);
 	else
-		drm_atomic_helper_commit_tail(state);
+		drm_atomic_helper_commit_tail(old_state);
 
-	drm_atomic_helper_commit_cleanup_done(state);
+	drm_atomic_helper_commit_cleanup_done(old_state);
 
-	drm_atomic_state_put(state);
+	drm_atomic_state_put(old_state);
 }
 
 static void commit_work(struct work_struct *work)
@@ -1498,10 +1498,10 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
 
 /**
  * drm_atomic_helper_wait_for_dependencies - wait for required preceeding commits
- * @state: new modeset state to be committed
+ * @old_state: atomic state object with old state structures
  *
  * This function waits for all preceeding commits that touch the same CRTC as
- * @state to both be committed to the hardware (as signalled by
+ * @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
  * by calling drm_crtc_vblank_send_event on the event member of
  * &drm_crtc_state).
@@ -1509,7 +1509,7 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
  * This is part of the atomic helper support for nonblocking commits, see
  * drm_atomic_helper_setup_commit() for an overview.
  */
-void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state)
+void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
@@ -1517,7 +1517,7 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state)
 	int i;
 	long ret;
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
 		spin_lock(&crtc->commit_lock);
 		commit = preceeding_commit(crtc);
 		if (commit)
@@ -1584,16 +1584,16 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
 
 /**
  * drm_atomic_helper_commit_cleanup_done - signal completion of commit
- * @state: new modeset state to be committed
+ * @old_state: atomic state object with old state structures
  *
- * This signals completion of the atomic update @state, including any cleanup
- * work. If used, it must be called right before calling
+ * This signals completion of the atomic update @old_state, including any
+ * cleanup work. If used, it must be called right before calling
  * drm_atomic_state_put().
  *
  * This is part of the atomic helper support for nonblocking commits, see
  * drm_atomic_helper_setup_commit() for an overview.
  */
-void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state)
+void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
@@ -1601,8 +1601,8 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state)
 	int i;
 	long ret;
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		commit = state->crtcs[i].commit;
+	for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
+		commit = old_state->crtcs[i].commit;
 		if (WARN_ON(!commit))
 			continue;
 
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 72478cf82147..e96d662ea572 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -999,10 +999,14 @@ struct drm_mode_config_helper_funcs {
 	 * to implement blocking and nonblocking commits easily. It is not used
 	 * by the atomic helpers
 	 *
-	 * This hook should first commit the given atomic state to the hardware.
-	 * But drivers can add more waiting calls at the start of their
-	 * implementation, e.g. to wait for driver-internal request for implicit
-	 * syncing, before starting to commit the update to the hardware.
+	 * This function is called when the new atomic state has already been
+	 * swapped into the various state pointers. The the passed in state therefore contains
+	 * copies of the old/previous state. This hook should commit the new
+	 * state into hardware. Note that the helpers have already waited for
+	 * preceedning atomic commits and fences, but drivers can add more
+	 * waiting calls at the start of their implementation, e.g. to wait for
+	 * driver-internal request for implicit syncing, before starting to
+	 * commit the update to the hardware.
 	 *
 	 * After the atomic update is committed to the hardware this hook needs
 	 * to call drm_atomic_helper_commit_hw_done(). Then wait for the upate
-- 
2.10.2

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

Re: [PATCH] drm/atomic: Unconfuse the old_state mess in commmit_tail

[Russell King - ARM Linux]

On Mon, Nov 21, 2016 at 05:35:20PM +0100, Daniel Vetter wrote:
> I totally butcherd the job on typing the kernel-doc for these, and no
> one realized. Noticed by Russell. Maarten has a more complete approach
> to this confusion, by making it more explicit what the new/old state
> is, instead of this magic switching behaviour.

Thanks for fixing this.  I noticed a couple of mistakes while reading
through the patch:

> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 72478cf82147..e96d662ea572 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -999,10 +999,14 @@ struct drm_mode_config_helper_funcs {
>  	 * to implement blocking and nonblocking commits easily. It is not used
>  	 * by the atomic helpers
>  	 *
> -	 * This hook should first commit the given atomic state to the hardware.
> -	 * But drivers can add more waiting calls at the start of their
> -	 * implementation, e.g. to wait for driver-internal request for implicit
> -	 * syncing, before starting to commit the update to the hardware.
> +	 * This function is called when the new atomic state has already been
> +	 * swapped into the various state pointers. The the passed in state therefore contains

"The the".  It also goes way over 80 columns.

> +	 * copies of the old/previous state. This hook should commit the new
> +	 * state into hardware. Note that the helpers have already waited for
> +	 * preceedning atomic commits and fences, but drivers can add more

"preceeding"

> +	 * waiting calls at the start of their implementation, e.g. to wait for
> +	 * driver-internal request for implicit syncing, before starting to
> +	 * commit the update to the hardware.
>  	 *
>  	 * After the atomic update is committed to the hardware this hook needs
>  	 * to call drm_atomic_helper_commit_hw_done(). Then wait for the upate
> -- 
> 2.10.2
> 

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/atomic: Unconfuse the old_state mess in commmit_tail

[Daniel Vetter]

I totally butcherd the job on typing the kernel-doc for these, and no
one realized. Noticed by Russell. Maarten has a more complete approach
to this confusion, by making it more explicit what the new/old state
is, instead of this magic switching behaviour.

v2:
- Liviu pointed out that wait_for_fences is even more magic. Leave
that as @state, and document @pre_swap better.
- While at it, patch in header for the reference section.
- Fix spelling issues Russell noticed.

Cc: Liviu Dudau <liviu.dudau@arm.com>
Reported-by: Russell King - ARM Linux <linux@armlinux.org.uk>
Cc: Russell King - ARM Linux <linux@armlinux.org.uk>
Fixes: 9f2a7950e77a ("drm/atomic-helper: nonblocking commit support")
Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Tomeu Vizoso <tomeu.vizoso@gmail.com>
Cc: Daniel Stone <daniels@collabora.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 Documentation/gpu/drm-kms-helpers.rst    |  3 ++
 drivers/gpu/drm/drm_atomic_helper.c      | 78 ++++++++++++++++++--------------
 include/drm/drm_modeset_helper_vtables.h | 12 +++--
 3 files changed, 54 insertions(+), 39 deletions(-)

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
index 4ca77f675967..03040aa14fe8 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -63,6 +63,9 @@ Atomic State Reset and Initialization
 .. kernel-doc:: drivers/gpu/drm/drm_atomic_helper.c
    :doc: atomic state reset and initialization
 
+Helper Functions Reference
+--------------------------
+
 .. kernel-doc:: include/drm/drm_atomic_helper.h
    :internal:
 
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 0b16587cdc62..86459554ef5f 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1006,13 +1006,21 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
  * drm_atomic_helper_wait_for_fences - wait for fences stashed in plane state
  * @dev: DRM device
  * @state: atomic state object with old state structures
- * @pre_swap: if true, do an interruptible wait
+ * @pre_swap: If true, do an interruptible wait, and @state is the new state.
+ * 	Otherwise @state is the old state.
  *
  * For implicit sync, driver should fish the exclusive fence out from the
  * incoming fb's and stash it in the drm_plane_state.  This is called after
  * drm_atomic_helper_swap_state() so it uses the current plane state (and
  * just uses the atomic state to find the changed planes)
  *
+ * Note that @pre_swap is needed since we the point where we block for fences
+ * moves around depending upon whether an atomic commit is synchronous or
+ * asynchronous. For async commit all waiting needs to happen after
+ * drm_atomic_helper_swap_state() is called, but for synchronous commits we want
+ * to wait _before_ we do anything that can't be easily rolled back. And hence
+ * before we call drm_atomic_helper_swap_state().
+ *
  * Returns zero if success or < 0 if dma_fence_wait() fails.
  */
 int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
@@ -1147,7 +1155,7 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
 
 /**
  * drm_atomic_helper_commit_tail - commit atomic update to hardware
- * @state: new modeset state to be committed
+ * @old_state: atomic state object with old state structures
  *
  * This is the default implemenation for the ->atomic_commit_tail() hook of the
  * &drm_mode_config_helper_funcs vtable.
@@ -1158,53 +1166,53 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
  *
  * For drivers supporting runtime PM the recommended sequence is instead ::
  *
- *     drm_atomic_helper_commit_modeset_disables(dev, state);
+ *     drm_atomic_helper_commit_modeset_disables(dev, old_state);
  *
- *     drm_atomic_helper_commit_modeset_enables(dev, state);
+ *     drm_atomic_helper_commit_modeset_enables(dev, old_state);
  *
- *     drm_atomic_helper_commit_planes(dev, state,
+ *     drm_atomic_helper_commit_planes(dev, old_state,
  *                                     DRM_PLANE_COMMIT_ACTIVE_ONLY);
  *
  * for committing the atomic update to hardware.  See the kerneldoc entries for
  * these three functions for more details.
  */
-void drm_atomic_helper_commit_tail(struct drm_atomic_state *state)
+void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
 {
-	struct drm_device *dev = state->dev;
+	struct drm_device *dev = old_state->dev;
 
-	drm_atomic_helper_commit_modeset_disables(dev, state);
+	drm_atomic_helper_commit_modeset_disables(dev, old_state);
 
-	drm_atomic_helper_commit_planes(dev, state, 0);
+	drm_atomic_helper_commit_planes(dev, old_state, 0);
 
-	drm_atomic_helper_commit_modeset_enables(dev, state);
+	drm_atomic_helper_commit_modeset_enables(dev, old_state);
 
-	drm_atomic_helper_commit_hw_done(state);
+	drm_atomic_helper_commit_hw_done(old_state);
 
-	drm_atomic_helper_wait_for_vblanks(dev, state);
+	drm_atomic_helper_wait_for_vblanks(dev, old_state);
 
-	drm_atomic_helper_cleanup_planes(dev, state);
+	drm_atomic_helper_cleanup_planes(dev, old_state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_tail);
 
-static void commit_tail(struct drm_atomic_state *state)
+static void commit_tail(struct drm_atomic_state *old_state)
 {
-	struct drm_device *dev = state->dev;
+	struct drm_device *dev = old_state->dev;
 	struct drm_mode_config_helper_funcs *funcs;
 
 	funcs = dev->mode_config.helper_private;
 
-	drm_atomic_helper_wait_for_fences(dev, state, false);
+	drm_atomic_helper_wait_for_fences(dev, old_state, false);
 
-	drm_atomic_helper_wait_for_dependencies(state);
+	drm_atomic_helper_wait_for_dependencies(old_state);
 
 	if (funcs && funcs->atomic_commit_tail)
-		funcs->atomic_commit_tail(state);
+		funcs->atomic_commit_tail(old_state);
 	else
-		drm_atomic_helper_commit_tail(state);
+		drm_atomic_helper_commit_tail(old_state);
 
-	drm_atomic_helper_commit_cleanup_done(state);
+	drm_atomic_helper_commit_cleanup_done(old_state);
 
-	drm_atomic_state_put(state);
+	drm_atomic_state_put(old_state);
 }
 
 static void commit_work(struct work_struct *work)
@@ -1498,10 +1506,10 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
 
 /**
  * drm_atomic_helper_wait_for_dependencies - wait for required preceeding commits
- * @state: new modeset state to be committed
+ * @old_state: atomic state object with old state structures
  *
  * This function waits for all preceeding commits that touch the same CRTC as
- * @state to both be committed to the hardware (as signalled by
+ * @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
  * by calling drm_crtc_vblank_send_event on the event member of
  * &drm_crtc_state).
@@ -1509,7 +1517,7 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
  * This is part of the atomic helper support for nonblocking commits, see
  * drm_atomic_helper_setup_commit() for an overview.
  */
-void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state)
+void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
@@ -1517,7 +1525,7 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state)
 	int i;
 	long ret;
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
 		spin_lock(&crtc->commit_lock);
 		commit = preceeding_commit(crtc);
 		if (commit)
@@ -1548,7 +1556,7 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
 
 /**
  * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
- * @state: new modeset state to be committed
+ * @old_state: atomic state object with old state structures
  *
  * This function is used to signal completion of the hardware commit step. After
  * this step the driver is not allowed to read or change any permanent software
@@ -1561,15 +1569,15 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
  * This is part of the atomic helper support for nonblocking commits, see
  * drm_atomic_helper_setup_commit() for an overview.
  */
-void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state)
+void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	struct drm_crtc_commit *commit;
 	int i;
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		commit = state->crtcs[i].commit;
+	for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
+		commit = old_state->crtcs[i].commit;
 		if (!commit)
 			continue;
 
@@ -1584,16 +1592,16 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
 
 /**
  * drm_atomic_helper_commit_cleanup_done - signal completion of commit
- * @state: new modeset state to be committed
+ * @old_state: atomic state object with old state structures
  *
- * This signals completion of the atomic update @state, including any cleanup
- * work. If used, it must be called right before calling
+ * This signals completion of the atomic update @old_state, including any
+ * cleanup work. If used, it must be called right before calling
  * drm_atomic_state_put().
  *
  * This is part of the atomic helper support for nonblocking commits, see
  * drm_atomic_helper_setup_commit() for an overview.
  */
-void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state)
+void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
@@ -1601,8 +1609,8 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state)
 	int i;
 	long ret;
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		commit = state->crtcs[i].commit;
+	for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
+		commit = old_state->crtcs[i].commit;
 		if (WARN_ON(!commit))
 			continue;
 
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 72478cf82147..69c3974bf133 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -999,10 +999,14 @@ struct drm_mode_config_helper_funcs {
 	 * to implement blocking and nonblocking commits easily. It is not used
 	 * by the atomic helpers
 	 *
-	 * This hook should first commit the given atomic state to the hardware.
-	 * But drivers can add more waiting calls at the start of their
-	 * implementation, e.g. to wait for driver-internal request for implicit
-	 * syncing, before starting to commit the update to the hardware.
+	 * This function is called when the new atomic state has already been
+	 * swapped into the various state pointers. The passed in state
+	 * therefore contains copies of the old/previous state. This hook should
+	 * commit the new state into hardware. Note that the helpers have
+	 * already waited for preceeding atomic commits and fences, but drivers
+	 * can add more waiting calls at the start of their implementation, e.g.
+	 * to wait for driver-internal request for implicit syncing, before
+	 * starting to commit the update to the hardware.
 	 *
 	 * After the atomic update is committed to the hardware this hook needs
 	 * to call drm_atomic_helper_commit_hw_done(). Then wait for the upate
-- 
2.10.2

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

Re: [PATCH] drm/atomic: Unconfuse the old_state mess in commmit_tail

[Liviu Dudau]

On Mon, Nov 21, 2016 at 05:52:57PM +0100, Daniel Vetter wrote:
> I totally butcherd the job on typing the kernel-doc for these, and no
> one realized. Noticed by Russell. Maarten has a more complete approach
> to this confusion, by making it more explicit what the new/old state
> is, instead of this magic switching behaviour.
> 
> v2:

I feel a v3 coming soon :)

> - Liviu pointed out that wait_for_fences is even more magic. Leave
> that as @state, and document @pre_swap better.
> - While at it, patch in header for the reference section.
> - Fix spelling issues Russell noticed.
> 
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Reported-by: Russell King - ARM Linux <linux@armlinux.org.uk>
> Cc: Russell King - ARM Linux <linux@armlinux.org.uk>
> Fixes: 9f2a7950e77a ("drm/atomic-helper: nonblocking commit support")
> Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Tomeu Vizoso <tomeu.vizoso@gmail.com>
> Cc: Daniel Stone <daniels@collabora.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  Documentation/gpu/drm-kms-helpers.rst    |  3 ++
>  drivers/gpu/drm/drm_atomic_helper.c      | 78 ++++++++++++++++++--------------
>  include/drm/drm_modeset_helper_vtables.h | 12 +++--
>  3 files changed, 54 insertions(+), 39 deletions(-)
> 
> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> index 4ca77f675967..03040aa14fe8 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -63,6 +63,9 @@ Atomic State Reset and Initialization
>  .. kernel-doc:: drivers/gpu/drm/drm_atomic_helper.c
>     :doc: atomic state reset and initialization
>  
> +Helper Functions Reference
> +--------------------------
> +
>  .. kernel-doc:: include/drm/drm_atomic_helper.h
>     :internal:
>  
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 0b16587cdc62..86459554ef5f 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1006,13 +1006,21 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
>   * drm_atomic_helper_wait_for_fences - wait for fences stashed in plane state
>   * @dev: DRM device
>   * @state: atomic state object with old state structures
> - * @pre_swap: if true, do an interruptible wait
> + * @pre_swap: If true, do an interruptible wait, and @state is the new state.
> + * 	Otherwise @state is the old state.
>   *
>   * For implicit sync, driver should fish the exclusive fence out from the
>   * incoming fb's and stash it in the drm_plane_state.  This is called after
>   * drm_atomic_helper_swap_state() so it uses the current plane state (and
>   * just uses the atomic state to find the changed planes)
>   *
> + * Note that @pre_swap is needed since we the point where we block for fences

confused about 'we the point' in there. Feels like you were trying to say something else?

> + * moves around depending upon whether an atomic commit is synchronous or
> + * asynchronous. For async commit all waiting needs to happen after
> + * drm_atomic_helper_swap_state() is called, but for synchronous commits we want
> + * to wait _before_ we do anything that can't be easily rolled back. And hence

s/And hence/That is/

Otherwise, it looks good to me.

Best regards,
Liviu

> + * before we call drm_atomic_helper_swap_state().
> + *
>   * Returns zero if success or < 0 if dma_fence_wait() fails.
>   */
>  int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
> @@ -1147,7 +1155,7 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
>  
>  /**
>   * drm_atomic_helper_commit_tail - commit atomic update to hardware
> - * @state: new modeset state to be committed
> + * @old_state: atomic state object with old state structures
>   *
>   * This is the default implemenation for the ->atomic_commit_tail() hook of the
>   * &drm_mode_config_helper_funcs vtable.
> @@ -1158,53 +1166,53 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
>   *
>   * For drivers supporting runtime PM the recommended sequence is instead ::
>   *
> - *     drm_atomic_helper_commit_modeset_disables(dev, state);
> + *     drm_atomic_helper_commit_modeset_disables(dev, old_state);
>   *
> - *     drm_atomic_helper_commit_modeset_enables(dev, state);
> + *     drm_atomic_helper_commit_modeset_enables(dev, old_state);
>   *
> - *     drm_atomic_helper_commit_planes(dev, state,
> + *     drm_atomic_helper_commit_planes(dev, old_state,
>   *                                     DRM_PLANE_COMMIT_ACTIVE_ONLY);
>   *
>   * for committing the atomic update to hardware.  See the kerneldoc entries for
>   * these three functions for more details.
>   */
> -void drm_atomic_helper_commit_tail(struct drm_atomic_state *state)
> +void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
>  {
> -	struct drm_device *dev = state->dev;
> +	struct drm_device *dev = old_state->dev;
>  
> -	drm_atomic_helper_commit_modeset_disables(dev, state);
> +	drm_atomic_helper_commit_modeset_disables(dev, old_state);
>  
> -	drm_atomic_helper_commit_planes(dev, state, 0);
> +	drm_atomic_helper_commit_planes(dev, old_state, 0);
>  
> -	drm_atomic_helper_commit_modeset_enables(dev, state);
> +	drm_atomic_helper_commit_modeset_enables(dev, old_state);
>  
> -	drm_atomic_helper_commit_hw_done(state);
> +	drm_atomic_helper_commit_hw_done(old_state);
>  
> -	drm_atomic_helper_wait_for_vblanks(dev, state);
> +	drm_atomic_helper_wait_for_vblanks(dev, old_state);
>  
> -	drm_atomic_helper_cleanup_planes(dev, state);
> +	drm_atomic_helper_cleanup_planes(dev, old_state);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_commit_tail);
>  
> -static void commit_tail(struct drm_atomic_state *state)
> +static void commit_tail(struct drm_atomic_state *old_state)
>  {
> -	struct drm_device *dev = state->dev;
> +	struct drm_device *dev = old_state->dev;
>  	struct drm_mode_config_helper_funcs *funcs;
>  
>  	funcs = dev->mode_config.helper_private;
>  
> -	drm_atomic_helper_wait_for_fences(dev, state, false);
> +	drm_atomic_helper_wait_for_fences(dev, old_state, false);
>  
> -	drm_atomic_helper_wait_for_dependencies(state);
> +	drm_atomic_helper_wait_for_dependencies(old_state);
>  
>  	if (funcs && funcs->atomic_commit_tail)
> -		funcs->atomic_commit_tail(state);
> +		funcs->atomic_commit_tail(old_state);
>  	else
> -		drm_atomic_helper_commit_tail(state);
> +		drm_atomic_helper_commit_tail(old_state);
>  
> -	drm_atomic_helper_commit_cleanup_done(state);
> +	drm_atomic_helper_commit_cleanup_done(old_state);
>  
> -	drm_atomic_state_put(state);
> +	drm_atomic_state_put(old_state);
>  }
>  
>  static void commit_work(struct work_struct *work)
> @@ -1498,10 +1506,10 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
>  
>  /**
>   * drm_atomic_helper_wait_for_dependencies - wait for required preceeding commits
> - * @state: new modeset state to be committed
> + * @old_state: atomic state object with old state structures
>   *
>   * This function waits for all preceeding commits that touch the same CRTC as
> - * @state to both be committed to the hardware (as signalled by
> + * @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
>   * by calling drm_crtc_vblank_send_event on the event member of
>   * &drm_crtc_state).
> @@ -1509,7 +1517,7 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
>   * This is part of the atomic helper support for nonblocking commits, see
>   * drm_atomic_helper_setup_commit() for an overview.
>   */
> -void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state)
> +void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
> @@ -1517,7 +1525,7 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state)
>  	int i;
>  	long ret;
>  
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +	for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
>  		spin_lock(&crtc->commit_lock);
>  		commit = preceeding_commit(crtc);
>  		if (commit)
> @@ -1548,7 +1556,7 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
>  
>  /**
>   * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
> - * @state: new modeset state to be committed
> + * @old_state: atomic state object with old state structures
>   *
>   * This function is used to signal completion of the hardware commit step. After
>   * this step the driver is not allowed to read or change any permanent software
> @@ -1561,15 +1569,15 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
>   * This is part of the atomic helper support for nonblocking commits, see
>   * drm_atomic_helper_setup_commit() for an overview.
>   */
> -void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state)
> +void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_crtc_commit *commit;
>  	int i;
>  
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		commit = state->crtcs[i].commit;
> +	for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
> +		commit = old_state->crtcs[i].commit;
>  		if (!commit)
>  			continue;
>  
> @@ -1584,16 +1592,16 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
>  
>  /**
>   * drm_atomic_helper_commit_cleanup_done - signal completion of commit
> - * @state: new modeset state to be committed
> + * @old_state: atomic state object with old state structures
>   *
> - * This signals completion of the atomic update @state, including any cleanup
> - * work. If used, it must be called right before calling
> + * This signals completion of the atomic update @old_state, including any
> + * cleanup work. If used, it must be called right before calling
>   * drm_atomic_state_put().
>   *
>   * This is part of the atomic helper support for nonblocking commits, see
>   * drm_atomic_helper_setup_commit() for an overview.
>   */
> -void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state)
> +void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
> @@ -1601,8 +1609,8 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state)
>  	int i;
>  	long ret;
>  
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		commit = state->crtcs[i].commit;
> +	for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
> +		commit = old_state->crtcs[i].commit;
>  		if (WARN_ON(!commit))
>  			continue;
>  
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 72478cf82147..69c3974bf133 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -999,10 +999,14 @@ struct drm_mode_config_helper_funcs {
>  	 * to implement blocking and nonblocking commits easily. It is not used
>  	 * by the atomic helpers
>  	 *
> -	 * This hook should first commit the given atomic state to the hardware.
> -	 * But drivers can add more waiting calls at the start of their
> -	 * implementation, e.g. to wait for driver-internal request for implicit
> -	 * syncing, before starting to commit the update to the hardware.
> +	 * This function is called when the new atomic state has already been
> +	 * swapped into the various state pointers. The passed in state
> +	 * therefore contains copies of the old/previous state. This hook should
> +	 * commit the new state into hardware. Note that the helpers have
> +	 * already waited for preceeding atomic commits and fences, but drivers
> +	 * can add more waiting calls at the start of their implementation, e.g.
> +	 * to wait for driver-internal request for implicit syncing, before
> +	 * starting to commit the update to the hardware.
>  	 *
>  	 * After the atomic update is committed to the hardware this hook needs
>  	 * to call drm_atomic_helper_commit_hw_done(). Then wait for the upate
> -- 
> 2.10.2
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/atomic: Unconfuse the old_state mess in commmit_tail

[Daniel Vetter]

I totally butcherd the job on typing the kernel-doc for these, and no
one realized. Noticed by Russell. Maarten has a more complete approach
to this confusion, by making it more explicit what the new/old state
is, instead of this magic switching behaviour.

v2:
- Liviu pointed out that wait_for_fences is even more magic. Leave
that as @state, and document @pre_swap better.
- While at it, patch in header for the reference section.
- Fix spelling issues Russell noticed.

v3: Fix up the @pre_swap note (Liviu): Also s/synchronous/blocking/,
since async flip is something else than non-blocking.

Cc: Liviu Dudau <liviu.dudau@arm.com>
Reported-by: Russell King - ARM Linux <linux@armlinux.org.uk>
Cc: Russell King - ARM Linux <linux@armlinux.org.uk>
Fixes: 9f2a7950e77a ("drm/atomic-helper: nonblocking commit support")
Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Tomeu Vizoso <tomeu.vizoso@gmail.com>
Cc: Daniel Stone <daniels@collabora.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 Documentation/gpu/drm-kms-helpers.rst    |  3 ++
 drivers/gpu/drm/drm_atomic_helper.c      | 78 ++++++++++++++++++--------------
 include/drm/drm_modeset_helper_vtables.h | 12 +++--
 3 files changed, 54 insertions(+), 39 deletions(-)

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
index 4ca77f675967..03040aa14fe8 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -63,6 +63,9 @@ Atomic State Reset and Initialization
 .. kernel-doc:: drivers/gpu/drm/drm_atomic_helper.c
    :doc: atomic state reset and initialization
 
+Helper Functions Reference
+--------------------------
+
 .. kernel-doc:: include/drm/drm_atomic_helper.h
    :internal:
 
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 0b16587cdc62..494680c9056e 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1006,13 +1006,21 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
  * drm_atomic_helper_wait_for_fences - wait for fences stashed in plane state
  * @dev: DRM device
  * @state: atomic state object with old state structures
- * @pre_swap: if true, do an interruptible wait
+ * @pre_swap: If true, do an interruptible wait, and @state is the new state.
+ * 	Otherwise @state is the old state.
  *
  * For implicit sync, driver should fish the exclusive fence out from the
  * incoming fb's and stash it in the drm_plane_state.  This is called after
  * drm_atomic_helper_swap_state() so it uses the current plane state (and
  * just uses the atomic state to find the changed planes)
  *
+ * Note that @pre_swap is needed since the point where we block for fences moves
+ * around depending upon whether an atomic commit is blocking or
+ * non-blocking. For async commit all waiting needs to happen after
+ * drm_atomic_helper_swap_state() is called, but for synchronous commits we want
+ * to wait **before** we do anything that can't be easily rolled back. That is
+ * before we call drm_atomic_helper_swap_state().
+ *
  * Returns zero if success or < 0 if dma_fence_wait() fails.
  */
 int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
@@ -1147,7 +1155,7 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
 
 /**
  * drm_atomic_helper_commit_tail - commit atomic update to hardware
- * @state: new modeset state to be committed
+ * @old_state: atomic state object with old state structures
  *
  * This is the default implemenation for the ->atomic_commit_tail() hook of the
  * &drm_mode_config_helper_funcs vtable.
@@ -1158,53 +1166,53 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
  *
  * For drivers supporting runtime PM the recommended sequence is instead ::
  *
- *     drm_atomic_helper_commit_modeset_disables(dev, state);
+ *     drm_atomic_helper_commit_modeset_disables(dev, old_state);
  *
- *     drm_atomic_helper_commit_modeset_enables(dev, state);
+ *     drm_atomic_helper_commit_modeset_enables(dev, old_state);
  *
- *     drm_atomic_helper_commit_planes(dev, state,
+ *     drm_atomic_helper_commit_planes(dev, old_state,
  *                                     DRM_PLANE_COMMIT_ACTIVE_ONLY);
  *
  * for committing the atomic update to hardware.  See the kerneldoc entries for
  * these three functions for more details.
  */
-void drm_atomic_helper_commit_tail(struct drm_atomic_state *state)
+void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
 {
-	struct drm_device *dev = state->dev;
+	struct drm_device *dev = old_state->dev;
 
-	drm_atomic_helper_commit_modeset_disables(dev, state);
+	drm_atomic_helper_commit_modeset_disables(dev, old_state);
 
-	drm_atomic_helper_commit_planes(dev, state, 0);
+	drm_atomic_helper_commit_planes(dev, old_state, 0);
 
-	drm_atomic_helper_commit_modeset_enables(dev, state);
+	drm_atomic_helper_commit_modeset_enables(dev, old_state);
 
-	drm_atomic_helper_commit_hw_done(state);
+	drm_atomic_helper_commit_hw_done(old_state);
 
-	drm_atomic_helper_wait_for_vblanks(dev, state);
+	drm_atomic_helper_wait_for_vblanks(dev, old_state);
 
-	drm_atomic_helper_cleanup_planes(dev, state);
+	drm_atomic_helper_cleanup_planes(dev, old_state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_tail);
 
-static void commit_tail(struct drm_atomic_state *state)
+static void commit_tail(struct drm_atomic_state *old_state)
 {
-	struct drm_device *dev = state->dev;
+	struct drm_device *dev = old_state->dev;
 	struct drm_mode_config_helper_funcs *funcs;
 
 	funcs = dev->mode_config.helper_private;
 
-	drm_atomic_helper_wait_for_fences(dev, state, false);
+	drm_atomic_helper_wait_for_fences(dev, old_state, false);
 
-	drm_atomic_helper_wait_for_dependencies(state);
+	drm_atomic_helper_wait_for_dependencies(old_state);
 
 	if (funcs && funcs->atomic_commit_tail)
-		funcs->atomic_commit_tail(state);
+		funcs->atomic_commit_tail(old_state);
 	else
-		drm_atomic_helper_commit_tail(state);
+		drm_atomic_helper_commit_tail(old_state);
 
-	drm_atomic_helper_commit_cleanup_done(state);
+	drm_atomic_helper_commit_cleanup_done(old_state);
 
-	drm_atomic_state_put(state);
+	drm_atomic_state_put(old_state);
 }
 
 static void commit_work(struct work_struct *work)
@@ -1498,10 +1506,10 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
 
 /**
  * drm_atomic_helper_wait_for_dependencies - wait for required preceeding commits
- * @state: new modeset state to be committed
+ * @old_state: atomic state object with old state structures
  *
  * This function waits for all preceeding commits that touch the same CRTC as
- * @state to both be committed to the hardware (as signalled by
+ * @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
  * by calling drm_crtc_vblank_send_event on the event member of
  * &drm_crtc_state).
@@ -1509,7 +1517,7 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
  * This is part of the atomic helper support for nonblocking commits, see
  * drm_atomic_helper_setup_commit() for an overview.
  */
-void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state)
+void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
@@ -1517,7 +1525,7 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state)
 	int i;
 	long ret;
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
 		spin_lock(&crtc->commit_lock);
 		commit = preceeding_commit(crtc);
 		if (commit)
@@ -1548,7 +1556,7 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
 
 /**
  * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
- * @state: new modeset state to be committed
+ * @old_state: atomic state object with old state structures
  *
  * This function is used to signal completion of the hardware commit step. After
  * this step the driver is not allowed to read or change any permanent software
@@ -1561,15 +1569,15 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
  * This is part of the atomic helper support for nonblocking commits, see
  * drm_atomic_helper_setup_commit() for an overview.
  */
-void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state)
+void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	struct drm_crtc_commit *commit;
 	int i;
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		commit = state->crtcs[i].commit;
+	for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
+		commit = old_state->crtcs[i].commit;
 		if (!commit)
 			continue;
 
@@ -1584,16 +1592,16 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
 
 /**
  * drm_atomic_helper_commit_cleanup_done - signal completion of commit
- * @state: new modeset state to be committed
+ * @old_state: atomic state object with old state structures
  *
- * This signals completion of the atomic update @state, including any cleanup
- * work. If used, it must be called right before calling
+ * This signals completion of the atomic update @old_state, including any
+ * cleanup work. If used, it must be called right before calling
  * drm_atomic_state_put().
  *
  * This is part of the atomic helper support for nonblocking commits, see
  * drm_atomic_helper_setup_commit() for an overview.
  */
-void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state)
+void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
@@ -1601,8 +1609,8 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state)
 	int i;
 	long ret;
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		commit = state->crtcs[i].commit;
+	for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
+		commit = old_state->crtcs[i].commit;
 		if (WARN_ON(!commit))
 			continue;
 
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 72478cf82147..69c3974bf133 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -999,10 +999,14 @@ struct drm_mode_config_helper_funcs {
 	 * to implement blocking and nonblocking commits easily. It is not used
 	 * by the atomic helpers
 	 *
-	 * This hook should first commit the given atomic state to the hardware.
-	 * But drivers can add more waiting calls at the start of their
-	 * implementation, e.g. to wait for driver-internal request for implicit
-	 * syncing, before starting to commit the update to the hardware.
+	 * This function is called when the new atomic state has already been
+	 * swapped into the various state pointers. The passed in state
+	 * therefore contains copies of the old/previous state. This hook should
+	 * commit the new state into hardware. Note that the helpers have
+	 * already waited for preceeding atomic commits and fences, but drivers
+	 * can add more waiting calls at the start of their implementation, e.g.
+	 * to wait for driver-internal request for implicit syncing, before
+	 * starting to commit the update to the hardware.
 	 *
 	 * After the atomic update is committed to the hardware this hook needs
 	 * to call drm_atomic_helper_commit_hw_done(). Then wait for the upate
-- 
2.10.2

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

Re: [PATCH] drm/atomic: Unconfuse the old_state mess in commmit_tail

[Gustavo Padovan]

2016-11-21 Daniel Vetter <daniel.vetter@ffwll.ch>:

> I totally butcherd the job on typing the kernel-doc for these, and no
> one realized. Noticed by Russell. Maarten has a more complete approach
> to this confusion, by making it more explicit what the new/old state
> is, instead of this magic switching behaviour.
> 
> v2:
> - Liviu pointed out that wait_for_fences is even more magic. Leave
> that as @state, and document @pre_swap better.
> - While at it, patch in header for the reference section.
> - Fix spelling issues Russell noticed.
> 
> v3: Fix up the @pre_swap note (Liviu): Also s/synchronous/blocking/,
> since async flip is something else than non-blocking.
> 
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Reported-by: Russell King - ARM Linux <linux@armlinux.org.uk>
> Cc: Russell King - ARM Linux <linux@armlinux.org.uk>
> Fixes: 9f2a7950e77a ("drm/atomic-helper: nonblocking commit support")
> Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Tomeu Vizoso <tomeu.vizoso@gmail.com>
> Cc: Daniel Stone <daniels@collabora.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  Documentation/gpu/drm-kms-helpers.rst    |  3 ++
>  drivers/gpu/drm/drm_atomic_helper.c      | 78 ++++++++++++++++++--------------
>  include/drm/drm_modeset_helper_vtables.h | 12 +++--
>  3 files changed, 54 insertions(+), 39 deletions(-)
> 
> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> index 4ca77f675967..03040aa14fe8 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -63,6 +63,9 @@ Atomic State Reset and Initialization
>  .. kernel-doc:: drivers/gpu/drm/drm_atomic_helper.c
>     :doc: atomic state reset and initialization
>  
> +Helper Functions Reference
> +--------------------------
> +
>  .. kernel-doc:: include/drm/drm_atomic_helper.h
>     :internal:
>  
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 0b16587cdc62..494680c9056e 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1006,13 +1006,21 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
>   * drm_atomic_helper_wait_for_fences - wait for fences stashed in plane state
>   * @dev: DRM device
>   * @state: atomic state object with old state structures
> - * @pre_swap: if true, do an interruptible wait
> + * @pre_swap: If true, do an interruptible wait, and @state is the new state.
> + * 	Otherwise @state is the old state.
>   *
>   * For implicit sync, driver should fish the exclusive fence out from the
>   * incoming fb's and stash it in the drm_plane_state.  This is called after
>   * drm_atomic_helper_swap_state() so it uses the current plane state (and
>   * just uses the atomic state to find the changed planes)
>   *
> + * Note that @pre_swap is needed since the point where we block for fences moves
> + * around depending upon whether an atomic commit is blocking or
> + * non-blocking. For async commit all waiting needs to happen after
> + * drm_atomic_helper_swap_state() is called, but for synchronous commits we want
> + * to wait **before** we do anything that can't be easily rolled back. That is
> + * before we call drm_atomic_helper_swap_state().
> + *
>   * Returns zero if success or < 0 if dma_fence_wait() fails.
>   */
>  int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
> @@ -1147,7 +1155,7 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
>  
>  /**
>   * drm_atomic_helper_commit_tail - commit atomic update to hardware
> - * @state: new modeset state to be committed
> + * @old_state: atomic state object with old state structures
>   *
>   * This is the default implemenation for the ->atomic_commit_tail() hook of the
>   * &drm_mode_config_helper_funcs vtable.
> @@ -1158,53 +1166,53 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
>   *
>   * For drivers supporting runtime PM the recommended sequence is instead ::
>   *
> - *     drm_atomic_helper_commit_modeset_disables(dev, state);
> + *     drm_atomic_helper_commit_modeset_disables(dev, old_state);
>   *
> - *     drm_atomic_helper_commit_modeset_enables(dev, state);
> + *     drm_atomic_helper_commit_modeset_enables(dev, old_state);
>   *
> - *     drm_atomic_helper_commit_planes(dev, state,
> + *     drm_atomic_helper_commit_planes(dev, old_state,
>   *                                     DRM_PLANE_COMMIT_ACTIVE_ONLY);
>   *
>   * for committing the atomic update to hardware.  See the kerneldoc entries for
>   * these three functions for more details.
>   */
> -void drm_atomic_helper_commit_tail(struct drm_atomic_state *state)
> +void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)

I thought we would commit the new state. Why the rename to old_state?

>  {
> -	struct drm_device *dev = state->dev;
> +	struct drm_device *dev = old_state->dev;
>  
> -	drm_atomic_helper_commit_modeset_disables(dev, state);
> +	drm_atomic_helper_commit_modeset_disables(dev, old_state);
>  
> -	drm_atomic_helper_commit_planes(dev, state, 0);
> +	drm_atomic_helper_commit_planes(dev, old_state, 0);
>  
> -	drm_atomic_helper_commit_modeset_enables(dev, state);
> +	drm_atomic_helper_commit_modeset_enables(dev, old_state);
>  
> -	drm_atomic_helper_commit_hw_done(state);
> +	drm_atomic_helper_commit_hw_done(old_state);
>  
> -	drm_atomic_helper_wait_for_vblanks(dev, state);
> +	drm_atomic_helper_wait_for_vblanks(dev, old_state);
>  
> -	drm_atomic_helper_cleanup_planes(dev, state);
> +	drm_atomic_helper_cleanup_planes(dev, old_state);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_commit_tail);
>  
> -static void commit_tail(struct drm_atomic_state *state)
> +static void commit_tail(struct drm_atomic_state *old_state)
>  {
> -	struct drm_device *dev = state->dev;
> +	struct drm_device *dev = old_state->dev;
>  	struct drm_mode_config_helper_funcs *funcs;
>  
>  	funcs = dev->mode_config.helper_private;
>  
> -	drm_atomic_helper_wait_for_fences(dev, state, false);
> +	drm_atomic_helper_wait_for_fences(dev, old_state, false);
>  
> -	drm_atomic_helper_wait_for_dependencies(state);
> +	drm_atomic_helper_wait_for_dependencies(old_state);
>  
>  	if (funcs && funcs->atomic_commit_tail)
> -		funcs->atomic_commit_tail(state);
> +		funcs->atomic_commit_tail(old_state);
>  	else
> -		drm_atomic_helper_commit_tail(state);
> +		drm_atomic_helper_commit_tail(old_state);
>  
> -	drm_atomic_helper_commit_cleanup_done(state);
> +	drm_atomic_helper_commit_cleanup_done(old_state);
>  
> -	drm_atomic_state_put(state);
> +	drm_atomic_state_put(old_state);
>  }
>  
>  static void commit_work(struct work_struct *work)
> @@ -1498,10 +1506,10 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
>  
>  /**
>   * drm_atomic_helper_wait_for_dependencies - wait for required preceeding commits
> - * @state: new modeset state to be committed
> + * @old_state: atomic state object with old state structures
>   *
>   * This function waits for all preceeding commits that touch the same CRTC as
> - * @state to both be committed to the hardware (as signalled by
> + * @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
>   * by calling drm_crtc_vblank_send_event on the event member of
>   * &drm_crtc_state).
> @@ -1509,7 +1517,7 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
>   * This is part of the atomic helper support for nonblocking commits, see
>   * drm_atomic_helper_setup_commit() for an overview.
>   */
> -void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state)
> +void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
> @@ -1517,7 +1525,7 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state)
>  	int i;
>  	long ret;
>  
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +	for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
>  		spin_lock(&crtc->commit_lock);
>  		commit = preceeding_commit(crtc);
>  		if (commit)
> @@ -1548,7 +1556,7 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
>  
>  /**
>   * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
> - * @state: new modeset state to be committed
> + * @old_state: atomic state object with old state structures
>   *
>   * This function is used to signal completion of the hardware commit step. After
>   * this step the driver is not allowed to read or change any permanent software
> @@ -1561,15 +1569,15 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
>   * This is part of the atomic helper support for nonblocking commits, see
>   * drm_atomic_helper_setup_commit() for an overview.
>   */
> -void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state)
> +void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_crtc_commit *commit;
>  	int i;
>  
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		commit = state->crtcs[i].commit;
> +	for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
> +		commit = old_state->crtcs[i].commit;
>  		if (!commit)
>  			continue;
>  
> @@ -1584,16 +1592,16 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
>  
>  /**
>   * drm_atomic_helper_commit_cleanup_done - signal completion of commit
> - * @state: new modeset state to be committed
> + * @old_state: atomic state object with old state structures
>   *
> - * This signals completion of the atomic update @state, including any cleanup
> - * work. If used, it must be called right before calling
> + * This signals completion of the atomic update @old_state, including any
> + * cleanup work. If used, it must be called right before calling
>   * drm_atomic_state_put().
>   *
>   * This is part of the atomic helper support for nonblocking commits, see
>   * drm_atomic_helper_setup_commit() for an overview.
>   */
> -void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state)
> +void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
> @@ -1601,8 +1609,8 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state)
>  	int i;
>  	long ret;
>  
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		commit = state->crtcs[i].commit;
> +	for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
> +		commit = old_state->crtcs[i].commit;
>  		if (WARN_ON(!commit))
>  			continue;
>  
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 72478cf82147..69c3974bf133 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -999,10 +999,14 @@ struct drm_mode_config_helper_funcs {
>  	 * to implement blocking and nonblocking commits easily. It is not used
>  	 * by the atomic helpers
>  	 *
> -	 * This hook should first commit the given atomic state to the hardware.
> -	 * But drivers can add more waiting calls at the start of their
> -	 * implementation, e.g. to wait for driver-internal request for implicit
> -	 * syncing, before starting to commit the update to the hardware.
> +	 * This function is called when the new atomic state has already been
> +	 * swapped into the various state pointers. The passed in state

Did mean 'Then' here? Or maybe something else?


> +	 * therefore contains copies of the old/previous state. This hook should
> +	 * commit the new state into hardware. Note that the helpers have
> +	 * already waited for preceeding atomic commits and fences, but drivers
> +	 * can add more waiting calls at the start of their implementation, e.g.
> +	 * to wait for driver-internal request for implicit syncing, before
> +	 * starting to commit the update to the hardware.
>  	 *
>  	 * After the atomic update is committed to the hardware this hook needs
>  	 * to call drm_atomic_helper_commit_hw_done(). Then wait for the upate
> -- 
> 2.10.2
> 


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

Re: [PATCH] drm/atomic: Unconfuse the old_state mess in commmit_tail

[Russell King - ARM Linux]

On Tue, Nov 22, 2016 at 08:51:11AM +0900, Gustavo Padovan wrote:
> 2016-11-21 Daniel Vetter <daniel.vetter@ffwll.ch>:
> >  /**
> >   * drm_atomic_helper_commit_tail - commit atomic update to hardware
> > - * @state: new modeset state to be committed
> > + * @old_state: atomic state object with old state structures
> >   *
> >   * This is the default implemenation for the ->atomic_commit_tail() hook of the
> >   * &drm_mode_config_helper_funcs vtable.
> > @@ -1158,53 +1166,53 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
> >   *
> >   * For drivers supporting runtime PM the recommended sequence is instead ::
> >   *
> > - *     drm_atomic_helper_commit_modeset_disables(dev, state);
> > + *     drm_atomic_helper_commit_modeset_disables(dev, old_state);
> >   *
> > - *     drm_atomic_helper_commit_modeset_enables(dev, state);
> > + *     drm_atomic_helper_commit_modeset_enables(dev, old_state);
> >   *
> > - *     drm_atomic_helper_commit_planes(dev, state,
> > + *     drm_atomic_helper_commit_planes(dev, old_state,
> >   *                                     DRM_PLANE_COMMIT_ACTIVE_ONLY);
> >   *
> >   * for committing the atomic update to hardware.  See the kerneldoc entries for
> >   * these three functions for more details.
> >   */
> > -void drm_atomic_helper_commit_tail(struct drm_atomic_state *state)
> > +void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
> 
> I thought we would commit the new state. Why the rename to old_state?

Because the argument is the _old_ state, and the new state can be
found in the various plane, crtc, etc structures.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: Unconfuse the old_state mess in commmit_tail

[Daniel Vetter]

On Mon, Nov 21, 2016 at 06:18:02PM +0100, Daniel Vetter wrote:
> I totally butcherd the job on typing the kernel-doc for these, and no
> one realized. Noticed by Russell. Maarten has a more complete approach
> to this confusion, by making it more explicit what the new/old state
> is, instead of this magic switching behaviour.
> 
> v2:
> - Liviu pointed out that wait_for_fences is even more magic. Leave
> that as @state, and document @pre_swap better.
> - While at it, patch in header for the reference section.
> - Fix spelling issues Russell noticed.
> 
> v3: Fix up the @pre_swap note (Liviu): Also s/synchronous/blocking/,
> since async flip is something else than non-blocking.
> 
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Reported-by: Russell King - ARM Linux <linux@armlinux.org.uk>
> Cc: Russell King - ARM Linux <linux@armlinux.org.uk>
> Fixes: 9f2a7950e77a ("drm/atomic-helper: nonblocking commit support")
> Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Tomeu Vizoso <tomeu.vizoso@gmail.com>
> Cc: Daniel Stone <daniels@collabora.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Applied with Liviu's irc-r-b.
-Daniel

> ---
>  Documentation/gpu/drm-kms-helpers.rst    |  3 ++
>  drivers/gpu/drm/drm_atomic_helper.c      | 78 ++++++++++++++++++--------------
>  include/drm/drm_modeset_helper_vtables.h | 12 +++--
>  3 files changed, 54 insertions(+), 39 deletions(-)
> 
> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> index 4ca77f675967..03040aa14fe8 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -63,6 +63,9 @@ Atomic State Reset and Initialization
>  .. kernel-doc:: drivers/gpu/drm/drm_atomic_helper.c
>     :doc: atomic state reset and initialization
>  
> +Helper Functions Reference
> +--------------------------
> +
>  .. kernel-doc:: include/drm/drm_atomic_helper.h
>     :internal:
>  
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 0b16587cdc62..494680c9056e 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1006,13 +1006,21 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
>   * drm_atomic_helper_wait_for_fences - wait for fences stashed in plane state
>   * @dev: DRM device
>   * @state: atomic state object with old state structures
> - * @pre_swap: if true, do an interruptible wait
> + * @pre_swap: If true, do an interruptible wait, and @state is the new state.
> + * 	Otherwise @state is the old state.
>   *
>   * For implicit sync, driver should fish the exclusive fence out from the
>   * incoming fb's and stash it in the drm_plane_state.  This is called after
>   * drm_atomic_helper_swap_state() so it uses the current plane state (and
>   * just uses the atomic state to find the changed planes)
>   *
> + * Note that @pre_swap is needed since the point where we block for fences moves
> + * around depending upon whether an atomic commit is blocking or
> + * non-blocking. For async commit all waiting needs to happen after
> + * drm_atomic_helper_swap_state() is called, but for synchronous commits we want
> + * to wait **before** we do anything that can't be easily rolled back. That is
> + * before we call drm_atomic_helper_swap_state().
> + *
>   * Returns zero if success or < 0 if dma_fence_wait() fails.
>   */
>  int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
> @@ -1147,7 +1155,7 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
>  
>  /**
>   * drm_atomic_helper_commit_tail - commit atomic update to hardware
> - * @state: new modeset state to be committed
> + * @old_state: atomic state object with old state structures
>   *
>   * This is the default implemenation for the ->atomic_commit_tail() hook of the
>   * &drm_mode_config_helper_funcs vtable.
> @@ -1158,53 +1166,53 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
>   *
>   * For drivers supporting runtime PM the recommended sequence is instead ::
>   *
> - *     drm_atomic_helper_commit_modeset_disables(dev, state);
> + *     drm_atomic_helper_commit_modeset_disables(dev, old_state);
>   *
> - *     drm_atomic_helper_commit_modeset_enables(dev, state);
> + *     drm_atomic_helper_commit_modeset_enables(dev, old_state);
>   *
> - *     drm_atomic_helper_commit_planes(dev, state,
> + *     drm_atomic_helper_commit_planes(dev, old_state,
>   *                                     DRM_PLANE_COMMIT_ACTIVE_ONLY);
>   *
>   * for committing the atomic update to hardware.  See the kerneldoc entries for
>   * these three functions for more details.
>   */
> -void drm_atomic_helper_commit_tail(struct drm_atomic_state *state)
> +void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
>  {
> -	struct drm_device *dev = state->dev;
> +	struct drm_device *dev = old_state->dev;
>  
> -	drm_atomic_helper_commit_modeset_disables(dev, state);
> +	drm_atomic_helper_commit_modeset_disables(dev, old_state);
>  
> -	drm_atomic_helper_commit_planes(dev, state, 0);
> +	drm_atomic_helper_commit_planes(dev, old_state, 0);
>  
> -	drm_atomic_helper_commit_modeset_enables(dev, state);
> +	drm_atomic_helper_commit_modeset_enables(dev, old_state);
>  
> -	drm_atomic_helper_commit_hw_done(state);
> +	drm_atomic_helper_commit_hw_done(old_state);
>  
> -	drm_atomic_helper_wait_for_vblanks(dev, state);
> +	drm_atomic_helper_wait_for_vblanks(dev, old_state);
>  
> -	drm_atomic_helper_cleanup_planes(dev, state);
> +	drm_atomic_helper_cleanup_planes(dev, old_state);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_commit_tail);
>  
> -static void commit_tail(struct drm_atomic_state *state)
> +static void commit_tail(struct drm_atomic_state *old_state)
>  {
> -	struct drm_device *dev = state->dev;
> +	struct drm_device *dev = old_state->dev;
>  	struct drm_mode_config_helper_funcs *funcs;
>  
>  	funcs = dev->mode_config.helper_private;
>  
> -	drm_atomic_helper_wait_for_fences(dev, state, false);
> +	drm_atomic_helper_wait_for_fences(dev, old_state, false);
>  
> -	drm_atomic_helper_wait_for_dependencies(state);
> +	drm_atomic_helper_wait_for_dependencies(old_state);
>  
>  	if (funcs && funcs->atomic_commit_tail)
> -		funcs->atomic_commit_tail(state);
> +		funcs->atomic_commit_tail(old_state);
>  	else
> -		drm_atomic_helper_commit_tail(state);
> +		drm_atomic_helper_commit_tail(old_state);
>  
> -	drm_atomic_helper_commit_cleanup_done(state);
> +	drm_atomic_helper_commit_cleanup_done(old_state);
>  
> -	drm_atomic_state_put(state);
> +	drm_atomic_state_put(old_state);
>  }
>  
>  static void commit_work(struct work_struct *work)
> @@ -1498,10 +1506,10 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
>  
>  /**
>   * drm_atomic_helper_wait_for_dependencies - wait for required preceeding commits
> - * @state: new modeset state to be committed
> + * @old_state: atomic state object with old state structures
>   *
>   * This function waits for all preceeding commits that touch the same CRTC as
> - * @state to both be committed to the hardware (as signalled by
> + * @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
>   * by calling drm_crtc_vblank_send_event on the event member of
>   * &drm_crtc_state).
> @@ -1509,7 +1517,7 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
>   * This is part of the atomic helper support for nonblocking commits, see
>   * drm_atomic_helper_setup_commit() for an overview.
>   */
> -void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state)
> +void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
> @@ -1517,7 +1525,7 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state)
>  	int i;
>  	long ret;
>  
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +	for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
>  		spin_lock(&crtc->commit_lock);
>  		commit = preceeding_commit(crtc);
>  		if (commit)
> @@ -1548,7 +1556,7 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
>  
>  /**
>   * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
> - * @state: new modeset state to be committed
> + * @old_state: atomic state object with old state structures
>   *
>   * This function is used to signal completion of the hardware commit step. After
>   * this step the driver is not allowed to read or change any permanent software
> @@ -1561,15 +1569,15 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
>   * This is part of the atomic helper support for nonblocking commits, see
>   * drm_atomic_helper_setup_commit() for an overview.
>   */
> -void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state)
> +void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_crtc_commit *commit;
>  	int i;
>  
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		commit = state->crtcs[i].commit;
> +	for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
> +		commit = old_state->crtcs[i].commit;
>  		if (!commit)
>  			continue;
>  
> @@ -1584,16 +1592,16 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
>  
>  /**
>   * drm_atomic_helper_commit_cleanup_done - signal completion of commit
> - * @state: new modeset state to be committed
> + * @old_state: atomic state object with old state structures
>   *
> - * This signals completion of the atomic update @state, including any cleanup
> - * work. If used, it must be called right before calling
> + * This signals completion of the atomic update @old_state, including any
> + * cleanup work. If used, it must be called right before calling
>   * drm_atomic_state_put().
>   *
>   * This is part of the atomic helper support for nonblocking commits, see
>   * drm_atomic_helper_setup_commit() for an overview.
>   */
> -void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state)
> +void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
> @@ -1601,8 +1609,8 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state)
>  	int i;
>  	long ret;
>  
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		commit = state->crtcs[i].commit;
> +	for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
> +		commit = old_state->crtcs[i].commit;
>  		if (WARN_ON(!commit))
>  			continue;
>  
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 72478cf82147..69c3974bf133 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -999,10 +999,14 @@ struct drm_mode_config_helper_funcs {
>  	 * to implement blocking and nonblocking commits easily. It is not used
>  	 * by the atomic helpers
>  	 *
> -	 * This hook should first commit the given atomic state to the hardware.
> -	 * But drivers can add more waiting calls at the start of their
> -	 * implementation, e.g. to wait for driver-internal request for implicit
> -	 * syncing, before starting to commit the update to the hardware.
> +	 * This function is called when the new atomic state has already been
> +	 * swapped into the various state pointers. The passed in state
> +	 * therefore contains copies of the old/previous state. This hook should
> +	 * commit the new state into hardware. Note that the helpers have
> +	 * already waited for preceeding atomic commits and fences, but drivers
> +	 * can add more waiting calls at the start of their implementation, e.g.
> +	 * to wait for driver-internal request for implicit syncing, before
> +	 * starting to commit the update to the hardware.
>  	 *
>  	 * After the atomic update is committed to the hardware this hook needs
>  	 * to call drm_atomic_helper_commit_hw_done(). Then wait for the upate
> -- 
> 2.10.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel