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-next>] [day] [month] [year] [list]

Message-Id: <DATYCMWH1X28.NE3M8KJ3SPV9@kode54.net>
Date: Mon, 23 Jun 2025 06:38:01 -0700
From: "Christopher Snowhill" <chris@...e54.net>
To: "Christopher Snowhill" <kode54@...il.com>,
 <amd-gfx@...ts.freedesktop.org>
Cc: "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>, "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

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.