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

[PATCH 0/4] 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.

User-space patch:
https://github.com/Plagman/gamescope/pull/595

IGT patch:
https://patchwork.freedesktop.org/series/107681/

Tested on an AMD Picasso iGPU.

Simon Ser (4):
  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 DCN

 drivers/gpu/drm/amd/amdgpu/dce_v10_0.c       |  1 +
 drivers/gpu/drm/amd/amdgpu/dce_v11_0.c       |  1 +
 drivers/gpu/drm/amd/amdgpu/dce_v6_0.c        |  1 +
 drivers/gpu/drm/amd/amdgpu/dce_v8_0.c        |  1 +
 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/radeon/radeon_display.c      |  1 +
 drivers/gpu/drm/vc4/vc4_kms.c                |  1 +
 include/drm/drm_mode_config.h                | 11 ++++++++
 include/uapi/drm/drm.h                       | 10 ++++++-
 13 files changed, 59 insertions(+), 4 deletions(-)

-- 
2.37.2

[PATCH 1/4] 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 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.

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Harry Wentland <hwentlan@amd.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/dce_v10_0.c            |  1 +
 drivers/gpu/drm/amd/amdgpu/dce_v11_0.c            |  1 +
 drivers/gpu/drm/amd/amdgpu/dce_v6_0.c             |  1 +
 drivers/gpu/drm/amd/amdgpu/dce_v8_0.c             |  1 +
 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/radeon/radeon_display.c           |  1 +
 drivers/gpu/drm/vc4/vc4_kms.c                     |  1 +
 include/drm/drm_mode_config.h                     | 11 +++++++++++
 11 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index 9c964cd3b5d4..76ccd57e3663 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -2791,6 +2791,7 @@ static int dce_v10_0_sw_init(void *handle)
 	adev_to_drm(adev)->mode_config.funcs = &amdgpu_mode_funcs;
 
 	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.max_width = 16384;
 	adev_to_drm(adev)->mode_config.max_height = 16384;
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
index e0ad9f27dc3f..c7702c5bfbd0 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -2909,6 +2909,7 @@ static int dce_v11_0_sw_init(void *handle)
 	adev_to_drm(adev)->mode_config.funcs = &amdgpu_mode_funcs;
 
 	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.max_width = 16384;
 	adev_to_drm(adev)->mode_config.max_height = 16384;
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
index 77f5e998a120..c9a2372f8ea2 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
@@ -2670,6 +2670,7 @@ static int dce_v6_0_sw_init(void *handle)
 
 	adev_to_drm(adev)->mode_config.funcs = &amdgpu_mode_funcs;
 	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.max_width = 16384;
 	adev_to_drm(adev)->mode_config.max_height = 16384;
 	adev_to_drm(adev)->mode_config.preferred_depth = 24;
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
index 802e5c753271..09c07820d494 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
@@ -2688,6 +2688,7 @@ static int dce_v8_0_sw_init(void *handle)
 	adev_to_drm(adev)->mode_config.funcs = &amdgpu_mode_funcs;
 
 	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.max_width = 16384;
 	adev_to_drm(adev)->mode_config.max_height = 16384;
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 9ab01c58bedb..ef816bf295eb 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3804,6 +3804,7 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
 	adev_to_drm(adev)->mode_config.prefer_shadow = 0;
 	/* 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 2a45a25e42fb..265492e6c135 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -8622,6 +8622,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/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index ca5598ae8bfc..cdfe3220db6b 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -1587,6 +1587,7 @@ int radeon_modeset_init(struct radeon_device *rdev)
 
 	if (radeon_use_pflipirq == 2 && rdev->family >= CHIP_R600)
 		rdev->ddev->mode_config.async_page_flip = true;
+	rdev->ddev->mode_config.atomic_async_page_flip_not_supported = true;
 
 	if (ASIC_IS_DCE5(rdev)) {
 		rdev->ddev->mode_config.max_width = 16384;
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.2

[PATCH 2/4] 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.

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Harry Wentland <hwentlan@amd.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 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) {
-- 
2.37.2

[PATCH 3/4] 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>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Harry Wentland <hwentlan@amd.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.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.2

Re: [PATCH 3/4] drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP

[Ville Syrjälä]

On Wed, Aug 24, 2022 at 03:08:55PM +0000, Simon Ser wrote:
> 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.

I think we'd need to clarify the semantics of the async flag
for atomic commits.

Eg. on Intel hw only pure page flips are possible async, if you do
anything else (change plane size/pos/scaling/etc.) you will need
to do a sync update. Technically not even all page flips (from the
uapi POV) might be possible as the exact scanout source address
is specified via two registers, only one of which can be update
async. So technically the two framebuffers might be laid out
just slightly differently which could prevent an async flip.
Also only some subset of planes actually support async flips.

And on hw where multiple planes support it on the same crtc, only one
plane can do it at a time. Well, more accurately we can only select
one plane at a time to give us the "flip done" interrupt. I guess
if the user wants to async flip multiple planes at the same time
we could do them serially as opposed to in parallel to make sure
all the flips actually happened before we signal completion of the
entire commit. Async flips of multiple planes probably won't
happen atomically anyway so doing them serially seems fine.

ATM in i915 we probably don't have sufficient state checks in
place to catch all the restrictions, and instead in part we rely
on the limited scope of the legacy async flip ioctl to make sure
the operation doesn't attempt something the hw can't do.

> Make it clear that DRM_CAP_ASYNC_PAGE_FLIP is for legacy uAPI only.
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Joshua Ashton <joshua@froggi.es>
> Cc: Melissa Wen <mwen@igalia.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Harry Wentland <hwentlan@amd.com>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.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.2
> 

-- 
Ville Syrjälä
Intel

Re: [PATCH 3/4] drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP

[Simon Ser]

On Friday, August 26th, 2022 at 10:19, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Wed, Aug 24, 2022 at 03:08:55PM +0000, Simon Ser wrote:
> > 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.
> 
> I think we'd need to clarify the semantics of the async flag
> for atomic commits.
> 
> Eg. on Intel hw only pure page flips are possible async, if you do
> anything else (change plane size/pos/scaling/etc.) you will need
> to do a sync update. Technically not even all page flips (from the
> uapi POV) might be possible as the exact scanout source address
> is specified via two registers, only one of which can be update
> async. So technically the two framebuffers might be laid out
> just slightly differently which could prevent an async flip.
> Also only some subset of planes actually support async flips.

Also IIRC some format modifiers don't support async flip at all (CCS)?

> And on hw where multiple planes support it on the same crtc, only one
> plane can do it at a time. Well, more accurately we can only select
> one plane at a time to give us the "flip done" interrupt. I guess
> if the user wants to async flip multiple planes at the same time
> we could do them serially as opposed to in parallel to make sure
> all the flips actually happened before we signal completion of the
> entire commit. Async flips of multiple planes probably won't
> happen atomically anyway so doing them serially seems fine.
> 
> ATM in i915 we probably don't have sufficient state checks in
> place to catch all the restrictions, and instead in part we rely
> on the limited scope of the legacy async flip ioctl to make sure
> the operation doesn't attempt something the hw can't do.

Yeah, that makes sense.

In the documentation patch discussion [1], it appears it's not clear what
drivers should do when async flip isn't possible with the legacy uAPI.

For the atomic uAPI, we need to pick on of these two approaches:

1. Let the kernel fall back to a sync flip if async isn't possible. This
   simplifies user-space, but then user-space has no reliable way to figure out
   what really happened (sync or async?). That could be fixed with a new
   read-only CRTC prop indicating whether the last flip was async or not.
   However, maybe someone will come up in the future with user-space which
   needs to only apply an update if async flip is possible, in which case this
   approach falls short.
2. Make the kernel return EINVAL if async flip isn't possible. This adds more
   complexity to user-space, but enables a more reliable and deterministic
   uAPI. This is also more consistent with the rest of the existing atomic
   uAPI.

Note, current user-space would only need to opportunistically enable async
flip. IOW, I think that for current user-space plans "async if possible,
otherwise sync" is good enough. That behavior maps well to the Vulkan present
modes as well (which says that "this mode *may* result in visible tearing", but
doesn't guarantee it).

Another possible shortcoming of the proposed new uAPI here is that user-space
cannot submit a single atomic commit which updates multiple CRTCs, and
individually select which CRTC does an async flip. This could be fixed with
a "ASYNC_FLIP" CRTC prop which the kernel always resets to 0 on commit. I'm not
sure we want/need to cross that bridge right now, it would be easy enough to
add as a second step if some user-space would benefit from it.

What do you think?

[1]: https://lore.kernel.org/dri-devel/ASSNOUe9wtsXskZjVlf1X4pl53T7pVE0MfEzkQ_h4cX0tjnF7e3cxpwGpRNPudmIHoRuW4kz_v1AeTpXgouLpTYcI8q-lPTzc1YMLR8JiJM=@emersion.fr/

Re: [PATCH 3/4] drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP

[Ville Syrjälä]

On Mon, Aug 29, 2022 at 04:01:44PM +0000, Simon Ser wrote:
> On Friday, August 26th, 2022 at 10:19, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 
> > On Wed, Aug 24, 2022 at 03:08:55PM +0000, Simon Ser wrote:
> > > 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.
> > 
> > I think we'd need to clarify the semantics of the async flag
> > for atomic commits.
> > 
> > Eg. on Intel hw only pure page flips are possible async, if you do
> > anything else (change plane size/pos/scaling/etc.) you will need
> > to do a sync update. Technically not even all page flips (from the
> > uapi POV) might be possible as the exact scanout source address
> > is specified via two registers, only one of which can be update
> > async. So technically the two framebuffers might be laid out
> > just slightly differently which could prevent an async flip.
> > Also only some subset of planes actually support async flips.
> 
> Also IIRC some format modifiers don't support async flip at all (CCS)?

Yeah, that too. Also planar YUV formats aren't allowed.

> 
> > And on hw where multiple planes support it on the same crtc, only one
> > plane can do it at a time. Well, more accurately we can only select
> > one plane at a time to give us the "flip done" interrupt. I guess
> > if the user wants to async flip multiple planes at the same time
> > we could do them serially as opposed to in parallel to make sure
> > all the flips actually happened before we signal completion of the
> > entire commit. Async flips of multiple planes probably won't
> > happen atomically anyway so doing them serially seems fine.
> > 
> > ATM in i915 we probably don't have sufficient state checks in
> > place to catch all the restrictions, and instead in part we rely
> > on the limited scope of the legacy async flip ioctl to make sure
> > the operation doesn't attempt something the hw can't do.
> 
> Yeah, that makes sense.
> 
> In the documentation patch discussion [1], it appears it's not clear what
> drivers should do when async flip isn't possible with the legacy uAPI.
> 
> For the atomic uAPI, we need to pick on of these two approaches:
> 
> 1. Let the kernel fall back to a sync flip if async isn't possible. This
>    simplifies user-space, but then user-space has no reliable way to figure out
>    what really happened (sync or async?). That could be fixed with a new
>    read-only CRTC prop indicating whether the last flip was async or not.
>    However, maybe someone will come up in the future with user-space which
>    needs to only apply an update if async flip is possible, in which case this
>    approach falls short.
> 2. Make the kernel return EINVAL if async flip isn't possible. This adds more
>    complexity to user-space, but enables a more reliable and deterministic
>    uAPI. This is also more consistent with the rest of the existing atomic
>    uAPI.

The current behaviour is somewhat a combination of the two. We return
an error if async flip is not possible at all given the current state.

When async flip is possible we return success, but may still internally
fall back to a sync flip for the first flip. That is required on some
borked hardware that can't switch from sync flips to async flips without
doing an extra sync flip. Also on some other hardware we intentionally
fall back to a sync flip for the first async flip, so that we can
reprogram some display FIFO stuff (aimed to make the subsequent async
flips faster).

> 
> Note, current user-space would only need to opportunistically enable async
> flip. IOW, I think that for current user-space plans "async if possible,
> otherwise sync" is good enough. That behavior maps well to the Vulkan present
> modes as well (which says that "this mode *may* result in visible tearing", but
> doesn't guarantee it).

The current behaviour is to fall back to a blit if the async
flip fails. So you still get the same effective behaviour, just
not as efficient. I think that's a reasonable way to handle it.

> 
> Another possible shortcoming of the proposed new uAPI here is that user-space
> cannot submit a single atomic commit which updates multiple CRTCs, and
> individually select which CRTC does an async flip. This could be fixed with
> a "ASYNC_FLIP" CRTC prop which the kernel always resets to 0 on commit. I'm not
> sure we want/need to cross that bridge right now, it would be easy enough to
> add as a second step if some user-space would benefit from it.

Technically it should really be per-plane since that is what does
the flip. But I have a feeling that allowing a mix of async and
sync in the same commit is just going to make everything more
complicated without really helping anything (async flips won't
happen atomically anyway with anything else).

One (crazy?) idea I had for the atomic api is that we could even
reject most of the properties already on the uapi level before anyone
gets to examine the final state. Ie. as soon as the atomic ioctl sees
eg. a gamma LUT property change it would just immediately return
an error if async flip is also requested.

> 
> What do you think?
> 
> [1]: https://lore.kernel.org/dri-devel/ASSNOUe9wtsXskZjVlf1X4pl53T7pVE0MfEzkQ_h4cX0tjnF7e3cxpwGpRNPudmIHoRuW4kz_v1AeTpXgouLpTYcI8q-lPTzc1YMLR8JiJM=@emersion.fr/

-- 
Ville Syrjälä
Intel

Re: [PATCH 3/4] drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP

[Pekka Paalanen]

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

On Tue, 30 Aug 2022 11:08:22 +0300
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Mon, Aug 29, 2022 at 04:01:44PM +0000, Simon Ser wrote:
> > On Friday, August 26th, 2022 at 10:19, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >   
> > > On Wed, Aug 24, 2022 at 03:08:55PM +0000, Simon Ser wrote:  
> > > > 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.  
> > > 
> > > I think we'd need to clarify the semantics of the async flag
> > > for atomic commits.
> > > 
> > > Eg. on Intel hw only pure page flips are possible async, if you do
> > > anything else (change plane size/pos/scaling/etc.) you will need
> > > to do a sync update. Technically not even all page flips (from the
> > > uapi POV) might be possible as the exact scanout source address
> > > is specified via two registers, only one of which can be update
> > > async. So technically the two framebuffers might be laid out
> > > just slightly differently which could prevent an async flip.
> > > Also only some subset of planes actually support async flips.  
> > 
> > Also IIRC some format modifiers don't support async flip at all (CCS)?  
> 
> Yeah, that too. Also planar YUV formats aren't allowed.
> 
> >   
> > > And on hw where multiple planes support it on the same crtc, only one
> > > plane can do it at a time. Well, more accurately we can only select
> > > one plane at a time to give us the "flip done" interrupt. I guess
> > > if the user wants to async flip multiple planes at the same time
> > > we could do them serially as opposed to in parallel to make sure
> > > all the flips actually happened before we signal completion of the
> > > entire commit. Async flips of multiple planes probably won't
> > > happen atomically anyway so doing them serially seems fine.
> > > 
> > > ATM in i915 we probably don't have sufficient state checks in
> > > place to catch all the restrictions, and instead in part we rely
> > > on the limited scope of the legacy async flip ioctl to make sure
> > > the operation doesn't attempt something the hw can't do.  
> > 
> > Yeah, that makes sense.
> > 
> > In the documentation patch discussion [1], it appears it's not clear what
> > drivers should do when async flip isn't possible with the legacy uAPI.
> > 
> > For the atomic uAPI, we need to pick on of these two approaches:
> > 
> > 1. Let the kernel fall back to a sync flip if async isn't possible. This
> >    simplifies user-space, but then user-space has no reliable way to figure out
> >    what really happened (sync or async?). That could be fixed with a new
> >    read-only CRTC prop indicating whether the last flip was async or not.
> >    However, maybe someone will come up in the future with user-space which
> >    needs to only apply an update if async flip is possible, in which case this
> >    approach falls short.

There is the pageflip completion timestamp in the DRM event. If
userspace knows the phase and period of the scanout cycle, the
completion timestamp should tell quite reliably if the update was
tearing.

For the phase, one can query KMS for the last vblank timestamp. This
should work also for VRR I assume.

For the period, fixed-frequency video mode has it straight. VRR gives
only a range or a minimum period.

> > 2. Make the kernel return EINVAL if async flip isn't possible. This adds more
> >    complexity to user-space, but enables a more reliable and deterministic
> >    uAPI. This is also more consistent with the rest of the existing atomic
> >    uAPI.  
> 
> The current behaviour is somewhat a combination of the two. We return
> an error if async flip is not possible at all given the current state.
> 
> When async flip is possible we return success, but may still internally
> fall back to a sync flip for the first flip. That is required on some
> borked hardware that can't switch from sync flips to async flips without
> doing an extra sync flip. Also on some other hardware we intentionally
> fall back to a sync flip for the first async flip, so that we can
> reprogram some display FIFO stuff (aimed to make the subsequent async
> flips faster).

Oh, so userspace should expect to run async for long periods of time,
and not use async this frame, sync next, then async again depending on
per-frame timings.

That seems important to note.

It's almost like the async flag should be a KMS property instead of a
commit ioctl flag.

> > Note, current user-space would only need to opportunistically enable async
> > flip. IOW, I think that for current user-space plans "async if possible,
> > otherwise sync" is good enough. That behavior maps well to the Vulkan present
> > modes as well (which says that "this mode *may* result in visible tearing", but
> > doesn't guarantee it).  
> 
> The current behaviour is to fall back to a blit if the async
> flip fails. So you still get the same effective behaviour, just
> not as efficient. I think that's a reasonable way to handle it.

That's purely an Xorg thing, right?

Should Wayland compositors implement the same thing is a good question.

> > Another possible shortcoming of the proposed new uAPI here is that user-space
> > cannot submit a single atomic commit which updates multiple CRTCs, and
> > individually select which CRTC does an async flip. This could be fixed with

I would think that you can just do per-CRTC atomic commits in that
case. You would do per-CRTC atomic commits anyway when the vblanks do
not coincide. I expect tearing updates to have unpredictable latency,
especially if they can silently fall back to sync flips, so doing
multi-CRTC async flips is not useful.

> > a "ASYNC_FLIP" CRTC prop which the kernel always resets to 0 on commit. I'm not
> > sure we want/need to cross that bridge right now, it would be easy enough to
> > add as a second step if some user-space would benefit from it.  
> 
> Technically it should really be per-plane since that is what does
> the flip. But I have a feeling that allowing a mix of async and
> sync in the same commit is just going to make everything more
> complicated without really helping anything (async flips won't
> happen atomically anyway with anything else).
> 
> One (crazy?) idea I had for the atomic api is that we could even
> reject most of the properties already on the uapi level before anyone
> gets to examine the final state. Ie. as soon as the atomic ioctl sees
> eg. a gamma LUT property change it would just immediately return
> an error if async flip is also requested.

I agree with these two paragraphs.

What about limiting async flag to atomic commits that update only a
single KMS plane (regardless of how many planes are active)? Maybe that
would be a good first step?

> 
> > 
> > What do you think?
> > 
> > [1]: https://lore.kernel.org/dri-devel/ASSNOUe9wtsXskZjVlf1X4pl53T7pVE0MfEzkQ_h4cX0tjnF7e3cxpwGpRNPudmIHoRuW4kz_v1AeTpXgouLpTYcI8q-lPTzc1YMLR8JiJM=@emersion.fr/  
> 

Thanks,
pq

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

Re: [PATCH 3/4] drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP

[Ville Syrjälä]

On Tue, Aug 30, 2022 at 11:40:10AM +0300, Pekka Paalanen wrote:
> On Tue, 30 Aug 2022 11:08:22 +0300
> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 
> > On Mon, Aug 29, 2022 at 04:01:44PM +0000, Simon Ser wrote:
> > > On Friday, August 26th, 2022 at 10:19, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > >   
> > > > On Wed, Aug 24, 2022 at 03:08:55PM +0000, Simon Ser wrote:  
> > > > > 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.  
> > > > 
> > > > I think we'd need to clarify the semantics of the async flag
> > > > for atomic commits.
> > > > 
> > > > Eg. on Intel hw only pure page flips are possible async, if you do
> > > > anything else (change plane size/pos/scaling/etc.) you will need
> > > > to do a sync update. Technically not even all page flips (from the
> > > > uapi POV) might be possible as the exact scanout source address
> > > > is specified via two registers, only one of which can be update
> > > > async. So technically the two framebuffers might be laid out
> > > > just slightly differently which could prevent an async flip.
> > > > Also only some subset of planes actually support async flips.  
> > > 
> > > Also IIRC some format modifiers don't support async flip at all (CCS)?  
> > 
> > Yeah, that too. Also planar YUV formats aren't allowed.
> > 
> > >   
> > > > And on hw where multiple planes support it on the same crtc, only one
> > > > plane can do it at a time. Well, more accurately we can only select
> > > > one plane at a time to give us the "flip done" interrupt. I guess
> > > > if the user wants to async flip multiple planes at the same time
> > > > we could do them serially as opposed to in parallel to make sure
> > > > all the flips actually happened before we signal completion of the
> > > > entire commit. Async flips of multiple planes probably won't
> > > > happen atomically anyway so doing them serially seems fine.
> > > > 
> > > > ATM in i915 we probably don't have sufficient state checks in
> > > > place to catch all the restrictions, and instead in part we rely
> > > > on the limited scope of the legacy async flip ioctl to make sure
> > > > the operation doesn't attempt something the hw can't do.  
> > > 
> > > Yeah, that makes sense.
> > > 
> > > In the documentation patch discussion [1], it appears it's not clear what
> > > drivers should do when async flip isn't possible with the legacy uAPI.
> > > 
> > > For the atomic uAPI, we need to pick on of these two approaches:
> > > 
> > > 1. Let the kernel fall back to a sync flip if async isn't possible. This
> > >    simplifies user-space, but then user-space has no reliable way to figure out
> > >    what really happened (sync or async?). That could be fixed with a new
> > >    read-only CRTC prop indicating whether the last flip was async or not.
> > >    However, maybe someone will come up in the future with user-space which
> > >    needs to only apply an update if async flip is possible, in which case this
> > >    approach falls short.
> 
> There is the pageflip completion timestamp in the DRM event. If
> userspace knows the phase and period of the scanout cycle, the
> completion timestamp should tell quite reliably if the update was
> tearing.
> 
> For the phase, one can query KMS for the last vblank timestamp. This
> should work also for VRR I assume.
> 
> For the period, fixed-frequency video mode has it straight. VRR gives
> only a range or a minimum period.
> 
> > > 2. Make the kernel return EINVAL if async flip isn't possible. This adds more
> > >    complexity to user-space, but enables a more reliable and deterministic
> > >    uAPI. This is also more consistent with the rest of the existing atomic
> > >    uAPI.  
> > 
> > The current behaviour is somewhat a combination of the two. We return
> > an error if async flip is not possible at all given the current state.
> > 
> > When async flip is possible we return success, but may still internally
> > fall back to a sync flip for the first flip. That is required on some
> > borked hardware that can't switch from sync flips to async flips without
> > doing an extra sync flip. Also on some other hardware we intentionally
> > fall back to a sync flip for the first async flip, so that we can
> > reprogram some display FIFO stuff (aimed to make the subsequent async
> > flips faster).
> 
> Oh, so userspace should expect to run async for long periods of time,
> and not use async this frame, sync next, then async again depending on
> per-frame timings.
> 
> That seems important to note.

Yeah, our hw can't really do the "tear if late, otherwise do a sync flip"
behaviour. GLX_swap_control_tear I think was the glx extension for that.

> 
> It's almost like the async flag should be a KMS property instead of a
> commit ioctl flag.

I guess that would be one way to implement it.

> 
> > > Note, current user-space would only need to opportunistically enable async
> > > flip. IOW, I think that for current user-space plans "async if possible,
> > > otherwise sync" is good enough. That behavior maps well to the Vulkan present
> > > modes as well (which says that "this mode *may* result in visible tearing", but
> > > doesn't guarantee it).  
> > 
> > The current behaviour is to fall back to a blit if the async
> > flip fails. So you still get the same effective behaviour, just
> > not as efficient. I think that's a reasonable way to handle it.
> 
> That's purely an Xorg thing, right?

Yes.

> 
> Should Wayland compositors implement the same thing is a good question.

Is the whole tear+wayland situation more or less up in the air still?

> 
> > > Another possible shortcoming of the proposed new uAPI here is that user-space
> > > cannot submit a single atomic commit which updates multiple CRTCs, and
> > > individually select which CRTC does an async flip. This could be fixed with
> 
> I would think that you can just do per-CRTC atomic commits in that
> case. You would do per-CRTC atomic commits anyway when the vblanks do
> not coincide. I expect tearing updates to have unpredictable latency,
> especially if they can silently fall back to sync flips, so doing
> multi-CRTC async flips is not useful.
> 
> > > a "ASYNC_FLIP" CRTC prop which the kernel always resets to 0 on commit. I'm not
> > > sure we want/need to cross that bridge right now, it would be easy enough to
> > > add as a second step if some user-space would benefit from it.  
> > 
> > Technically it should really be per-plane since that is what does
> > the flip. But I have a feeling that allowing a mix of async and
> > sync in the same commit is just going to make everything more
> > complicated without really helping anything (async flips won't
> > happen atomically anyway with anything else).
> > 
> > One (crazy?) idea I had for the atomic api is that we could even
> > reject most of the properties already on the uapi level before anyone
> > gets to examine the final state. Ie. as soon as the atomic ioctl sees
> > eg. a gamma LUT property change it would just immediately return
> > an error if async flip is also requested.
> 
> I agree with these two paragraphs.
> 
> What about limiting async flag to atomic commits that update only a
> single KMS plane (regardless of how many planes are active)? Maybe that
> would be a good first step?

Sure, that might help a bit. But I'm rather more worried about
all the state that you can now include in the atomic commit but
aren't actually allowed to change. Eg. probably all crtc/connector
properties, and most plane properties. The legacy ioctl blocks most
of that implicitly, but the atomic ioctl does not. Hence my idea
about rejecting basically everything except the plane fb property
changes already at the ioctl level. Would avoid that stuff
leaking in by accident if the driver forgets to check absolutely
everything.

-- 
Ville Syrjälä
Intel

Re: [PATCH 3/4] drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP

[Simon Ser]

On Tuesday, August 30th, 2022 at 12:24, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> > > The current behaviour is to fall back to a blit if the async
> > > flip fails. So you still get the same effective behaviour, just
> > > not as efficient. I think that's a reasonable way to handle it.
> >
> > That's purely an Xorg thing, right?
> 
> Yes.
> 
> >
> > Should Wayland compositors implement the same thing is a good question.
> 
> Is the whole tear+wayland situation more or less up in the air still?

The goal of the patches I'm sending is to allow Wayland compositors to request
tearing. We also have a WIP protocol extension for this [1].

[1]: https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/65

Re: [PATCH 3/4] drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP

[Simon Ser]

On Tuesday, August 30th, 2022 at 10:08, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> > In the documentation patch discussion [1], it appears it's not clear what
> > drivers should do when async flip isn't possible with the legacy uAPI.
> >
> > For the atomic uAPI, we need to pick on of these two approaches:
> >
> > 1. Let the kernel fall back to a sync flip if async isn't possible. This
> >    simplifies user-space, but then user-space has no reliable way to figure out
> >    what really happened (sync or async?). That could be fixed with a new
> >    read-only CRTC prop indicating whether the last flip was async or not.
> >    However, maybe someone will come up in the future with user-space which
> >    needs to only apply an update if async flip is possible, in which case this
> >    approach falls short.
> > 2. Make the kernel return EINVAL if async flip isn't possible. This adds more
> >    complexity to user-space, but enables a more reliable and deterministic
> >    uAPI. This is also more consistent with the rest of the existing atomic
> >    uAPI.
> 
> The current behaviour is somewhat a combination of the two. We return
> an error if async flip is not possible at all given the current state.
> 
> When async flip is possible we return success, but may still internally
> fall back to a sync flip for the first flip. That is required on some
> borked hardware that can't switch from sync flips to async flips without
> doing an extra sync flip. Also on some other hardware we intentionally
> fall back to a sync flip for the first async flip, so that we can
> reprogram some display FIFO stuff (aimed to make the subsequent async
> flips faster).

Hm. Would it be possible for the atomic uAPI to return EINVAL in this case too,
to let user-space know what really happened? I suppose user-space could then
(mistakenly) assume that async flip is never possible, and never try again?

> > Note, current user-space would only need to opportunistically enable async
> > flip. IOW, I think that for current user-space plans "async if possible,
> > otherwise sync" is good enough. That behavior maps well to the Vulkan present
> > modes as well (which says that "this mode *may* result in visible tearing", but
> > doesn't guarantee it).
> 
> The current behaviour is to fall back to a blit if the async
> flip fails. So you still get the same effective behaviour, just
> not as efficient. I think that's a reasonable way to handle it.

After some discussion on IRC: the above describes Xorg's behavior.

To reproduce this behavior with the atomic uAPI, it is necessary to use
approach (2): make the atomic commit fail if async flip isn't possible, to let
user-space fall back to a blit.

> > Another possible shortcoming of the proposed new uAPI here is that user-space
> > cannot submit a single atomic commit which updates multiple CRTCs, and
> > individually select which CRTC does an async flip. This could be fixed with
> > a "ASYNC_FLIP" CRTC prop which the kernel always resets to 0 on commit. I'm not
> > sure we want/need to cross that bridge right now, it would be easy enough to
> > add as a second step if some user-space would benefit from it.
> 
> Technically it should really be per-plane since that is what does
> the flip. But I have a feeling that allowing a mix of async and
> sync in the same commit is just going to make everything more
> complicated without really helping anything (async flips won't
> happen atomically anyway with anything else).

Agreed.

> One (crazy?) idea I had for the atomic api is that we could even
> reject most of the properties already on the uapi level before anyone
> gets to examine the final state. Ie. as soon as the atomic ioctl sees
> eg. a gamma LUT property change it would just immediately return
> an error if async flip is also requested.

Hm, I guess... Although amdgpu doesn't really need this, it already has all
of the logic checking for stuff like gamma LUT property change + async. Could
maybe be a DRM helper if other drivers need it.

Re: [PATCH 3/4] drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP

[Michel Dänzer]

On 2022-08-29 18:01, Simon Ser wrote:
> On Friday, August 26th, 2022 at 10:19, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> On Wed, Aug 24, 2022 at 03:08:55PM +0000, Simon Ser wrote:
>>> 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.
>>
>> I think we'd need to clarify the semantics of the async flag
>> for atomic commits.
>>
>> Eg. on Intel hw only pure page flips are possible async, if you do
>> anything else (change plane size/pos/scaling/etc.) you will need
>> to do a sync update. Technically not even all page flips (from the
>> uapi POV) might be possible as the exact scanout source address
>> is specified via two registers, only one of which can be update
>> async. So technically the two framebuffers might be laid out
>> just slightly differently which could prevent an async flip.
>> Also only some subset of planes actually support async flips.
> 
> Also IIRC some format modifiers don't support async flip at all (CCS)?
> 
>> And on hw where multiple planes support it on the same crtc, only one
>> plane can do it at a time. Well, more accurately we can only select
>> one plane at a time to give us the "flip done" interrupt. I guess
>> if the user wants to async flip multiple planes at the same time
>> we could do them serially as opposed to in parallel to make sure
>> all the flips actually happened before we signal completion of the
>> entire commit. Async flips of multiple planes probably won't
>> happen atomically anyway so doing them serially seems fine.
>>
>> ATM in i915 we probably don't have sufficient state checks in
>> place to catch all the restrictions, and instead in part we rely
>> on the limited scope of the legacy async flip ioctl to make sure
>> the operation doesn't attempt something the hw can't do.
> 
> Yeah, that makes sense.
> 
> In the documentation patch discussion [1], it appears it's not clear what
> drivers should do when async flip isn't possible with the legacy uAPI.
> 
> For the atomic uAPI, we need to pick on of these two approaches:
> 
> 1. Let the kernel fall back to a sync flip if async isn't possible. This
>    simplifies user-space, but then user-space has no reliable way to figure out
>    what really happened (sync or async?). That could be fixed with a new
>    read-only CRTC prop indicating whether the last flip was async or not.
>    However, maybe someone will come up in the future with user-space which
>    needs to only apply an update if async flip is possible, in which case this
>    approach falls short.

The future is now. :)

As I described in the documentation patch discussion, this approach would make it tricky for a Wayland compositor to decide if it should use an async commit (which needs to be done ASAP to serve the intended purpose) or not (in which case the compositor may want to delay the commit as long as possible for minimal latency).


> Another possible shortcoming of the proposed new uAPI here is that user-space
> cannot submit a single atomic commit which updates multiple CRTCs, and
> individually select which CRTC does an async flip. This could be fixed with
> a "ASYNC_FLIP" CRTC prop which the kernel always resets to 0 on commit. I'm not
> sure we want/need to cross that bridge right now, it would be easy enough to
> add as a second step if some user-space would benefit from it.

I thought about this as well, but I came to the conclusion it shouldn't be needed. User space can do one commit for the sync CRTC/s planes first and another commit for the async ones afterwards.


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer

Re: [PATCH 3/4] drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP

[Simon Ser]

(Oops, I replied to the wrong thread. Re-sending to the correct one.)

On Tuesday, August 30th, 2022 at 10:41, Michel Dänzer <michel.daenzer@mailbox.org> wrote:

> > For the atomic uAPI, we need to pick on of these two approaches:
> >
> > 1. Let the kernel fall back to a sync flip if async isn't possible. This
> >    simplifies user-space, but then user-space has no reliable way to figure out
> >    what really happened (sync or async?). That could be fixed with a new
> >    read-only CRTC prop indicating whether the last flip was async or not.
> >    However, maybe someone will come up in the future with user-space which
> >    needs to only apply an update if async flip is possible, in which case this
> >    approach falls short.
>
> The future is now. :)
>
> As I described in the documentation patch discussion, this approach would
> make it tricky for a Wayland compositor to decide if it should use an async
> commit (which needs to be done ASAP to serve the intended purpose) or not (in
> which case the compositor may want to delay the commit as long as possible
> for minimal latency).

Ah right. If an async page-flip is not possible, then a Wayland compositor
could want to wait the "last moment" before the next vblank to see if a more
up-to-date buffer is available.

With that + Xorg usage, I think we have a rather solid case for failing async
flips rather than silently falling back to vsync.

[PATCH 4/4] amd/display: indicate support for atomic async page-flips on DCN

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

Note, async page-flips are still unsupported on DCE with the atomic
uAPI. The mode_set_base callbacks unconditionally set the
GRPH_SURFACE_UPDATE_H_RETRACE_EN field of the GRPH_FLIP_CONTROL
register to 0, which disables async page-flips.

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Joshua Ashton <joshua@froggi.es>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Alex Deucher <alexander.deucher@amd.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 ef816bf295eb..9ab01c58bedb 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3804,7 +3804,6 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
 	adev_to_drm(adev)->mode_config.prefer_shadow = 0;
 	/* 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.2

Re: [PATCH 4/4] amd/display: indicate support for atomic async page-flips on DCN

[Alex Deucher]

On Wed, Aug 24, 2022 at 11:09 AM Simon Ser <contact@emersion.fr> wrote:
>
> 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.
>
> Note, async page-flips are still unsupported on DCE with the atomic
> uAPI. The mode_set_base callbacks unconditionally set the
> GRPH_SURFACE_UPDATE_H_RETRACE_EN field of the GRPH_FLIP_CONTROL
> register to 0, which disables async page-flips.

Can you elaborate a bit on this? We don't use hsync flips at all, even
in non-atomic, as far as I recall.  The hardware can also do immediate
flips which take effect as soon as you update the base address
register which is what we use for async updates today IIRC.

Alex

>
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Joshua Ashton <joshua@froggi.es>
> Cc: Melissa Wen <mwen@igalia.com>
> Cc: Alex Deucher <alexander.deucher@amd.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 ef816bf295eb..9ab01c58bedb 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3804,7 +3804,6 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
>         adev_to_drm(adev)->mode_config.prefer_shadow = 0;
>         /* 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.2
>
>

Re: [PATCH 4/4] amd/display: indicate support for atomic async page-flips on DCN

[Simon Ser]

On Thursday, August 25th, 2022 at 20:22, Alex Deucher <alexdeucher@gmail.com> wrote:

> On Wed, Aug 24, 2022 at 11:09 AM Simon Ser contact@emersion.fr wrote:
> 
> > 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.
> > 
> > Note, async page-flips are still unsupported on DCE with the atomic
> > uAPI. The mode_set_base callbacks unconditionally set the
> > GRPH_SURFACE_UPDATE_H_RETRACE_EN field of the GRPH_FLIP_CONTROL
> > register to 0, which disables async page-flips.
> 
> Can you elaborate a bit on this? We don't use hsync flips at all, even
> in non-atomic, as far as I recall. The hardware can also do immediate
> flips which take effect as soon as you update the base address
> register which is what we use for async updates today IIRC.

When user-space performs a page-flip with the legacy KMS uAPI on DCE
ASICs, amdgpu_display_crtc_page_flip_target() is called. This function
checks for the DRM_MODE_PAGE_FLIP_ASYNC flag, sets work->async, which
is then passed as an argument to adev->mode_info.funcs->page_flip() by
amdgpu_display_flip_work_func(). Looking at an implementation, for
instance dce_v10_0_page_flip(), the async flag is used to set that
GRPH_FLIP_CONTROL register:

	/* flip at hsync for async, default is vsync */
	tmp = RREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset);
	tmp = REG_SET_FIELD(tmp, GRPH_FLIP_CONTROL,
			    GRPH_SURFACE_UPDATE_H_RETRACE_EN, async ? 1 : 0);
	WREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset, tmp);

I don't know how the hardware works, but I assumed it would be
necessary to do the same in the atomic uAPI code-path as well. However
dce_v10_0_crtc_do_set_base() has this code block:

	/* Make sure surface address is updated at vertical blank rather than
	 * horizontal blank
	 */
	tmp = RREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset);
	tmp = REG_SET_FIELD(tmp, GRPH_FLIP_CONTROL,
			    GRPH_SURFACE_UPDATE_H_RETRACE_EN, 0);
	WREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset, tmp);

Which unconditionally sets that same register.

Either way, it's not a very big deal for this patch series, DCE and DCN
are separate, DCE can be sorted out separately.

Am I completely mistaken here?

Thanks,

Simon

Re: [PATCH 4/4] amd/display: indicate support for atomic async page-flips on DCN

[Alex Deucher]

On Fri, Aug 26, 2022 at 3:38 AM Simon Ser <contact@emersion.fr> wrote:
>
> On Thursday, August 25th, 2022 at 20:22, Alex Deucher <alexdeucher@gmail.com> wrote:
>
> > On Wed, Aug 24, 2022 at 11:09 AM Simon Ser contact@emersion.fr wrote:
> >
> > > 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.
> > >
> > > Note, async page-flips are still unsupported on DCE with the atomic
> > > uAPI. The mode_set_base callbacks unconditionally set the
> > > GRPH_SURFACE_UPDATE_H_RETRACE_EN field of the GRPH_FLIP_CONTROL
> > > register to 0, which disables async page-flips.
> >
> > Can you elaborate a bit on this? We don't use hsync flips at all, even
> > in non-atomic, as far as I recall. The hardware can also do immediate
> > flips which take effect as soon as you update the base address
> > register which is what we use for async updates today IIRC.
>
> When user-space performs a page-flip with the legacy KMS uAPI on DCE
> ASICs, amdgpu_display_crtc_page_flip_target() is called. This function
> checks for the DRM_MODE_PAGE_FLIP_ASYNC flag, sets work->async, which
> is then passed as an argument to adev->mode_info.funcs->page_flip() by
> amdgpu_display_flip_work_func(). Looking at an implementation, for
> instance dce_v10_0_page_flip(), the async flag is used to set that
> GRPH_FLIP_CONTROL register:
>
>         /* flip at hsync for async, default is vsync */
>         tmp = RREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset);
>         tmp = REG_SET_FIELD(tmp, GRPH_FLIP_CONTROL,
>                             GRPH_SURFACE_UPDATE_H_RETRACE_EN, async ? 1 : 0);
>         WREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset, tmp);
>
> I don't know how the hardware works, but I assumed it would be
> necessary to do the same in the atomic uAPI code-path as well. However
> dce_v10_0_crtc_do_set_base() has this code block:
>
>         /* Make sure surface address is updated at vertical blank rather than
>          * horizontal blank
>          */
>         tmp = RREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset);
>         tmp = REG_SET_FIELD(tmp, GRPH_FLIP_CONTROL,
>                             GRPH_SURFACE_UPDATE_H_RETRACE_EN, 0);
>         WREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset, tmp);
>
> Which unconditionally sets that same register.
>
> Either way, it's not a very big deal for this patch series, DCE and DCN
> are separate, DCE can be sorted out separately.
>
> Am I completely mistaken here?

I checked the code and it looks like only DCE11 and newer support
immediate flips.  E.g.,

        /* flip immediate for async, default is vsync */
        tmp = RREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset);
        tmp = REG_SET_FIELD(tmp, GRPH_FLIP_CONTROL,
                            GRPH_SURFACE_UPDATE_IMMEDIATE_EN, async ? 1 : 0);

in dce_v11_0.c.

Either way, the non-DC display code is not atomic anyway, so I don't
think this is an issue.  We still support async flips via the
non-atomic API.  I agree this is not blocking for the patch series,
just thinking out loud mostly.

Alex

>
> Thanks,
>
> Simon

Re: [PATCH 4/4] amd/display: indicate support for atomic async page-flips on DCN

[Simon Ser]

On Friday, August 26th, 2022 at 16:39, Alex Deucher <alexdeucher@gmail.com> wrote:

> On Fri, Aug 26, 2022 at 3:38 AM Simon Ser <contact@emersion.fr> wrote:
> >
> > On Thursday, August 25th, 2022 at 20:22, Alex Deucher <alexdeucher@gmail.com> wrote:
> >
> > > On Wed, Aug 24, 2022 at 11:09 AM Simon Ser contact@emersion.fr wrote:
> > >
> > > > 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.
> > > >
> > > > Note, async page-flips are still unsupported on DCE with the atomic
> > > > uAPI. The mode_set_base callbacks unconditionally set the
> > > > GRPH_SURFACE_UPDATE_H_RETRACE_EN field of the GRPH_FLIP_CONTROL
> > > > register to 0, which disables async page-flips.
> > >
> > > Can you elaborate a bit on this? We don't use hsync flips at all, even
> > > in non-atomic, as far as I recall. The hardware can also do immediate
> > > flips which take effect as soon as you update the base address
> > > register which is what we use for async updates today IIRC.
> >
> > When user-space performs a page-flip with the legacy KMS uAPI on DCE
> > ASICs, amdgpu_display_crtc_page_flip_target() is called. This function
> > checks for the DRM_MODE_PAGE_FLIP_ASYNC flag, sets work->async, which
> > is then passed as an argument to adev->mode_info.funcs->page_flip() by
> > amdgpu_display_flip_work_func(). Looking at an implementation, for
> > instance dce_v10_0_page_flip(), the async flag is used to set that
> > GRPH_FLIP_CONTROL register:
> >
> >         /* flip at hsync for async, default is vsync */
> >         tmp = RREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset);
> >         tmp = REG_SET_FIELD(tmp, GRPH_FLIP_CONTROL,
> >                             GRPH_SURFACE_UPDATE_H_RETRACE_EN, async ? 1 : 0);
> >         WREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset, tmp);
> >
> > I don't know how the hardware works, but I assumed it would be
> > necessary to do the same in the atomic uAPI code-path as well. However
> > dce_v10_0_crtc_do_set_base() has this code block:
> >
> >         /* Make sure surface address is updated at vertical blank rather than
> >          * horizontal blank
> >          */
> >         tmp = RREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset);
> >         tmp = REG_SET_FIELD(tmp, GRPH_FLIP_CONTROL,
> >                             GRPH_SURFACE_UPDATE_H_RETRACE_EN, 0);
> >         WREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset, tmp);
> >
> > Which unconditionally sets that same register.
> >
> > Either way, it's not a very big deal for this patch series, DCE and DCN
> > are separate, DCE can be sorted out separately.
> >
> > Am I completely mistaken here?
> 
> I checked the code and it looks like only DCE11 and newer support
> immediate flips.  E.g.,
> 
>         /* flip immediate for async, default is vsync */
>         tmp = RREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset);
>         tmp = REG_SET_FIELD(tmp, GRPH_FLIP_CONTROL,
>                             GRPH_SURFACE_UPDATE_IMMEDIATE_EN, async ? 1 : 0);
> 
> in dce_v11_0.c.
> 
> Either way, the non-DC display code is not atomic anyway, so I don't
> think this is an issue.  We still support async flips via the
> non-atomic API.  I agree this is not blocking for the patch series,
> just thinking out loud mostly.

Michel pointed out that DC can drive both DCN and DCE. This was a
misunderstanding on my end, I thought DC could only drive DCN. I'll reword the
commit message to refer to DC instead of DCN.

This begs the question, should we bother to set the
atomic_async_page_flip_not_supported flag on non-atomic drivers? I've just
slapped the flag everywhere for simplicity's sake, but maybe it would make more
sense to just set it for atomic-capable drivers. Especially if the long-term
goal is to convert all atomic drivers to support async flips and eventually
remove atomic_async_page_flip_not_supported.

Thanks for the hint regarding DCE10. It sounds like it may be worthwhile to
unset drm_mode_config.async_page_flip on DCE10 and earlier, to indicate to
user-space that async page-flips are not supported on these ASICs? Right now it
seems like we indicate that we support them, and then ignore the ASYNC_FLIP
flag?

Re: [PATCH 4/4] amd/display: indicate support for atomic async page-flips on DCN

[Alex Deucher]

On Tue, Aug 30, 2022 at 3:08 AM Simon Ser <contact@emersion.fr> wrote:
>
> On Friday, August 26th, 2022 at 16:39, Alex Deucher <alexdeucher@gmail.com> wrote:
>
> > On Fri, Aug 26, 2022 at 3:38 AM Simon Ser <contact@emersion.fr> wrote:
> > >
> > > On Thursday, August 25th, 2022 at 20:22, Alex Deucher <alexdeucher@gmail.com> wrote:
> > >
> > > > On Wed, Aug 24, 2022 at 11:09 AM Simon Ser contact@emersion.fr wrote:
> > > >
> > > > > 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.
> > > > >
> > > > > Note, async page-flips are still unsupported on DCE with the atomic
> > > > > uAPI. The mode_set_base callbacks unconditionally set the
> > > > > GRPH_SURFACE_UPDATE_H_RETRACE_EN field of the GRPH_FLIP_CONTROL
> > > > > register to 0, which disables async page-flips.
> > > >
> > > > Can you elaborate a bit on this? We don't use hsync flips at all, even
> > > > in non-atomic, as far as I recall. The hardware can also do immediate
> > > > flips which take effect as soon as you update the base address
> > > > register which is what we use for async updates today IIRC.
> > >
> > > When user-space performs a page-flip with the legacy KMS uAPI on DCE
> > > ASICs, amdgpu_display_crtc_page_flip_target() is called. This function
> > > checks for the DRM_MODE_PAGE_FLIP_ASYNC flag, sets work->async, which
> > > is then passed as an argument to adev->mode_info.funcs->page_flip() by
> > > amdgpu_display_flip_work_func(). Looking at an implementation, for
> > > instance dce_v10_0_page_flip(), the async flag is used to set that
> > > GRPH_FLIP_CONTROL register:
> > >
> > >         /* flip at hsync for async, default is vsync */
> > >         tmp = RREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset);
> > >         tmp = REG_SET_FIELD(tmp, GRPH_FLIP_CONTROL,
> > >                             GRPH_SURFACE_UPDATE_H_RETRACE_EN, async ? 1 : 0);
> > >         WREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset, tmp);
> > >
> > > I don't know how the hardware works, but I assumed it would be
> > > necessary to do the same in the atomic uAPI code-path as well. However
> > > dce_v10_0_crtc_do_set_base() has this code block:
> > >
> > >         /* Make sure surface address is updated at vertical blank rather than
> > >          * horizontal blank
> > >          */
> > >         tmp = RREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset);
> > >         tmp = REG_SET_FIELD(tmp, GRPH_FLIP_CONTROL,
> > >                             GRPH_SURFACE_UPDATE_H_RETRACE_EN, 0);
> > >         WREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset, tmp);
> > >
> > > Which unconditionally sets that same register.
> > >
> > > Either way, it's not a very big deal for this patch series, DCE and DCN
> > > are separate, DCE can be sorted out separately.
> > >
> > > Am I completely mistaken here?
> >
> > I checked the code and it looks like only DCE11 and newer support
> > immediate flips.  E.g.,
> >
> >         /* flip immediate for async, default is vsync */
> >         tmp = RREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset);
> >         tmp = REG_SET_FIELD(tmp, GRPH_FLIP_CONTROL,
> >                             GRPH_SURFACE_UPDATE_IMMEDIATE_EN, async ? 1 : 0);
> >
> > in dce_v11_0.c.
> >
> > Either way, the non-DC display code is not atomic anyway, so I don't
> > think this is an issue.  We still support async flips via the
> > non-atomic API.  I agree this is not blocking for the patch series,
> > just thinking out loud mostly.
>
> Michel pointed out that DC can drive both DCN and DCE. This was a
> misunderstanding on my end, I thought DC could only drive DCN. I'll reword the
> commit message to refer to DC instead of DCN.
>
> This begs the question, should we bother to set the
> atomic_async_page_flip_not_supported flag on non-atomic drivers? I've just
> slapped the flag everywhere for simplicity's sake, but maybe it would make more
> sense to just set it for atomic-capable drivers. Especially if the long-term
> goal is to convert all atomic drivers to support async flips and eventually
> remove atomic_async_page_flip_not_supported.

yeah, I think we can drop the flag for non-atomic.  amdgpu at least
already supports async flips.

>
> Thanks for the hint regarding DCE10. It sounds like it may be worthwhile to
> unset drm_mode_config.async_page_flip on DCE10 and earlier, to indicate to
> user-space that async page-flips are not supported on these ASICs? Right now it
> seems like we indicate that we support them, and then ignore the ASYNC_FLIP
> flag?

Async flips work fine with the current code.  I think I did the
initial implementation on DCE10.  We set
GRPH_SURFACE_UPDATE_H_RETRACE_EN dynamically in dce_v10_0_page_flip()
based on the type of flip selected.

Alex

Re: [PATCH 4/4] amd/display: indicate support for atomic async page-flips on DCN

[Simon Ser]

On Tuesday, August 30th, 2022 at 16:06, Alex Deucher <alexdeucher@gmail.com> wrote:

> On Tue, Aug 30, 2022 at 3:08 AM Simon Ser contact@emersion.fr wrote:
> 
> > On Friday, August 26th, 2022 at 16:39, Alex Deucher alexdeucher@gmail.com wrote:
> > 
> > > On Fri, Aug 26, 2022 at 3:38 AM Simon Ser contact@emersion.fr wrote:
> > > 
> > > > On Thursday, August 25th, 2022 at 20:22, Alex Deucher alexdeucher@gmail.com wrote:
> > > > 
> > > > > On Wed, Aug 24, 2022 at 11:09 AM Simon Ser contact@emersion.fr wrote:
> > > > > 
> > > > > > 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.
> > > > > > 
> > > > > > Note, async page-flips are still unsupported on DCE with the atomic
> > > > > > uAPI. The mode_set_base callbacks unconditionally set the
> > > > > > GRPH_SURFACE_UPDATE_H_RETRACE_EN field of the GRPH_FLIP_CONTROL
> > > > > > register to 0, which disables async page-flips.
> > > > > 
> > > > > Can you elaborate a bit on this? We don't use hsync flips at all, even
> > > > > in non-atomic, as far as I recall. The hardware can also do immediate
> > > > > flips which take effect as soon as you update the base address
> > > > > register which is what we use for async updates today IIRC.
> > > > 
> > > > When user-space performs a page-flip with the legacy KMS uAPI on DCE
> > > > ASICs, amdgpu_display_crtc_page_flip_target() is called. This function
> > > > checks for the DRM_MODE_PAGE_FLIP_ASYNC flag, sets work->async, which
> > > > is then passed as an argument to adev->mode_info.funcs->page_flip() by
> > > > amdgpu_display_flip_work_func(). Looking at an implementation, for
> > > > instance dce_v10_0_page_flip(), the async flag is used to set that
> > > > GRPH_FLIP_CONTROL register:
> > > > 
> > > > /* flip at hsync for async, default is vsync */
> > > > tmp = RREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset);
> > > > tmp = REG_SET_FIELD(tmp, GRPH_FLIP_CONTROL,
> > > > GRPH_SURFACE_UPDATE_H_RETRACE_EN, async ? 1 : 0);
> > > > WREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset, tmp);
> > > > 
> > > > I don't know how the hardware works, but I assumed it would be
> > > > necessary to do the same in the atomic uAPI code-path as well. However
> > > > dce_v10_0_crtc_do_set_base() has this code block:
> > > > 
> > > > /* Make sure surface address is updated at vertical blank rather than
> > > > * horizontal blank
> > > > */
> > > > tmp = RREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset);
> > > > tmp = REG_SET_FIELD(tmp, GRPH_FLIP_CONTROL,
> > > > GRPH_SURFACE_UPDATE_H_RETRACE_EN, 0);
> > > > WREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset, tmp);
> > > > 
> > > > Which unconditionally sets that same register.
> > > > 
> > > > Either way, it's not a very big deal for this patch series, DCE and DCN
> > > > are separate, DCE can be sorted out separately.
> > > > 
> > > > Am I completely mistaken here?
> > > 
> > > I checked the code and it looks like only DCE11 and newer support
> > > immediate flips. E.g.,
> > > 
> > > /* flip immediate for async, default is vsync */
> > > tmp = RREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset);
> > > tmp = REG_SET_FIELD(tmp, GRPH_FLIP_CONTROL,
> > > GRPH_SURFACE_UPDATE_IMMEDIATE_EN, async ? 1 : 0);
> > > 
> > > in dce_v11_0.c.
> > > 
> > > Either way, the non-DC display code is not atomic anyway, so I don't
> > > think this is an issue. We still support async flips via the
> > > non-atomic API. I agree this is not blocking for the patch series,
> > > just thinking out loud mostly.
> > 
> > Michel pointed out that DC can drive both DCN and DCE. This was a
> > misunderstanding on my end, I thought DC could only drive DCN. I'll reword the
> > commit message to refer to DC instead of DCN.
> > 
> > This begs the question, should we bother to set the
> > atomic_async_page_flip_not_supported flag on non-atomic drivers? I've just
> > slapped the flag everywhere for simplicity's sake, but maybe it would make more
> > sense to just set it for atomic-capable drivers. Especially if the long-term
> > goal is to convert all atomic drivers to support async flips and eventually
> > remove atomic_async_page_flip_not_supported.
> 
> yeah, I think we can drop the flag for non-atomic. amdgpu at least
> already supports async flips.
> 
> > Thanks for the hint regarding DCE10. It sounds like it may be worthwhile to
> > unset drm_mode_config.async_page_flip on DCE10 and earlier, to indicate to
> > user-space that async page-flips are not supported on these ASICs? Right now it
> > seems like we indicate that we support them, and then ignore the ASYNC_FLIP
> > flag?
> 
> Async flips work fine with the current code. I think I did the
> initial implementation on DCE10. We set
> GRPH_SURFACE_UPDATE_H_RETRACE_EN dynamically in dce_v10_0_page_flip()
> based on the type of flip selected.

Hm, can you elaborate on the difference between "immediate flip" (as in
UNP_FLIP_CONTROL) and GRPH_SURFACE_UPDATE_H_RETRACE_EN? What are their
relationship with KMS's concept of "async flips"?

Also you said earlier:

> We don't use hsync flips at all, even in non-atomic, as far as I recall.

Is "hsync flip" controlled by GRPH_SURFACE_UPDATE_H_RETRACE_EN, or is it
something else entirely?

Re: [PATCH 4/4] amd/display: indicate support for atomic async page-flips on DCN

[Alex Deucher]

On Tue, Aug 30, 2022 at 10:24 AM Simon Ser <contact@emersion.fr> wrote:
>
> On Tuesday, August 30th, 2022 at 16:06, Alex Deucher <alexdeucher@gmail.com> wrote:
>
> > On Tue, Aug 30, 2022 at 3:08 AM Simon Ser contact@emersion.fr wrote:
> >
> > > On Friday, August 26th, 2022 at 16:39, Alex Deucher alexdeucher@gmail.com wrote:
> > >
> > > > On Fri, Aug 26, 2022 at 3:38 AM Simon Ser contact@emersion.fr wrote:
> > > >
> > > > > On Thursday, August 25th, 2022 at 20:22, Alex Deucher alexdeucher@gmail.com wrote:
> > > > >
> > > > > > On Wed, Aug 24, 2022 at 11:09 AM Simon Ser contact@emersion.fr wrote:
> > > > > >
> > > > > > > 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.
> > > > > > >
> > > > > > > Note, async page-flips are still unsupported on DCE with the atomic
> > > > > > > uAPI. The mode_set_base callbacks unconditionally set the
> > > > > > > GRPH_SURFACE_UPDATE_H_RETRACE_EN field of the GRPH_FLIP_CONTROL
> > > > > > > register to 0, which disables async page-flips.
> > > > > >
> > > > > > Can you elaborate a bit on this? We don't use hsync flips at all, even
> > > > > > in non-atomic, as far as I recall. The hardware can also do immediate
> > > > > > flips which take effect as soon as you update the base address
> > > > > > register which is what we use for async updates today IIRC.
> > > > >
> > > > > When user-space performs a page-flip with the legacy KMS uAPI on DCE
> > > > > ASICs, amdgpu_display_crtc_page_flip_target() is called. This function
> > > > > checks for the DRM_MODE_PAGE_FLIP_ASYNC flag, sets work->async, which
> > > > > is then passed as an argument to adev->mode_info.funcs->page_flip() by
> > > > > amdgpu_display_flip_work_func(). Looking at an implementation, for
> > > > > instance dce_v10_0_page_flip(), the async flag is used to set that
> > > > > GRPH_FLIP_CONTROL register:
> > > > >
> > > > > /* flip at hsync for async, default is vsync */
> > > > > tmp = RREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset);
> > > > > tmp = REG_SET_FIELD(tmp, GRPH_FLIP_CONTROL,
> > > > > GRPH_SURFACE_UPDATE_H_RETRACE_EN, async ? 1 : 0);
> > > > > WREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset, tmp);
> > > > >
> > > > > I don't know how the hardware works, but I assumed it would be
> > > > > necessary to do the same in the atomic uAPI code-path as well. However
> > > > > dce_v10_0_crtc_do_set_base() has this code block:
> > > > >
> > > > > /* Make sure surface address is updated at vertical blank rather than
> > > > > * horizontal blank
> > > > > */
> > > > > tmp = RREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset);
> > > > > tmp = REG_SET_FIELD(tmp, GRPH_FLIP_CONTROL,
> > > > > GRPH_SURFACE_UPDATE_H_RETRACE_EN, 0);
> > > > > WREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset, tmp);
> > > > >
> > > > > Which unconditionally sets that same register.
> > > > >
> > > > > Either way, it's not a very big deal for this patch series, DCE and DCN
> > > > > are separate, DCE can be sorted out separately.
> > > > >
> > > > > Am I completely mistaken here?
> > > >
> > > > I checked the code and it looks like only DCE11 and newer support
> > > > immediate flips. E.g.,
> > > >
> > > > /* flip immediate for async, default is vsync */
> > > > tmp = RREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset);
> > > > tmp = REG_SET_FIELD(tmp, GRPH_FLIP_CONTROL,
> > > > GRPH_SURFACE_UPDATE_IMMEDIATE_EN, async ? 1 : 0);
> > > >
> > > > in dce_v11_0.c.
> > > >
> > > > Either way, the non-DC display code is not atomic anyway, so I don't
> > > > think this is an issue. We still support async flips via the
> > > > non-atomic API. I agree this is not blocking for the patch series,
> > > > just thinking out loud mostly.
> > >
> > > Michel pointed out that DC can drive both DCN and DCE. This was a
> > > misunderstanding on my end, I thought DC could only drive DCN. I'll reword the
> > > commit message to refer to DC instead of DCN.
> > >
> > > This begs the question, should we bother to set the
> > > atomic_async_page_flip_not_supported flag on non-atomic drivers? I've just
> > > slapped the flag everywhere for simplicity's sake, but maybe it would make more
> > > sense to just set it for atomic-capable drivers. Especially if the long-term
> > > goal is to convert all atomic drivers to support async flips and eventually
> > > remove atomic_async_page_flip_not_supported.
> >
> > yeah, I think we can drop the flag for non-atomic. amdgpu at least
> > already supports async flips.
> >
> > > Thanks for the hint regarding DCE10. It sounds like it may be worthwhile to
> > > unset drm_mode_config.async_page_flip on DCE10 and earlier, to indicate to
> > > user-space that async page-flips are not supported on these ASICs? Right now it
> > > seems like we indicate that we support them, and then ignore the ASYNC_FLIP
> > > flag?
> >
> > Async flips work fine with the current code. I think I did the
> > initial implementation on DCE10. We set
> > GRPH_SURFACE_UPDATE_H_RETRACE_EN dynamically in dce_v10_0_page_flip()
> > based on the type of flip selected.
>
> Hm, can you elaborate on the difference between "immediate flip" (as in
> UNP_FLIP_CONTROL) and GRPH_SURFACE_UPDATE_H_RETRACE_EN? What are their
> relationship with KMS's concept of "async flips"?

The display surface registers are double buffered.  The default is for
the swap to take place during vblank.  However, you can select
different behavior via the GRPH_FLIP_CONTROL register.  On DCE10 and
older you can set GRPH_SURFACE_UPDATE_H_RETRACE_EN to select swapping
at hsync.  On DCE11 and newer, you can set
GRPH_SURFACE_UPDATE_IMMEDIATE_EN which causes the swap to immediately
(IIRC as soon as GRPH_PRIMARY_SURFACE_ADDRESS is written).

>
> Also you said earlier:
>
> > We don't use hsync flips at all, even in non-atomic, as far as I recall.
>
> Is "hsync flip" controlled by GRPH_SURFACE_UPDATE_H_RETRACE_EN, or is it
> something else entirely?

Yes, GRPH_SURFACE_UPDATE_H_RETRACE_EN.  We use hsync swaps on older
DCE parts that don't support immediate swaps.

Alex

Re: [PATCH 4/4] amd/display: indicate support for atomic async page-flips on DCN

[Simon Ser]

On Tuesday, August 30th, 2022 at 16:42, Alex Deucher <alexdeucher@gmail.com> wrote:

> > Hm, can you elaborate on the difference between "immediate flip" (as in
> > UNP_FLIP_CONTROL) and GRPH_SURFACE_UPDATE_H_RETRACE_EN? What are their
> > relationship with KMS's concept of "async flips"?
> 
> The display surface registers are double buffered. The default is for
> the swap to take place during vblank. However, you can select
> different behavior via the GRPH_FLIP_CONTROL register. On DCE10 and
> older you can set GRPH_SURFACE_UPDATE_H_RETRACE_EN to select swapping
> at hsync. On DCE11 and newer, you can set
> GRPH_SURFACE_UPDATE_IMMEDIATE_EN which causes the swap to immediately
> (IIRC as soon as GRPH_PRIMARY_SURFACE_ADDRESS is written).
> 
> > Also you said earlier:
> > 
> > > We don't use hsync flips at all, even in non-atomic, as far as I recall.
> > 
> > Is "hsync flip" controlled by GRPH_SURFACE_UPDATE_H_RETRACE_EN, or is it
> > something else entirely?
> 
> Yes, GRPH_SURFACE_UPDATE_H_RETRACE_EN. We use hsync swaps on older
> DCE parts that don't support immediate swaps.

Ah, that makes a lot of sense. Thanks for the explanation!

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

[Melissa Wen]

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

On 08/24, 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.

Hi Simon,

I'm cc'ing André as he has been actively working on it lately and must
be quite familiar with the async flip machinery.

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

Also, André recently generalized the kms_async_flip to test drivers
other than i915, so I think he can provide some thoughts about the IGT
test too.

Thanks,

Melissa

> 
> Tested on an AMD Picasso iGPU.
> 
> Simon Ser (4):
>   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 DCN
> 
>  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c       |  1 +
>  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c       |  1 +
>  drivers/gpu/drm/amd/amdgpu/dce_v6_0.c        |  1 +
>  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c        |  1 +
>  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/radeon/radeon_display.c      |  1 +
>  drivers/gpu/drm/vc4/vc4_kms.c                |  1 +
>  include/drm/drm_mode_config.h                | 11 ++++++++
>  include/uapi/drm/drm.h                       | 10 ++++++-
>  13 files changed, 59 insertions(+), 4 deletions(-)
> 
> -- 
> 2.37.2
> 
> 

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