[PATCH v3 0/4] async vs amend - UAPI

[PATCH v3 0/4] async vs amend - UAPI

[Helen Koike]

Hi,

This patch series is an attempt to clarify some concepts and how things
are hooked inside drm.

There are two main concepts that are similar but different and are
causing some confusion:

    - Asynchronous update: is the ability change the hw state at any time, not
    only during vblank.

    - Amend update: is the ability to perform 1000 commits to be applied as soon
    as possible without waiting for 1000 vblanks.

async updates can be seen as amend, but the opposite is not true.
Please see documentation on the commit
	"drm/atomic: rename async_{update,check} to amend_{update,check}"
for a more detailed explanation.

To perform an async update, we already have the DRM_MODE_PAGE_FLIP_ASYNC
flag in the atomic API and it is already being used by amdgpu in the
atomic path.

The first two commits clarifies these differences. The last two are
RFCs that exposes new async and amend features to userspace.

We introduce in this series the flag DRM_MODE_ATOMIC_AMEND to expose
the amend feature to userspace.
The main reasons to expose this through atomic api is to avoid mixing legacy
with modern/atomic API (since their interactions are not well defined)
and to be able to explicitly manage the cursor plane.

And the last commit hooks the current async implementations with the
DRM_MODE_PAGE_FLIP_ASYNC flag.

Please, see the message in each commit and the documentation that was
added for more details and let me know what you think.

Thanks
Helen

Changes in v3:
- rebase tree
- rebase on top of renaming async_update to amend_update
- improve documentation
- don't fall back to a normal commit if amend is not possible when
requested through the atomic api

Changes in v2:
- rebase tree
- do not fall back to a non-async update if if there isn't any
pending commit to amend

Changes in v1:
- https://patchwork.freedesktop.org/patch/243088/
- Only enable it if userspace requests it.
- Only allow async update for cursor type planes.
- Rename ASYNC_UPDATE for ATOMIC_AMEND.

Helen Koike (4):
  drm/uapi: add documentation for atomic flags
  drm/atomic: rename async_{update,check} to amend_{update,check}
  drm/atomic: add ATOMIC_AMEND flag to the Atomic IOCTL.
  drm/atomic: hook atomic_async_{check,update} with PAGE_FLIP_ASYNC flag

 Documentation/gpu/drm-kms-helpers.rst         |   8 +-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  15 +-
 drivers/gpu/drm/drm_atomic_helper.c           | 157 ++++++++++++++----
 drivers/gpu/drm/drm_atomic_uapi.c             |   9 +
 drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c    |   6 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c   |  12 +-
 drivers/gpu/drm/vc4/vc4_kms.c                 |   4 +-
 drivers/gpu/drm/vc4/vc4_plane.c               |   6 +
 include/drm/drm_atomic.h                      |   4 +-
 include/drm/drm_atomic_helper.h               |   9 +-
 include/drm/drm_modeset_helper_vtables.h      |  69 ++++++--
 include/uapi/drm/drm_mode.h                   |  27 ++-
 12 files changed, 264 insertions(+), 62 deletions(-)

-- 
2.20.1

[PATCH v3 1/4] drm/uapi: add documentation for atomic flags

[Helen Koike]

add a brief description of the flags used in an atomic commit

Signed-off-by: Helen Koike <helen.koike@collabora.com>
---

Changes in v3: None
Changes in v2: None
Changes in v1: None

 include/uapi/drm/drm_mode.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 83cd1636b9be..88ef2cf04d13 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -729,7 +729,23 @@ struct drm_mode_destroy_dumb {
 	__u32 handle;
 };
 
-/* page-flip flags are valid, plus: */
+/*
+ * drm atomic flags
+ *
+ * page-flip flags are valid, plus:
+ *
+ * DRM_MODE_ATOMIC_TEST_ONLY
+ * Used with fences to check if the Sync File is a valid one.
+ *
+ * DRM_MODE_ATOMIC_NONBLOCK
+ * Perform a normal atomic update but do not block the ioctl until the request
+ * is finished, return the ioctl call immediately.
+ *
+ * DRM_MODE_ATOMIC_ALLOW_MODESET
+ * Indicates whether a full modeset is acceptable or not.
+ */
+
+/*  */
 #define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
 #define DRM_MODE_ATOMIC_NONBLOCK  0x0200
 #define DRM_MODE_ATOMIC_ALLOW_MODESET 0x0400
-- 
2.20.1

[PATCH v3 2/4] drm/atomic: rename async_{update,check} to amend_{update,check}

[Helen Koike]

Asynchronous update is the ability change the hw state at any time, not
only during vblank.

Amend mode is the ability to perform 1000 commits to be applied as soon
as possible without waiting for 1000 vblanks.

async updates can be seen as amend, but the opposite is not true.

&drm_plane_helper_funcs.atomic_async_{update,check}() was being used by
drivers to implement amend and not async. So rename them to amend.

Also improve docs explaining the difference.

If asynchronous is required, normal page flip can be performed using
DRM_MODE_PAGE_FLIP_ASYNC flag.

Signed-off-by: Helen Koike <helen.koike@collabora.com>

---
Hello,

I would like to officially clarify what async update means by adding it
in the docs.
Please correct me if I am wrong, but the current async_{update,check}
callbacks are being used to do amend (the legacy cursor behavior), i.e.
to allow 1000 updates without waiting for 1000 vblanks.

So I would like to clarify this in the docs and rename the current
callbacks to reflect this behaviour.

I also see that for real async updates, the flag
DRM_MODE_PAGE_FLIP_ASYNC can be used in a normal sync update (it is
already being used by some drivers actually, in the atomic path, not only
in the legacy page flip, at least is what I understood from
amdgpu_dm_atomic_commit_tail() implementation).

Please, let me know if I misunderstood anything. Otherwise renaming and
improving the docs would put us all in sync.

Thanks!
Helen

Changes in v3: None
Changes in v2: None
Changes in v1: None

 Documentation/gpu/drm-kms-helpers.rst         |   8 +-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  10 +-
 drivers/gpu/drm/drm_atomic_helper.c           | 111 +++++++++++++-----
 drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c    |   8 +-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c   |   8 +-
 drivers/gpu/drm/vc4/vc4_kms.c                 |   4 +-
 drivers/gpu/drm/vc4/vc4_plane.c               |   8 +-
 include/drm/drm_atomic.h                      |   4 +-
 include/drm/drm_atomic_helper.h               |   4 +-
 include/drm/drm_modeset_helper_vtables.h      |  38 +++---
 10 files changed, 139 insertions(+), 64 deletions(-)

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
index 58b375e47615..c067a196902d 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -53,12 +53,18 @@ Overview
 .. kernel-doc:: drivers/gpu/drm/drm_atomic_helper.c
    :doc: overview
 
-Implementing Asynchronous Atomic Commit
+Implementing Nonblocking Atomic Commit
 ---------------------------------------
 
 .. kernel-doc:: drivers/gpu/drm/drm_atomic_helper.c
    :doc: implementing nonblocking commit
 
+Amend Mode Atomic Commit
+------------------------
+
+.. kernel-doc:: drivers/gpu/drm/drm_atomic_helper.c
+   :doc: amend mode atomic commit
+
 Helper Functions Reference
 --------------------------
 
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 2f26581b93ff..711e7715e112 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3779,8 +3779,12 @@ static const struct drm_plane_helper_funcs dm_plane_helper_funcs = {
 	.prepare_fb = dm_plane_helper_prepare_fb,
 	.cleanup_fb = dm_plane_helper_cleanup_fb,
 	.atomic_check = dm_plane_atomic_check,
-	.atomic_async_check = dm_plane_atomic_async_check,
-	.atomic_async_update = dm_plane_atomic_async_update
+	/*
+	 * FIXME: ideally, instead of hooking async updates to amend,
+	 * we could avoid tearing by modifying the pending commit.
+	 */
+	.atomic_amend_check = dm_plane_atomic_async_check,
+	.atomic_amend_update = dm_plane_atomic_async_update
 };
 
 /*
@@ -6140,7 +6144,7 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
 		 * helper, check if it can be done asynchronously for better
 		 * performance.
 		 */
-		state->async_update = !drm_atomic_helper_async_check(dev, state);
+		state->amend_update = !drm_atomic_helper_amend_check(dev, state);
 	}
 
 	/* Must be success */
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 2453678d1186..eb5dcd84fea7 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -948,7 +948,7 @@ int drm_atomic_helper_check(struct drm_device *dev,
 		return ret;
 
 	if (state->legacy_cursor_update)
-		state->async_update = !drm_atomic_helper_async_check(dev, state);
+		state->amend_update = !drm_atomic_helper_amend_check(dev, state);
 
 	return ret;
 }
@@ -1569,19 +1569,68 @@ static void commit_work(struct work_struct *work)
 }
 
 /**
- * drm_atomic_helper_async_check - check if state can be commited asynchronously
+ * DOC: amend mode atomic commit
+ *
+ * The amend feature provides a way to perform 1000 updates to be applied as
+ * soon as possible without waiting for 1000 vblanks.
+
+ * Currently, only the legacy cursor update uses amend mode, where historically,
+ * userspace performs several updates before the next vblank and don't want to
+ * see a delay in the cursor's movement.
+ * If amend is not supported, legacy cursor falls back to a normal sync update.
+ *
+ * To implement the legacy cursor update, drivers should provide
+ * &drm_plane_helper_funcs.atomic_amend_check() and
+ * &drm_plane_helper_funcs.atomic_amend_update()
+ *
+ * Drivers just need to make sure the last state overrides the previous one, so
+ * that if X updates were performed, then, in some point in the near future,
+ * preferentially in the next vblank, the Xth state will be the hardware state.
+ *
+ * If the hardware supports asynchronous update, i.e, changing its state without
+ * waiting for vblank, then &drm_plane_helper_funcs.atomic_amend_update() can be
+ * implemented using asynchronous update (the amend mode property is held), but
+ * it can cause tearing in the image.
+ *
+ * Otherwise (if async is not supported by the hw), drivers need to override the
+ * commit to be applied in the next vblank, and also they need to take care of
+ * framebuffer references when programming a new framebuffer, as hw can still be
+ * scanning out the old framebuffer. For now drivers must implement their own
+ * workers for deferring if needed, until a common solution is created.
+ *
+ *
+ * Notes / highlights:
+ *
+ * - amend update is performed on legacy cursor updates.
+ *
+ * - amend update won't happen if there is an outstanding commit modifying the
+ *   same plane.
+ *
+ * - amend update won't happen if atomic_amend_check() returns false.
+ *
+ * - if atomic_amend_check() fails, it falls back to a normal synchronous
+ *   update.
+ *
+ * - if userspace wants to ensure an asynchronous page flip, i.e. change hw
+ *   state immediately, see DRM_MODE_PAGE_FLIP_ASYNC flag
+ *   (asynchronous page flip maintains the amend property by definition).
+ *
+ * - Asynchronous modeset doesn't make sense, only asynchronous page flip.
+ */
+
+/**
+ * drm_atomic_helper_amend_check - check if state can be amended.
  * @dev: DRM device
  * @state: the driver state object
  *
- * This helper will check if it is possible to commit the state asynchronously.
- * Async commits are not supposed to swap the states like normal sync commits
- * but just do in-place changes on the current state.
+ * This helper will check if it is possible perform a commit in amend mode.
+ * For amend mode definition see :doc: amend mode atomic commit
  *
- * It will return 0 if the commit can happen in an asynchronous fashion or error
- * if not. Note that error just mean it can't be commited asynchronously, if it
- * fails the commit should be treated like a normal synchronous commit.
+ * It will return 0 if the commit can happen in an amend fashion or error
+ * if not. Note that error just mean it can't be committed in amend mode, if it
+ * fails the commit should be treated like a normal commit.
  */
-int drm_atomic_helper_async_check(struct drm_device *dev,
+int drm_atomic_helper_amend_check(struct drm_device *dev,
 				   struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
@@ -1610,7 +1659,7 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
 
 	/*
 	 * FIXME: Since prepare_fb and cleanup_fb are always called on
-	 * the new_plane_state for async updates we need to block framebuffer
+	 * the new_plane_state for amend updates, we need to block framebuffer
 	 * changes. This prevents use of a fb that's been cleaned up and
 	 * double cleanups from occuring.
 	 */
@@ -1618,37 +1667,43 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
 		return -EINVAL;
 
 	funcs = plane->helper_private;
-	if (!funcs->atomic_async_update)
+	if (!funcs->atomic_amend_update)
 		return -EINVAL;
 
 	if (new_plane_state->fence)
 		return -EINVAL;
 
 	/*
-	 * Don't do an async update if there is an outstanding commit modifying
-	 * the plane.  This prevents our async update's changes from getting
-	 * overridden by a previous synchronous update's state.
+	 * Don't do an amend update if there is an outstanding commit modifying
+	 * the plane.
+	 * TODO: add support for modifying the outstanding commit.
 	 */
 	if (old_plane_state->commit &&
 	    !try_wait_for_completion(&old_plane_state->commit->hw_done))
 		return -EBUSY;
 
-	return funcs->atomic_async_check(plane, new_plane_state);
+	return funcs->atomic_amend_check(plane, new_plane_state);
 }
-EXPORT_SYMBOL(drm_atomic_helper_async_check);
+EXPORT_SYMBOL(drm_atomic_helper_amend_check);
 
 /**
- * drm_atomic_helper_async_commit - commit state asynchronously
+ * drm_atomic_helper_amend_commit - commit state in amend mode
  * @dev: DRM device
  * @state: the driver state object
  *
- * This function commits a state asynchronously, i.e., not vblank
- * synchronized. It should be used on a state only when
- * drm_atomic_async_check() succeeds. Async commits are not supposed to swap
- * the states like normal sync commits, but just do in-place changes on the
- * current state.
+ * This function commits a state in amend mode.
+ * For amend mode definition see :doc: amend mode atomic commit
+ *
+ * It should be used on a state only when drm_atomic_amend_check() succeeds.
+ *
+ * Amend commits are not supposed to swap the states like normal sync commits,
+ * but just do in-place changes on the current state.
+ *
+ * TODO: instead of doing in-place changes, modify the new_state and perform an
+ * immediate flip. Drivers could reuse the page_flip code and even use the
+ * DRM_MODE_PAGE_FLIP_ASYNC flag if the hardware supports asyncronous update.
  */
-void drm_atomic_helper_async_commit(struct drm_device *dev,
+void drm_atomic_helper_amend_commit(struct drm_device *dev,
 				    struct drm_atomic_state *state)
 {
 	struct drm_plane *plane;
@@ -1658,10 +1713,10 @@ void drm_atomic_helper_async_commit(struct drm_device *dev,
 
 	for_each_new_plane_in_state(state, plane, plane_state, i) {
 		funcs = plane->helper_private;
-		funcs->atomic_async_update(plane, plane_state);
+		funcs->atomic_amend_update(plane, plane_state);
 
 		/*
-		 * ->atomic_async_update() is supposed to update the
+		 * ->atomic_amend_update() is supposed to update the
 		 * plane->state in-place, make sure at least common
 		 * properties have been properly updated.
 		 */
@@ -1672,7 +1727,7 @@ void drm_atomic_helper_async_commit(struct drm_device *dev,
 		WARN_ON_ONCE(plane->state->src_y != plane_state->src_y);
 	}
 }
-EXPORT_SYMBOL(drm_atomic_helper_async_commit);
+EXPORT_SYMBOL(drm_atomic_helper_amend_commit);
 
 /**
  * drm_atomic_helper_commit - commit validated state object
@@ -1698,12 +1753,12 @@ int drm_atomic_helper_commit(struct drm_device *dev,
 {
 	int ret;
 
-	if (state->async_update) {
+	if (state->amend_update) {
 		ret = drm_atomic_helper_prepare_planes(dev, state);
 		if (ret)
 			return ret;
 
-		drm_atomic_helper_async_commit(dev, state);
+		drm_atomic_helper_amend_commit(dev, state);
 		drm_atomic_helper_cleanup_planes(dev, state);
 
 		return 0;
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
index be13140967b4..814e8230cec6 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
@@ -531,8 +531,12 @@ static const struct drm_plane_helper_funcs mdp5_plane_helper_funcs = {
 		.cleanup_fb = mdp5_plane_cleanup_fb,
 		.atomic_check = mdp5_plane_atomic_check,
 		.atomic_update = mdp5_plane_atomic_update,
-		.atomic_async_check = mdp5_plane_atomic_async_check,
-		.atomic_async_update = mdp5_plane_atomic_async_update,
+		/*
+		 * FIXME: ideally, instead of hooking async updates to amend,
+		 * we could avoid tearing by modifying the pending commit.
+		 */
+		.atomic_amend_check = mdp5_plane_atomic_async_check,
+		.atomic_amend_update = mdp5_plane_atomic_async_update,
 };
 
 static void set_scanout_locked(struct mdp5_kms *mdp5_kms,
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index a7cbf6c9a153..216ad76118dc 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -877,7 +877,7 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
 	spin_unlock(&vop->reg_lock);
 }
 
-static int vop_plane_atomic_async_check(struct drm_plane *plane,
+static int vop_plane_atomic_amend_check(struct drm_plane *plane,
 					struct drm_plane_state *state)
 {
 	struct vop_win *vop_win = to_vop_win(plane);
@@ -908,7 +908,7 @@ static int vop_plane_atomic_async_check(struct drm_plane *plane,
 						   true, true);
 }
 
-static void vop_plane_atomic_async_update(struct drm_plane *plane,
+static void vop_plane_atomic_amend_update(struct drm_plane *plane,
 					  struct drm_plane_state *new_state)
 {
 	struct vop *vop = to_vop(plane->state->crtc);
@@ -952,8 +952,8 @@ static const struct drm_plane_helper_funcs plane_helper_funcs = {
 	.atomic_check = vop_plane_atomic_check,
 	.atomic_update = vop_plane_atomic_update,
 	.atomic_disable = vop_plane_atomic_disable,
-	.atomic_async_check = vop_plane_atomic_async_check,
-	.atomic_async_update = vop_plane_atomic_async_update,
+	.atomic_amend_check = vop_plane_atomic_amend_check,
+	.atomic_amend_update = vop_plane_atomic_amend_update,
 	.prepare_fb = drm_gem_fb_prepare_fb,
 };
 
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 295dacc8bcb9..229032b1b315 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -216,7 +216,7 @@ static int vc4_atomic_commit(struct drm_device *dev,
 	struct vc4_dev *vc4 = to_vc4_dev(dev);
 	int ret;
 
-	if (state->async_update) {
+	if (state->amend_update) {
 		ret = down_interruptible(&vc4->async_modeset);
 		if (ret)
 			return ret;
@@ -227,7 +227,7 @@ static int vc4_atomic_commit(struct drm_device *dev,
 			return ret;
 		}
 
-		drm_atomic_helper_async_commit(dev, state);
+		drm_atomic_helper_amend_commit(dev, state);
 
 		drm_atomic_helper_cleanup_planes(dev, state);
 
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 4d918d3e4858..ea560000222d 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -1169,8 +1169,12 @@ static const struct drm_plane_helper_funcs vc4_plane_helper_funcs = {
 	.atomic_update = vc4_plane_atomic_update,
 	.prepare_fb = vc4_prepare_fb,
 	.cleanup_fb = vc4_cleanup_fb,
-	.atomic_async_check = vc4_plane_atomic_async_check,
-	.atomic_async_update = vc4_plane_atomic_async_update,
+	/*
+	 * FIXME: ideally, instead of hooking async updates to amend,
+	 * we could avoid tearing by modifying the pending commit.
+	 */
+	.atomic_amend_check = vc4_plane_atomic_async_check,
+	.atomic_amend_update = vc4_plane_atomic_async_update,
 };
 
 static void vc4_plane_destroy(struct drm_plane *plane)
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 824a5ed4e216..b1ced069ffc1 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -300,7 +300,7 @@ struct __drm_private_objs_state {
  * @ref: count of all references to this state (will not be freed until zero)
  * @dev: parent DRM device
  * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics
- * @async_update: hint for asynchronous plane update
+ * @amend_update: hint for amend plane update
  * @planes: pointer to array of structures with per-plane data
  * @crtcs: pointer to array of CRTC pointers
  * @num_connector: size of the @connectors and @connector_states arrays
@@ -328,7 +328,7 @@ struct drm_atomic_state {
 	 */
 	bool allow_modeset : 1;
 	bool legacy_cursor_update : 1;
-	bool async_update : 1;
+	bool amend_update : 1;
 	/**
 	 * @duplicated:
 	 *
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 58214be3bf3d..8ce0594ccfb9 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -55,9 +55,9 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *state);
 int drm_atomic_helper_commit(struct drm_device *dev,
 			     struct drm_atomic_state *state,
 			     bool nonblock);
-int drm_atomic_helper_async_check(struct drm_device *dev,
+int drm_atomic_helper_amend_check(struct drm_device *dev,
 				  struct drm_atomic_state *state);
-void drm_atomic_helper_async_commit(struct drm_device *dev,
+void drm_atomic_helper_amend_commit(struct drm_device *dev,
 				    struct drm_atomic_state *state);
 
 int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index cfb7be40bed7..d92e62dd76c4 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -1136,53 +1136,55 @@ struct drm_plane_helper_funcs {
 			       struct drm_plane_state *old_state);
 
 	/**
-	 * @atomic_async_check:
+	 * @atomic_amend_check:
 	 *
 	 * Drivers should set this function pointer to check if the plane state
-	 * can be updated in a async fashion. Here async means "not vblank
-	 * synchronized".
+	 * can be updated in amend mode.
+	 * For amend mode definition see :doc: amend mode atomic commit
 	 *
-	 * This hook is called by drm_atomic_async_check() to establish if a
-	 * given update can be committed asynchronously, that is, if it can
+	 * This hook is called by drm_atomic_amend_check() to establish if a
+	 * given update can be committed in amend mode, that is, if it can
 	 * jump ahead of the state currently queued for update.
 	 *
 	 * RETURNS:
 	 *
 	 * Return 0 on success and any error returned indicates that the update
-	 * can not be applied in asynchronous manner.
+	 * can not be applied in amend mode.
 	 */
-	int (*atomic_async_check)(struct drm_plane *plane,
+	int (*atomic_amend_check)(struct drm_plane *plane,
 				  struct drm_plane_state *state);
 
 	/**
-	 * @atomic_async_update:
+	 * @atomic_amend_update:
 	 *
-	 * Drivers should set this function pointer to perform asynchronous
-	 * updates of planes, that is, jump ahead of the currently queued
-	 * state and update the plane. Here async means "not vblank
-	 * synchronized".
+	 * Drivers should set this function pointer to perform amend
+	 * updates of planes.
+	 * The amend feature provides a way to perform 1000 commits, without
+	 * waiting for 1000 vblanks to get the last state applied.
 	 *
-	 * This hook is called by drm_atomic_helper_async_commit().
+	 * For amend mode definition see :doc: amend mode atomic commit
 	 *
-	 * An async update will happen on legacy cursor updates. An async
+	 * This hook is called by drm_atomic_helper_amend_commit().
+	 *
+	 * An amend update will happen on legacy cursor updates. An amend
 	 * update won't happen if there is an outstanding commit modifying
 	 * the same plane.
 	 *
 	 * Note that unlike &drm_plane_helper_funcs.atomic_update this hook
-	 * takes the new &drm_plane_state as parameter. When doing async_update
+	 * takes the new &drm_plane_state as parameter. When doing amend_update
 	 * drivers shouldn't replace the &drm_plane_state but update the
 	 * current one with the new plane configurations in the new
 	 * plane_state.
 	 *
 	 * FIXME:
 	 *  - It only works for single plane updates
-	 *  - Async Pageflips are not supported yet
-	 *  - Some hw might still scan out the old buffer until the next
+	 *  - If hardware don't support asyncronous update to implement amend,
+	 *    some hw might still scan out the old buffer until the next
 	 *    vblank, however we let go of the fb references as soon as
 	 *    we run this hook. For now drivers must implement their own workers
 	 *    for deferring if needed, until a common solution is created.
 	 */
-	void (*atomic_async_update)(struct drm_plane *plane,
+	void (*atomic_amend_update)(struct drm_plane *plane,
 				    struct drm_plane_state *new_state);
 };
 
-- 
2.20.1

[PATCH RFC v3 3/4] drm/atomic: add ATOMIC_AMEND flag to the Atomic IOCTL.

[Helen Koike]

This flag tells core to jump ahead the queued update if the conditions
in drm_atomic_amend_check() are met. That means we are only able to do an
amend update if no modeset is pending and update for the same plane is
not queued.

It uses the already in place infrastructure for amend updates.

It is useful for cursor updates over the atomic ioctl.
Otherwise in some cases updates may be delayed to the point the
user will notice it.
Note that for now it's only enabled for cursor planes.

DRM_MODE_ATOMIC_AMEND should be passed to the Atomic IOCTL to use this
feature.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
[updated for upstream]
Signed-off-by: Helen Koike <helen.koike@collabora.com>
---
Hi,

This is the third attempt to introduce the new ATOMIC_AMEND flag for atomic
operations, see the commit message for a more detailed description.
I am sending this patch in the series as an RFC as I still have things
to define/discuss.

The main reasons to expose this through atomic api is to avoid mixing legacy
with modern/atomic API (since their interactions are not well defined)
and to be able to explicitly manage the cursor plane.

The way I envision it working is:

1) userspace just use DRM_MODE_PAGE_FLIP_ASYNC if true async is desired.
2) if it fails, then userspace can try DRM_MODE_ATOMIC_AMEND if true async is
not required, but a legacy cursor behavior is required
3) if amend is not possible, we can or:
        A) fallback to a non amend update
        B) fail
        C) add another flag to define what it should do.

Right now the code does A for legacy cursor and B for atomic api using
the DRM_MODE_ATOMIC_AMEND flag.
Discussing with some people it seems that failing is better for Xorg,
but Ozone (chrome compositor) doesn't expect the amend to fail, but I
think it could just retry the same atomic commit without the AMEND flag
if it wants to fallback.

Alexandros also did a proof-of-concept to use this flag and draw cursors
using atomic if possible on ozone (Chrome compositor) [1].

I'm sending this as an RFC for now as I still need to verify the
requirements from other compositors and make some tests (and also to
justify the s/async/amend renaming).

Please, let me know if you detect issues with this.
If not, I'll start implementing tests in igt and push the adoption in
other compositors.

Thanks
Helen

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1092711
[2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability


Changes in v3:
- rebase tree
- rebase on top of renaming async_update to amend_update
- improve documentation
- don't fall back to a normal commit if amend is not possible when
requested through the atomic api

Changes in v2:
- rebase tree
- do not fall back to a non-async update if if there isn't any
pending commit to amend

Changes in v1:
- https://patchwork.freedesktop.org/patch/243088/
- Only enable it if userspace requests it.
- Only allow async update for cursor type planes.
- Rename ASYNC_UPDATE for ATOMIC_AMEND.

 drivers/gpu/drm/drm_atomic_helper.c | 39 +++++++++++++++++++++++------
 drivers/gpu/drm/drm_atomic_uapi.c   |  8 ++++++
 include/uapi/drm/drm_mode.h         |  9 ++++++-
 3 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index eb5dcd84fea7..9b0df08836c3 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -947,10 +947,18 @@ int drm_atomic_helper_check(struct drm_device *dev,
 	if (ret)
 		return ret;
 
+	/*
+	 * If amend was explicitly requested, but it is not possible,
+	 * return error instead of falling back to a normal commit.
+	 */
+	if (state->amend_update)
+		return drm_atomic_helper_amend_check(dev, state);
+
+	/* Legacy mode falls back to a normal commit if amend isn't possible. */
 	if (state->legacy_cursor_update)
 		state->amend_update = !drm_atomic_helper_amend_check(dev, state);
 
-	return ret;
+	return 0;
 }
 EXPORT_SYMBOL(drm_atomic_helper_check);
 
@@ -1574,12 +1582,20 @@ static void commit_work(struct work_struct *work)
  * The amend feature provides a way to perform 1000 updates to be applied as
  * soon as possible without waiting for 1000 vblanks.
 
- * Currently, only the legacy cursor update uses amend mode, where historically,
+ * Legacy cursor update uses amend mode, where historically,
  * userspace performs several updates before the next vblank and don't want to
  * see a delay in the cursor's movement.
  * If amend is not supported, legacy cursor falls back to a normal sync update.
  *
- * To implement the legacy cursor update, drivers should provide
+ * Amend can also be performed through the atomic API using the flag
+ * DRM_MODE_ATOMIC_AMEND. The atomic commit will fail if this flag is set but
+ * the driver doesn't support it.
+ * Amend can't perform modeset, thus atomic API will fail if
+ * DRM_MODE_ATOMIC_AMEND is used in conjunction with
+ * DRM_MODE_ATOMIC_ALLOW_MODESET flag.
+ *
+ * To implement the legacy cursor update and amend mode through atomic, drivers
+ * should provide:
  * &drm_plane_helper_funcs.atomic_amend_check() and
  * &drm_plane_helper_funcs.atomic_amend_update()
  *
@@ -1601,16 +1617,21 @@ static void commit_work(struct work_struct *work)
  *
  * Notes / highlights:
  *
- * - amend update is performed on legacy cursor updates.
+ * - amend update is performed on legacy cursor updates, but it will fallback to
+ *   a normal commit if amend is not possible. However, if amend was requested
+ *   through atomic, the ioctl will fail instead of fall back if it can't be
+ *   amended.
+ *
+ * - atomic api will reject amend if DRM_MODE_ATOMIC_ALLOW_MODESET flag is used.
+ *
+ * - If DRM_MODE_PAGE_FLIP_ASYNC is set, then the DRM_MODE_ATOMIC_AMEND flag
+ *   will be ignored.
  *
  * - amend update won't happen if there is an outstanding commit modifying the
  *   same plane.
  *
  * - amend update won't happen if atomic_amend_check() returns false.
  *
- * - if atomic_amend_check() fails, it falls back to a normal synchronous
- *   update.
- *
  * - if userspace wants to ensure an asynchronous page flip, i.e. change hw
  *   state immediately, see DRM_MODE_PAGE_FLIP_ASYNC flag
  *   (asynchronous page flip maintains the amend property by definition).
@@ -1673,6 +1694,10 @@ int drm_atomic_helper_amend_check(struct drm_device *dev,
 	if (new_plane_state->fence)
 		return -EINVAL;
 
+	/* Only allow amend update for cursor type planes. */
+	if (plane->type != DRM_PLANE_TYPE_CURSOR)
+		return -EINVAL;
+
 	/*
 	 * Don't do an amend update if there is an outstanding commit modifying
 	 * the plane.
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 4eb81f10bc54..d1962cdea602 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -28,6 +28,7 @@
 
 #include <drm/drm_atomic_uapi.h>
 #include <drm/drm_atomic.h>
+#include <drm/drm_atomic_uapi.h>
 #include <drm/drm_print.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_writeback.h>
@@ -1300,6 +1301,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 			(arg->flags & DRM_MODE_PAGE_FLIP_EVENT))
 		return -EINVAL;
 
+	if ((arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET) &&
+			(arg->flags & DRM_MODE_ATOMIC_AMEND))
+		return -EINVAL;
+
 	state = drm_atomic_state_alloc(dev);
 	if (!state)
 		return -ENOMEM;
@@ -1307,6 +1312,9 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
 	state->acquire_ctx = &ctx;
 	state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
+	/* async takes precedence over amend */
+	state->amend_update = arg->flags & DRM_MODE_PAGE_FLIP_ASYNC ? 0 :
+					!!(arg->flags & DRM_MODE_ATOMIC_AMEND);
 
 retry:
 	copied_objs = 0;
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 88ef2cf04d13..3831d04f3b38 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -743,19 +743,26 @@ struct drm_mode_destroy_dumb {
  *
  * DRM_MODE_ATOMIC_ALLOW_MODESET
  * Indicates whether a full modeset is acceptable or not.
+ *
+ * DRM_MODE_ATOMIC_AMEND
+ * Used to perform an atomic amend. See "DOC: amend mode atomic commit" in
+ * drm_atomic_helper.c for more details.
+ * This flag can't be used with DRM_MODE_ATOMIC_ALLOW_MODESET.
  */
 
 /*  */
 #define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
 #define DRM_MODE_ATOMIC_NONBLOCK  0x0200
 #define DRM_MODE_ATOMIC_ALLOW_MODESET 0x0400
+#define DRM_MODE_ATOMIC_AMEND 0x0800
 
 #define DRM_MODE_ATOMIC_FLAGS (\
 		DRM_MODE_PAGE_FLIP_EVENT |\
 		DRM_MODE_PAGE_FLIP_ASYNC |\
 		DRM_MODE_ATOMIC_TEST_ONLY |\
 		DRM_MODE_ATOMIC_NONBLOCK |\
-		DRM_MODE_ATOMIC_ALLOW_MODESET)
+		DRM_MODE_ATOMIC_ALLOW_MODESET |\
+		DRM_MODE_ATOMIC_AMEND)
 
 struct drm_mode_atomic {
 	__u32 flags;
-- 
2.20.1

[PATCH RFC v3 4/4] drm/atomic: hook atomic_async_{check,update} with PAGE_FLIP_ASYNC flag

[Helen Koike]

Add atomic_async_{check,update} hooks in drm_plane_helper_funcs.
These hooks are called when userspace requests asyncronous page flip in
the atomic api through the flag DRM_MODE_PAGE_FLIP_ASYNC.

Update those hooks in the drivers that implement async functions, except
for amdgpu who handles the flag in the normal path, and rockchip who
doesn't support async page flip.

Signed-off-by: Helen Koike <helen.koike@collabora.com>

---
Hi,

This patch is an attempt to expose the implementation that already exist
for true async page flips updates through atomic api when the
DRM_MODE_PAGE_FLIP_ASYNC is used.

In this commit I'm re-introducing the atomic_async_{check,update} hooks.
I know it is a bit weird to remove them first and them re-add them, but
I did this in the last commit to avoid any state of inconsistency
between commits, as rockchip doesn't support async page flip and they were
being used as amend.
So I reverted to amend first and then re-introced async again
tied to the DRM_MODE_PAGE_FLIP_ASYNC flag (I also think this is easier
to read).

To use async, it is required to have
dev->mode_config.async_page_flip = true;
but I see that only amdgpu and vc4 have this, so this patch won't take
effect on mdp5.
Nouveau and radeon also have this flag, but they don't implement the
async hooks yet.

Please let me know what you think.

Thanks
Helen

Changes in v3: None
Changes in v2: None
Changes in v1: None

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  5 +++
 drivers/gpu/drm/drm_atomic_helper.c           | 31 ++++++++++++----
 drivers/gpu/drm/drm_atomic_uapi.c             |  3 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c    |  2 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c   |  4 ++
 drivers/gpu/drm/vc4/vc4_plane.c               |  2 +
 include/drm/drm_atomic.h                      |  2 +
 include/drm/drm_atomic_helper.h               |  9 +++--
 include/drm/drm_modeset_helper_vtables.h      | 37 +++++++++++++++++++
 9 files changed, 83 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 711e7715e112..bb8a5f1997d6 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3785,6 +3785,11 @@ static const struct drm_plane_helper_funcs dm_plane_helper_funcs = {
 	 */
 	.atomic_amend_check = dm_plane_atomic_async_check,
 	.atomic_amend_update = dm_plane_atomic_async_update
+	/*
+	 * Note: amdgpu takes care of DRM_MODE_PAGE_FLIP_ASYNC flag in the
+	 * normal commit path, thus .atomic_async_check and .atomic_async_update
+	 * are not provided here.
+	 */
 };
 
 /*
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 9b0df08836c3..bfcf88359de5 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -947,16 +947,31 @@ int drm_atomic_helper_check(struct drm_device *dev,
 	if (ret)
 		return ret;
 
+	/*
+	 * If async page flip was explicitly requested, but it is not possible,
+	 * return error instead of falling back to a normal commit.
+	 * If async_amend_check returns -EOPNOTSUPP, it means
+	 * ->atomic_async_update hook doesn't exist, so fallback to normal
+	 *  commit and let the driver handle DRM_MODE_PAGE_FLIP_ASYNC flag
+	 *  through normal commit code path.
+	 */
+	if (state->async_update) {
+		ret = drm_atomic_helper_async_amend_check(dev, state, false);
+		state->async_update = !ret;
+		return !ret || ret == -EOPNOTSUPP ? 0 : ret;
+	}
+
 	/*
 	 * If amend was explicitly requested, but it is not possible,
 	 * return error instead of falling back to a normal commit.
 	 */
 	if (state->amend_update)
-		return drm_atomic_helper_amend_check(dev, state);
+		return drm_atomic_helper_async_amend_check(dev, state, true);
 
 	/* Legacy mode falls back to a normal commit if amend isn't possible. */
 	if (state->legacy_cursor_update)
-		state->amend_update = !drm_atomic_helper_amend_check(dev, state);
+		state->amend_update =
+			!drm_atomic_helper_async_amend_check(dev, state, true);
 
 	return 0;
 }
@@ -1651,8 +1666,9 @@ static void commit_work(struct work_struct *work)
  * if not. Note that error just mean it can't be committed in amend mode, if it
  * fails the commit should be treated like a normal commit.
  */
-int drm_atomic_helper_amend_check(struct drm_device *dev,
-				   struct drm_atomic_state *state)
+int drm_atomic_helper_async_amend_check(struct drm_device *dev,
+					struct drm_atomic_state *state,
+					bool amend)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
@@ -1695,7 +1711,7 @@ int drm_atomic_helper_amend_check(struct drm_device *dev,
 		return -EINVAL;
 
 	/* Only allow amend update for cursor type planes. */
-	if (plane->type != DRM_PLANE_TYPE_CURSOR)
+	if (amend && plane->type != DRM_PLANE_TYPE_CURSOR)
 		return -EINVAL;
 
 	/*
@@ -1707,9 +1723,10 @@ int drm_atomic_helper_amend_check(struct drm_device *dev,
 	    !try_wait_for_completion(&old_plane_state->commit->hw_done))
 		return -EBUSY;
 
-	return funcs->atomic_amend_check(plane, new_plane_state);
+	return amend ? funcs->atomic_amend_check(plane, new_plane_state) :
+		       funcs->atomic_async_check(plane, new_plane_state);
 }
-EXPORT_SYMBOL(drm_atomic_helper_amend_check);
+EXPORT_SYMBOL(drm_atomic_helper_async_amend_check);
 
 /**
  * drm_atomic_helper_amend_commit - commit state in amend mode
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index d1962cdea602..1d9a6142218e 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1312,8 +1312,9 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
 	state->acquire_ctx = &ctx;
 	state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
+	state->async_update = !!(arg->flags & DRM_MODE_PAGE_FLIP_ASYNC);
 	/* async takes precedence over amend */
-	state->amend_update = arg->flags & DRM_MODE_PAGE_FLIP_ASYNC ? 0 :
+	state->amend_update = state->async_update ? 0 :
 					!!(arg->flags & DRM_MODE_ATOMIC_AMEND);
 
 retry:
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
index 814e8230cec6..e3b2a2c74852 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
@@ -531,6 +531,8 @@ static const struct drm_plane_helper_funcs mdp5_plane_helper_funcs = {
 		.cleanup_fb = mdp5_plane_cleanup_fb,
 		.atomic_check = mdp5_plane_atomic_check,
 		.atomic_update = mdp5_plane_atomic_update,
+		.atomic_async_check = mdp5_plane_atomic_async_check,
+		.atomic_async_update = mdp5_plane_atomic_async_update,
 		/*
 		 * FIXME: ideally, instead of hooking async updates to amend,
 		 * we could avoid tearing by modifying the pending commit.
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 216ad76118dc..c2f7201e52a9 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -954,6 +954,10 @@ static const struct drm_plane_helper_funcs plane_helper_funcs = {
 	.atomic_disable = vop_plane_atomic_disable,
 	.atomic_amend_check = vop_plane_atomic_amend_check,
 	.atomic_amend_update = vop_plane_atomic_amend_update,
+	/*
+	 * Note: rockchip doesn't support async page flip, thus
+	 * .atomic_async_update and .atomic_async_check are not provided.
+	 */
 	.prepare_fb = drm_gem_fb_prepare_fb,
 };
 
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index ea560000222d..24a9befe89e6 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -1175,6 +1175,8 @@ static const struct drm_plane_helper_funcs vc4_plane_helper_funcs = {
 	 */
 	.atomic_amend_check = vc4_plane_atomic_async_check,
 	.atomic_amend_update = vc4_plane_atomic_async_update,
+	.atomic_async_check = vc4_plane_atomic_async_check,
+	.atomic_async_update = vc4_plane_atomic_async_update,
 };
 
 static void vc4_plane_destroy(struct drm_plane *plane)
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index b1ced069ffc1..05756a09e762 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -300,6 +300,7 @@ struct __drm_private_objs_state {
  * @ref: count of all references to this state (will not be freed until zero)
  * @dev: parent DRM device
  * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics
+ * @async_update: hint for asyncronous page flip update
  * @amend_update: hint for amend plane update
  * @planes: pointer to array of structures with per-plane data
  * @crtcs: pointer to array of CRTC pointers
@@ -328,6 +329,7 @@ struct drm_atomic_state {
 	 */
 	bool allow_modeset : 1;
 	bool legacy_cursor_update : 1;
+	bool async_update : 1;
 	bool amend_update : 1;
 	/**
 	 * @duplicated:
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 8ce0594ccfb9..39e57d559a30 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -55,10 +55,11 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *state);
 int drm_atomic_helper_commit(struct drm_device *dev,
 			     struct drm_atomic_state *state,
 			     bool nonblock);
-int drm_atomic_helper_amend_check(struct drm_device *dev,
-				  struct drm_atomic_state *state);
-void drm_atomic_helper_amend_commit(struct drm_device *dev,
-				    struct drm_atomic_state *state);
+int drm_atomic_helper_async_amend_check(struct drm_device *dev,
+					struct drm_atomic_state *state,
+					bool amend);
+void drm_atomic_helper_async_amend_commit(struct drm_device *dev,
+					  struct drm_atomic_state *state);
 
 int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
 					struct drm_atomic_state *state,
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index d92e62dd76c4..c2863d62f160 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -1135,6 +1135,43 @@ struct drm_plane_helper_funcs {
 	void (*atomic_disable)(struct drm_plane *plane,
 			       struct drm_plane_state *old_state);
 
+	/**
+	 * @atomic_async_check:
+	 *
+	 * Drivers should set this function pointer to check if a page flip can
+	 * be performed asynchronously, i.e., immediately without waiting for a
+	 * vblank.
+	 *
+	 * This hook is called by drm_atomic_async_check() to establish if a
+	 * given update can be committed in async mode, that is, if it can
+	 * jump ahead of the state currently queued for update.
+	 *
+	 * RETURNS:
+	 *
+	 * Return 0 on success and any error returned indicates that the update
+	 * can not be applied in asynd mode.
+	 */
+	int (*atomic_async_check)(struct drm_plane *plane,
+				  struct drm_plane_state *state);
+
+	/**
+	 * @atomic_async_update:
+	 *
+	 * Drivers should set this function pointer to perform asynchronous page
+	 * flips through the atomic api, i.e. not vblank synchronized.
+	 *
+	 * Note that unlike &drm_plane_helper_funcs.atomic_update this hook
+	 * takes the new &drm_plane_state as parameter. When doing async_update
+	 * drivers shouldn't replace the &drm_plane_state but update the
+	 * current one with the new plane configurations in the new
+	 * plane_state.
+	 *
+	 * FIXME:
+	 *  - It only works for single plane updates
+	 */
+	void (*atomic_async_update)(struct drm_plane *plane,
+				    struct drm_plane_state *new_state);
+
 	/**
 	 * @atomic_amend_check:
 	 *
-- 
2.20.1

Re: [PATCH v3 1/4] drm/uapi: add documentation for atomic flags

[Boris Brezillon]

Hello Helen,

On Fri, 12 Apr 2019 09:58:24 -0300
Helen Koike <helen.koike@collabora.com> wrote:

> add a brief description of the flags used in an atomic commit
> 
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> ---
> 
> Changes in v3: None
> Changes in v2: None
> Changes in v1: None
> 
>  include/uapi/drm/drm_mode.h | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 83cd1636b9be..88ef2cf04d13 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -729,7 +729,23 @@ struct drm_mode_destroy_dumb {
>  	__u32 handle;
>  };
>  
> -/* page-flip flags are valid, plus: */
> +/*

You might want to use a standard kernel/sphynx doc header so that it
can be parsed by sphynx and included in the DRM doc.

> + * drm atomic flags
> + *
> + * page-flip flags are valid, plus:
> + *
> + * DRM_MODE_ATOMIC_TEST_ONLY
> + * Used with fences to check if the Sync File is a valid one.

I think it can be used for any kind of tests, like when you want to
know if a combination of property updates is supported. It's fine
having the fence example, but I think you should be clear that it's
just one use case.

> + *
> + * DRM_MODE_ATOMIC_NONBLOCK
> + * Perform a normal atomic update but do not block the ioctl until the request
> + * is finished, return the ioctl call immediately.
> + *
> + * DRM_MODE_ATOMIC_ALLOW_MODESET
> + * Indicates whether a full modeset is acceptable or not.
> + */
> +
> +/*  */
>  #define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
>  #define DRM_MODE_ATOMIC_NONBLOCK  0x0200
>  #define DRM_MODE_ATOMIC_ALLOW_MODESET 0x0400

Regards,

Boris

Re: [PATCH RFC v3 4/4] drm/atomic: hook atomic_async_{check,update} with PAGE_FLIP_ASYNC flag

[Helen Koike]

On 4/12/19 9:58 AM, Helen Koike wrote:
> Add atomic_async_{check,update} hooks in drm_plane_helper_funcs.
> These hooks are called when userspace requests asyncronous page flip in
> the atomic api through the flag DRM_MODE_PAGE_FLIP_ASYNC.
> 
> Update those hooks in the drivers that implement async functions, except
> for amdgpu who handles the flag in the normal path, and rockchip who
> doesn't support async page flip.
> 
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> 
> ---
> Hi,
> 
> This patch is an attempt to expose the implementation that already exist
> for true async page flips updates through atomic api when the
> DRM_MODE_PAGE_FLIP_ASYNC is used.
> 
> In this commit I'm re-introducing the atomic_async_{check,update} hooks.
> I know it is a bit weird to remove them first and them re-add them, but
> I did this in the last commit to avoid any state of inconsistency
> between commits, as rockchip doesn't support async page flip and they were
> being used as amend.
> So I reverted to amend first and then re-introced async again
> tied to the DRM_MODE_PAGE_FLIP_ASYNC flag (I also think this is easier
> to read).
> 
> To use async, it is required to have
> dev->mode_config.async_page_flip = true;
> but I see that only amdgpu and vc4 have this, so this patch won't take
> effect on mdp5.
> Nouveau and radeon also have this flag, but they don't implement the
> async hooks yet.
> 
> Please let me know what you think.
> 
> Thanks
> Helen
> 
> Changes in v3: None
> Changes in v2: None
> Changes in v1: None
> 
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  5 +++
>  drivers/gpu/drm/drm_atomic_helper.c           | 31 ++++++++++++----
>  drivers/gpu/drm/drm_atomic_uapi.c             |  3 +-
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c    |  2 +
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c   |  4 ++
>  drivers/gpu/drm/vc4/vc4_plane.c               |  2 +
>  include/drm/drm_atomic.h                      |  2 +
>  include/drm/drm_atomic_helper.h               |  9 +++--
>  include/drm/drm_modeset_helper_vtables.h      | 37 +++++++++++++++++++
>  9 files changed, 83 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 711e7715e112..bb8a5f1997d6 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3785,6 +3785,11 @@ static const struct drm_plane_helper_funcs dm_plane_helper_funcs = {
>  	 */
>  	.atomic_amend_check = dm_plane_atomic_async_check,
>  	.atomic_amend_update = dm_plane_atomic_async_update
> +	/*
> +	 * Note: amdgpu takes care of DRM_MODE_PAGE_FLIP_ASYNC flag in the
> +	 * normal commit path, thus .atomic_async_check and .atomic_async_update
> +	 * are not provided here.
> +	 */
>  };
>  
>  /*
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 9b0df08836c3..bfcf88359de5 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -947,16 +947,31 @@ int drm_atomic_helper_check(struct drm_device *dev,
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * If async page flip was explicitly requested, but it is not possible,
> +	 * return error instead of falling back to a normal commit.
> +	 * If async_amend_check returns -EOPNOTSUPP, it means
> +	 * ->atomic_async_update hook doesn't exist, so fallback to normal
> +	 *  commit and let the driver handle DRM_MODE_PAGE_FLIP_ASYNC flag
> +	 *  through normal commit code path.
> +	 */
> +	if (state->async_update) {
> +		ret = drm_atomic_helper_async_amend_check(dev, state, false);
> +		state->async_update = !ret;
> +		return !ret || ret == -EOPNOTSUPP ? 0 : ret;
> +	}
> +
>  	/*
>  	 * If amend was explicitly requested, but it is not possible,
>  	 * return error instead of falling back to a normal commit.
>  	 */
>  	if (state->amend_update)
> -		return drm_atomic_helper_amend_check(dev, state);
> +		return drm_atomic_helper_async_amend_check(dev, state, true);
>  
>  	/* Legacy mode falls back to a normal commit if amend isn't possible. */
>  	if (state->legacy_cursor_update)
> -		state->amend_update = !drm_atomic_helper_amend_check(dev, state);
> +		state->amend_update =
> +			!drm_atomic_helper_async_amend_check(dev, state, true);
>  
>  	return 0;
>  }
> @@ -1651,8 +1666,9 @@ static void commit_work(struct work_struct *work)
>   * if not. Note that error just mean it can't be committed in amend mode, if it
>   * fails the commit should be treated like a normal commit.
>   */
> -int drm_atomic_helper_amend_check(struct drm_device *dev,
> -				   struct drm_atomic_state *state)
> +int drm_atomic_helper_async_amend_check(struct drm_device *dev,
> +					struct drm_atomic_state *state,
> +					bool amend)
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
> @@ -1695,7 +1711,7 @@ int drm_atomic_helper_amend_check(struct drm_device *dev,
>  		return -EINVAL;
>  

Sorry, I forgot a modif here:

-       if (!funcs->atomic_amend_update)
-               return -EINVAL;
+       if ((amend && !funcs->atomic_amend_update) ||
+           !funcs->atomic_async_update)
+               return -EOPNOTSUPP;

I need to return -EOPNOTSUPP so I can know if async should fallback to
the normal async path, this is required for amdgpu as it handles the
PAGE_FLIP_ASYNC flag it self.

I'll correct this in the next version after your comments.

You can also see the series on
https://gitlab.collabora.com/koike/linux/commits/drm/amend/uapi

Thanks


>  	/* Only allow amend update for cursor type planes. */
> -	if (plane->type != DRM_PLANE_TYPE_CURSOR)
> +	if (amend && plane->type != DRM_PLANE_TYPE_CURSOR)
>  		return -EINVAL;
>  
>  	/*
> @@ -1707,9 +1723,10 @@ int drm_atomic_helper_amend_check(struct drm_device *dev,
>  	    !try_wait_for_completion(&old_plane_state->commit->hw_done))
>  		return -EBUSY;
>  
> -	return funcs->atomic_amend_check(plane, new_plane_state);
> +	return amend ? funcs->atomic_amend_check(plane, new_plane_state) :
> +		       funcs->atomic_async_check(plane, new_plane_state);
>  }
> -EXPORT_SYMBOL(drm_atomic_helper_amend_check);
> +EXPORT_SYMBOL(drm_atomic_helper_async_amend_check);
>  
>  /**
>   * drm_atomic_helper_amend_commit - commit state in amend mode
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index d1962cdea602..1d9a6142218e 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1312,8 +1312,9 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
>  	state->acquire_ctx = &ctx;
>  	state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
> +	state->async_update = !!(arg->flags & DRM_MODE_PAGE_FLIP_ASYNC);
>  	/* async takes precedence over amend */
> -	state->amend_update = arg->flags & DRM_MODE_PAGE_FLIP_ASYNC ? 0 :
> +	state->amend_update = state->async_update ? 0 :
>  					!!(arg->flags & DRM_MODE_ATOMIC_AMEND);
>  
>  retry:
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> index 814e8230cec6..e3b2a2c74852 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> @@ -531,6 +531,8 @@ static const struct drm_plane_helper_funcs mdp5_plane_helper_funcs = {
>  		.cleanup_fb = mdp5_plane_cleanup_fb,
>  		.atomic_check = mdp5_plane_atomic_check,
>  		.atomic_update = mdp5_plane_atomic_update,
> +		.atomic_async_check = mdp5_plane_atomic_async_check,
> +		.atomic_async_update = mdp5_plane_atomic_async_update,
>  		/*
>  		 * FIXME: ideally, instead of hooking async updates to amend,
>  		 * we could avoid tearing by modifying the pending commit.
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 216ad76118dc..c2f7201e52a9 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -954,6 +954,10 @@ static const struct drm_plane_helper_funcs plane_helper_funcs = {
>  	.atomic_disable = vop_plane_atomic_disable,
>  	.atomic_amend_check = vop_plane_atomic_amend_check,
>  	.atomic_amend_update = vop_plane_atomic_amend_update,
> +	/*
> +	 * Note: rockchip doesn't support async page flip, thus
> +	 * .atomic_async_update and .atomic_async_check are not provided.
> +	 */
>  	.prepare_fb = drm_gem_fb_prepare_fb,
>  };
>  
> diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> index ea560000222d..24a9befe89e6 100644
> --- a/drivers/gpu/drm/vc4/vc4_plane.c
> +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> @@ -1175,6 +1175,8 @@ static const struct drm_plane_helper_funcs vc4_plane_helper_funcs = {
>  	 */
>  	.atomic_amend_check = vc4_plane_atomic_async_check,
>  	.atomic_amend_update = vc4_plane_atomic_async_update,
> +	.atomic_async_check = vc4_plane_atomic_async_check,
> +	.atomic_async_update = vc4_plane_atomic_async_update,
>  };
>  
>  static void vc4_plane_destroy(struct drm_plane *plane)
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index b1ced069ffc1..05756a09e762 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -300,6 +300,7 @@ struct __drm_private_objs_state {
>   * @ref: count of all references to this state (will not be freed until zero)
>   * @dev: parent DRM device
>   * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics
> + * @async_update: hint for asyncronous page flip update
>   * @amend_update: hint for amend plane update
>   * @planes: pointer to array of structures with per-plane data
>   * @crtcs: pointer to array of CRTC pointers
> @@ -328,6 +329,7 @@ struct drm_atomic_state {
>  	 */
>  	bool allow_modeset : 1;
>  	bool legacy_cursor_update : 1;
> +	bool async_update : 1;
>  	bool amend_update : 1;
>  	/**
>  	 * @duplicated:
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 8ce0594ccfb9..39e57d559a30 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -55,10 +55,11 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *state);
>  int drm_atomic_helper_commit(struct drm_device *dev,
>  			     struct drm_atomic_state *state,
>  			     bool nonblock);
> -int drm_atomic_helper_amend_check(struct drm_device *dev,
> -				  struct drm_atomic_state *state);
> -void drm_atomic_helper_amend_commit(struct drm_device *dev,
> -				    struct drm_atomic_state *state);
> +int drm_atomic_helper_async_amend_check(struct drm_device *dev,
> +					struct drm_atomic_state *state,
> +					bool amend);
> +void drm_atomic_helper_async_amend_commit(struct drm_device *dev,
> +					  struct drm_atomic_state *state);
>  
>  int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
>  					struct drm_atomic_state *state,
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index d92e62dd76c4..c2863d62f160 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -1135,6 +1135,43 @@ struct drm_plane_helper_funcs {
>  	void (*atomic_disable)(struct drm_plane *plane,
>  			       struct drm_plane_state *old_state);
>  
> +	/**
> +	 * @atomic_async_check:
> +	 *
> +	 * Drivers should set this function pointer to check if a page flip can
> +	 * be performed asynchronously, i.e., immediately without waiting for a
> +	 * vblank.
> +	 *
> +	 * This hook is called by drm_atomic_async_check() to establish if a
> +	 * given update can be committed in async mode, that is, if it can
> +	 * jump ahead of the state currently queued for update.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * Return 0 on success and any error returned indicates that the update
> +	 * can not be applied in asynd mode.
> +	 */
> +	int (*atomic_async_check)(struct drm_plane *plane,
> +				  struct drm_plane_state *state);
> +
> +	/**
> +	 * @atomic_async_update:
> +	 *
> +	 * Drivers should set this function pointer to perform asynchronous page
> +	 * flips through the atomic api, i.e. not vblank synchronized.
> +	 *
> +	 * Note that unlike &drm_plane_helper_funcs.atomic_update this hook
> +	 * takes the new &drm_plane_state as parameter. When doing async_update
> +	 * drivers shouldn't replace the &drm_plane_state but update the
> +	 * current one with the new plane configurations in the new
> +	 * plane_state.
> +	 *
> +	 * FIXME:
> +	 *  - It only works for single plane updates
> +	 */
> +	void (*atomic_async_update)(struct drm_plane *plane,
> +				    struct drm_plane_state *new_state);
> +
>  	/**
>  	 * @atomic_amend_check:
>  	 *
> 

Re: [PATCH v3 2/4] drm/atomic: rename async_{update,check} to amend_{update,check}

[Boris Brezillon]

Hi Helen,

On Fri, 12 Apr 2019 09:58:25 -0300
Helen Koike <helen.koike@collabora.com> wrote:

> Asynchronous update is the ability change the hw state at any time, not
> only during vblank.
> 
> Amend mode is the ability to perform 1000 commits to be applied as soon
> as possible without waiting for 1000 vblanks.
> 
> async updates can be seen as amend, but the opposite is not true.
> 
> &drm_plane_helper_funcs.atomic_async_{update,check}() was being used by
> drivers to implement amend and not async. So rename them to amend.
> 
> Also improve docs explaining the difference.
> 
> If asynchronous is required, normal page flip can be performed using
> DRM_MODE_PAGE_FLIP_ASYNC flag.
> 
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> 
> ---
> Hello,
> 
> I would like to officially clarify what async update means by adding it
> in the docs.
> Please correct me if I am wrong, but the current async_{update,check}
> callbacks are being used to do amend (the legacy cursor behavior), i.e.
> to allow 1000 updates without waiting for 1000 vblanks.

Right now, the semantic of async update is driver dependent. Some
drivers will amend the last commit touching that plane (amend semantic),
others will update the plane buffer immediately which might cause
tearing (async semantic).

> 
> So I would like to clarify this in the docs and rename the current
> callbacks to reflect this behaviour.

I'm all for this clarification, but I don't think renaming the async
hooks is a good idea, since some drivers will not do real 'amend'. So,
you're changing the name, but it's still confusing :-).

How about adding new hooks (and/or flags) for the AMEND case, and
keeping the async path untouched. We can then let drivers that
currently implement async as amend implement the amend hooks instead.
Once you've done that, you can hook that up to the legacy cursor update
path so that it first tries one then the other and finally falls back
to a sync update if none of ASYNC/AMEND is possible.

> 
> I also see that for real async updates, the flag
> DRM_MODE_PAGE_FLIP_ASYNC can be used in a normal sync update (it is
> already being used by some drivers actually, in the atomic path, not only
> in the legacy page flip, at least is what I understood from
> amdgpu_dm_atomic_commit_tail() implementation).

Yes, right now, async does not necessarily imply non-block, but
Daniel seemed to think that most users want non-block when they do an
async page flip, so maybe it should be clarified too.

Regards,

Boris

Re: [PATCH v3 2/4] drm/atomic: rename async_{update,check} to amend_{update,check}

[Helen Koike]

Hi Boris,

On 4/12/19 10:49 AM, Boris Brezillon wrote:
> Hi Helen,
> 
> On Fri, 12 Apr 2019 09:58:25 -0300
> Helen Koike <helen.koike@collabora.com> wrote:
> 
>> Asynchronous update is the ability change the hw state at any time, not
>> only during vblank.
>>
>> Amend mode is the ability to perform 1000 commits to be applied as soon
>> as possible without waiting for 1000 vblanks.
>>
>> async updates can be seen as amend, but the opposite is not true.
>>
>> &drm_plane_helper_funcs.atomic_async_{update,check}() was being used by
>> drivers to implement amend and not async. So rename them to amend.
>>
>> Also improve docs explaining the difference.
>>
>> If asynchronous is required, normal page flip can be performed using
>> DRM_MODE_PAGE_FLIP_ASYNC flag.
>>
>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>
>> ---
>> Hello,
>>
>> I would like to officially clarify what async update means by adding it
>> in the docs.
>> Please correct me if I am wrong, but the current async_{update,check}
>> callbacks are being used to do amend (the legacy cursor behavior), i.e.
>> to allow 1000 updates without waiting for 1000 vblanks.
> 
> Right now, the semantic of async update is driver dependent. Some
> drivers will amend the last commit touching that plane (amend semantic),
> others will update the plane buffer immediately which might cause
> tearing (async semantic).

In my pov, async updates holds the properties of an amend update, so all
async updates we have are amend, but the opposite is not true.

> 
>>
>> So I would like to clarify this in the docs and rename the current
>> callbacks to reflect this behaviour.
> 
> I'm all for this clarification, but I don't think renaming the async
> hooks is a good idea, since some drivers will not do real 'amend'. So,
> you're changing the name, but it's still confusing :-).
> 
> How about adding new hooks (and/or flags) for the AMEND case, and
> keeping the async path untouched. We can then let drivers that
> currently implement async as amend implement the amend hooks instead.
> Once you've done that, you can hook that up to the legacy cursor update
> path so that it first tries one then the other and finally falls back
> to a sync update if none of ASYNC/AMEND is possible.

I kinda did this (I re-introduced async in the last patch in the
series). I know this order is confusing, but as rockchip doesn't
implement true async, I would have to do a bunch of modifs at once to
keep the commits consistent, but I can re-work on that if it makes
things clearer.

> 
>>
>> I also see that for real async updates, the flag
>> DRM_MODE_PAGE_FLIP_ASYNC can be used in a normal sync update (it is
>> already being used by some drivers actually, in the atomic path, not only
>> in the legacy page flip, at least is what I understood from
>> amdgpu_dm_atomic_commit_tail() implementation).
> 
> Yes, right now, async does not necessarily imply non-block, but
> Daniel seemed to think that most users want non-block when they do an
> async page flip, so maybe it should be clarified too.

users could combine NONBLOCK flag with PAGE_FLIP_ASYNC, no? (we need to
add code for it of course).

Thanks
Helen

> 
> Regards,
> 
> Boris
> 

Re: [PATCH v3 2/4] drm/atomic: rename async_{update,check} to amend_{update,check}

[Boris Brezillon]

On Fri, 12 Apr 2019 11:06:13 -0300
Helen Koike <helen.koike@collabora.com> wrote:

> Hi Boris,
> 
> On 4/12/19 10:49 AM, Boris Brezillon wrote:
> > Hi Helen,
> > 
> > On Fri, 12 Apr 2019 09:58:25 -0300
> > Helen Koike <helen.koike@collabora.com> wrote:
> >   
> >> Asynchronous update is the ability change the hw state at any time, not
> >> only during vblank.
> >>
> >> Amend mode is the ability to perform 1000 commits to be applied as soon
> >> as possible without waiting for 1000 vblanks.
> >>
> >> async updates can be seen as amend, but the opposite is not true.
> >>
> >> &drm_plane_helper_funcs.atomic_async_{update,check}() was being used by
> >> drivers to implement amend and not async. So rename them to amend.
> >>
> >> Also improve docs explaining the difference.
> >>
> >> If asynchronous is required, normal page flip can be performed using
> >> DRM_MODE_PAGE_FLIP_ASYNC flag.
> >>
> >> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> >>
> >> ---
> >> Hello,
> >>
> >> I would like to officially clarify what async update means by adding it
> >> in the docs.
> >> Please correct me if I am wrong, but the current async_{update,check}
> >> callbacks are being used to do amend (the legacy cursor behavior), i.e.
> >> to allow 1000 updates without waiting for 1000 vblanks.  
> > 
> > Right now, the semantic of async update is driver dependent. Some
> > drivers will amend the last commit touching that plane (amend semantic),
> > others will update the plane buffer immediately which might cause
> > tearing (async semantic).  
> 
> In my pov, async updates holds the properties of an amend update, so all
> async updates we have are amend, but the opposite is not true.

By doing that you keep the definition quite vague, and developers will
still confused about what they should do when AMEND is requested. Why
not distinguishing AMEND and ASYNC now?

AMEND: replace the pending plane update (if any) by the new one, and
make sure things will be applied on the next VBLANK (if nothing else
amends the commit in the meantime)

ASYNC: update things immediately (don't wait for VBLANK)

> 
> >   
> >>
> >> So I would like to clarify this in the docs and rename the current
> >> callbacks to reflect this behaviour.  
> > 
> > I'm all for this clarification, but I don't think renaming the async
> > hooks is a good idea, since some drivers will not do real 'amend'. So,
> > you're changing the name, but it's still confusing :-).
> > 
> > How about adding new hooks (and/or flags) for the AMEND case, and
> > keeping the async path untouched. We can then let drivers that
> > currently implement async as amend implement the amend hooks instead.
> > Once you've done that, you can hook that up to the legacy cursor update
> > path so that it first tries one then the other and finally falls back
> > to a sync update if none of ASYNC/AMEND is possible.  
> 
> I kinda did this (I re-introduced async in the last patch in the
> series). I know this order is confusing, but as rockchip doesn't
> implement true async, I would have to do a bunch of modifs at once to
> keep the commits consistent, but I can re-work on that if it makes
> things clearer.

Let's wait for more feedback before taking a decision on this aspect.

> 
> >   
> >>
> >> I also see that for real async updates, the flag
> >> DRM_MODE_PAGE_FLIP_ASYNC can be used in a normal sync update (it is
> >> already being used by some drivers actually, in the atomic path, not only
> >> in the legacy page flip, at least is what I understood from
> >> amdgpu_dm_atomic_commit_tail() implementation).  
> > 
> > Yes, right now, async does not necessarily imply non-block, but
> > Daniel seemed to think that most users want non-block when they do an
> > async page flip, so maybe it should be clarified too.  
> 
> users could combine NONBLOCK flag with PAGE_FLIP_ASYNC, no? (we need to
> add code for it of course).

I guess AMD goes through the sync update path to guarantee that fences
are signaled before doing the page flip, but most of the time, when you
do an async page flip you don't want to block :-).