linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [RFC PATCH] drm/amdgpu: Enable async flip for cursor planes
       [not found]     ` <DATV4CAOHVGV.1UJ803EX21II6@gmail.com>
@ 2025-06-23 13:38       ` Christopher Snowhill
  2025-06-23 15:33         ` Alex Deucher
  2025-08-15 16:11         ` Leo Li
  0 siblings, 2 replies; 5+ messages in thread
From: Christopher Snowhill @ 2025-06-23 13:38 UTC (permalink / raw)
  To: Christopher Snowhill, amd-gfx
  Cc: Alex Deucher, Christian König, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann, David Airlie, David Airlie,
	dri-devel, linux-kernel

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC PATCH] drm/amdgpu: Enable async flip for cursor planes
  2025-06-23 13:38       ` [RFC PATCH] drm/amdgpu: Enable async flip for cursor planes Christopher Snowhill
@ 2025-06-23 15:33         ` Alex Deucher
  2025-08-15 13:17           ` Alex Deucher
  2025-08-15 16:11         ` Leo Li
  1 sibling, 1 reply; 5+ messages in thread
From: Alex Deucher @ 2025-06-23 15:33 UTC (permalink / raw)
  To: Christopher Snowhill, Wentland, Harry, Leo (Sunpeng) Li
  Cc: Christopher Snowhill, amd-gfx, Alex Deucher, Christian König,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
	dri-devel, linux-kernel

+ 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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC PATCH] drm/amdgpu: Enable async flip for cursor planes
  2025-06-23 15:33         ` Alex Deucher
@ 2025-08-15 13:17           ` Alex Deucher
  2025-08-15 23:21             ` Christopher Snowhill
  0 siblings, 1 reply; 5+ messages in thread
From: Alex Deucher @ 2025-08-15 13:17 UTC (permalink / raw)
  To: Christopher Snowhill, Wentland, Harry, Leo (Sunpeng) Li
  Cc: Christopher Snowhill, amd-gfx, Alex Deucher, Christian König,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
	dri-devel, linux-kernel

@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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC PATCH] drm/amdgpu: Enable async flip for cursor planes
  2025-06-23 13:38       ` [RFC PATCH] drm/amdgpu: Enable async flip for cursor planes Christopher Snowhill
  2025-06-23 15:33         ` Alex Deucher
@ 2025-08-15 16:11         ` Leo Li
  1 sibling, 0 replies; 5+ messages in thread
From: Leo Li @ 2025-08-15 16:11 UTC (permalink / raw)
  To: Christopher Snowhill, Christopher Snowhill, amd-gfx
  Cc: Alex Deucher, Christian König, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann, David Airlie, dri-devel,
	linux-kernel



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.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC PATCH] drm/amdgpu: Enable async flip for cursor planes
  2025-08-15 13:17           ` Alex Deucher
@ 2025-08-15 23:21             ` Christopher Snowhill
  0 siblings, 0 replies; 5+ messages in thread
From: Christopher Snowhill @ 2025-08-15 23:21 UTC (permalink / raw)
  To: Alex Deucher, Christopher Snowhill, Wentland, Harry,
	Leo (Sunpeng) Li
  Cc: amd-gfx, Alex Deucher, Christian König, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann, David Airlie, dri-devel,
	linux-kernel

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.


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2025-08-15 23:21 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20250619125507.54384-1-kode54@gmail.com>
     [not found] ` <DARA1U86AS72.QOIEVZWCFPYC@kode54.net>
     [not found]   ` <DATUOZZD8316.2INSL3KL5RA80@kode54.net>
     [not found]     ` <DATV4CAOHVGV.1UJ803EX21II6@gmail.com>
2025-06-23 13:38       ` [RFC PATCH] drm/amdgpu: Enable async flip for cursor planes Christopher Snowhill
2025-06-23 15:33         ` Alex Deucher
2025-08-15 13:17           ` Alex Deucher
2025-08-15 23:21             ` Christopher Snowhill
2025-08-15 16:11         ` Leo Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).