[RFC PATCH] drm/amdgpu: Enable async flip for cursor planes

[RFC PATCH] drm/amdgpu: Enable async flip for cursor planes

[Christopher Snowhill]

From: Christopher Snowhill <chris@kode54.net>

amdgpu apparently fudges atomic flips if some software is also tweaking
the cursor planes during flips, which results in lovely fallback to non-
atomic flips.

Fixes: 41129e236f14 ("drm/amdgpu: Enable async flip on overlay planes")
Signed-off-by: Christopher Snowhill <chris@kode54.net>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

 Is there some reason why this was decided to be this way? I see it
 hasn't changed much since 2018, until the addition of overlay
 plane checking.

 Re: https://gitlab.freedesktop.org/drm/amd/-/issues/4263

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
index b7c6e8d13435..f0c98d67e1f3 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
@@ -1268,7 +1268,8 @@ static int amdgpu_dm_plane_atomic_async_check(struct drm_plane *plane,
 	struct dm_crtc_state *dm_new_crtc_state;
 
 	if (flip) {
-		if (plane->type != DRM_PLANE_TYPE_OVERLAY)
+		if (plane->type != DRM_PLANE_TYPE_OVERLAY &&
+		    plane->type != DRM_PLANE_TYPE_CURSOR)
 			return -EINVAL;
 	} else if (plane->type != DRM_PLANE_TYPE_CURSOR) {
 		return -EINVAL;
-- 
2.50.0

Re: [RFC PATCH] drm/amdgpu: Enable async flip for cursor planes

[Christopher Snowhill]

On Thu Jun 19, 2025 at 5:55 AM PDT, Christopher Snowhill wrote:
> From: Christopher Snowhill <chris@kode54.net>
>
> amdgpu apparently fudges atomic flips if some software is also tweaking
> the cursor planes during flips, which results in lovely fallback to non-
> atomic flips.
>
> Fixes: 41129e236f14 ("drm/amdgpu: Enable async flip on overlay planes")
> Signed-off-by: Christopher Snowhill <chris@kode54.net>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
>  Is there some reason why this was decided to be this way? I see it
>  hasn't changed much since 2018, until the addition of overlay
>  plane checking.
>
>  Re: https://gitlab.freedesktop.org/drm/amd/-/issues/4263
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> index b7c6e8d13435..f0c98d67e1f3 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> @@ -1268,7 +1268,8 @@ static int amdgpu_dm_plane_atomic_async_check(struct drm_plane *plane,
>  	struct dm_crtc_state *dm_new_crtc_state;
>  
>  	if (flip) {
> -		if (plane->type != DRM_PLANE_TYPE_OVERLAY)
> +		if (plane->type != DRM_PLANE_TYPE_OVERLAY &&
> +		    plane->type != DRM_PLANE_TYPE_CURSOR)
>  			return -EINVAL;
>  	} else if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>  		return -EINVAL;

This is called from drm_atomic_set_property in
drivers/gpu/drm/drm_atomic_uapi.c, and clearly, that wants to perform an
async_check even if an above function call determines that the
properties being applied are a no-op according to check_prop_changes.

I do not know whether all of the tearing capable DEs or compositors are
in the wrong here, or whether the DRM subsystem is doing something wrong
for AMD, and possibly other GPU vendors if they support async cursor
changes. Clearly something is sending prop changes on the cursor plane,
whether they are allowed-for-primary fb_id, in_fence_fd, or
fb_damage_clips, or whether they are no-ops. The only thing stopping the
transaction is that a cursor plane check hits async_check, and it's
failing for AMD.

Perhaps I should be running full DRM debug logging against Plasma and
labwc to determine what they're writing during the game presentation?
Seems strange that there are atomic cursor plane updates, when KDE is
supposed to be disabling the cursor plane for tearing, and in this case,
the game itself is not presenting a moving cursor at the time, as it is
in the main game view, and even if the target reticle is a cursor, it is
neither moving from the center of the screen nor is its plane being
changed in a visible way.

Something screwy is going on, that's for sure.

Re: [RFC PATCH] drm/amdgpu: Enable async flip for cursor planes

[Christopher Snowhill]

Here's another alternative change, which may be more thorough. It does
seem to fix the issue, at least. The issue does indeed appear to be
no-op plane changes sent to the cursor plane.

If anyone wants to propose style changes, and suggest a proper commit
message, if this is indeed a welcome fix for the problem, please let me
know.

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index c2726af6698e..b741939698e8 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1087,17 +1087,22 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
 			}

 			/* ask the driver if this non-primary plane is supported */
-			if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
-				ret = -EINVAL;
+			else if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
+				ret = drm_atomic_plane_get_property(plane, plane_state,
+								    prop, &old_val);
+
+				if (ret || old_val != prop_value) {
+					ret = -EINVAL;

-				if (plane_funcs && plane_funcs->atomic_async_check)
-					ret = plane_funcs->atomic_async_check(plane, state, true);
+					if (plane_funcs && plane_funcs->atomic_async_check)
+						ret = plane_funcs->atomic_async_check(plane, state, true);

-				if (ret) {
-					drm_dbg_atomic(prop->dev,
-						       "[PLANE:%d:%s] does not support async flips\n",
-						       obj->id, plane->name);
-					break;
+					if (ret) {
+						drm_dbg_atomic(prop->dev,
+							       "[PLANE:%d:%s] does not support async flips\n",
+							       obj->id, plane->name);
+						break;
+					}
 				}
 			}
 		}

Re: [RFC PATCH] drm/amdgpu: Enable async flip for cursor planes

[Christopher Snowhill]

On Fri Jun 20, 2025 at 3:10 AM PDT, Christopher Snowhill wrote:
> Here's another alternative change, which may be more thorough. It does
> seem to fix the issue, at least. The issue does indeed appear to be
> no-op plane changes sent to the cursor plane.
>
> If anyone wants to propose style changes, and suggest a proper commit
> message, if this is indeed a welcome fix for the problem, please let me
> know.
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index c2726af6698e..b741939698e8 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1087,17 +1087,22 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>  			}
>
>  			/* ask the driver if this non-primary plane is supported */
> -			if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
> -				ret = -EINVAL;
> +			else if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
> +				ret = drm_atomic_plane_get_property(plane, plane_state,
> +								    prop, &old_val);
> +
> +				if (ret || old_val != prop_value) {
> +					ret = -EINVAL;
>
> -				if (plane_funcs && plane_funcs->atomic_async_check)
> -					ret = plane_funcs->atomic_async_check(plane, state, true);
> +					if (plane_funcs && plane_funcs->atomic_async_check)
> +						ret = plane_funcs->atomic_async_check(plane, state, true);
>
> -				if (ret) {
> -					drm_dbg_atomic(prop->dev,
> -						       "[PLANE:%d:%s] does not support async flips\n",
> -						       obj->id, plane->name);
> -					break;
> +					if (ret) {
> +						drm_dbg_atomic(prop->dev,
> +							       "[PLANE:%d:%s] does not support async flips\n",
> +							       obj->id, plane->name);
> +						break;
> +					}
>  				}
>  			}
>  		}

Upon further testing and reflection, I have come to the conclusion that
this is indeed best handled by a kernel fix, rather than breaking user
space.

I attempted to work around this in wlroots, adjusting 0.18, 0.19, and
0.20 git with similar patches. First I attempted to stash all the
written properties for the atomic code, storing an initial value of all
0xFE so it was always likely to write the first time, and only setting a
property if it changed from the last commit.

This resulted in whole commits breaking for one or both framebuffers
until I ctrl-alt-fx switched to a tty and back again, and this would
work again temporarily.

So I went back to the drawing board and only withheld seemingly
duplicate plane properties. This "worked", until I attempted to play a
game, and then it started glitching spectacularly, and not updating at
all if the game was doing direct scanout and vrr.

Clearly this is wrong.

The wlroots library queues up properties for each commit. On every
commit where the cursor is disabled, it queues up both fb_id=0 and
crtc_id=0. Every commit. Is this wrong? Should it only be queueing up
the disablement properties once? It also queues up the full plane and
hotspot properties when enabled, even if the cursor doesn't change
position or appearance.

Re: [RFC PATCH] drm/amdgpu: Enable async flip for cursor planes

[Christopher Snowhill]

On Mon Jun 23, 2025 at 3:46 AM PDT, Christopher Snowhill wrote:
> On Fri Jun 20, 2025 at 3:10 AM PDT, Christopher Snowhill wrote:
>> Here's another alternative change, which may be more thorough. It does
>> seem to fix the issue, at least. The issue does indeed appear to be
>> no-op plane changes sent to the cursor plane.
>>
>> If anyone wants to propose style changes, and suggest a proper commit
>> message, if this is indeed a welcome fix for the problem, please let me
>> know.
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>> index c2726af6698e..b741939698e8 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -1087,17 +1087,22 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>>  			}
>>
>>  			/* ask the driver if this non-primary plane is supported */
>> -			if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
>> -				ret = -EINVAL;
>> +			else if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
>> +				ret = drm_atomic_plane_get_property(plane, plane_state,
>> +								    prop, &old_val);
>> +
>> +				if (ret || old_val != prop_value) {
>> +					ret = -EINVAL;
>>
>> -				if (plane_funcs && plane_funcs->atomic_async_check)
>> -					ret = plane_funcs->atomic_async_check(plane, state, true);
>> +					if (plane_funcs && plane_funcs->atomic_async_check)
>> +						ret = plane_funcs->atomic_async_check(plane, state, true);
>>
>> -				if (ret) {
>> -					drm_dbg_atomic(prop->dev,
>> -						       "[PLANE:%d:%s] does not support async flips\n",
>> -						       obj->id, plane->name);
>> -					break;
>> +					if (ret) {
>> +						drm_dbg_atomic(prop->dev,
>> +							       "[PLANE:%d:%s] does not support async flips\n",
>> +							       obj->id, plane->name);
>> +						break;
>> +					}
>>  				}
>>  			}
>>  		}
>
> Upon further testing and reflection, I have come to the conclusion that
> this is indeed best handled by a kernel fix, rather than breaking user
> space.
>
> I attempted to work around this in wlroots, adjusting 0.18, 0.19, and
> 0.20 git with similar patches. First I attempted to stash all the
> written properties for the atomic code, storing an initial value of all
> 0xFE so it was always likely to write the first time, and only setting a
> property if it changed from the last commit.
>
> This resulted in whole commits breaking for one or both framebuffers
> until I ctrl-alt-fx switched to a tty and back again, and this would
> work again temporarily.
>
> So I went back to the drawing board and only withheld seemingly
> duplicate plane properties. This "worked", until I attempted to play a
> game, and then it started glitching spectacularly, and not updating at
> all if the game was doing direct scanout and vrr.
>
> Clearly this is wrong.
>
> The wlroots library queues up properties for each commit. On every
> commit where the cursor is disabled, it queues up both fb_id=0 and
> crtc_id=0. Every commit. Is this wrong? Should it only be queueing up
> the disablement properties once? It also queues up the full plane and
> hotspot properties when enabled, even if the cursor doesn't change
> position or appearance.

Probably should have CC'd the drm misc maintainers when I started poking
drm misc instead of amdgpu. Pity there isn't a list for that...

Re: [RFC PATCH] drm/amdgpu: Enable async flip for cursor planes

[Christopher Snowhill]

On Mon Jun 23, 2025 at 4:06 AM PDT, Christopher Snowhill wrote:
> On Mon Jun 23, 2025 at 3:46 AM PDT, Christopher Snowhill wrote:
>> On Fri Jun 20, 2025 at 3:10 AM PDT, Christopher Snowhill wrote:
>>> Here's another alternative change, which may be more thorough. It does
>>> seem to fix the issue, at least. The issue does indeed appear to be
>>> no-op plane changes sent to the cursor plane.
>>>
>>> If anyone wants to propose style changes, and suggest a proper commit
>>> message, if this is indeed a welcome fix for the problem, please let me
>>> know.
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>>> index c2726af6698e..b741939698e8 100644
>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>>> @@ -1087,17 +1087,22 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>>>  			}
>>>
>>>  			/* ask the driver if this non-primary plane is supported */
>>> -			if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
>>> -				ret = -EINVAL;
>>> +			else if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
>>> +				ret = drm_atomic_plane_get_property(plane, plane_state,
>>> +								    prop, &old_val);
>>> +
>>> +				if (ret || old_val != prop_value) {
>>> +					ret = -EINVAL;
>>>
>>> -				if (plane_funcs && plane_funcs->atomic_async_check)
>>> -					ret = plane_funcs->atomic_async_check(plane, state, true);
>>> +					if (plane_funcs && plane_funcs->atomic_async_check)
>>> +						ret = plane_funcs->atomic_async_check(plane, state, true);
>>>
>>> -				if (ret) {
>>> -					drm_dbg_atomic(prop->dev,
>>> -						       "[PLANE:%d:%s] does not support async flips\n",
>>> -						       obj->id, plane->name);
>>> -					break;
>>> +					if (ret) {
>>> +						drm_dbg_atomic(prop->dev,
>>> +							       "[PLANE:%d:%s] does not support async flips\n",
>>> +							       obj->id, plane->name);
>>> +						break;
>>> +					}
>>>  				}
>>>  			}
>>>  		}
>>
>> Upon further testing and reflection, I have come to the conclusion that
>> this is indeed best handled by a kernel fix, rather than breaking user
>> space.
>>
>> I attempted to work around this in wlroots, adjusting 0.18, 0.19, and
>> 0.20 git with similar patches. First I attempted to stash all the
>> written properties for the atomic code, storing an initial value of all
>> 0xFE so it was always likely to write the first time, and only setting a
>> property if it changed from the last commit.
>>
>> This resulted in whole commits breaking for one or both framebuffers
>> until I ctrl-alt-fx switched to a tty and back again, and this would
>> work again temporarily.
>>
>> So I went back to the drawing board and only withheld seemingly
>> duplicate plane properties. This "worked", until I attempted to play a
>> game, and then it started glitching spectacularly, and not updating at
>> all if the game was doing direct scanout and vrr.
>>
>> Clearly this is wrong.
>>
>> The wlroots library queues up properties for each commit. On every
>> commit where the cursor is disabled, it queues up both fb_id=0 and
>> crtc_id=0. Every commit. Is this wrong? Should it only be queueing up
>> the disablement properties once? It also queues up the full plane and
>> hotspot properties when enabled, even if the cursor doesn't change
>> position or appearance.
>
> Probably should have CC'd the drm misc maintainers when I started poking
> drm misc instead of amdgpu. Pity there isn't a list for that...

I am a dumbass, I didn't notice get_maintainer.pl. Added more people,
and the correct list. Not sure if I should remove amd-gfx, since this
affects them, somewhat...

However, the intention of this thread was to seek commentary on the
situation as it is.

Re: [RFC PATCH] drm/amdgpu: Enable async flip for cursor planes

[Alex Deucher]

+ Harry, Leo

On Mon, Jun 23, 2025 at 9:38 AM Christopher Snowhill <chris@kode54.net> wrote:
>
> On Mon Jun 23, 2025 at 4:06 AM PDT, Christopher Snowhill wrote:
> > On Mon Jun 23, 2025 at 3:46 AM PDT, Christopher Snowhill wrote:
> >> On Fri Jun 20, 2025 at 3:10 AM PDT, Christopher Snowhill wrote:
> >>> Here's another alternative change, which may be more thorough. It does
> >>> seem to fix the issue, at least. The issue does indeed appear to be
> >>> no-op plane changes sent to the cursor plane.
> >>>
> >>> If anyone wants to propose style changes, and suggest a proper commit
> >>> message, if this is indeed a welcome fix for the problem, please let me
> >>> know.
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> >>> index c2726af6698e..b741939698e8 100644
> >>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> >>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> >>> @@ -1087,17 +1087,22 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
> >>>                     }
> >>>
> >>>                     /* ask the driver if this non-primary plane is supported */
> >>> -                   if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
> >>> -                           ret = -EINVAL;
> >>> +                   else if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
> >>> +                           ret = drm_atomic_plane_get_property(plane, plane_state,
> >>> +                                                               prop, &old_val);
> >>> +
> >>> +                           if (ret || old_val != prop_value) {
> >>> +                                   ret = -EINVAL;
> >>>
> >>> -                           if (plane_funcs && plane_funcs->atomic_async_check)
> >>> -                                   ret = plane_funcs->atomic_async_check(plane, state, true);
> >>> +                                   if (plane_funcs && plane_funcs->atomic_async_check)
> >>> +                                           ret = plane_funcs->atomic_async_check(plane, state, true);
> >>>
> >>> -                           if (ret) {
> >>> -                                   drm_dbg_atomic(prop->dev,
> >>> -                                                  "[PLANE:%d:%s] does not support async flips\n",
> >>> -                                                  obj->id, plane->name);
> >>> -                                   break;
> >>> +                                   if (ret) {
> >>> +                                           drm_dbg_atomic(prop->dev,
> >>> +                                                          "[PLANE:%d:%s] does not support async flips\n",
> >>> +                                                          obj->id, plane->name);
> >>> +                                           break;
> >>> +                                   }
> >>>                             }
> >>>                     }
> >>>             }
> >>
> >> Upon further testing and reflection, I have come to the conclusion that
> >> this is indeed best handled by a kernel fix, rather than breaking user
> >> space.
> >>
> >> I attempted to work around this in wlroots, adjusting 0.18, 0.19, and
> >> 0.20 git with similar patches. First I attempted to stash all the
> >> written properties for the atomic code, storing an initial value of all
> >> 0xFE so it was always likely to write the first time, and only setting a
> >> property if it changed from the last commit.
> >>
> >> This resulted in whole commits breaking for one or both framebuffers
> >> until I ctrl-alt-fx switched to a tty and back again, and this would
> >> work again temporarily.
> >>
> >> So I went back to the drawing board and only withheld seemingly
> >> duplicate plane properties. This "worked", until I attempted to play a
> >> game, and then it started glitching spectacularly, and not updating at
> >> all if the game was doing direct scanout and vrr.
> >>
> >> Clearly this is wrong.
> >>
> >> The wlroots library queues up properties for each commit. On every
> >> commit where the cursor is disabled, it queues up both fb_id=0 and
> >> crtc_id=0. Every commit. Is this wrong? Should it only be queueing up
> >> the disablement properties once? It also queues up the full plane and
> >> hotspot properties when enabled, even if the cursor doesn't change
> >> position or appearance.
> >
> > Probably should have CC'd the drm misc maintainers when I started poking
> > drm misc instead of amdgpu. Pity there isn't a list for that...
>
> I am a dumbass, I didn't notice get_maintainer.pl. Added more people,
> and the correct list. Not sure if I should remove amd-gfx, since this
> affects them, somewhat...
>
> However, the intention of this thread was to seek commentary on the
> situation as it is.

Re: [RFC PATCH] drm/amdgpu: Enable async flip for cursor planes

[Alex Deucher]

@Wentland, Harry
, @Leo (Sunpeng) Li Can you guys take a look?  This patch fixes a regression.

Thanks,

Alex

On Mon, Jun 23, 2025 at 11:33 AM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> + Harry, Leo
>
> On Mon, Jun 23, 2025 at 9:38 AM Christopher Snowhill <chris@kode54.net> wrote:
> >
> > On Mon Jun 23, 2025 at 4:06 AM PDT, Christopher Snowhill wrote:
> > > On Mon Jun 23, 2025 at 3:46 AM PDT, Christopher Snowhill wrote:
> > >> On Fri Jun 20, 2025 at 3:10 AM PDT, Christopher Snowhill wrote:
> > >>> Here's another alternative change, which may be more thorough. It does
> > >>> seem to fix the issue, at least. The issue does indeed appear to be
> > >>> no-op plane changes sent to the cursor plane.
> > >>>
> > >>> If anyone wants to propose style changes, and suggest a proper commit
> > >>> message, if this is indeed a welcome fix for the problem, please let me
> > >>> know.
> > >>>
> > >>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > >>> index c2726af6698e..b741939698e8 100644
> > >>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > >>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > >>> @@ -1087,17 +1087,22 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
> > >>>                     }
> > >>>
> > >>>                     /* ask the driver if this non-primary plane is supported */
> > >>> -                   if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
> > >>> -                           ret = -EINVAL;
> > >>> +                   else if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
> > >>> +                           ret = drm_atomic_plane_get_property(plane, plane_state,
> > >>> +                                                               prop, &old_val);
> > >>> +
> > >>> +                           if (ret || old_val != prop_value) {
> > >>> +                                   ret = -EINVAL;
> > >>>
> > >>> -                           if (plane_funcs && plane_funcs->atomic_async_check)
> > >>> -                                   ret = plane_funcs->atomic_async_check(plane, state, true);
> > >>> +                                   if (plane_funcs && plane_funcs->atomic_async_check)
> > >>> +                                           ret = plane_funcs->atomic_async_check(plane, state, true);
> > >>>
> > >>> -                           if (ret) {
> > >>> -                                   drm_dbg_atomic(prop->dev,
> > >>> -                                                  "[PLANE:%d:%s] does not support async flips\n",
> > >>> -                                                  obj->id, plane->name);
> > >>> -                                   break;
> > >>> +                                   if (ret) {
> > >>> +                                           drm_dbg_atomic(prop->dev,
> > >>> +                                                          "[PLANE:%d:%s] does not support async flips\n",
> > >>> +                                                          obj->id, plane->name);
> > >>> +                                           break;
> > >>> +                                   }
> > >>>                             }
> > >>>                     }
> > >>>             }
> > >>
> > >> Upon further testing and reflection, I have come to the conclusion that
> > >> this is indeed best handled by a kernel fix, rather than breaking user
> > >> space.
> > >>
> > >> I attempted to work around this in wlroots, adjusting 0.18, 0.19, and
> > >> 0.20 git with similar patches. First I attempted to stash all the
> > >> written properties for the atomic code, storing an initial value of all
> > >> 0xFE so it was always likely to write the first time, and only setting a
> > >> property if it changed from the last commit.
> > >>
> > >> This resulted in whole commits breaking for one or both framebuffers
> > >> until I ctrl-alt-fx switched to a tty and back again, and this would
> > >> work again temporarily.
> > >>
> > >> So I went back to the drawing board and only withheld seemingly
> > >> duplicate plane properties. This "worked", until I attempted to play a
> > >> game, and then it started glitching spectacularly, and not updating at
> > >> all if the game was doing direct scanout and vrr.
> > >>
> > >> Clearly this is wrong.
> > >>
> > >> The wlroots library queues up properties for each commit. On every
> > >> commit where the cursor is disabled, it queues up both fb_id=0 and
> > >> crtc_id=0. Every commit. Is this wrong? Should it only be queueing up
> > >> the disablement properties once? It also queues up the full plane and
> > >> hotspot properties when enabled, even if the cursor doesn't change
> > >> position or appearance.
> > >
> > > Probably should have CC'd the drm misc maintainers when I started poking
> > > drm misc instead of amdgpu. Pity there isn't a list for that...
> >
> > I am a dumbass, I didn't notice get_maintainer.pl. Added more people,
> > and the correct list. Not sure if I should remove amd-gfx, since this
> > affects them, somewhat...
> >
> > However, the intention of this thread was to seek commentary on the
> > situation as it is.

Re: [RFC PATCH] drm/amdgpu: Enable async flip for cursor planes

[Leo Li]

On 2025-06-23 09:38, Christopher Snowhill wrote:
> On Mon Jun 23, 2025 at 4:06 AM PDT, Christopher Snowhill wrote:
>> On Mon Jun 23, 2025 at 3:46 AM PDT, Christopher Snowhill wrote:
>>> On Fri Jun 20, 2025 at 3:10 AM PDT, Christopher Snowhill wrote:
>>>> Here's another alternative change, which may be more thorough. It does
>>>> seem to fix the issue, at least. The issue does indeed appear to be
>>>> no-op plane changes sent to the cursor plane.
>>>>
>>>> If anyone wants to propose style changes, and suggest a proper commit
>>>> message, if this is indeed a welcome fix for the problem, please let me
>>>> know.
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>>>> index c2726af6698e..b741939698e8 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>>>> @@ -1087,17 +1087,22 @@ int drm_atomic_set_property(struct drm_atomic_state *state,

Hi Christopher,

Adding some additional context lines here:

>>>>		if (async_flip) {
>>>>			/* check if the prop does a nop change */
>>>>			if ((prop != config->prop_fb_id &&
>>>>			     prop != config->prop_in_fence_fd &&
>>>>			     prop != config->prop_fb_damage_clips)) {
>>>>				ret = drm_atomic_plane_get_property(plane, plane_state,
>>>>								    prop, &old_val);
>>>>				ret = drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);## end of additional context ##>>>>  			}
>>>>
>>>>  			/* ask the driver if this non-primary plane is supported */
>>>> -			if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
>>>> -				ret = -EINVAL;

Firstly, apologies for the delay, and thanks for the patch.

The original code definitely looks fishy. The value of `ret =
drm_atomic_check_prop_changes` is not used afterwards and is ignored.

Looking at the history (v6.14), it seems the original intention was:
* For PRIMARY planes
    * Allow modifying of properties fb_id, in_fence_fd, and fb_damage_clips
    * Allow setting same value for all other properties
* For OVERLAY and CURSOR planes :
    * Allow setting same value for all properties

(https://elixir.bootlin.com/linux/v6.14.11/source/drivers/gpu/drm/drm_atomic_uapi.c#L1078)


Then, the behavior changed in v6.15:
* For PRIMARY planes
    * Allow modifying of (including setting same value for) **all** properties
* For OVERLAY and CURSOR planes
    * **Allow modifying of all properties** if `atomic_async_check` callback
      exists and passes
    * **Setting same value can fail**, as behavior is now driver-specific.

(https://elixir.bootlin.com/linux/v6.15-rc1/source/drivers/gpu/drm/drm_atomic_uapi.c#L1081)


But IIUC, the desired behavior is actually:
* For PRIMARY planes
    * Allow modifying of properties fb_id, in_fence_fd, and fb_damage_clips
    * Allow setting same value for all other properties
* For OVERLAY and CURSOR planes :
    * **Allow setting same value on all properties**
    * Allow modifying of all properties if `atomic_async_check` callback exists
      and passes


Is this the desired behavior? If so, I think it makes sense, but the existing
conditions need some cleaning up.

Thanks,
Leo

>>>> +			else if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
>>>> +				ret = drm_atomic_plane_get_property(plane, plane_state,
>>>> +								    prop, &old_val);
>>>> +
>>>> +				if (ret || old_val != prop_value) {
>>>> +					ret = -EINVAL;
>>>>
>>>> -				if (plane_funcs && plane_funcs->atomic_async_check)
>>>> -					ret = plane_funcs->atomic_async_check(plane, state, true);
>>>> +					if (plane_funcs && plane_funcs->atomic_async_check)
>>>> +						ret = plane_funcs->atomic_async_check(plane, state, true);
>>>>
>>>> -				if (ret) {
>>>> -					drm_dbg_atomic(prop->dev,
>>>> -						       "[PLANE:%d:%s] does not support async flips\n",
>>>> -						       obj->id, plane->name);
>>>> -					break;
>>>> +					if (ret) {
>>>> +						drm_dbg_atomic(prop->dev,
>>>> +							       "[PLANE:%d:%s] does not support async flips\n",
>>>> +							       obj->id, plane->name);
>>>> +						break;
>>>> +					}
>>>>  				}
>>>>  			}
>>>>  		}
>>>
>>> Upon further testing and reflection, I have come to the conclusion that
>>> this is indeed best handled by a kernel fix, rather than breaking user
>>> space.
>>>
>>> I attempted to work around this in wlroots, adjusting 0.18, 0.19, and
>>> 0.20 git with similar patches. First I attempted to stash all the
>>> written properties for the atomic code, storing an initial value of all
>>> 0xFE so it was always likely to write the first time, and only setting a
>>> property if it changed from the last commit.
>>>
>>> This resulted in whole commits breaking for one or both framebuffers
>>> until I ctrl-alt-fx switched to a tty and back again, and this would
>>> work again temporarily.
>>>
>>> So I went back to the drawing board and only withheld seemingly
>>> duplicate plane properties. This "worked", until I attempted to play a
>>> game, and then it started glitching spectacularly, and not updating at
>>> all if the game was doing direct scanout and vrr.
>>>
>>> Clearly this is wrong.
>>>
>>> The wlroots library queues up properties for each commit. On every
>>> commit where the cursor is disabled, it queues up both fb_id=0 and
>>> crtc_id=0. Every commit. Is this wrong? Should it only be queueing up
>>> the disablement properties once? It also queues up the full plane and
>>> hotspot properties when enabled, even if the cursor doesn't change
>>> position or appearance.
>>
>> Probably should have CC'd the drm misc maintainers when I started poking
>> drm misc instead of amdgpu. Pity there isn't a list for that...
> 
> I am a dumbass, I didn't notice get_maintainer.pl. Added more people,
> and the correct list. Not sure if I should remove amd-gfx, since this
> affects them, somewhat...
> 
> However, the intention of this thread was to seek commentary on the
> situation as it is.

Re: [RFC PATCH] drm/amdgpu: Enable async flip for cursor planes

[Christopher Snowhill]

This may be a more appropriate patch:

https://lore.kernel.org/amd-gfx/20250723150413.18445-1-xaver.hugl@kde.org/

On Fri Aug 15, 2025 at 6:17 AM PDT, Alex Deucher wrote:
> @Wentland, Harry
> , @Leo (Sunpeng) Li Can you guys take a look?  This patch fixes a regression.
>
> Thanks,
>
> Alex
>
> On Mon, Jun 23, 2025 at 11:33 AM Alex Deucher <alexdeucher@gmail.com> wrote:
>>
>> + Harry, Leo
>>
>> On Mon, Jun 23, 2025 at 9:38 AM Christopher Snowhill <chris@kode54.net> wrote:
>> >
>> > On Mon Jun 23, 2025 at 4:06 AM PDT, Christopher Snowhill wrote:
>> > > On Mon Jun 23, 2025 at 3:46 AM PDT, Christopher Snowhill wrote:
>> > >> On Fri Jun 20, 2025 at 3:10 AM PDT, Christopher Snowhill wrote:
>> > >>> Here's another alternative change, which may be more thorough. It does
>> > >>> seem to fix the issue, at least. The issue does indeed appear to be
>> > >>> no-op plane changes sent to the cursor plane.
>> > >>>
>> > >>> If anyone wants to propose style changes, and suggest a proper commit
>> > >>> message, if this is indeed a welcome fix for the problem, please let me
>> > >>> know.
>> > >>>
>> > >>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>> > >>> index c2726af6698e..b741939698e8 100644
>> > >>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> > >>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> > >>> @@ -1087,17 +1087,22 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>> > >>>                     }
>> > >>>
>> > >>>                     /* ask the driver if this non-primary plane is supported */
>> > >>> -                   if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
>> > >>> -                           ret = -EINVAL;
>> > >>> +                   else if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
>> > >>> +                           ret = drm_atomic_plane_get_property(plane, plane_state,
>> > >>> +                                                               prop, &old_val);
>> > >>> +
>> > >>> +                           if (ret || old_val != prop_value) {
>> > >>> +                                   ret = -EINVAL;
>> > >>>
>> > >>> -                           if (plane_funcs && plane_funcs->atomic_async_check)
>> > >>> -                                   ret = plane_funcs->atomic_async_check(plane, state, true);
>> > >>> +                                   if (plane_funcs && plane_funcs->atomic_async_check)
>> > >>> +                                           ret = plane_funcs->atomic_async_check(plane, state, true);
>> > >>>
>> > >>> -                           if (ret) {
>> > >>> -                                   drm_dbg_atomic(prop->dev,
>> > >>> -                                                  "[PLANE:%d:%s] does not support async flips\n",
>> > >>> -                                                  obj->id, plane->name);
>> > >>> -                                   break;
>> > >>> +                                   if (ret) {
>> > >>> +                                           drm_dbg_atomic(prop->dev,
>> > >>> +                                                          "[PLANE:%d:%s] does not support async flips\n",
>> > >>> +                                                          obj->id, plane->name);
>> > >>> +                                           break;
>> > >>> +                                   }
>> > >>>                             }
>> > >>>                     }
>> > >>>             }
>> > >>
>> > >> Upon further testing and reflection, I have come to the conclusion that
>> > >> this is indeed best handled by a kernel fix, rather than breaking user
>> > >> space.
>> > >>
>> > >> I attempted to work around this in wlroots, adjusting 0.18, 0.19, and
>> > >> 0.20 git with similar patches. First I attempted to stash all the
>> > >> written properties for the atomic code, storing an initial value of all
>> > >> 0xFE so it was always likely to write the first time, and only setting a
>> > >> property if it changed from the last commit.
>> > >>
>> > >> This resulted in whole commits breaking for one or both framebuffers
>> > >> until I ctrl-alt-fx switched to a tty and back again, and this would
>> > >> work again temporarily.
>> > >>
>> > >> So I went back to the drawing board and only withheld seemingly
>> > >> duplicate plane properties. This "worked", until I attempted to play a
>> > >> game, and then it started glitching spectacularly, and not updating at
>> > >> all if the game was doing direct scanout and vrr.
>> > >>
>> > >> Clearly this is wrong.
>> > >>
>> > >> The wlroots library queues up properties for each commit. On every
>> > >> commit where the cursor is disabled, it queues up both fb_id=0 and
>> > >> crtc_id=0. Every commit. Is this wrong? Should it only be queueing up
>> > >> the disablement properties once? It also queues up the full plane and
>> > >> hotspot properties when enabled, even if the cursor doesn't change
>> > >> position or appearance.
>> > >
>> > > Probably should have CC'd the drm misc maintainers when I started poking
>> > > drm misc instead of amdgpu. Pity there isn't a list for that...
>> >
>> > I am a dumbass, I didn't notice get_maintainer.pl. Added more people,
>> > and the correct list. Not sure if I should remove amd-gfx, since this
>> > affects them, somewhat...
>> >
>> > However, the intention of this thread was to seek commentary on the
>> > situation as it is.