Message-ID: <1b7fb3d2-1df4-42eb-90ae-d032feadaae2@amd.com>
Date: Fri, 15 Aug 2025 12:11:42 -0400
From: Leo Li <sunpeng.li@....com>
To: Christopher Snowhill <chris@...e54.net>, 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>,
	<dri-devel@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH] drm/amdgpu: Enable async flip for cursor planes



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.

Powered by blists - more mailing lists