On 2022-09-14 10:55, Michel Dänzer wrote:
[ Adding the dri-devel list ]
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.h...@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)? 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?
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, as shown in the below diff:
+static bool does_crtc_have_active_cursor(struct drm_crtc_state
*new_crtc_state)
+{
+ struct drm_device *dev = new_crtc_state->crtc->dev;
+ struct drm_plane *plane;
+
+ drm_for_each_plane_mask(plane, dev, new_crtc_state->plane_mask) {
+ if (plane->type == DRM_PLANE_TYPE_CURSOR)
+ return true;
+ }
+
+ return false;
+}
+
static int dm_crtc_helper_atomic_check(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
@@ -383,7 +396,8 @@ static int dm_crtc_helper_atomic_check(struct
drm_crtc *crtc,
* userspace which stops using the HW cursor altogether in
response to the resulting EINVAL.
*/
if (crtc_state->enable &&
- !(crtc_state->plane_mask & drm_plane_mask(crtc->primary))) {
+ !(crtc_state->plane_mask & drm_plane_mask(crtc->primary)) &&
+ does_crtc_have_active_cursor(crtc_state)) {
Note: This would fix kms_universal_plane but not kms_cursor_legacy.
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@kms_cursor_legacy@flip-vs-cursor@atomic-transitions
igt@kms_cursor_legacy@flip-vs-cursor@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.
tests/kms_universal_plane.c:
192 /* Step 5: Universal API's, disable primary plane (CRC 5) */
193 igt_plane_set_fb(primary, NULL);
194 igt_display_commit2(display, COMMIT_UNIVERSAL);
195 igt_pipe_crc_collect_crc(test.pipe_crc, &test.crc_5);
...
230 /* Step 11: Disable primary plane */
231 igt_plane_set_fb(primary, NULL);
232 igt_display_commit2(display, COMMIT_UNIVERSAL);
and so on.
P.S. Per above, this patch should never have made it this far without getting
in touch with me directly.
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
index c89594f3a5cb..099a226407a3 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
@@ -376,18 +376,6 @@ static int dm_crtc_helper_atomic_check(struct drm_crtc
*crtc,
return ret;
}
- /*
- * We require the primary plane to be enabled whenever the CRTC is,
otherwise
- * drm_mode_cursor_universal may end up trying to enable the cursor plane
while all other
- * planes are disabled, which is not supported by the hardware. And there
is legacy
- * userspace which stops using the HW cursor altogether in response to the
resulting EINVAL.
- */
- if (crtc_state->enable &&
- !(crtc_state->plane_mask & drm_plane_mask(crtc->primary))) {
- DRM_DEBUG_ATOMIC("Can't enable a CRTC without enabling the primary
plane\n");
- return -EINVAL;
- }
-
/* In some use cases, like reset, no stream is attached */
if (!dm_crtc_state->stream)
return 0;