@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 <alexdeuc...@gmail.com> wrote:
>
> + Harry, Leo
>
> On Mon, Jun 23, 2025 at 9:38 AM Christopher Snowhill <ch...@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.
