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

["Christopher Snowhill" <kode54@gmail.com>]

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.