[PATCH V3 46/47] drm/amd/display: Fix failures of disabling primary plans

[Michel Dänzer]

On 2022-09-14 22:08, Alex Hung wrote:
> On 2022-09-14 10:55, Michel Dänzer wrote:
>> On 2022-09-14 18:30, Alex Hung wrote:
>>> On 2022-09-14 07:40, Michel Dänzer wrote:
>>>> On 2022-09-14 15:31, Michel Dänzer wrote:
>>>>> On 2022-09-14 07:10, Wayne Lin wrote:
>>>>>> From: Alex Hung <alex.hung at amd.com>
>>>>>>
>>>>>> [Why & How]
>>>>>> This fixes kernel errors when IGT disables primary planes during the
>>>>>> tests kms_universal_plane::functional_test_pipe/pageflip_test_pipe.
>>>>>
>>>>> NAK.
>>>>>
>>>>> This essentially reverts commit b836a274b797 ("drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is") (except that it goes even further and completely removes the requirement for any HW plane to be enabled when the HW cursor is), so it would reintroduce the issues described in that commit log.
>>>>
>>>> Actually not exactly the same issues, due to going even further than reverting my fix.
>>>>
>>>> Instead, the driver will claim that an atomic commit which enables the CRTC and the cursor plane, while leaving all other KMS planes disabled, succeeds. But the HW cursor will not actually be visible.
>>>
>>> I did not observe problems with cursors (GNOME 42.4 - desktop and youtube/mpv video playback: windowed/fullscreen). Are there steps to reproduce cursor problems?
>>
>> As described in my last follow-up e-mail: An atomic KMS commit which enables the CRTC and the cursor plane, but disables all other KMS planes for the CRTC. The commit will succeed, but will not result in the HW cursor being actually visible. (I don't know of any specific application or test which exercises this)
> 
> Did you mean cursor plane depends on primary plane (i.e. no primary plane = no visible HW cursor)?

Sort of. I understand the HW cursor isn't an actual separate plane in AMD HW. Instead, the HW cursor can be displayed as part of any other HW plane. This means that the HW cursor can only be visible if any other plane is enabled.


> If there is no primary plane, what scenario would it be required to draw a cursor?
> 
> If this is a rare case, would it still be a concern?

Yes. In the KMS API, the cursor plane is like any other plane. A Wayland compositor or other user space may legitimately try to display something (not necessarily a "cursor") using only the cursor plane. The driver must accurately signal that this cannot work. The established way to do so (if a bit indirectly) is to require the primary plane to be enabled whenever the CRTC is.


>> Also see the commit log of bc92c06525e5 ("drm/amd/display: Allow commits with no planes active").
> 
> Does it mean dm_crtc_helper_atomic_check does not need to return -EINVAL if there is no active cursor because there are no cursors to be shown anyways, [...]

This was considered in the review discussion for b836a274b797 ("drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is"), see https://patchwork.freedesktop.org/patch/387230/ .

TL;DR: There were already other KMS drivers requiring the primary plane to be enabled whenever the CRTC is, and there's a special case for that in atomic_remove_fb. Adding another special case for the cursor plane would make things much more complicated for common DRM code and user space (and possibly even introduce issues which cannot be solved at all).


>>>>> If IGT tests disable the primary plane while leaving the CRTC enabled, those tests are broken and need to be fixed instead.
>>>
>>> There are IGT cursor tests fixed by this:
>>>
>>>    igt at kms_cursor_legacy@flip-vs-cursor at atomic-transitions
>>>    igt at kms_cursor_legacy@flip-vs-cursor at atomic-transitions-varying-size
>>>
>>> It also reduces amdgpu workaround in IGT's kms_concurrent:
>>>    https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fpatch%2F499382%2F%3Fseries%3D107734%26rev%3D1&data=05%7C01%7Calex.hung%40amd.com%7Cc757c9e4fbda4f8474a308da9671f920%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637987713521806985%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=XRRvilVZMBALIWHAOLArxjiAcgqQ%2FwNRnuUUJCTOYzY%3D&reserved=0
>>>
>>> The change affect multiple IGT tests. Adding amdgpu-specific workarounds to each of the IGT tests is not an ideal solution.
>>
>> It's not amdgpu specific, other atomic KMS drivers have the same restriction. IGT tests need to be able to handle this. See e.g. 88e379cef970 ("kms_cursor_legacy: Keep primary plane enabled for XRGB overlay fallback").
> 
> 
> Commit 88e379cef970 adds the following change to keep primary plane enabled:
> 
> +               igt_plane_set_fb(primary, prim_fb)
> 
> but kms_universal_plane fails at testing disabling primary plane, ex.
> 
> [...]

User space just cannot rely on being able to disable the primary plane while the CRTC is enabled. Any IGT tests which do so are broken and need to be fixed.

See also https://patchwork.freedesktop.org/series/80904/ .


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer