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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CADnq5_OjUp+YpXSdvWrYN+6ofFfyES9Jvwkswf3JmTTMGL=MVQ@mail.gmail.com>
Date: Mon, 23 Jun 2025 11:33:27 -0400
From: Alex Deucher <alexdeucher@...il.com>
To: Christopher Snowhill <chris@...e54.net>, "Wentland, Harry" <Harry.Wentland@....com>, 
	"Leo (Sunpeng) Li" <Sunpeng.Li@....com>
Cc: Christopher Snowhill <kode54@...il.com>, amd-gfx@...ts.freedesktop.org, 
	Alex Deucher <alexander.deucher@....com>, Christian König <christian.koenig@....com>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, dri-devel@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] drm/amdgpu: Enable async flip for cursor planes

+ Harry, Leo

On Mon, Jun 23, 2025 at 9:38 AM Christopher Snowhill <chris@...e54.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.