[PATCH 0/1] amd/display: only accept async flips for fast updates

[PATCH 0/1] amd/display: only accept async flips for fast updates

[André Almeida]

Hi,

This fix was developed with the "Add support for atomic async page-flips"
patchset, it's not dependent on the new feature but it wasn't applied by
the time so I'm resending it now.

Extracted from: https://lore.kernel.org/dri-devel/20220929184307.258331-3-contact@emersion.fr/

Simon Ser (1):
  amd/display: only accept async flips for fast updates

 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(+)

-- 
2.41.0

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

[André Almeida]

From: Simon Ser <contact@emersion.fr>

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.

Signed-off-by: Simon Ser <contact@emersion.fr>
Reviewed-by: André Almeida <andrealmeid@igalia.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c    |  8 ++++++++
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c   | 12 ++++++++++++
 2 files changed, 20 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 514f6785a020..1d9b84e5835f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -8136,7 +8136,15 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 		 * Only allow immediate flips for fast updates that don't
 		 * change memory domain, FB pitch, DCC state, rotation or
 		 * mirroring.
+		 *
+		 * 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 440fc0869a34..30d4c6fd95f5 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
@@ -398,6 +398,18 @@ 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.41.0

Re: [PATCH] amd/display: only accept async flips for fast updates

[Harry Wentland]

On 2023-06-21 16:24, André Almeida wrote:
> From: Simon Ser <contact@emersion.fr>
> 
> 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.
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Reviewed-by: André Almeida <andrealmeid@igalia.com>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: André Almeida <andrealmeid@igalia.com>

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c    |  8 ++++++++
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c   | 12 ++++++++++++
>  2 files changed, 20 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 514f6785a020..1d9b84e5835f 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -8136,7 +8136,15 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>  		 * Only allow immediate flips for fast updates that don't
>  		 * change memory domain, FB pitch, DCC state, rotation or
>  		 * mirroring.
> +		 *
> +		 * 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 440fc0869a34..30d4c6fd95f5 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
> @@ -398,6 +398,18 @@ 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;

Re: [PATCH] amd/display: only accept async flips for fast updates

[Hamza Mahfooz]

On 6/21/23 16:24, André Almeida wrote:
> From: Simon Ser <contact@emersion.fr>
> 
> 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.
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Reviewed-by: André Almeida <andrealmeid@igalia.com>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: André Almeida <andrealmeid@igalia.com>

Applied, thanks!

> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c    |  8 ++++++++
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c   | 12 ++++++++++++
>   2 files changed, 20 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 514f6785a020..1d9b84e5835f 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -8136,7 +8136,15 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>   		 * Only allow immediate flips for fast updates that don't
>   		 * change memory domain, FB pitch, DCC state, rotation or
>   		 * mirroring.
> +		 *
> +		 * 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 440fc0869a34..30d4c6fd95f5 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
> @@ -398,6 +398,18 @@ 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;
-- 
Hamza