[PATCH v3 0/6] Add support for atomic async page-flips

[PATCH v3 0/6] Add support for atomic async page-flips

[Simon Ser]

This series adds support for DRM_MODE_PAGE_FLIP_ASYNC for atomic
commits, aka. "immediate flip" (which might result in tearing).
The feature was only available via the legacy uAPI, however for
gaming use-cases it may be desirable to enable it via the atomic
uAPI too.

- Patchwork: https://patchwork.freedesktop.org/series/107683/
- User-space patch: https://github.com/Plagman/gamescope/pull/595
- IGT patch: https://patchwork.freedesktop.org/series/107681/

Main changes in v2: add docs, fail atomic commit if async flip isn't
possible.

Changes in v3: add a note in the documentation about Intel hardware,
add R-b tags.

Tested on an AMD Picasso iGPU (Simon) and an AMD Vangogh GPU (André).

Simon Ser (6):
  drm: document DRM_MODE_PAGE_FLIP_ASYNC
  amd/display: only accept async flips for fast updates
  drm: introduce drm_mode_config.atomic_async_page_flip_not_supported
  drm: allow DRM_MODE_PAGE_FLIP_ASYNC for atomic commits
  drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP
  amd/display: indicate support for atomic async page-flips on DC

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  8 ++++++
 .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    | 10 +++++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c  |  1 +
 drivers/gpu/drm/drm_atomic_uapi.c             | 28 +++++++++++++++++--
 drivers/gpu/drm/drm_ioctl.c                   |  5 ++++
 drivers/gpu/drm/i915/display/intel_display.c  |  1 +
 drivers/gpu/drm/nouveau/nouveau_display.c     |  1 +
 drivers/gpu/drm/vc4/vc4_kms.c                 |  1 +
 include/drm/drm_mode_config.h                 | 11 ++++++++
 include/uapi/drm/drm.h                        | 10 ++++++-
 include/uapi/drm/drm_mode.h                   | 16 +++++++++++
 11 files changed, 88 insertions(+), 4 deletions(-)

-- 
2.37.3

[PATCH v3 0/6] Add support for atomic async page-flips

[Simon Ser]

This series adds support for DRM_MODE_PAGE_FLIP_ASYNC for atomic
commits, aka. "immediate flip" (which might result in tearing).
The feature was only available via the legacy uAPI, however for
gaming use-cases it may be desirable to enable it via the atomic
uAPI too.

- Patchwork: https://patchwork.freedesktop.org/series/107683/
- User-space patch: https://github.com/Plagman/gamescope/pull/595
- IGT patch: https://patchwork.freedesktop.org/series/107681/

Main changes in v2: add docs, fail atomic commit if async flip isn't
possible.

Changes in v3: add a note in the documentation about Intel hardware,
add R-b tags.

Tested on an AMD Picasso iGPU (Simon) and an AMD Vangogh GPU (André).

Simon Ser (6):
  drm: document DRM_MODE_PAGE_FLIP_ASYNC
  amd/display: only accept async flips for fast updates
  drm: introduce drm_mode_config.atomic_async_page_flip_not_supported
  drm: allow DRM_MODE_PAGE_FLIP_ASYNC for atomic commits
  drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP
  amd/display: indicate support for atomic async page-flips on DC

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  8 ++++++
 .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    | 10 +++++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c  |  1 +
 drivers/gpu/drm/drm_atomic_uapi.c             | 28 +++++++++++++++++--
 drivers/gpu/drm/drm_ioctl.c                   |  5 ++++
 drivers/gpu/drm/i915/display/intel_display.c  |  1 +
 drivers/gpu/drm/nouveau/nouveau_display.c     |  1 +
 drivers/gpu/drm/vc4/vc4_kms.c                 |  1 +
 include/drm/drm_mode_config.h                 | 11 ++++++++
 include/uapi/drm/drm.h                        | 10 ++++++-
 include/uapi/drm/drm_mode.h                   | 16 +++++++++++
 11 files changed, 88 insertions(+), 4 deletions(-)

-- 
2.37.3

[PATCH v3 1/6] drm: document DRM_MODE_PAGE_FLIP_ASYNC

[Simon Ser]

This is a subset of [1], included here because a subsequent patch
needs to document the behavior of this flag under the atomic uAPI.

v2: new patch

[1]: https://patchwork.freedesktop.org/patch/500177/

Signed-off-by: Simon Ser <contact@emersion.fr>
Reviewed-by: André Almeida <andrealmeid@igalia.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
---
 include/uapi/drm/drm_mode.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index fa953309d9ce..86a292c3185a 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -936,6 +936,13 @@ struct hdr_output_metadata {
 };
 
 #define DRM_MODE_PAGE_FLIP_EVENT 0x01
+/**
+ * DRM_MODE_PAGE_FLIP_ASYNC
+ *
+ * Request that the page-flip is performed as soon as possible, ie. with no
+ * delay due to waiting for vblank. This may cause tearing to be visible on
+ * the screen.
+ */
 #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
 #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
 #define DRM_MODE_PAGE_FLIP_TARGET_RELATIVE 0x8
-- 
2.37.3

[PATCH v3 1/6] drm: document DRM_MODE_PAGE_FLIP_ASYNC

[Simon Ser]

This is a subset of [1], included here because a subsequent patch
needs to document the behavior of this flag under the atomic uAPI.

v2: new patch

[1]: https://patchwork.freedesktop.org/patch/500177/

Signed-off-by: Simon Ser <contact@emersion.fr>
Reviewed-by: André Almeida <andrealmeid@igalia.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
---
 include/uapi/drm/drm_mode.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index fa953309d9ce..86a292c3185a 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -936,6 +936,13 @@ struct hdr_output_metadata {
 };
 
 #define DRM_MODE_PAGE_FLIP_EVENT 0x01
+/**
+ * DRM_MODE_PAGE_FLIP_ASYNC
+ *
+ * Request that the page-flip is performed as soon as possible, ie. with no
+ * delay due to waiting for vblank. This may cause tearing to be visible on
+ * the screen.
+ */
 #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
 #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
 #define DRM_MODE_PAGE_FLIP_TARGET_RELATIVE 0x8
-- 
2.37.3

[PATCH v3 2/6] amd/display: only accept async flips for fast updates

[Simon Ser]

Up until now, amdgpu was silently degrading to vsync when
user-space requested an async flip but the hardware didn't support
it.

The hardware doesn't support immediate flips when the update changes
the FB pitch, the DCC state, the rotation, enables or disables CRTCs
or planes, etc. This is reflected in the dm_crtc_state.update_type
field: UPDATE_TYPE_FAST means that immediate flip is supported.

Silently degrading async flips to vsync is not the expected behavior
from a uAPI point-of-view. Xorg expects async flips to fail if
unsupported, to be able to fall back to a blit. i915 already behaves
this way.

This patch aligns amdgpu with uAPI expectations and returns a failure
when an async flip is not possible.

v2: new patch

Signed-off-by: Simon Ser <contact@emersion.fr>
Reviewed-by: André Almeida <andrealmeid@igalia.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Harry Wentland <hwentlan@amd.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c      |  8 ++++++++
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c | 10 ++++++++++
 2 files changed, 18 insertions(+)

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 7b19f444624c..44235345fd57 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7629,7 +7629,15 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 		/*
 		 * Only allow immediate flips for fast updates that don't
 		 * change FB pitch, DCC state, rotation or mirroing.
+		 *
+		 * dm_crtc_helper_atomic_check() only accepts async flips with
+		 * fast updates.
 		 */
+		if (crtc->state->async_flip &&
+		    acrtc_state->update_type != UPDATE_TYPE_FAST)
+			drm_warn_once(state->dev,
+				      "[PLANE:%d:%s] async flip with non-fast update\n",
+				      plane->base.id, plane->name);
 		bundle->flip_addrs[planes_count].flip_immediate =
 			crtc->state->async_flip &&
 			acrtc_state->update_type == UPDATE_TYPE_FAST;
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
index 594fe8a4d02b..97ead857f507 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
@@ -388,6 +388,16 @@ static int dm_crtc_helper_atomic_check(struct drm_crtc *crtc,
 		return -EINVAL;
 	}
 
+	/* Only allow async flips for fast updates that don't change the FB
+	 * pitch, the DCC state, rotation, etc. */
+	if (crtc_state->async_flip &&
+	    dm_crtc_state->update_type != UPDATE_TYPE_FAST) {
+		drm_dbg_atomic(crtc->dev,
+			       "[CRTC:%d:%s] async flips are only supported for fast updates\n",
+			       crtc->base.id, crtc->name);
+		return -EINVAL;
+	}
+
 	/* In some use cases, like reset, no stream is attached */
 	if (!dm_crtc_state->stream)
 		return 0;
-- 
2.37.3

[PATCH v3 2/6] amd/display: only accept async flips for fast updates

[Simon Ser]

Up until now, amdgpu was silently degrading to vsync when
user-space requested an async flip but the hardware didn't support
it.

The hardware doesn't support immediate flips when the update changes
the FB pitch, the DCC state, the rotation, enables or disables CRTCs
or planes, etc. This is reflected in the dm_crtc_state.update_type
field: UPDATE_TYPE_FAST means that immediate flip is supported.

Silently degrading async flips to vsync is not the expected behavior
from a uAPI point-of-view. Xorg expects async flips to fail if
unsupported, to be able to fall back to a blit. i915 already behaves
this way.

This patch aligns amdgpu with uAPI expectations and returns a failure
when an async flip is not possible.

v2: new patch

Signed-off-by: Simon Ser <contact@emersion.fr>
Reviewed-by: André Almeida <andrealmeid@igalia.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Harry Wentland <hwentlan@amd.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c      |  8 ++++++++
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c | 10 ++++++++++
 2 files changed, 18 insertions(+)

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 7b19f444624c..44235345fd57 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7629,7 +7629,15 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 		/*
 		 * Only allow immediate flips for fast updates that don't
 		 * change FB pitch, DCC state, rotation or mirroing.
+		 *
+		 * dm_crtc_helper_atomic_check() only accepts async flips with
+		 * fast updates.
 		 */
+		if (crtc->state->async_flip &&
+		    acrtc_state->update_type != UPDATE_TYPE_FAST)
+			drm_warn_once(state->dev,
+				      "[PLANE:%d:%s] async flip with non-fast update\n",
+				      plane->base.id, plane->name);
 		bundle->flip_addrs[planes_count].flip_immediate =
 			crtc->state->async_flip &&
 			acrtc_state->update_type == UPDATE_TYPE_FAST;
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
index 594fe8a4d02b..97ead857f507 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
@@ -388,6 +388,16 @@ static int dm_crtc_helper_atomic_check(struct drm_crtc *crtc,
 		return -EINVAL;
 	}
 
+	/* Only allow async flips for fast updates that don't change the FB
+	 * pitch, the DCC state, rotation, etc. */
+	if (crtc_state->async_flip &&
+	    dm_crtc_state->update_type != UPDATE_TYPE_FAST) {
+		drm_dbg_atomic(crtc->dev,
+			       "[CRTC:%d:%s] async flips are only supported for fast updates\n",
+			       crtc->base.id, crtc->name);
+		return -EINVAL;
+	}
+
 	/* In some use cases, like reset, no stream is attached */
 	if (!dm_crtc_state->stream)
 		return 0;
-- 
2.37.3

[PATCH v3 3/6] drm: introduce drm_mode_config.atomic_async_page_flip_not_supported

[Simon Ser]

This new field indicates whether the driver has the necessary logic
to support async page-flips via the atomic uAPI. This is leveraged by
the next commit to allow user-space to use this functionality.

All atomic drivers setting drm_mode_config.async_page_flip are updated
to also set drm_mode_config.atomic_async_page_flip_not_supported. We
will gradually check and update these drivers to properly handle
drm_crtc_state.async_flip in their atomic logic.

The goal of this negative flag is the same as
fb_modifiers_not_supported: we want to eventually get rid of all
drivers missing atomic support for async flips. New drivers should not
set this flag, instead they should support atomic async flips (if
they support async flips at all). IOW, we don't want more drivers
with async flip support for legacy but not atomic.

v2: only set the flag on atomic drivers (remove it on amdgpu DCE and
on radeon)

Signed-off-by: Simon Ser <contact@emersion.fr>
Reviewed-by: André Almeida <andrealmeid@igalia.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Harry Wentland <hwentlan@amd.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  1 +
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c      |  1 +
 drivers/gpu/drm/i915/display/intel_display.c      |  1 +
 drivers/gpu/drm/nouveau/nouveau_display.c         |  1 +
 drivers/gpu/drm/vc4/vc4_kms.c                     |  1 +
 include/drm/drm_mode_config.h                     | 11 +++++++++++
 6 files changed, 16 insertions(+)

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 44235345fd57..7500e82cf06a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3808,6 +3808,7 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
 		adev_to_drm(adev)->mode_config.prefer_shadow = 1;
 	/* indicates support for immediate flip */
 	adev_to_drm(adev)->mode_config.async_page_flip = true;
+	adev_to_drm(adev)->mode_config.atomic_async_page_flip_not_supported = true;
 
 	adev_to_drm(adev)->mode_config.fb_base = adev->gmc.aper_base;
 
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index f7e7f4e919c7..ffb3a2fa797f 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -639,6 +639,7 @@ static int atmel_hlcdc_dc_modeset_init(struct drm_device *dev)
 	dev->mode_config.max_height = dc->desc->max_height;
 	dev->mode_config.funcs = &mode_config_funcs;
 	dev->mode_config.async_page_flip = true;
+	dev->mode_config.atomic_async_page_flip_not_supported = true;
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 40fbf8a296e2..e025b3499c9d 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -8621,6 +8621,7 @@ static void intel_mode_config_init(struct drm_i915_private *i915)
 	mode_config->helper_private = &intel_mode_config_funcs;
 
 	mode_config->async_page_flip = HAS_ASYNC_FLIPS(i915);
+	mode_config->atomic_async_page_flip_not_supported = true;
 
 	/*
 	 * Maximum framebuffer dimensions, chosen to match
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index a2f5df568ca5..2b5c4f24aedd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -699,6 +699,7 @@ nouveau_display_create(struct drm_device *dev)
 		dev->mode_config.async_page_flip = false;
 	else
 		dev->mode_config.async_page_flip = true;
+	dev->mode_config.atomic_async_page_flip_not_supported = true;
 
 	drm_kms_helper_poll_init(dev);
 	drm_kms_helper_poll_disable(dev);
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 4419e810103d..3fe59c6b2cf0 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -1047,6 +1047,7 @@ int vc4_kms_load(struct drm_device *dev)
 	dev->mode_config.helper_private = &vc4_mode_config_helpers;
 	dev->mode_config.preferred_depth = 24;
 	dev->mode_config.async_page_flip = true;
+	dev->mode_config.atomic_async_page_flip_not_supported = true;
 
 	ret = vc4_ctm_obj_init(vc4);
 	if (ret)
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 6b5e01295348..1b535d94f2f4 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -917,6 +917,17 @@ struct drm_mode_config {
 	 */
 	bool async_page_flip;
 
+	/**
+	 * @atomic_async_page_flip_not_supported:
+	 *
+	 * If true, the driver does not support async page-flips with the
+	 * atomic uAPI. This is only used by old drivers which haven't yet
+	 * accomodated for &drm_crtc_state.async_flip in their atomic logic,
+	 * even if they have &drm_mode_config.async_page_flip set to true.
+	 * New drivers shall not set this flag.
+	 */
+	bool atomic_async_page_flip_not_supported;
+
 	/**
 	 * @fb_modifiers_not_supported:
 	 *
-- 
2.37.3

[PATCH v3 3/6] drm: introduce drm_mode_config.atomic_async_page_flip_not_supported

[Simon Ser]

This new field indicates whether the driver has the necessary logic
to support async page-flips via the atomic uAPI. This is leveraged by
the next commit to allow user-space to use this functionality.

All atomic drivers setting drm_mode_config.async_page_flip are updated
to also set drm_mode_config.atomic_async_page_flip_not_supported. We
will gradually check and update these drivers to properly handle
drm_crtc_state.async_flip in their atomic logic.

The goal of this negative flag is the same as
fb_modifiers_not_supported: we want to eventually get rid of all
drivers missing atomic support for async flips. New drivers should not
set this flag, instead they should support atomic async flips (if
they support async flips at all). IOW, we don't want more drivers
with async flip support for legacy but not atomic.

v2: only set the flag on atomic drivers (remove it on amdgpu DCE and
on radeon)

Signed-off-by: Simon Ser <contact@emersion.fr>
Reviewed-by: André Almeida <andrealmeid@igalia.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Harry Wentland <hwentlan@amd.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  1 +
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c      |  1 +
 drivers/gpu/drm/i915/display/intel_display.c      |  1 +
 drivers/gpu/drm/nouveau/nouveau_display.c         |  1 +
 drivers/gpu/drm/vc4/vc4_kms.c                     |  1 +
 include/drm/drm_mode_config.h                     | 11 +++++++++++
 6 files changed, 16 insertions(+)

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 44235345fd57..7500e82cf06a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3808,6 +3808,7 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
 		adev_to_drm(adev)->mode_config.prefer_shadow = 1;
 	/* indicates support for immediate flip */
 	adev_to_drm(adev)->mode_config.async_page_flip = true;
+	adev_to_drm(adev)->mode_config.atomic_async_page_flip_not_supported = true;
 
 	adev_to_drm(adev)->mode_config.fb_base = adev->gmc.aper_base;
 
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index f7e7f4e919c7..ffb3a2fa797f 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -639,6 +639,7 @@ static int atmel_hlcdc_dc_modeset_init(struct drm_device *dev)
 	dev->mode_config.max_height = dc->desc->max_height;
 	dev->mode_config.funcs = &mode_config_funcs;
 	dev->mode_config.async_page_flip = true;
+	dev->mode_config.atomic_async_page_flip_not_supported = true;
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 40fbf8a296e2..e025b3499c9d 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -8621,6 +8621,7 @@ static void intel_mode_config_init(struct drm_i915_private *i915)
 	mode_config->helper_private = &intel_mode_config_funcs;
 
 	mode_config->async_page_flip = HAS_ASYNC_FLIPS(i915);
+	mode_config->atomic_async_page_flip_not_supported = true;
 
 	/*
 	 * Maximum framebuffer dimensions, chosen to match
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index a2f5df568ca5..2b5c4f24aedd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -699,6 +699,7 @@ nouveau_display_create(struct drm_device *dev)
 		dev->mode_config.async_page_flip = false;
 	else
 		dev->mode_config.async_page_flip = true;
+	dev->mode_config.atomic_async_page_flip_not_supported = true;
 
 	drm_kms_helper_poll_init(dev);
 	drm_kms_helper_poll_disable(dev);
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 4419e810103d..3fe59c6b2cf0 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -1047,6 +1047,7 @@ int vc4_kms_load(struct drm_device *dev)
 	dev->mode_config.helper_private = &vc4_mode_config_helpers;
 	dev->mode_config.preferred_depth = 24;
 	dev->mode_config.async_page_flip = true;
+	dev->mode_config.atomic_async_page_flip_not_supported = true;
 
 	ret = vc4_ctm_obj_init(vc4);
 	if (ret)
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 6b5e01295348..1b535d94f2f4 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -917,6 +917,17 @@ struct drm_mode_config {
 	 */
 	bool async_page_flip;
 
+	/**
+	 * @atomic_async_page_flip_not_supported:
+	 *
+	 * If true, the driver does not support async page-flips with the
+	 * atomic uAPI. This is only used by old drivers which haven't yet
+	 * accomodated for &drm_crtc_state.async_flip in their atomic logic,
+	 * even if they have &drm_mode_config.async_page_flip set to true.
+	 * New drivers shall not set this flag.
+	 */
+	bool atomic_async_page_flip_not_supported;
+
 	/**
 	 * @fb_modifiers_not_supported:
 	 *
-- 
2.37.3

Re: [PATCH v3 3/6] drm: introduce drm_mode_config.atomic_async_page_flip_not_supported

[Ville Syrjälä]

On Thu, Sep 29, 2022 at 06:43:42PM +0000, Simon Ser wrote:
> This new field indicates whether the driver has the necessary logic
> to support async page-flips via the atomic uAPI. This is leveraged by
> the next commit to allow user-space to use this functionality.
> 
> All atomic drivers setting drm_mode_config.async_page_flip are updated
> to also set drm_mode_config.atomic_async_page_flip_not_supported. We
> will gradually check and update these drivers to properly handle
> drm_crtc_state.async_flip in their atomic logic.
> 
> The goal of this negative flag is the same as
> fb_modifiers_not_supported: we want to eventually get rid of all
> drivers missing atomic support for async flips. New drivers should not
> set this flag, instead they should support atomic async flips (if
> they support async flips at all). IOW, we don't want more drivers
> with async flip support for legacy but not atomic.
> 
> v2: only set the flag on atomic drivers (remove it on amdgpu DCE and
> on radeon)
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Reviewed-by: André Almeida <andrealmeid@igalia.com>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Joshua Ashton <joshua@froggi.es>
> Cc: Melissa Wen <mwen@igalia.com>
> Cc: Harry Wentland <hwentlan@amd.com>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  1 +
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c      |  1 +
>  drivers/gpu/drm/i915/display/intel_display.c      |  1 +
>  drivers/gpu/drm/nouveau/nouveau_display.c         |  1 +
>  drivers/gpu/drm/vc4/vc4_kms.c                     |  1 +
>  include/drm/drm_mode_config.h                     | 11 +++++++++++
>  6 files changed, 16 insertions(+)
> 
> 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 44235345fd57..7500e82cf06a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3808,6 +3808,7 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
>  		adev_to_drm(adev)->mode_config.prefer_shadow = 1;
>  	/* indicates support for immediate flip */
>  	adev_to_drm(adev)->mode_config.async_page_flip = true;
> +	adev_to_drm(adev)->mode_config.atomic_async_page_flip_not_supported = true;

The flag polarity seems weird. Why opt out and not opt in?

>  
>  	adev_to_drm(adev)->mode_config.fb_base = adev->gmc.aper_base;
>  
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> index f7e7f4e919c7..ffb3a2fa797f 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -639,6 +639,7 @@ static int atmel_hlcdc_dc_modeset_init(struct drm_device *dev)
>  	dev->mode_config.max_height = dc->desc->max_height;
>  	dev->mode_config.funcs = &mode_config_funcs;
>  	dev->mode_config.async_page_flip = true;
> +	dev->mode_config.atomic_async_page_flip_not_supported = true;
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 40fbf8a296e2..e025b3499c9d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -8621,6 +8621,7 @@ static void intel_mode_config_init(struct drm_i915_private *i915)
>  	mode_config->helper_private = &intel_mode_config_funcs;
>  
>  	mode_config->async_page_flip = HAS_ASYNC_FLIPS(i915);
> +	mode_config->atomic_async_page_flip_not_supported = true;
>  
>  	/*
>  	 * Maximum framebuffer dimensions, chosen to match
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> index a2f5df568ca5..2b5c4f24aedd 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> @@ -699,6 +699,7 @@ nouveau_display_create(struct drm_device *dev)
>  		dev->mode_config.async_page_flip = false;
>  	else
>  		dev->mode_config.async_page_flip = true;
> +	dev->mode_config.atomic_async_page_flip_not_supported = true;
>  
>  	drm_kms_helper_poll_init(dev);
>  	drm_kms_helper_poll_disable(dev);
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 4419e810103d..3fe59c6b2cf0 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -1047,6 +1047,7 @@ int vc4_kms_load(struct drm_device *dev)
>  	dev->mode_config.helper_private = &vc4_mode_config_helpers;
>  	dev->mode_config.preferred_depth = 24;
>  	dev->mode_config.async_page_flip = true;
> +	dev->mode_config.atomic_async_page_flip_not_supported = true;
>  
>  	ret = vc4_ctm_obj_init(vc4);
>  	if (ret)
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 6b5e01295348..1b535d94f2f4 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -917,6 +917,17 @@ struct drm_mode_config {
>  	 */
>  	bool async_page_flip;
>  
> +	/**
> +	 * @atomic_async_page_flip_not_supported:
> +	 *
> +	 * If true, the driver does not support async page-flips with the
> +	 * atomic uAPI. This is only used by old drivers which haven't yet
> +	 * accomodated for &drm_crtc_state.async_flip in their atomic logic,
> +	 * even if they have &drm_mode_config.async_page_flip set to true.
> +	 * New drivers shall not set this flag.
> +	 */
> +	bool atomic_async_page_flip_not_supported;
> +
>  	/**
>  	 * @fb_modifiers_not_supported:
>  	 *
> -- 
> 2.37.3
> 

-- 
Ville Syrjälä
Intel

Re: [PATCH v3 3/6] drm: introduce drm_mode_config.atomic_async_page_flip_not_supported

[Simon Ser]

On Friday, September 30th, 2022 at 15:53, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> > 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 44235345fd57..7500e82cf06a 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -3808,6 +3808,7 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
> >  		adev_to_drm(adev)->mode_config.prefer_shadow = 1;
> >  	/* indicates support for immediate flip */
> >  	adev_to_drm(adev)->mode_config.async_page_flip = true;
> > +	adev_to_drm(adev)->mode_config.atomic_async_page_flip_not_supported = true;
> 
> The flag polarity seems weird. Why opt out and not opt in?

Explained in the commit message.

Re: [PATCH v3 3/6] drm: introduce drm_mode_config.atomic_async_page_flip_not_supported

[Ville Syrjälä]

On Fri, Sep 30, 2022 at 01:56:25PM +0000, Simon Ser wrote:
> On Friday, September 30th, 2022 at 15:53, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 
> > > 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 44235345fd57..7500e82cf06a 100644
> > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > @@ -3808,6 +3808,7 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
> > >  		adev_to_drm(adev)->mode_config.prefer_shadow = 1;
> > >  	/* indicates support for immediate flip */
> > >  	adev_to_drm(adev)->mode_config.async_page_flip = true;
> > > +	adev_to_drm(adev)->mode_config.atomic_async_page_flip_not_supported = true;
> > 
> > The flag polarity seems weird. Why opt out and not opt in?
> 
> Explained in the commit message.

Seems a bit ambitious, but sure.

-- 
Ville Syrjälä
Intel

[PATCH v3 4/6] drm: allow DRM_MODE_PAGE_FLIP_ASYNC for atomic commits

[Simon Ser]

If the driver supports it, allow user-space to supply the
DRM_MODE_PAGE_FLIP_ASYNC flag to request an async page-flip.
Set drm_crtc_state.async_flip accordingly.

Document that drivers will reject atomic commits if an async
flip isn't possible. This allows user-space to fall back to
something else. For instance, Xorg falls back to a blit.
Another option is to wait as close to the next vblank as
possible before performing the page-flip to reduce latency.

v2: document new uAPI

v3: add comment about Intel hardware which needs one last
sync page-flip before being able to switch to async (Ville, Pekka)

Signed-off-by: Simon Ser <contact@emersion.fr>
Co-developed-by: André Almeida <andrealmeid@igalia.com>
Signed-off-by: André Almeida <andrealmeid@igalia.com>
Reviewed-by: André Almeida <andrealmeid@igalia.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Harry Wentland <hwentlan@amd.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c | 28 +++++++++++++++++++++++++---
 include/uapi/drm/drm_mode.h       |  9 +++++++++
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 79730fa1dd8e..ee24ed7e2edb 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1278,6 +1278,18 @@ static void complete_signaling(struct drm_device *dev,
 	kfree(fence_state);
 }
 
+static void
+set_async_flip(struct drm_atomic_state *state)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	int i;
+
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+		crtc_state->async_flip = true;
+	}
+}
+
 int drm_mode_atomic_ioctl(struct drm_device *dev,
 			  void *data, struct drm_file *file_priv)
 {
@@ -1318,9 +1330,16 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	}
 
 	if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC) {
-		drm_dbg_atomic(dev,
-			       "commit failed: invalid flag DRM_MODE_PAGE_FLIP_ASYNC\n");
-		return -EINVAL;
+		if (!dev->mode_config.async_page_flip) {
+			drm_dbg_atomic(dev,
+				       "commit failed: DRM_MODE_PAGE_FLIP_ASYNC not supported\n");
+			return -EINVAL;
+		}
+		if (dev->mode_config.atomic_async_page_flip_not_supported) {
+			drm_dbg_atomic(dev,
+				       "commit failed: DRM_MODE_PAGE_FLIP_ASYNC not supported with atomic\n");
+			return -EINVAL;
+		}
 	}
 
 	/* can't test and expect an event at the same time. */
@@ -1418,6 +1437,9 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	if (ret)
 		goto out;
 
+	if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC)
+		set_async_flip(state);
+
 	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
 		ret = drm_atomic_check_only(state);
 	} else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 86a292c3185a..b39e78117b18 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -942,6 +942,15 @@ struct hdr_output_metadata {
  * Request that the page-flip is performed as soon as possible, ie. with no
  * delay due to waiting for vblank. This may cause tearing to be visible on
  * the screen.
+ *
+ * When used with atomic uAPI, the driver will return an error if the hardware
+ * doesn't support performing an asynchronous page-flip for this update.
+ * User-space should handle this, e.g. by falling back to a regular page-flip.
+ *
+ * Note, some hardware might need to perform one last synchronous page-flip
+ * before being able to switch to asynchronous page-flips. As an exception,
+ * the driver will return success even though that first page-flip is not
+ * asynchronous.
  */
 #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
 #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
-- 
2.37.3

[PATCH v3 4/6] drm: allow DRM_MODE_PAGE_FLIP_ASYNC for atomic commits

[Simon Ser]

If the driver supports it, allow user-space to supply the
DRM_MODE_PAGE_FLIP_ASYNC flag to request an async page-flip.
Set drm_crtc_state.async_flip accordingly.

Document that drivers will reject atomic commits if an async
flip isn't possible. This allows user-space to fall back to
something else. For instance, Xorg falls back to a blit.
Another option is to wait as close to the next vblank as
possible before performing the page-flip to reduce latency.

v2: document new uAPI

v3: add comment about Intel hardware which needs one last
sync page-flip before being able to switch to async (Ville, Pekka)

Signed-off-by: Simon Ser <contact@emersion.fr>
Co-developed-by: André Almeida <andrealmeid@igalia.com>
Signed-off-by: André Almeida <andrealmeid@igalia.com>
Reviewed-by: André Almeida <andrealmeid@igalia.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Harry Wentland <hwentlan@amd.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c | 28 +++++++++++++++++++++++++---
 include/uapi/drm/drm_mode.h       |  9 +++++++++
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 79730fa1dd8e..ee24ed7e2edb 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1278,6 +1278,18 @@ static void complete_signaling(struct drm_device *dev,
 	kfree(fence_state);
 }
 
+static void
+set_async_flip(struct drm_atomic_state *state)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	int i;
+
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+		crtc_state->async_flip = true;
+	}
+}
+
 int drm_mode_atomic_ioctl(struct drm_device *dev,
 			  void *data, struct drm_file *file_priv)
 {
@@ -1318,9 +1330,16 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	}
 
 	if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC) {
-		drm_dbg_atomic(dev,
-			       "commit failed: invalid flag DRM_MODE_PAGE_FLIP_ASYNC\n");
-		return -EINVAL;
+		if (!dev->mode_config.async_page_flip) {
+			drm_dbg_atomic(dev,
+				       "commit failed: DRM_MODE_PAGE_FLIP_ASYNC not supported\n");
+			return -EINVAL;
+		}
+		if (dev->mode_config.atomic_async_page_flip_not_supported) {
+			drm_dbg_atomic(dev,
+				       "commit failed: DRM_MODE_PAGE_FLIP_ASYNC not supported with atomic\n");
+			return -EINVAL;
+		}
 	}
 
 	/* can't test and expect an event at the same time. */
@@ -1418,6 +1437,9 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	if (ret)
 		goto out;
 
+	if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC)
+		set_async_flip(state);
+
 	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
 		ret = drm_atomic_check_only(state);
 	} else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 86a292c3185a..b39e78117b18 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -942,6 +942,15 @@ struct hdr_output_metadata {
  * Request that the page-flip is performed as soon as possible, ie. with no
  * delay due to waiting for vblank. This may cause tearing to be visible on
  * the screen.
+ *
+ * When used with atomic uAPI, the driver will return an error if the hardware
+ * doesn't support performing an asynchronous page-flip for this update.
+ * User-space should handle this, e.g. by falling back to a regular page-flip.
+ *
+ * Note, some hardware might need to perform one last synchronous page-flip
+ * before being able to switch to asynchronous page-flips. As an exception,
+ * the driver will return success even though that first page-flip is not
+ * asynchronous.
  */
 #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
 #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
-- 
2.37.3

[PATCH v3 5/6] drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP

[Simon Ser]

This new kernel capability indicates whether async page-flips are
supported via the atomic uAPI. DRM clients can use it to check
for support before feeding DRM_MODE_PAGE_FLIP_ASYNC to the kernel.

Make it clear that DRM_CAP_ASYNC_PAGE_FLIP is for legacy uAPI only.

Signed-off-by: Simon Ser <contact@emersion.fr>
Reviewed-by: André Almeida <andrealmeid@igalia.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Harry Wentland <hwentlan@amd.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_ioctl.c |  5 +++++
 include/uapi/drm/drm.h      | 10 +++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index ca2a6e6101dc..5b1591e2b46c 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -302,6 +302,11 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
 	case DRM_CAP_CRTC_IN_VBLANK_EVENT:
 		req->value = 1;
 		break;
+	case DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP:
+		req->value = drm_core_check_feature(dev, DRIVER_ATOMIC) &&
+			     dev->mode_config.async_page_flip &&
+			     !dev->mode_config.atomic_async_page_flip_not_supported;
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 642808520d92..b1962628ecda 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -706,7 +706,8 @@ struct drm_gem_open {
 /**
  * DRM_CAP_ASYNC_PAGE_FLIP
  *
- * If set to 1, the driver supports &DRM_MODE_PAGE_FLIP_ASYNC.
+ * If set to 1, the driver supports &DRM_MODE_PAGE_FLIP_ASYNC for legacy
+ * page-flips.
  */
 #define DRM_CAP_ASYNC_PAGE_FLIP		0x7
 /**
@@ -767,6 +768,13 @@ struct drm_gem_open {
  * Documentation/gpu/drm-mm.rst, section "DRM Sync Objects".
  */
 #define DRM_CAP_SYNCOBJ_TIMELINE	0x14
+/**
+ * DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP
+ *
+ * If set to 1, the driver supports &DRM_MODE_PAGE_FLIP_ASYNC for atomic
+ * commits.
+ */
+#define DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP	0x15
 
 /* DRM_IOCTL_GET_CAP ioctl argument type */
 struct drm_get_cap {
-- 
2.37.3

[PATCH v3 5/6] drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP

[Simon Ser]

This new kernel capability indicates whether async page-flips are
supported via the atomic uAPI. DRM clients can use it to check
for support before feeding DRM_MODE_PAGE_FLIP_ASYNC to the kernel.

Make it clear that DRM_CAP_ASYNC_PAGE_FLIP is for legacy uAPI only.

Signed-off-by: Simon Ser <contact@emersion.fr>
Reviewed-by: André Almeida <andrealmeid@igalia.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Harry Wentland <hwentlan@amd.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_ioctl.c |  5 +++++
 include/uapi/drm/drm.h      | 10 +++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index ca2a6e6101dc..5b1591e2b46c 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -302,6 +302,11 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
 	case DRM_CAP_CRTC_IN_VBLANK_EVENT:
 		req->value = 1;
 		break;
+	case DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP:
+		req->value = drm_core_check_feature(dev, DRIVER_ATOMIC) &&
+			     dev->mode_config.async_page_flip &&
+			     !dev->mode_config.atomic_async_page_flip_not_supported;
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 642808520d92..b1962628ecda 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -706,7 +706,8 @@ struct drm_gem_open {
 /**
  * DRM_CAP_ASYNC_PAGE_FLIP
  *
- * If set to 1, the driver supports &DRM_MODE_PAGE_FLIP_ASYNC.
+ * If set to 1, the driver supports &DRM_MODE_PAGE_FLIP_ASYNC for legacy
+ * page-flips.
  */
 #define DRM_CAP_ASYNC_PAGE_FLIP		0x7
 /**
@@ -767,6 +768,13 @@ struct drm_gem_open {
  * Documentation/gpu/drm-mm.rst, section "DRM Sync Objects".
  */
 #define DRM_CAP_SYNCOBJ_TIMELINE	0x14
+/**
+ * DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP
+ *
+ * If set to 1, the driver supports &DRM_MODE_PAGE_FLIP_ASYNC for atomic
+ * commits.
+ */
+#define DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP	0x15
 
 /* DRM_IOCTL_GET_CAP ioctl argument type */
 struct drm_get_cap {
-- 
2.37.3

[PATCH v3 6/6] amd/display: indicate support for atomic async page-flips on DC

[Simon Ser]

amdgpu_dm_commit_planes() already sets the flip_immediate flag for
async page-flips. This flag is used to set the UNP_FLIP_CONTROL
register. Thus, no additional change is required to handle async
page-flips with the atomic uAPI.

v2: make it clear this commit is about DC and not only DCN

Signed-off-by: Simon Ser <contact@emersion.fr>
Reviewed-by: André Almeida <andrealmeid@igalia.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Harry Wentland <hwentlan@amd.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 1 -
 1 file changed, 1 deletion(-)

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 7500e82cf06a..44235345fd57 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3808,7 +3808,6 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
 		adev_to_drm(adev)->mode_config.prefer_shadow = 1;
 	/* indicates support for immediate flip */
 	adev_to_drm(adev)->mode_config.async_page_flip = true;
-	adev_to_drm(adev)->mode_config.atomic_async_page_flip_not_supported = true;
 
 	adev_to_drm(adev)->mode_config.fb_base = adev->gmc.aper_base;
 
-- 
2.37.3

[PATCH v3 6/6] amd/display: indicate support for atomic async page-flips on DC

[Simon Ser]

amdgpu_dm_commit_planes() already sets the flip_immediate flag for
async page-flips. This flag is used to set the UNP_FLIP_CONTROL
register. Thus, no additional change is required to handle async
page-flips with the atomic uAPI.

v2: make it clear this commit is about DC and not only DCN

Signed-off-by: Simon Ser <contact@emersion.fr>
Reviewed-by: André Almeida <andrealmeid@igalia.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Harry Wentland <hwentlan@amd.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 1 -
 1 file changed, 1 deletion(-)

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 7500e82cf06a..44235345fd57 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3808,7 +3808,6 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
 		adev_to_drm(adev)->mode_config.prefer_shadow = 1;
 	/* indicates support for immediate flip */
 	adev_to_drm(adev)->mode_config.async_page_flip = true;
-	adev_to_drm(adev)->mode_config.atomic_async_page_flip_not_supported = true;
 
 	adev_to_drm(adev)->mode_config.fb_base = adev->gmc.aper_base;
 
-- 
2.37.3

Re: [PATCH v3 0/6] Add support for atomic async page-flips

[Harry Wentland]

On 9/29/22 14:43, Simon Ser wrote:
> This series adds support for DRM_MODE_PAGE_FLIP_ASYNC for atomic
> commits, aka. "immediate flip" (which might result in tearing).
> The feature was only available via the legacy uAPI, however for
> gaming use-cases it may be desirable to enable it via the atomic
> uAPI too.
> 
> - Patchwork: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fseries%2F107683%2F&data=05%7C01%7CHarry.Wentland%40amd.com%7Cac97e739abd04e7d3f9c08daa24a7de9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638000738084626501%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=J3DbO0agtOXfEt0XNA%2FKvmSLmJrFPW048T214BSl4dA%3D&reserved=0
> - User-space patch: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FPlagman%2Fgamescope%2Fpull%2F595&data=05%7C01%7CHarry.Wentland%40amd.com%7Cac97e739abd04e7d3f9c08daa24a7de9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638000738084626501%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=6vJftf%2Bl5h4vR3ANZKWw9AhghZpPhAcHdXR5EI8Xwrs%3D&reserved=0
> - IGT patch: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fseries%2F107681%2F&data=05%7C01%7CHarry.Wentland%40amd.com%7Cac97e739abd04e7d3f9c08daa24a7de9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638000738084626501%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=hOJssKhjj39wIHkrd%2FEI8csLvpZDoENbakqqkN%2F22O8%3D&reserved=0
> 
> Main changes in v2: add docs, fail atomic commit if async flip isn't
> possible.
> 
> Changes in v3: add a note in the documentation about Intel hardware,
> add R-b tags.
> 
> Tested on an AMD Picasso iGPU (Simon) and an AMD Vangogh GPU (André).
> 
> Simon Ser (6):
>   drm: document DRM_MODE_PAGE_FLIP_ASYNC
>   amd/display: only accept async flips for fast updates
>   drm: introduce drm_mode_config.atomic_async_page_flip_not_supported
>   drm: allow DRM_MODE_PAGE_FLIP_ASYNC for atomic commits
>   drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP
>   amd/display: indicate support for atomic async page-flips on DC
> 

Finally had a chance to go through this. Series looks good
and is
Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  8 ++++++
>  .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    | 10 +++++++
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c  |  1 +
>  drivers/gpu/drm/drm_atomic_uapi.c             | 28 +++++++++++++++++--
>  drivers/gpu/drm/drm_ioctl.c                   |  5 ++++
>  drivers/gpu/drm/i915/display/intel_display.c  |  1 +
>  drivers/gpu/drm/nouveau/nouveau_display.c     |  1 +
>  drivers/gpu/drm/vc4/vc4_kms.c                 |  1 +
>  include/drm/drm_mode_config.h                 | 11 ++++++++
>  include/uapi/drm/drm.h                        | 10 ++++++-
>  include/uapi/drm/drm_mode.h                   | 16 +++++++++++
>  11 files changed, 88 insertions(+), 4 deletions(-)
> 

Re: [PATCH v3 0/6] Add support for atomic async page-flips

[Harry Wentland]

On 9/29/22 14:43, Simon Ser wrote:
> This series adds support for DRM_MODE_PAGE_FLIP_ASYNC for atomic
> commits, aka. "immediate flip" (which might result in tearing).
> The feature was only available via the legacy uAPI, however for
> gaming use-cases it may be desirable to enable it via the atomic
> uAPI too.
> 
> - Patchwork: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fseries%2F107683%2F&data=05%7C01%7CHarry.Wentland%40amd.com%7Cac97e739abd04e7d3f9c08daa24a7de9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638000738084626501%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=J3DbO0agtOXfEt0XNA%2FKvmSLmJrFPW048T214BSl4dA%3D&reserved=0
> - User-space patch: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FPlagman%2Fgamescope%2Fpull%2F595&data=05%7C01%7CHarry.Wentland%40amd.com%7Cac97e739abd04e7d3f9c08daa24a7de9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638000738084626501%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=6vJftf%2Bl5h4vR3ANZKWw9AhghZpPhAcHdXR5EI8Xwrs%3D&reserved=0
> - IGT patch: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fseries%2F107681%2F&data=05%7C01%7CHarry.Wentland%40amd.com%7Cac97e739abd04e7d3f9c08daa24a7de9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638000738084626501%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=hOJssKhjj39wIHkrd%2FEI8csLvpZDoENbakqqkN%2F22O8%3D&reserved=0
> 
> Main changes in v2: add docs, fail atomic commit if async flip isn't
> possible.
> 
> Changes in v3: add a note in the documentation about Intel hardware,
> add R-b tags.
> 
> Tested on an AMD Picasso iGPU (Simon) and an AMD Vangogh GPU (André).
> 
> Simon Ser (6):
>   drm: document DRM_MODE_PAGE_FLIP_ASYNC
>   amd/display: only accept async flips for fast updates
>   drm: introduce drm_mode_config.atomic_async_page_flip_not_supported
>   drm: allow DRM_MODE_PAGE_FLIP_ASYNC for atomic commits
>   drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP
>   amd/display: indicate support for atomic async page-flips on DC
> 

Finally had a chance to go through this. Series looks good
and is
Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  8 ++++++
>  .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    | 10 +++++++
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c  |  1 +
>  drivers/gpu/drm/drm_atomic_uapi.c             | 28 +++++++++++++++++--
>  drivers/gpu/drm/drm_ioctl.c                   |  5 ++++
>  drivers/gpu/drm/i915/display/intel_display.c  |  1 +
>  drivers/gpu/drm/nouveau/nouveau_display.c     |  1 +
>  drivers/gpu/drm/vc4/vc4_kms.c                 |  1 +
>  include/drm/drm_mode_config.h                 | 11 ++++++++
>  include/uapi/drm/drm.h                        | 10 ++++++-
>  include/uapi/drm/drm_mode.h                   | 16 +++++++++++
>  11 files changed, 88 insertions(+), 4 deletions(-)
> 

Re: [PATCH v3 0/6] Add support for atomic async page-flips

[Ville Syrjälä]

On Thu, Sep 29, 2022 at 06:43:15PM +0000, Simon Ser wrote:
> This series adds support for DRM_MODE_PAGE_FLIP_ASYNC for atomic
> commits, aka. "immediate flip" (which might result in tearing).
> The feature was only available via the legacy uAPI, however for
> gaming use-cases it may be desirable to enable it via the atomic
> uAPI too.
> 
> - Patchwork: https://patchwork.freedesktop.org/series/107683/
> - User-space patch: https://github.com/Plagman/gamescope/pull/595
> - IGT patch: https://patchwork.freedesktop.org/series/107681/

So no tests that actually verify that the kernel properly rejects
stuff stuff like modesets, gamma LUT updates, plane movement,
etc.?

> 
> Main changes in v2: add docs, fail atomic commit if async flip isn't
> possible.
> 
> Changes in v3: add a note in the documentation about Intel hardware,
> add R-b tags.
> 
> Tested on an AMD Picasso iGPU (Simon) and an AMD Vangogh GPU (André).
> 
> Simon Ser (6):
>   drm: document DRM_MODE_PAGE_FLIP_ASYNC
>   amd/display: only accept async flips for fast updates
>   drm: introduce drm_mode_config.atomic_async_page_flip_not_supported
>   drm: allow DRM_MODE_PAGE_FLIP_ASYNC for atomic commits
>   drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP
>   amd/display: indicate support for atomic async page-flips on DC
> 
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  8 ++++++
>  .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    | 10 +++++++
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c  |  1 +
>  drivers/gpu/drm/drm_atomic_uapi.c             | 28 +++++++++++++++++--
>  drivers/gpu/drm/drm_ioctl.c                   |  5 ++++
>  drivers/gpu/drm/i915/display/intel_display.c  |  1 +
>  drivers/gpu/drm/nouveau/nouveau_display.c     |  1 +
>  drivers/gpu/drm/vc4/vc4_kms.c                 |  1 +
>  include/drm/drm_mode_config.h                 | 11 ++++++++
>  include/uapi/drm/drm.h                        | 10 ++++++-
>  include/uapi/drm/drm_mode.h                   | 16 +++++++++++
>  11 files changed, 88 insertions(+), 4 deletions(-)
> 
> -- 
> 2.37.3
> 

-- 
Ville Syrjälä
Intel

Re: [PATCH v3 0/6] Add support for atomic async page-flips

[Ville Syrjälä]

On Fri, Sep 30, 2022 at 04:52:56PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 29, 2022 at 06:43:15PM +0000, Simon Ser wrote:
> > This series adds support for DRM_MODE_PAGE_FLIP_ASYNC for atomic
> > commits, aka. "immediate flip" (which might result in tearing).
> > The feature was only available via the legacy uAPI, however for
> > gaming use-cases it may be desirable to enable it via the atomic
> > uAPI too.
> > 
> > - Patchwork: https://patchwork.freedesktop.org/series/107683/
> > - User-space patch: https://github.com/Plagman/gamescope/pull/595
> > - IGT patch: https://patchwork.freedesktop.org/series/107681/
> 
> So no tests that actually verify that the kernel properly rejects
> stuff stuff like modesets, gamma LUT updates, plane movement,
> etc.?

Pondering this a bit more, it just occurred to me the current driver
level checks might easily lead to confusing behaviour. Eg. is
the ioctl going to succeed if you ask for an async change of some
random property while the crtc disabled, but fails if you ask for
the same async property change when the crtc is active?

So another reason why rejecting most properties already at
the uapi level might be a good idea.

-- 
Ville Syrjälä
Intel

Re: [PATCH v3 0/6] Add support for atomic async page-flips

[Ville Syrjälä]

On Fri, Sep 30, 2022 at 05:19:07PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 30, 2022 at 04:52:56PM +0300, Ville Syrjälä wrote:
> > On Thu, Sep 29, 2022 at 06:43:15PM +0000, Simon Ser wrote:
> > > This series adds support for DRM_MODE_PAGE_FLIP_ASYNC for atomic
> > > commits, aka. "immediate flip" (which might result in tearing).
> > > The feature was only available via the legacy uAPI, however for
> > > gaming use-cases it may be desirable to enable it via the atomic
> > > uAPI too.
> > > 
> > > - Patchwork: https://patchwork.freedesktop.org/series/107683/
> > > - User-space patch: https://github.com/Plagman/gamescope/pull/595
> > > - IGT patch: https://patchwork.freedesktop.org/series/107681/
> > 
> > So no tests that actually verify that the kernel properly rejects
> > stuff stuff like modesets, gamma LUT updates, plane movement,
> > etc.?
> 
> Pondering this a bit more, it just occurred to me the current driver
> level checks might easily lead to confusing behaviour. Eg. is
> the ioctl going to succeed if you ask for an async change of some
> random property while the crtc disabled, but fails if you ask for
> the same async property change when the crtc is active?
> 
> So another reason why rejecting most properties already at
> the uapi level might be a good idea.

And just to be clear this is pretty much what I suggest:

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 79730fa1dd8e..471a2c703847 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1392,6 +1392,13 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 				goto out;
 			}
 
+			if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC &&
+			    prop != dev->mode_config.prop_fb_id) {
+				drm_mode_object_put(obj);
+				ret = -EINVAL;
+				goto out;
+			}
+
 			if (copy_from_user(&prop_value,
 					   prop_values_ptr + copied_props,
 					   sizeof(prop_value))) {


That would actively discourage people from even attempting the
"just dump all the state into the ioctl" approach with async flips
since even the props whose value isn't even changing would be rejected.

-- 
Ville Syrjälä
Intel

KMS atomic state sets, full vs. minimal (Re: [PATCH v3 0/6] Add support for atomic async page-flips)

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 1004 bytes --]

On Fri, 30 Sep 2022 18:09:55 +0300
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> That would actively discourage people from even attempting the
> "just dump all the state into the ioctl" approach with async flips
> since even the props whose value isn't even changing would be rejected.

About that.

To me it looks like just a classic case of broken communication.

The kernel developers assume that of course userspace will minimize the
state set to only those properties that change, because...?

Userspace developers think that of course the kernel will optimize the
state set into minimal changes, because the kernel actually has the
authoritative knowledge of what the current state is, and the driver
actually knows which properties are costly and need to be optimized and
which ones don't matter. It has never even occurred to me that the
kernel would not compare next state to current state.

No-one ever documented any expectations, did they?


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

KMS atomic state sets, full vs. minimal (Re: [PATCH v3 0/6] Add support for atomic async page-flips)

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 1004 bytes --]

On Fri, 30 Sep 2022 18:09:55 +0300
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> That would actively discourage people from even attempting the
> "just dump all the state into the ioctl" approach with async flips
> since even the props whose value isn't even changing would be rejected.

About that.

To me it looks like just a classic case of broken communication.

The kernel developers assume that of course userspace will minimize the
state set to only those properties that change, because...?

Userspace developers think that of course the kernel will optimize the
state set into minimal changes, because the kernel actually has the
authoritative knowledge of what the current state is, and the driver
actually knows which properties are costly and need to be optimized and
which ones don't matter. It has never even occurred to me that the
kernel would not compare next state to current state.

No-one ever documented any expectations, did they?


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: KMS atomic state sets, full vs. minimal (Re: [PATCH v3 0/6] Add support for atomic async page-flips)

[Ville Syrjälä]

On Fri, Sep 30, 2022 at 06:37:00PM +0300, Pekka Paalanen wrote:
> On Fri, 30 Sep 2022 18:09:55 +0300
> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 
> > That would actively discourage people from even attempting the
> > "just dump all the state into the ioctl" approach with async flips
> > since even the props whose value isn't even changing would be rejected.
> 
> About that.
> 
> To me it looks like just a classic case of broken communication.
> 
> The kernel developers assume that of course userspace will minimize the
> state set to only those properties that change, because...?
> 
> Userspace developers think that of course the kernel will optimize the
> state set into minimal changes, because the kernel actually has the
> authoritative knowledge of what the current state is, and the driver
> actually knows which properties are costly and need to be optimized and
> which ones don't matter. It has never even occurred to me that the
> kernel would not compare next state to current state.
> 
> No-one ever documented any expectations, did they?

Do you really want that for async flips? Async flips can't be
done atomically with anything else, so why are you even asking
the kernel to do that?

-- 
Ville Syrjälä
Intel

Re: KMS atomic state sets, full vs. minimal (Re: [PATCH v3 0/6] Add support for atomic async page-flips)

[Ville Syrjälä]

On Fri, Sep 30, 2022 at 06:37:00PM +0300, Pekka Paalanen wrote:
> On Fri, 30 Sep 2022 18:09:55 +0300
> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 
> > That would actively discourage people from even attempting the
> > "just dump all the state into the ioctl" approach with async flips
> > since even the props whose value isn't even changing would be rejected.
> 
> About that.
> 
> To me it looks like just a classic case of broken communication.
> 
> The kernel developers assume that of course userspace will minimize the
> state set to only those properties that change, because...?
> 
> Userspace developers think that of course the kernel will optimize the
> state set into minimal changes, because the kernel actually has the
> authoritative knowledge of what the current state is, and the driver
> actually knows which properties are costly and need to be optimized and
> which ones don't matter. It has never even occurred to me that the
> kernel would not compare next state to current state.
> 
> No-one ever documented any expectations, did they?

Do you really want that for async flips? Async flips can't be
done atomically with anything else, so why are you even asking
the kernel to do that?

-- 
Ville Syrjälä
Intel

Re: KMS atomic state sets, full vs. minimal (Re: [PATCH v3 0/6] Add support for atomic async page-flips)

[Ville Syrjälä]

On Fri, Sep 30, 2022 at 06:45:09PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 30, 2022 at 06:37:00PM +0300, Pekka Paalanen wrote:
> > On Fri, 30 Sep 2022 18:09:55 +0300
> > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > 
> > > That would actively discourage people from even attempting the
> > > "just dump all the state into the ioctl" approach with async flips
> > > since even the props whose value isn't even changing would be rejected.
> > 
> > About that.
> > 
> > To me it looks like just a classic case of broken communication.
> > 
> > The kernel developers assume that of course userspace will minimize the
> > state set to only those properties that change, because...?
> > 
> > Userspace developers think that of course the kernel will optimize the
> > state set into minimal changes, because the kernel actually has the
> > authoritative knowledge of what the current state is, and the driver
> > actually knows which properties are costly and need to be optimized and
> > which ones don't matter. It has never even occurred to me that the
> > kernel would not compare next state to current state.
> > 
> > No-one ever documented any expectations, did they?
> 
> Do you really want that for async flips? Async flips can't be
> done atomically with anything else, so why are you even asking
> the kernel to do that?

Also what if you want plane 1 to do an async flip, and plane 2 to do
a sync flip? With the single async flag per commit you will end up
with one of the following results:

plane 1      plane 2      outcome
can async    can async    async flip on both planes (totally not what you wanted)
can async    can't async  -EINVAL (what you actually wanted but got unfairly rejected)
can't async  can async    -EINVAL
can't async  can't async  -EINVAL

Those last two are actually reasonable outcomes since the plane
where you did want to async flip didn't support it. But the
first two are just nonsense results.

-- 
Ville Syrjälä
Intel

Re: KMS atomic state sets, full vs. minimal (Re: [PATCH v3 0/6] Add support for atomic async page-flips)

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 2537 bytes --]

On Fri, 30 Sep 2022 18:45:09 +0300
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Fri, Sep 30, 2022 at 06:37:00PM +0300, Pekka Paalanen wrote:
> > On Fri, 30 Sep 2022 18:09:55 +0300
> > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >   
> > > That would actively discourage people from even attempting the
> > > "just dump all the state into the ioctl" approach with async flips
> > > since even the props whose value isn't even changing would be rejected.  
> > 
> > About that.
> > 
> > To me it looks like just a classic case of broken communication.
> > 
> > The kernel developers assume that of course userspace will minimize the
> > state set to only those properties that change, because...?
> > 
> > Userspace developers think that of course the kernel will optimize the
> > state set into minimal changes, because the kernel actually has the
> > authoritative knowledge of what the current state is, and the driver
> > actually knows which properties are costly and need to be optimized and
> > which ones don't matter. It has never even occurred to me that the
> > kernel would not compare next state to current state.
> > 
> > No-one ever documented any expectations, did they?  
> 
> Do you really want that for async flips? Async flips can't be
> done atomically with anything else, so why are you even asking
> the kernel to do that?

I'm not talking about async flips only.

I'm talking about atomic commits in general. I don't think it's a good
idea to make async atomic commits behave fundamentally different from
sync atomic commits wrt. what non-changing state you are allowed to
list in your state set or not.

Isn't there common DRM code to convert an atomic commit state set into
state objects? It probably starts by copying the current state, and
then playing through the commit state set, setting all listed
properties to their new values? Why wouldn't that loop maintain the
knowledge of what actually changed?

When you copy the current data, reset all changed-flags to false. When
you apply each property from the commit set, compare the value and set
the changed-flag only if the value changes.

This manufacturing of the new tentative state set happens at atomic
commit ioctl time before the ioctl returns, right? So the current state
is well-defined: any previous atomic sync or async commit can be assumed to
have succeeded even if it hasn't applied in hardware yet if the commit
ioctl for it succeeded, right?


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: KMS atomic state sets, full vs. minimal (Re: [PATCH v3 0/6] Add support for atomic async page-flips)

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 2537 bytes --]

On Fri, 30 Sep 2022 18:45:09 +0300
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Fri, Sep 30, 2022 at 06:37:00PM +0300, Pekka Paalanen wrote:
> > On Fri, 30 Sep 2022 18:09:55 +0300
> > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >   
> > > That would actively discourage people from even attempting the
> > > "just dump all the state into the ioctl" approach with async flips
> > > since even the props whose value isn't even changing would be rejected.  
> > 
> > About that.
> > 
> > To me it looks like just a classic case of broken communication.
> > 
> > The kernel developers assume that of course userspace will minimize the
> > state set to only those properties that change, because...?
> > 
> > Userspace developers think that of course the kernel will optimize the
> > state set into minimal changes, because the kernel actually has the
> > authoritative knowledge of what the current state is, and the driver
> > actually knows which properties are costly and need to be optimized and
> > which ones don't matter. It has never even occurred to me that the
> > kernel would not compare next state to current state.
> > 
> > No-one ever documented any expectations, did they?  
> 
> Do you really want that for async flips? Async flips can't be
> done atomically with anything else, so why are you even asking
> the kernel to do that?

I'm not talking about async flips only.

I'm talking about atomic commits in general. I don't think it's a good
idea to make async atomic commits behave fundamentally different from
sync atomic commits wrt. what non-changing state you are allowed to
list in your state set or not.

Isn't there common DRM code to convert an atomic commit state set into
state objects? It probably starts by copying the current state, and
then playing through the commit state set, setting all listed
properties to their new values? Why wouldn't that loop maintain the
knowledge of what actually changed?

When you copy the current data, reset all changed-flags to false. When
you apply each property from the commit set, compare the value and set
the changed-flag only if the value changes.

This manufacturing of the new tentative state set happens at atomic
commit ioctl time before the ioctl returns, right? So the current state
is well-defined: any previous atomic sync or async commit can be assumed to
have succeeded even if it hasn't applied in hardware yet if the commit
ioctl for it succeeded, right?


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: KMS atomic state sets, full vs. minimal (Re: [PATCH v3 0/6] Add support for atomic async page-flips)

[Ville Syrjälä]

On Mon, Oct 03, 2022 at 11:48:49AM +0300, Pekka Paalanen wrote:
> On Fri, 30 Sep 2022 18:45:09 +0300
> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 
> > On Fri, Sep 30, 2022 at 06:37:00PM +0300, Pekka Paalanen wrote:
> > > On Fri, 30 Sep 2022 18:09:55 +0300
> > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > >   
> > > > That would actively discourage people from even attempting the
> > > > "just dump all the state into the ioctl" approach with async flips
> > > > since even the props whose value isn't even changing would be rejected.  
> > > 
> > > About that.
> > > 
> > > To me it looks like just a classic case of broken communication.
> > > 
> > > The kernel developers assume that of course userspace will minimize the
> > > state set to only those properties that change, because...?
> > > 
> > > Userspace developers think that of course the kernel will optimize the
> > > state set into minimal changes, because the kernel actually has the
> > > authoritative knowledge of what the current state is, and the driver
> > > actually knows which properties are costly and need to be optimized and
> > > which ones don't matter. It has never even occurred to me that the
> > > kernel would not compare next state to current state.
> > > 
> > > No-one ever documented any expectations, did they?  
> > 
> > Do you really want that for async flips? Async flips can't be
> > done atomically with anything else, so why are you even asking
> > the kernel to do that?
> 
> I'm not talking about async flips only.
> 
> I'm talking about atomic commits in general. I don't think it's a good
> idea to make async atomic commits behave fundamentally different from
> sync atomic commits wrt. what non-changing state you are allowed to
> list in your state set or not.
> 
> Isn't there common DRM code to convert an atomic commit state set into
> state objects? It probably starts by copying the current state, and
> then playing through the commit state set, setting all listed
> properties to their new values? Why wouldn't that loop maintain the
> knowledge of what actually changed?

Any such book keeping is entirely ad-hoc atm. It's also not super
obvious how much of it is actually useful.

You have to do a real commit on the crtc anyway if the crtc (or
on any of its associated objects) is listed in the commit, so
there's not necessarily much to be gained by tracking chages in
all properties.

And that behaviour again enters very muddy waters when combined
with the async flip flag for the entire commit. The current approach
being proposed seems to suggest that we can instead short circuit
async commits when nothing has changed. That is not at all how
sync atomic commits work.

> When you copy the current data, reset all changed-flags to false. When
> you apply each property from the commit set, compare the value and set
> the changed-flag only if the value changes.
> 
> This manufacturing of the new tentative state set happens at atomic
> commit ioctl time before the ioctl returns, right? So the current state
> is well-defined: any previous atomic sync or async commit can be assumed to
> have succeeded even if it hasn't applied in hardware yet if the commit
> ioctl for it succeeded, right?

Yes. We could certainly try to fully track all changes in
properties, but no has measured if there's any real benefit
of doing so.

-- 
Ville Syrjälä
Intel

Re: KMS atomic state sets, full vs. minimal (Re: [PATCH v3 0/6] Add support for atomic async page-flips)

[Ville Syrjälä]

On Mon, Oct 03, 2022 at 11:48:49AM +0300, Pekka Paalanen wrote:
> On Fri, 30 Sep 2022 18:45:09 +0300
> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 
> > On Fri, Sep 30, 2022 at 06:37:00PM +0300, Pekka Paalanen wrote:
> > > On Fri, 30 Sep 2022 18:09:55 +0300
> > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > >   
> > > > That would actively discourage people from even attempting the
> > > > "just dump all the state into the ioctl" approach with async flips
> > > > since even the props whose value isn't even changing would be rejected.  
> > > 
> > > About that.
> > > 
> > > To me it looks like just a classic case of broken communication.
> > > 
> > > The kernel developers assume that of course userspace will minimize the
> > > state set to only those properties that change, because...?
> > > 
> > > Userspace developers think that of course the kernel will optimize the
> > > state set into minimal changes, because the kernel actually has the
> > > authoritative knowledge of what the current state is, and the driver
> > > actually knows which properties are costly and need to be optimized and
> > > which ones don't matter. It has never even occurred to me that the
> > > kernel would not compare next state to current state.
> > > 
> > > No-one ever documented any expectations, did they?  
> > 
> > Do you really want that for async flips? Async flips can't be
> > done atomically with anything else, so why are you even asking
> > the kernel to do that?
> 
> I'm not talking about async flips only.
> 
> I'm talking about atomic commits in general. I don't think it's a good
> idea to make async atomic commits behave fundamentally different from
> sync atomic commits wrt. what non-changing state you are allowed to
> list in your state set or not.
> 
> Isn't there common DRM code to convert an atomic commit state set into
> state objects? It probably starts by copying the current state, and
> then playing through the commit state set, setting all listed
> properties to their new values? Why wouldn't that loop maintain the
> knowledge of what actually changed?

Any such book keeping is entirely ad-hoc atm. It's also not super
obvious how much of it is actually useful.

You have to do a real commit on the crtc anyway if the crtc (or
on any of its associated objects) is listed in the commit, so
there's not necessarily much to be gained by tracking chages in
all properties.

And that behaviour again enters very muddy waters when combined
with the async flip flag for the entire commit. The current approach
being proposed seems to suggest that we can instead short circuit
async commits when nothing has changed. That is not at all how
sync atomic commits work.

> When you copy the current data, reset all changed-flags to false. When
> you apply each property from the commit set, compare the value and set
> the changed-flag only if the value changes.
> 
> This manufacturing of the new tentative state set happens at atomic
> commit ioctl time before the ioctl returns, right? So the current state
> is well-defined: any previous atomic sync or async commit can be assumed to
> have succeeded even if it hasn't applied in hardware yet if the commit
> ioctl for it succeeded, right?

Yes. We could certainly try to fully track all changes in
properties, but no has measured if there's any real benefit
of doing so.

-- 
Ville Syrjälä
Intel

Re: [PATCH v3 0/6] Add support for atomic async page-flips

[Simon Ser]

> > > So no tests that actually verify that the kernel properly rejects
> > > stuff stuff like modesets, gamma LUT updates, plane movement,
> > > etc.?
> >
> > Pondering this a bit more, it just occurred to me the current driver
> > level checks might easily lead to confusing behaviour. Eg. is
> > the ioctl going to succeed if you ask for an async change of some
> > random property while the crtc disabled, but fails if you ask for
> > the same async property change when the crtc is active?
> >
> > So another reason why rejecting most properties already at
> > the uapi level might be a good idea.
> 
> And just to be clear this is pretty much what I suggest:
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 79730fa1dd8e..471a2c703847 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1392,6 +1392,13 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  				goto out;
>  			}
> 
> +			if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC &&
> +			    prop != dev->mode_config.prop_fb_id) {
> +				drm_mode_object_put(obj);
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +
>  			if (copy_from_user(&prop_value,
>  					   prop_values_ptr + copied_props,
>  					   sizeof(prop_value))) {
> 
> 
> That would actively discourage people from even attempting the
> "just dump all the state into the ioctl" approach with async flips
> since even the props whose value isn't even changing would be rejected.

How does this sound?

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 945761968428..ffd16bdc7b83 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -972,14 +972,26 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
 			    struct drm_file *file_priv,
 			    struct drm_mode_object *obj,
 			    struct drm_property *prop,
-			    uint64_t prop_value)
+			    uint64_t prop_value,
+			    bool async_flip)
 {
 	struct drm_mode_object *ref;
 	int ret;
+	uint64_t old_val;
 
 	if (!drm_property_change_valid_get(prop, prop_value, &ref))
 		return -EINVAL;
 
+	if (async_flip && prop != prop->dev->mode_config.prop_fb_id) {
+		ret = drm_atomic_get_property(obj, prop, &old_val);
+		if (ret != 0 || old_val != prop_value) {
+			drm_dbg_atomic(prop->dev,
+				       "[PROP:%d:%s] cannot be changed during async flip\n",
+				       prop->base.id, prop->name);
+			return -EINVAL;
+		}
+	}
+
 	switch (obj->type) {
 	case DRM_MODE_OBJECT_CONNECTOR: {
 		struct drm_connector *connector = obj_to_connector(obj);

Re: [PATCH v3 0/6] Add support for atomic async page-flips

[André Almeida]

On 10/13/22 13:02, Simon Ser wrote:
>>>> So no tests that actually verify that the kernel properly rejects
>>>> stuff stuff like modesets, gamma LUT updates, plane movement,
>>>> etc.?
>>>
>>> Pondering this a bit more, it just occurred to me the current driver
>>> level checks might easily lead to confusing behaviour. Eg. is
>>> the ioctl going to succeed if you ask for an async change of some
>>> random property while the crtc disabled, but fails if you ask for
>>> the same async property change when the crtc is active?
>>>
>>> So another reason why rejecting most properties already at
>>> the uapi level might be a good idea.
>>
>> And just to be clear this is pretty much what I suggest:
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>> index 79730fa1dd8e..471a2c703847 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -1392,6 +1392,13 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>>   				goto out;
>>   			}
>>
>> +			if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC &&
>> +			    prop != dev->mode_config.prop_fb_id) {
>> +				drm_mode_object_put(obj);
>> +				ret = -EINVAL;
>> +				goto out;
>> +			}
>> +
>>   			if (copy_from_user(&prop_value,
>>   					   prop_values_ptr + copied_props,
>>   					   sizeof(prop_value))) {
>>
>>
>> That would actively discourage people from even attempting the
>> "just dump all the state into the ioctl" approach with async flips
>> since even the props whose value isn't even changing would be rejected.
> 
> How does this sound?
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 945761968428..ffd16bdc7b83 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -972,14 +972,26 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>   			    struct drm_file *file_priv,
>   			    struct drm_mode_object *obj,
>   			    struct drm_property *prop,
> -			    uint64_t prop_value)
> +			    uint64_t prop_value,
> +			    bool async_flip)
>   {
>   	struct drm_mode_object *ref;
>   	int ret;
> +	uint64_t old_val;
>   
>   	if (!drm_property_change_valid_get(prop, prop_value, &ref))
>   		return -EINVAL;
>   
> +	if (async_flip && prop != prop->dev->mode_config.prop_fb_id) {
> +		ret = drm_atomic_get_property(obj, prop, &old_val);
> +		if (ret != 0 || old_val != prop_value) {
> +			drm_dbg_atomic(prop->dev,
> +				       "[PROP:%d:%s] cannot be changed during async flip\n",
> +				       prop->base.id, prop->name);

I would write this as "[PROP:%d:%s] No prop can be changed during async 
flips" to make it clear that it's not just this prop that can't, but any.

> +			return -EINVAL;
> +		}
> +	}
> +
>   	switch (obj->type) {
>   	case DRM_MODE_OBJECT_CONNECTOR: {
>   		struct drm_connector *connector = obj_to_connector(obj);

Re: [PATCH v3 0/6] Add support for atomic async page-flips

[André Almeida]

On 10/13/22 13:02, Simon Ser wrote:
>>>> So no tests that actually verify that the kernel properly rejects
>>>> stuff stuff like modesets, gamma LUT updates, plane movement,
>>>> etc.?
>>>
>>> Pondering this a bit more, it just occurred to me the current driver
>>> level checks might easily lead to confusing behaviour. Eg. is
>>> the ioctl going to succeed if you ask for an async change of some
>>> random property while the crtc disabled, but fails if you ask for
>>> the same async property change when the crtc is active?
>>>
>>> So another reason why rejecting most properties already at
>>> the uapi level might be a good idea.
>>
>> And just to be clear this is pretty much what I suggest:
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>> index 79730fa1dd8e..471a2c703847 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -1392,6 +1392,13 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>>   				goto out;
>>   			}
>>
>> +			if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC &&
>> +			    prop != dev->mode_config.prop_fb_id) {
>> +				drm_mode_object_put(obj);
>> +				ret = -EINVAL;
>> +				goto out;
>> +			}
>> +
>>   			if (copy_from_user(&prop_value,
>>   					   prop_values_ptr + copied_props,
>>   					   sizeof(prop_value))) {
>>
>>
>> That would actively discourage people from even attempting the
>> "just dump all the state into the ioctl" approach with async flips
>> since even the props whose value isn't even changing would be rejected.
> 
> How does this sound?
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 945761968428..ffd16bdc7b83 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -972,14 +972,26 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>   			    struct drm_file *file_priv,
>   			    struct drm_mode_object *obj,
>   			    struct drm_property *prop,
> -			    uint64_t prop_value)
> +			    uint64_t prop_value,
> +			    bool async_flip)
>   {
>   	struct drm_mode_object *ref;
>   	int ret;
> +	uint64_t old_val;
>   
>   	if (!drm_property_change_valid_get(prop, prop_value, &ref))
>   		return -EINVAL;
>   
> +	if (async_flip && prop != prop->dev->mode_config.prop_fb_id) {
> +		ret = drm_atomic_get_property(obj, prop, &old_val);
> +		if (ret != 0 || old_val != prop_value) {
> +			drm_dbg_atomic(prop->dev,
> +				       "[PROP:%d:%s] cannot be changed during async flip\n",
> +				       prop->base.id, prop->name);

I would write this as "[PROP:%d:%s] No prop can be changed during async 
flips" to make it clear that it's not just this prop that can't, but any.

> +			return -EINVAL;
> +		}
> +	}
> +
>   	switch (obj->type) {
>   	case DRM_MODE_OBJECT_CONNECTOR: {
>   		struct drm_connector *connector = obj_to_connector(obj);

Re: [PATCH v3 0/6] Add support for atomic async page-flips

[André Almeida]

On 10/13/22 13:02, Simon Ser wrote:
>>>> So no tests that actually verify that the kernel properly rejects
>>>> stuff stuff like modesets, gamma LUT updates, plane movement,
>>>> etc.?
>>>
>>> Pondering this a bit more, it just occurred to me the current driver
>>> level checks might easily lead to confusing behaviour. Eg. is
>>> the ioctl going to succeed if you ask for an async change of some
>>> random property while the crtc disabled, but fails if you ask for
>>> the same async property change when the crtc is active?
>>>
>>> So another reason why rejecting most properties already at
>>> the uapi level might be a good idea.
>>
>> And just to be clear this is pretty much what I suggest:
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>> index 79730fa1dd8e..471a2c703847 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -1392,6 +1392,13 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>>   				goto out;
>>   			}
>>
>> +			if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC &&
>> +			    prop != dev->mode_config.prop_fb_id) {
>> +				drm_mode_object_put(obj);
>> +				ret = -EINVAL;
>> +				goto out;
>> +			}
>> +
>>   			if (copy_from_user(&prop_value,
>>   					   prop_values_ptr + copied_props,
>>   					   sizeof(prop_value))) {
>>
>>
>> That would actively discourage people from even attempting the
>> "just dump all the state into the ioctl" approach with async flips
>> since even the props whose value isn't even changing would be rejected.
> 
> How does this sound?
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 945761968428..ffd16bdc7b83 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -972,14 +972,26 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>   			    struct drm_file *file_priv,
>   			    struct drm_mode_object *obj,
>   			    struct drm_property *prop,
> -			    uint64_t prop_value)
> +			    uint64_t prop_value,
> +			    bool async_flip)
>   {
>   	struct drm_mode_object *ref;
>   	int ret;
> +	uint64_t old_val;
>   
>   	if (!drm_property_change_valid_get(prop, prop_value, &ref))
>   		return -EINVAL;
>   
> +	if (async_flip && prop != prop->dev->mode_config.prop_fb_id) {
> +		ret = drm_atomic_get_property(obj, prop, &old_val);
> +		if (ret != 0 || old_val != prop_value) {
> +			drm_dbg_atomic(prop->dev,
> +				       "[PROP:%d:%s] cannot be changed during async flip\n",
> +				       prop->base.id, prop->name);
> +			return -EINVAL;
> +		}
> +	}
> +
>   	switch (obj->type) {
>   	case DRM_MODE_OBJECT_CONNECTOR: {
>   		struct drm_connector *connector = obj_to_connector(obj);


drm_atomic_get_property() needs the object lock to be used, so we need 
to check the property inside the switch-case like this:

-- >8 --

 From f3ee5a1163bfe5a88109d7084208940fe5566967 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Almeida?= <andrealmeid@igalia.com>
Date: Thu, 27 Oct 2022 17:23:09 -0300
Subject: [PATCH] drm: Check for prop changes in atomic async flip
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

No prop changes are allowed during an async flip via atomic DRM API, so
make sure to reject them.

Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
  drivers/gpu/drm/drm_atomic_uapi.c   | 47 +++++++++++++++++++++++++++--
  drivers/gpu/drm/drm_crtc_internal.h |  2 +-
  drivers/gpu/drm/drm_mode_object.c   |  2 +-
  3 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c 
b/drivers/gpu/drm/drm_atomic_uapi.c
index ee24ed7e2edb..f63f23305621 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -964,13 +964,28 @@ int drm_atomic_connector_commit_dpms(struct 
drm_atomic_state *state,
  	return ret;
  }

+static bool drm_atomic_check_prop_changes(int ret, uint64_t old_val, 
uint64_t prop_value,
+				  struct drm_property *prop)
+{
+	if (ret != 0 || old_val != prop_value) {
+		drm_dbg_atomic(prop->dev,
+			       "[PROP:%d:%s] No prop can be changed during async flip\n",
+			       prop->base.id, prop->name);
+		return true;
+	}
+
+	return false;
+}
+
  int drm_atomic_set_property(struct drm_atomic_state *state,
  			    struct drm_file *file_priv,
  			    struct drm_mode_object *obj,
  			    struct drm_property *prop,
-			    uint64_t prop_value)
+			    uint64_t prop_value,
+			    bool async_flip)
  {
  	struct drm_mode_object *ref;
+	uint64_t old_val;
  	int ret;

  	if (!drm_property_change_valid_get(prop, prop_value, &ref))
@@ -987,6 +1002,15 @@ int drm_atomic_set_property(struct 
drm_atomic_state *state,
  			break;
  		}

+		if (async_flip) {
+			ret = drm_atomic_connector_get_property(connector, connector_state,
+					prop, &old_val);
+			if (drm_atomic_check_prop_changes(ret, old_val, prop_value, prop)) {
+				ret = -EINVAL;
+				break;
+			}
+		}
+
  		ret = drm_atomic_connector_set_property(connector,
  				connector_state, file_priv,
  				prop, prop_value);
@@ -1002,6 +1026,14 @@ int drm_atomic_set_property(struct 
drm_atomic_state *state,
  			break;
  		}

+		if (async_flip) {
+			ret = drm_atomic_crtc_get_property(crtc, crtc_state, prop, &old_val);
+			if (drm_atomic_check_prop_changes(ret, old_val, prop_value, prop)) {
+				ret = -EINVAL;
+				break;
+			}
+		}
+
  		ret = drm_atomic_crtc_set_property(crtc,
  				crtc_state, prop, prop_value);
  		break;
@@ -1016,6 +1048,14 @@ int drm_atomic_set_property(struct 
drm_atomic_state *state,
  			break;
  		}

+		if (async_flip && prop != prop->dev->mode_config.prop_fb_id) {
+			ret = drm_atomic_plane_get_property(plane, plane_state, prop, &old_val);
+			if (drm_atomic_check_prop_changes(ret, old_val, prop_value, prop)) {
+				ret = -EINVAL;
+				break;
+			}
+		}
+
  		ret = drm_atomic_plane_set_property(plane,
  				plane_state, file_priv,
  				prop, prop_value);
@@ -1304,6 +1344,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
  	struct drm_out_fence_state *fence_state;
  	int ret = 0;
  	unsigned int i, j, num_fences;
+	bool async = false;

  	/* disallow for drivers not supporting atomic: */
  	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
@@ -1340,6 +1381,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
  				       "commit failed: DRM_MODE_PAGE_FLIP_ASYNC not supported with 
atomic\n");
  			return -EINVAL;
  		}
+
+		async = true;
  	}

  	/* can't test and expect an event at the same time. */
@@ -1420,7 +1463,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
  			}

  			ret = drm_atomic_set_property(state, file_priv,
-						      obj, prop, prop_value);
+						      obj, prop, prop_value, async);
  			if (ret) {
  				drm_mode_object_put(obj);
  				goto out;
diff --git a/drivers/gpu/drm/drm_crtc_internal.h 
b/drivers/gpu/drm/drm_crtc_internal.h
index 56041b604881..42ff11706fd4 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -250,7 +250,7 @@ int drm_atomic_set_property(struct drm_atomic_state 
*state,
  			    struct drm_file *file_priv,
  			    struct drm_mode_object *obj,
  			    struct drm_property *prop,
-			    uint64_t prop_value);
+			    uint64_t prop_value, bool async_flip);
  int drm_atomic_get_property(struct drm_mode_object *obj,
  			    struct drm_property *property, uint64_t *val);

diff --git a/drivers/gpu/drm/drm_mode_object.c 
b/drivers/gpu/drm/drm_mode_object.c
index ba1608effc0f..64f519254895 100644
--- a/drivers/gpu/drm/drm_mode_object.c
+++ b/drivers/gpu/drm/drm_mode_object.c
@@ -536,7 +536,7 @@ static int set_property_atomic(struct 
drm_mode_object *obj,
  						       obj_to_connector(obj),
  						       prop_value);
  	} else {
-		ret = drm_atomic_set_property(state, file_priv, obj, prop, prop_value);
+		ret = drm_atomic_set_property(state, file_priv, obj, prop, 
prop_value, false);
  		if (ret)
  			goto out;
  		ret = drm_atomic_commit(state);
-- 
2.37.3

Re: [PATCH v3 0/6] Add support for atomic async page-flips

[André Almeida]

On 10/13/22 13:02, Simon Ser wrote:
>>>> So no tests that actually verify that the kernel properly rejects
>>>> stuff stuff like modesets, gamma LUT updates, plane movement,
>>>> etc.?
>>>
>>> Pondering this a bit more, it just occurred to me the current driver
>>> level checks might easily lead to confusing behaviour. Eg. is
>>> the ioctl going to succeed if you ask for an async change of some
>>> random property while the crtc disabled, but fails if you ask for
>>> the same async property change when the crtc is active?
>>>
>>> So another reason why rejecting most properties already at
>>> the uapi level might be a good idea.
>>
>> And just to be clear this is pretty much what I suggest:
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>> index 79730fa1dd8e..471a2c703847 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -1392,6 +1392,13 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>>   				goto out;
>>   			}
>>
>> +			if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC &&
>> +			    prop != dev->mode_config.prop_fb_id) {
>> +				drm_mode_object_put(obj);
>> +				ret = -EINVAL;
>> +				goto out;
>> +			}
>> +
>>   			if (copy_from_user(&prop_value,
>>   					   prop_values_ptr + copied_props,
>>   					   sizeof(prop_value))) {
>>
>>
>> That would actively discourage people from even attempting the
>> "just dump all the state into the ioctl" approach with async flips
>> since even the props whose value isn't even changing would be rejected.
> 
> How does this sound?
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 945761968428..ffd16bdc7b83 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -972,14 +972,26 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>   			    struct drm_file *file_priv,
>   			    struct drm_mode_object *obj,
>   			    struct drm_property *prop,
> -			    uint64_t prop_value)
> +			    uint64_t prop_value,
> +			    bool async_flip)
>   {
>   	struct drm_mode_object *ref;
>   	int ret;
> +	uint64_t old_val;
>   
>   	if (!drm_property_change_valid_get(prop, prop_value, &ref))
>   		return -EINVAL;
>   
> +	if (async_flip && prop != prop->dev->mode_config.prop_fb_id) {
> +		ret = drm_atomic_get_property(obj, prop, &old_val);
> +		if (ret != 0 || old_val != prop_value) {
> +			drm_dbg_atomic(prop->dev,
> +				       "[PROP:%d:%s] cannot be changed during async flip\n",
> +				       prop->base.id, prop->name);
> +			return -EINVAL;
> +		}
> +	}
> +
>   	switch (obj->type) {
>   	case DRM_MODE_OBJECT_CONNECTOR: {
>   		struct drm_connector *connector = obj_to_connector(obj);


drm_atomic_get_property() needs the object lock to be used, so we need 
to check the property inside the switch-case like this:

-- >8 --

 From f3ee5a1163bfe5a88109d7084208940fe5566967 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Almeida?= <andrealmeid@igalia.com>
Date: Thu, 27 Oct 2022 17:23:09 -0300
Subject: [PATCH] drm: Check for prop changes in atomic async flip
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

No prop changes are allowed during an async flip via atomic DRM API, so
make sure to reject them.

Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
  drivers/gpu/drm/drm_atomic_uapi.c   | 47 +++++++++++++++++++++++++++--
  drivers/gpu/drm/drm_crtc_internal.h |  2 +-
  drivers/gpu/drm/drm_mode_object.c   |  2 +-
  3 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c 
b/drivers/gpu/drm/drm_atomic_uapi.c
index ee24ed7e2edb..f63f23305621 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -964,13 +964,28 @@ int drm_atomic_connector_commit_dpms(struct 
drm_atomic_state *state,
  	return ret;
  }

+static bool drm_atomic_check_prop_changes(int ret, uint64_t old_val, 
uint64_t prop_value,
+				  struct drm_property *prop)
+{
+	if (ret != 0 || old_val != prop_value) {
+		drm_dbg_atomic(prop->dev,
+			       "[PROP:%d:%s] No prop can be changed during async flip\n",
+			       prop->base.id, prop->name);
+		return true;
+	}
+
+	return false;
+}
+
  int drm_atomic_set_property(struct drm_atomic_state *state,
  			    struct drm_file *file_priv,
  			    struct drm_mode_object *obj,
  			    struct drm_property *prop,
-			    uint64_t prop_value)
+			    uint64_t prop_value,
+			    bool async_flip)
  {
  	struct drm_mode_object *ref;
+	uint64_t old_val;
  	int ret;

  	if (!drm_property_change_valid_get(prop, prop_value, &ref))
@@ -987,6 +1002,15 @@ int drm_atomic_set_property(struct 
drm_atomic_state *state,
  			break;
  		}

+		if (async_flip) {
+			ret = drm_atomic_connector_get_property(connector, connector_state,
+					prop, &old_val);
+			if (drm_atomic_check_prop_changes(ret, old_val, prop_value, prop)) {
+				ret = -EINVAL;
+				break;
+			}
+		}
+
  		ret = drm_atomic_connector_set_property(connector,
  				connector_state, file_priv,
  				prop, prop_value);
@@ -1002,6 +1026,14 @@ int drm_atomic_set_property(struct 
drm_atomic_state *state,
  			break;
  		}

+		if (async_flip) {
+			ret = drm_atomic_crtc_get_property(crtc, crtc_state, prop, &old_val);
+			if (drm_atomic_check_prop_changes(ret, old_val, prop_value, prop)) {
+				ret = -EINVAL;
+				break;
+			}
+		}
+
  		ret = drm_atomic_crtc_set_property(crtc,
  				crtc_state, prop, prop_value);
  		break;
@@ -1016,6 +1048,14 @@ int drm_atomic_set_property(struct 
drm_atomic_state *state,
  			break;
  		}

+		if (async_flip && prop != prop->dev->mode_config.prop_fb_id) {
+			ret = drm_atomic_plane_get_property(plane, plane_state, prop, &old_val);
+			if (drm_atomic_check_prop_changes(ret, old_val, prop_value, prop)) {
+				ret = -EINVAL;
+				break;
+			}
+		}
+
  		ret = drm_atomic_plane_set_property(plane,
  				plane_state, file_priv,
  				prop, prop_value);
@@ -1304,6 +1344,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
  	struct drm_out_fence_state *fence_state;
  	int ret = 0;
  	unsigned int i, j, num_fences;
+	bool async = false;

  	/* disallow for drivers not supporting atomic: */
  	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
@@ -1340,6 +1381,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
  				       "commit failed: DRM_MODE_PAGE_FLIP_ASYNC not supported with 
atomic\n");
  			return -EINVAL;
  		}
+
+		async = true;
  	}

  	/* can't test and expect an event at the same time. */
@@ -1420,7 +1463,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
  			}

  			ret = drm_atomic_set_property(state, file_priv,
-						      obj, prop, prop_value);
+						      obj, prop, prop_value, async);
  			if (ret) {
  				drm_mode_object_put(obj);
  				goto out;
diff --git a/drivers/gpu/drm/drm_crtc_internal.h 
b/drivers/gpu/drm/drm_crtc_internal.h
index 56041b604881..42ff11706fd4 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -250,7 +250,7 @@ int drm_atomic_set_property(struct drm_atomic_state 
*state,
  			    struct drm_file *file_priv,
  			    struct drm_mode_object *obj,
  			    struct drm_property *prop,
-			    uint64_t prop_value);
+			    uint64_t prop_value, bool async_flip);
  int drm_atomic_get_property(struct drm_mode_object *obj,
  			    struct drm_property *property, uint64_t *val);

diff --git a/drivers/gpu/drm/drm_mode_object.c 
b/drivers/gpu/drm/drm_mode_object.c
index ba1608effc0f..64f519254895 100644
--- a/drivers/gpu/drm/drm_mode_object.c
+++ b/drivers/gpu/drm/drm_mode_object.c
@@ -536,7 +536,7 @@ static int set_property_atomic(struct 
drm_mode_object *obj,
  						       obj_to_connector(obj),
  						       prop_value);
  	} else {
-		ret = drm_atomic_set_property(state, file_priv, obj, prop, prop_value);
+		ret = drm_atomic_set_property(state, file_priv, obj, prop, 
prop_value, false);
  		if (ret)
  			goto out;
  		ret = drm_atomic_commit(state);
-- 
2.37.3

Re: [PATCH v3 0/6] Add support for atomic async page-flips

[Simon Ser]

Ville, any news on this?

Re: [PATCH v3 0/6] Add support for atomic async page-flips

[Simon Ser]

Hm, thinking about this again, there's still something which is a bit
off with the new approach. Let's say the caller sets MODE_ID to another
blob ID, but with the same blob payload. DRM core is smart enough to
figure out that the mode didn't change and skip the modeset. However,
the check implemented here isn't smart enough.

tl;dr there are still discrepancies between the regular code-path and
the async page-flip one. Does that matter?