[PATCH] drm/i915/display: Ignore disabled planes in async flip checks

[PATCH] drm/i915/display: Ignore disabled planes in async flip checks

[Erwin Springer via B4 Relay]

From: Erwin Springer <aot.lerwin@gmail.com>

Atomic async flips may carry properties that don't change, so compositors
tend to send their whole plane state with every commit. KWin for example
adds all planes of the pipe, including the ones that are disabled and stay
disabled (CRTC_ID=0, FB_ID=0).

intel_async_flip_check_uapi() fails the commit as soon as the state
contains any plane of the pipe that can't do async flips:

  [drm:intel_atomic_check [i915]] [PLANE:66:plane 2A] async flip not supported
  [drm:drm_atomic_check_only.cold] atomic driver check for ... failed: -22

So with such a compositor tearing never works on a pipe with sprite or
cursor planes, even though only the FB of the primary plane changes. KWin
silently falls back to vsynced flips, about 60 rejected commits per second.

A plane without a framebuffer before and after the commit takes no part in
the flip. Skip it in both async flip checks.

Tested on Raptor Lake-U (Iris Xe) with the patch applied to the i915 of
Linux 7.3.4, unmodified KWin 6.8.2 and a fullscreen client that requests
tearing through wp_tearing_control_v1. Before, none of about 350 frames per
run was presented without vsync. With the patch 3334 of 3340 frames tear
and the message above is gone. What is still rejected is the first flip
after the modifier or format changed, once per run, as intended.

Signed-off-by: Erwin Springer <aot.lerwin@gmail.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index f4d9148ef..58f45dd68 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -6281,6 +6281,14 @@ static int intel_async_flip_check_uapi(struct intel_atomic_state *state,
 		if (plane->pipe != crtc->pipe)
 			continue;
 
+		/*
+		 * Userspace may add planes it doesn't change to the commit.
+		 * A plane that is disabled and stays disabled takes no part
+		 * in the async flip.
+		 */
+		if (!old_plane_state->uapi.fb && !new_plane_state->uapi.fb)
+			continue;
+
 		/*
 		 * TODO: Async flip is only supported through the page flip IOCTL
 		 * as of now. So support currently added for primary plane only.
@@ -6346,6 +6354,10 @@ static int intel_async_flip_check_hw(struct intel_atomic_state *state, struct in
 		if (plane->pipe != crtc->pipe)
 			continue;
 
+		/* Disabled planes that stay disabled take no part in the flip. */
+		if (!old_plane_state->hw.fb && !new_plane_state->hw.fb)
+			continue;
+
 		/*
 		 * Only async flip capable planes should be in the state
 		 * if we're really about to ask the hardware to perform

---
base-commit: 419bb0e4cadd214a0847010c02ade95e5fc511f7
change-id: 20261115-i915-async-planes-d532ca94cf3c

Best regards,
--  
Erwin Springer <aot.lerwin@gmail.com>

[PATCH] drm/i915/display: Ignore disabled planes in async flip checks

[Erwin Springer]

Atomic async flips may carry properties that don't change, so compositors
tend to send their whole plane state with every commit. KWin for example
adds all planes of the pipe, including the ones that are disabled and stay
disabled (CRTC_ID=0, FB_ID=0).

intel_async_flip_check_uapi() fails the commit as soon as the state
contains any plane of the pipe that can't do async flips:

  [drm:intel_atomic_check [i915]] [PLANE:66:plane 2A] async flip not supported
  [drm:drm_atomic_check_only.cold] atomic driver check for ... failed: -22

So with such a compositor tearing never works on a pipe with sprite or
cursor planes, even though only the FB of the primary plane changes. KWin
silently falls back to vsynced flips, about 60 rejected commits per second.

A plane without a framebuffer before and after the commit takes no part in
the flip. Skip it in both async flip checks.

Tested on Raptor Lake-U (Iris Xe) with the patch applied to the i915 of
Linux 7.3.4, unmodified KWin 6.8.2 and a fullscreen client that requests
tearing through wp_tearing_control_v1. Before, none of about 350 frames per
run was presented without vsync. With the patch 3334 of 3340 frames tear
and the message above is gone. What is still rejected is the first flip
after the modifier or format changed, once per run, as intended.

Signed-off-by: Erwin Springer <aot.lerwin@gmail.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index f4d9148ef..58f45dd68 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -6281,6 +6281,14 @@ static int intel_async_flip_check_uapi(struct intel_atomic_state *state,
 		if (plane->pipe != crtc->pipe)
 			continue;
 
+		/*
+		 * Userspace may add planes it doesn't change to the commit.
+		 * A plane that is disabled and stays disabled takes no part
+		 * in the async flip.
+		 */
+		if (!old_plane_state->uapi.fb && !new_plane_state->uapi.fb)
+			continue;
+
 		/*
 		 * TODO: Async flip is only supported through the page flip IOCTL
 		 * as of now. So support currently added for primary plane only.
@@ -6346,6 +6354,10 @@ static int intel_async_flip_check_hw(struct intel_atomic_state *state, struct in
 		if (plane->pipe != crtc->pipe)
 			continue;
 
+		/* Disabled planes that stay disabled take no part in the flip. */
+		if (!old_plane_state->hw.fb && !new_plane_state->hw.fb)
+			continue;
+
 		/*
 		 * Only async flip capable planes should be in the state
 		 * if we're really about to ask the hardware to perform

---
base-commit: 419bb0e4cadd214a0847010c02ade95e5fc511f7
change-id: 20261115-i915-async-planes-d532ca94cf3c

Best regards,
--  
Erwin Springer <aot.lerwin@gmail.com>

Re: [PATCH] drm/i915/display: Ignore disabled planes in async flip checks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/i915/display: reachable drm_WARN_ON() in
  intel_async_flip_check_hw() with NV12 Y-plane
--

commit b042ec62b54473b2633218f68a54b0d394afbc1e
Author: Erwin Springer <aot.lerwin@gmail.com>

drm/i915/display: Ignore disabled planes in async flip checks

This commit skips planes that have neither an old nor a new framebuffer
in intel_async_flip_check_uapi() and intel_async_flip_check_hw() so that
unchanged disabled planes included in an atomic commit do not cause async
flips to be rejected.

> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index f79caeb5a82ec..defa55da52a74 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -6173,6 +6173,14 @@ static int intel_async_flip_check_uapi(struct intel_atomic_state *state,
>  		if (plane->pipe != crtc->pipe)
>  			continue;
>  
> +		/*
> +		 * Userspace may add planes it doesn't change to the commit.
> +		 * A plane that is disabled and stays disabled takes no part
> +		 * in the async flip.
> +		 */
> +		if (!old_plane_state->uapi.fb && !new_plane_state->uapi.fb)
> +			continue;
> +
>  		/*
>  		 * TODO: Async flip is only supported through the page flip IOCTL
>  		 * as of now. So support currently added for primary plane only.
> @@ -6238,6 +6246,10 @@ static int intel_async_flip_check_hw(struct intel_atomic_state *state, struct in
>  		if (plane->pipe != crtc->pipe)
>  			continue;
>  
> +		/* Disabled planes that stay disabled take no part in the flip. */
> +		if (!old_plane_state->hw.fb && !new_plane_state->hw.fb)
> +			continue;
> +
>  		/*
>  		 * Only async flip capable planes should be in the state
>  		 * if we're really about to ask the hardware to perform

[Severity: Medium]
Can this trigger drm_WARN_ON(display->drm, new_crtc_state->do_async_flip
&& !plane->async_flip) in intel_async_flip_check_hw() when a non-primary
plane is scanning out a planar YUV framebuffer (such as NV12 on ICL+)?

When a plane scans out a planar format, icl_check_nv12_planes() links it
to an unused SDR Y-plane via link_nv12_planes().  That Y-plane remains
disabled in UAPI (uapi.fb is NULL), but has a non-NULL hw.fb and
planar_linked_plane set.

If userspace submits atomic async flips on the primary plane while
including all UAPI-disabled planes on the pipe,
intel_async_flip_check_uapi() now skips the Y-plane because its old and
new uapi.fb are NULL.  Later in intel_atomic_check_planes(),
intel_joiner_add_affected_planes() sees the Y-plane's planar_linked_plane
and pulls the UV plane into the atomic state, and icl_check_nv12_planes()
re-links them with non-NULL hw.fb.  Once new_crtc_state->do_async_flip is
set on the following async flip, both planes bypass the !hw.fb check in
intel_async_flip_check_hw() and hit the drm_WARN_ON() because
plane->async_flip is NULL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115-i915-async-planes-v1-1-d13498d0b6be@gmail.com?part=1