* [PATCH] tests/kms_universal_plane: Re-work expectations for non-intel hw
@ 2024-01-10 20:04 Rob Clark
2024-01-10 20:59 ` ✗ Fi.CI.BAT: failure for " Patchwork
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Rob Clark @ 2024-01-10 20:04 UTC (permalink / raw)
To: igt-dev; +Cc: Rob Clark
From: Rob Clark <robdclark@chromium.org>
The expectation assumptions do not fit for non-intel hw. So re-work the
expectations and add msm support.
Signed-off-by: Rob Clark <robdclark@chromium.org>
---
tests/kms_universal_plane.c | 102 +++++++++++++++++++++++++-----------
1 file changed, 72 insertions(+), 30 deletions(-)
diff --git a/tests/kms_universal_plane.c b/tests/kms_universal_plane.c
index 6a39f93cc5aa..3158ff816046 100644
--- a/tests/kms_universal_plane.c
+++ b/tests/kms_universal_plane.c
@@ -402,8 +402,7 @@ sanity_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
sanity_test_t test = { .data = data };
igt_plane_t *primary;
drmModeModeInfo *mode;
- int i;
- int expect = 0;
+ int i, ret;
igt_output_set_pipe(output, pipe);
mode = igt_output_get_mode(output);
@@ -418,43 +417,86 @@ sanity_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
/*
* Try to use universal plane API to set primary plane that
- * doesn't cover CRTC (should fail on pre-gen9 and succeed on
- * gen9+).
+ * doesn't cover CRTC.
*/
igt_plane_set_fb(primary, &test.undersized_fb);
- if (is_intel_device(data->drm_fd))
- expect = (data->display_ver < 9) ? -EINVAL : 0;
- igt_assert(igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL) == expect);
+ ret = igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL);
+
+ if (is_intel_device(data->drm_fd)) {
+ /* should fail on pre-gen9 and succeed on gen9+: */
+ if (data->display_ver < 9)
+ igt_assert_eq(ret, -EINVAL);
+ else
+ igt_assert_eq(ret, 0);
+ } else if (is_msm_device(data->drm_fd)) {
+ /* Should be supported on all gens: */
+ igt_assert_eq(ret, 0);
+ } else {
+ igt_assert_eq(ret, 0);
+ }
- /* Same as above, but different plane positioning. */
+ /* Same as above, but different plane positioning: */
igt_plane_set_position(primary, 100, 100);
- igt_assert(igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL) == expect);
+ ret = igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL);
+
+ if (is_intel_device(data->drm_fd)) {
+ /* should fail on pre-gen9 and succeed on gen9+: */
+ if (data->display_ver < 9)
+ igt_assert_eq(ret, -EINVAL);
+ else
+ igt_assert_eq(ret, 0);
+ } else if (is_msm_device(data->drm_fd)) {
+ /* Should be supported on all gens: */
+ igt_assert_eq(ret, 0);
+ } else {
+ igt_assert_eq(ret, 0);
+ }
igt_plane_set_position(primary, 0, 0);
- /* Try to use universal plane API to scale down (should fail on pre-gen9) */
- if (is_intel_device(data->drm_fd))
- expect = (data->display_ver < 9) ? -ERANGE : 0;
- igt_assert(drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
- output->config.crtc->crtc_id,
- test.oversized_fb.fb_id, 0,
- 0, 0,
- mode->hdisplay + 100,
- mode->vdisplay + 100,
- IGT_FIXED(0,0), IGT_FIXED(0,0),
- IGT_FIXED(mode->hdisplay,0),
- IGT_FIXED(mode->vdisplay,0)) == expect);
+ /* Try to use universal plane API to scale down */
+ ret = drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
+ output->config.crtc->crtc_id,
+ test.oversized_fb.fb_id, 0,
+ 0, 0,
+ mode->hdisplay + 100,
+ mode->vdisplay + 100,
+ IGT_FIXED(0,0), IGT_FIXED(0,0),
+ IGT_FIXED(mode->hdisplay,0),
+ IGT_FIXED(mode->vdisplay,0));
+
+ if (is_intel_device(data->drm_fd)) {
+ /* should fail on pre-gen9 and succeed on gen9+: */
+ if (data->display_ver < 9)
+ igt_assert_eq(ret, -ERANGE);
+ else
+ igt_assert_eq(ret, 0);
+ } else {
+ /* Could succeed or fail with -ERANGE depending on hw limits: */
+ igt_assert((ret == 0) || (ret == -ERANGE));
+ }
/* Try to use universal plane API to scale up (should fail on pre-gen9) */
- igt_assert(drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
- output->config.crtc->crtc_id,
- test.oversized_fb.fb_id, 0,
- 0, 0,
- mode->hdisplay,
- mode->vdisplay,
- IGT_FIXED(0,0), IGT_FIXED(0,0),
- IGT_FIXED(mode->hdisplay - 100,0),
- IGT_FIXED(mode->vdisplay - 100,0)) == expect);
+ ret = drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
+ output->config.crtc->crtc_id,
+ test.oversized_fb.fb_id, 0,
+ 0, 0,
+ mode->hdisplay,
+ mode->vdisplay,
+ IGT_FIXED(0,0), IGT_FIXED(0,0),
+ IGT_FIXED(mode->hdisplay - 100,0),
+ IGT_FIXED(mode->vdisplay - 100,0));
+
+ if (is_intel_device(data->drm_fd)) {
+ /* should fail on pre-gen9 and succeed on gen9+: */
+ if (data->display_ver < 9)
+ igt_assert_eq(ret, -ERANGE);
+ else
+ igt_assert_eq(ret, 0);
+ } else {
+ /* Could succeed or fail with -ERANGE depending on hw limits: */
+ igt_assert((ret == 0) || (ret == -ERANGE));
+ }
/* Find other crtcs and try to program our primary plane on them */
for (i = 0; i < test.moderes->count_crtcs; i++)
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* ✗ Fi.CI.BAT: failure for tests/kms_universal_plane: Re-work expectations for non-intel hw
2024-01-10 20:04 [PATCH] tests/kms_universal_plane: Re-work expectations for non-intel hw Rob Clark
@ 2024-01-10 20:59 ` Patchwork
2024-01-10 22:06 ` ✓ CI.xeBAT: success " Patchwork
2024-01-11 13:36 ` [PATCH] " Kamil Konieczny
2 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2024-01-10 20:59 UTC (permalink / raw)
To: Rob Clark; +Cc: igt-dev
[-- Attachment #1: Type: text/plain, Size: 13794 bytes --]
== Series Details ==
Series: tests/kms_universal_plane: Re-work expectations for non-intel hw
URL : https://patchwork.freedesktop.org/series/128450/
State : failure
== Summary ==
CI Bug Log - changes from IGT_7668 -> IGTPW_10504
====================================================
Summary
-------
**FAILURE**
Serious unknown changes coming with IGTPW_10504 absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in IGTPW_10504, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/index.html
Participating hosts (37 -> 38)
------------------------------
Additional (3): bat-dg2-8 bat-rpls-2 bat-dg2-9
Missing (2): bat-jsl-1 fi-snb-2520m
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in IGTPW_10504:
### IGT changes ###
#### Possible regressions ####
* igt@kms_pipe_crc_basic@suspend-read-crc@pipe-b-hdmi-a-1:
- bat-rpls-2: NOTRUN -> [ABORT][1]
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-rpls-2/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-b-hdmi-a-1.html
#### Suppressed ####
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* igt@i915_selftest@live@gt_pm:
- {bat-adls-6}: [PASS][2] -> [INCOMPLETE][3]
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7668/bat-adls-6/igt@i915_selftest@live@gt_pm.html
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-adls-6/igt@i915_selftest@live@gt_pm.html
Known issues
------------
Here are the changes found in IGTPW_10504 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@debugfs_test@basic-hwmon:
- bat-rpls-2: NOTRUN -> [SKIP][4] ([i915#9318])
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-rpls-2/igt@debugfs_test@basic-hwmon.html
* igt@gem_lmem_swapping@verify-random:
- bat-rpls-2: NOTRUN -> [SKIP][5] ([i915#4613]) +3 other tests skip
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-rpls-2/igt@gem_lmem_swapping@verify-random.html
* igt@gem_mmap@basic:
- bat-dg2-9: NOTRUN -> [SKIP][6] ([i915#4083])
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-9/igt@gem_mmap@basic.html
- bat-dg2-8: NOTRUN -> [SKIP][7] ([i915#4083])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-8/igt@gem_mmap@basic.html
* igt@gem_mmap_gtt@basic:
- bat-dg2-9: NOTRUN -> [SKIP][8] ([i915#4077]) +2 other tests skip
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-9/igt@gem_mmap_gtt@basic.html
- bat-dg2-8: NOTRUN -> [SKIP][9] ([i915#4077]) +2 other tests skip
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-8/igt@gem_mmap_gtt@basic.html
* igt@gem_render_tiled_blits@basic:
- bat-dg2-9: NOTRUN -> [SKIP][10] ([i915#4079]) +1 other test skip
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-9/igt@gem_render_tiled_blits@basic.html
* igt@gem_tiled_pread_basic:
- bat-dg2-8: NOTRUN -> [SKIP][11] ([i915#4079]) +1 other test skip
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-8/igt@gem_tiled_pread_basic.html
- bat-rpls-2: NOTRUN -> [SKIP][12] ([i915#3282])
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-rpls-2/igt@gem_tiled_pread_basic.html
* igt@i915_pm_rps@basic-api:
- bat-rpls-2: NOTRUN -> [SKIP][13] ([i915#6621])
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-rpls-2/igt@i915_pm_rps@basic-api.html
- bat-dg2-9: NOTRUN -> [SKIP][14] ([i915#6621])
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-9/igt@i915_pm_rps@basic-api.html
- bat-dg2-8: NOTRUN -> [SKIP][15] ([i915#6621])
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-8/igt@i915_pm_rps@basic-api.html
* igt@i915_selftest@live@gt_pm:
- bat-rpls-2: NOTRUN -> [DMESG-FAIL][16] ([i915#10010])
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-rpls-2/igt@i915_selftest@live@gt_pm.html
* igt@i915_suspend@basic-s3-without-i915:
- bat-dg2-8: NOTRUN -> [SKIP][17] ([i915#6645])
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-8/igt@i915_suspend@basic-s3-without-i915.html
* igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
- bat-dg2-9: NOTRUN -> [SKIP][18] ([i915#5190])
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-9/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html
- bat-dg2-8: NOTRUN -> [SKIP][19] ([i915#5190])
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-8/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html
* igt@kms_addfb_basic@basic-y-tiled-legacy:
- bat-dg2-9: NOTRUN -> [SKIP][20] ([i915#4215] / [i915#5190])
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-9/igt@kms_addfb_basic@basic-y-tiled-legacy.html
- bat-dg2-8: NOTRUN -> [SKIP][21] ([i915#4215] / [i915#5190])
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-8/igt@kms_addfb_basic@basic-y-tiled-legacy.html
* igt@kms_addfb_basic@framebuffer-vs-set-tiling:
- bat-dg2-9: NOTRUN -> [SKIP][22] ([i915#4212]) +7 other tests skip
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-9/igt@kms_addfb_basic@framebuffer-vs-set-tiling.html
- bat-dg2-8: NOTRUN -> [SKIP][23] ([i915#4212]) +7 other tests skip
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-8/igt@kms_addfb_basic@framebuffer-vs-set-tiling.html
* igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
- bat-dg2-9: NOTRUN -> [SKIP][24] ([i915#4103] / [i915#4213]) +1 other test skip
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-9/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
- bat-rpls-2: NOTRUN -> [SKIP][25] ([i915#4103]) +1 other test skip
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-rpls-2/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
- bat-dg2-8: NOTRUN -> [SKIP][26] ([i915#4103] / [i915#4213]) +1 other test skip
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-8/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
* igt@kms_dsc@dsc-basic:
- bat-rpls-2: NOTRUN -> [SKIP][27] ([i915#3555] / [i915#3840] / [i915#9886])
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-rpls-2/igt@kms_dsc@dsc-basic.html
* igt@kms_force_connector_basic@force-load-detect:
- bat-dg2-9: NOTRUN -> [SKIP][28] ([fdo#109285])
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-9/igt@kms_force_connector_basic@force-load-detect.html
- bat-rpls-2: NOTRUN -> [SKIP][29] ([fdo#109285])
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-rpls-2/igt@kms_force_connector_basic@force-load-detect.html
- bat-dg2-8: NOTRUN -> [SKIP][30] ([fdo#109285])
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-8/igt@kms_force_connector_basic@force-load-detect.html
* igt@kms_force_connector_basic@prune-stale-modes:
- bat-dg2-9: NOTRUN -> [SKIP][31] ([i915#5274])
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-9/igt@kms_force_connector_basic@prune-stale-modes.html
- bat-dg2-8: NOTRUN -> [SKIP][32] ([i915#5274])
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-8/igt@kms_force_connector_basic@prune-stale-modes.html
* igt@kms_pm_backlight@basic-brightness:
- bat-dg2-8: NOTRUN -> [SKIP][33] ([i915#5354])
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-8/igt@kms_pm_backlight@basic-brightness.html
- bat-dg2-9: NOTRUN -> [SKIP][34] ([i915#5354])
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-9/igt@kms_pm_backlight@basic-brightness.html
- bat-rpls-2: NOTRUN -> [SKIP][35] ([i915#5354])
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-rpls-2/igt@kms_pm_backlight@basic-brightness.html
* igt@kms_setmode@basic-clone-single-crtc:
- bat-dg2-9: NOTRUN -> [SKIP][36] ([i915#3555])
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-9/igt@kms_setmode@basic-clone-single-crtc.html
- bat-dg2-8: NOTRUN -> [SKIP][37] ([i915#3555])
[37]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-8/igt@kms_setmode@basic-clone-single-crtc.html
- bat-rpls-2: NOTRUN -> [SKIP][38] ([i915#3555])
[38]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-rpls-2/igt@kms_setmode@basic-clone-single-crtc.html
* igt@prime_vgem@basic-fence-flip:
- bat-dg2-9: NOTRUN -> [SKIP][39] ([i915#3708])
[39]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-9/igt@prime_vgem@basic-fence-flip.html
- bat-dg2-8: NOTRUN -> [SKIP][40] ([i915#3708])
[40]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-8/igt@prime_vgem@basic-fence-flip.html
* igt@prime_vgem@basic-fence-mmap:
- bat-dg2-8: NOTRUN -> [SKIP][41] ([i915#3708] / [i915#4077]) +1 other test skip
[41]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-8/igt@prime_vgem@basic-fence-mmap.html
- bat-dg2-9: NOTRUN -> [SKIP][42] ([i915#3708] / [i915#4077]) +1 other test skip
[42]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-9/igt@prime_vgem@basic-fence-mmap.html
* igt@prime_vgem@basic-fence-read:
- bat-rpls-2: NOTRUN -> [SKIP][43] ([fdo#109295] / [i915#3708]) +2 other tests skip
[43]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-rpls-2/igt@prime_vgem@basic-fence-read.html
* igt@prime_vgem@basic-write:
- bat-dg2-9: NOTRUN -> [SKIP][44] ([i915#3291] / [i915#3708]) +2 other tests skip
[44]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-9/igt@prime_vgem@basic-write.html
- bat-dg2-8: NOTRUN -> [SKIP][45] ([i915#3291] / [i915#3708]) +2 other tests skip
[45]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-dg2-8/igt@prime_vgem@basic-write.html
#### Possible fixes ####
* igt@i915_selftest@live@execlists:
- fi-bsw-n3050: [ABORT][46] ([i915#7911]) -> [PASS][47]
[46]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7668/fi-bsw-n3050/igt@i915_selftest@live@execlists.html
[47]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/fi-bsw-n3050/igt@i915_selftest@live@execlists.html
* igt@i915_selftest@live@hangcheck:
- {bat-rpls-3}: [DMESG-WARN][48] ([i915#5591]) -> [PASS][49]
[48]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7668/bat-rpls-3/igt@i915_selftest@live@hangcheck.html
[49]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/bat-rpls-3/igt@i915_selftest@live@hangcheck.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
[fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
[i915#10010]: https://gitlab.freedesktop.org/drm/intel/issues/10010
[i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
[i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
[i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
[i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
[i915#3840]: https://gitlab.freedesktop.org/drm/intel/issues/3840
[i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
[i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
[i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
[i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
[i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
[i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
[i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
[i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
[i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
[i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
[i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
[i915#5591]: https://gitlab.freedesktop.org/drm/intel/issues/5591
[i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
[i915#6645]: https://gitlab.freedesktop.org/drm/intel/issues/6645
[i915#7911]: https://gitlab.freedesktop.org/drm/intel/issues/7911
[i915#9318]: https://gitlab.freedesktop.org/drm/intel/issues/9318
[i915#9673]: https://gitlab.freedesktop.org/drm/intel/issues/9673
[i915#9732]: https://gitlab.freedesktop.org/drm/intel/issues/9732
[i915#9886]: https://gitlab.freedesktop.org/drm/intel/issues/9886
Build changes
-------------
* CI: CI-20190529 -> None
* IGT: IGT_7668 -> IGTPW_10504
CI-20190529: 20190529
CI_DRM_14108: c58d1352dd193d8df380a14e95c05455acaf5b82 @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_10504: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/index.html
IGT_7668: 3f2879fef93c0c546a2f1c0aa48a9cc2a594b9d2 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/index.html
[-- Attachment #2: Type: text/html, Size: 17002 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* ✓ CI.xeBAT: success for tests/kms_universal_plane: Re-work expectations for non-intel hw
2024-01-10 20:04 [PATCH] tests/kms_universal_plane: Re-work expectations for non-intel hw Rob Clark
2024-01-10 20:59 ` ✗ Fi.CI.BAT: failure for " Patchwork
@ 2024-01-10 22:06 ` Patchwork
2024-01-11 13:36 ` [PATCH] " Kamil Konieczny
2 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2024-01-10 22:06 UTC (permalink / raw)
To: Rob Clark; +Cc: igt-dev
[-- Attachment #1: Type: text/plain, Size: 3677 bytes --]
== Series Details ==
Series: tests/kms_universal_plane: Re-work expectations for non-intel hw
URL : https://patchwork.freedesktop.org/series/128450/
State : success
== Summary ==
CI Bug Log - changes from XEIGT_7668_BAT -> XEIGTPW_10504_BAT
====================================================
Summary
-------
**SUCCESS**
No regressions found.
Participating hosts (3 -> 4)
------------------------------
Additional (1): bat-dg2-oem2
Known issues
------------
Here are the changes found in XEIGTPW_10504_BAT that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
- bat-dg2-oem2: NOTRUN -> [SKIP][1] ([Intel XE#623])
[1]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10504/bat-dg2-oem2/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html
* igt@kms_dsc@dsc-basic:
- bat-dg2-oem2: NOTRUN -> [SKIP][2] ([Intel XE#455])
[2]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10504/bat-dg2-oem2/igt@kms_dsc@dsc-basic.html
* igt@kms_force_connector_basic@prune-stale-modes:
- bat-dg2-oem2: NOTRUN -> [SKIP][3] ([i915#5274])
[3]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10504/bat-dg2-oem2/igt@kms_force_connector_basic@prune-stale-modes.html
* igt@kms_frontbuffer_tracking@basic:
- bat-dg2-oem2: NOTRUN -> [FAIL][4] ([Intel XE#608])
[4]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10504/bat-dg2-oem2/igt@kms_frontbuffer_tracking@basic.html
* igt@xe_exec_fault_mode@twice-bindexecqueue-userptr-invalidate:
- bat-dg2-oem2: NOTRUN -> [SKIP][5] ([Intel XE#288]) +32 other tests skip
[5]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10504/bat-dg2-oem2/igt@xe_exec_fault_mode@twice-bindexecqueue-userptr-invalidate.html
* igt@xe_huc_copy@huc_copy:
- bat-dg2-oem2: NOTRUN -> [SKIP][6] ([Intel XE#255])
[6]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10504/bat-dg2-oem2/igt@xe_huc_copy@huc_copy.html
* igt@xe_pat@pat-index-xe2:
- bat-dg2-oem2: NOTRUN -> [SKIP][7] ([Intel XE#977])
[7]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10504/bat-dg2-oem2/igt@xe_pat@pat-index-xe2.html
* igt@xe_pat@pat-index-xehpc:
- bat-dg2-oem2: NOTRUN -> [SKIP][8] ([Intel XE#979]) +1 other test skip
[8]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10504/bat-dg2-oem2/igt@xe_pat@pat-index-xehpc.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[Intel XE#255]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/255
[Intel XE#288]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/288
[Intel XE#455]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/455
[Intel XE#608]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/608
[Intel XE#623]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/623
[Intel XE#929]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/929
[Intel XE#977]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/977
[Intel XE#979]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/979
[i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
Build changes
-------------
* IGT: IGT_7668 -> IGTPW_10504
IGTPW_10504: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10504/index.html
IGT_7668: 3f2879fef93c0c546a2f1c0aa48a9cc2a594b9d2 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
xe-614-bb9e8031d2feb59becdea41e54e62f1bc47f3ef9: bb9e8031d2feb59becdea41e54e62f1bc47f3ef9
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10504/index.html
[-- Attachment #2: Type: text/html, Size: 4362 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tests/kms_universal_plane: Re-work expectations for non-intel hw
2024-01-10 20:04 [PATCH] tests/kms_universal_plane: Re-work expectations for non-intel hw Rob Clark
2024-01-10 20:59 ` ✗ Fi.CI.BAT: failure for " Patchwork
2024-01-10 22:06 ` ✓ CI.xeBAT: success " Patchwork
@ 2024-01-11 13:36 ` Kamil Konieczny
2024-01-11 17:30 ` Rob Clark
2 siblings, 1 reply; 8+ messages in thread
From: Kamil Konieczny @ 2024-01-11 13:36 UTC (permalink / raw)
To: igt-dev; +Cc: Rob Clark
Hi Rob,
On 2024-01-10 at 12:04:34 -0800, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
>
> The expectation assumptions do not fit for non-intel hw. So re-work the
> expectations and add msm support.
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
> tests/kms_universal_plane.c | 102 +++++++++++++++++++++++++-----------
> 1 file changed, 72 insertions(+), 30 deletions(-)
>
> diff --git a/tests/kms_universal_plane.c b/tests/kms_universal_plane.c
> index 6a39f93cc5aa..3158ff816046 100644
> --- a/tests/kms_universal_plane.c
> +++ b/tests/kms_universal_plane.c
> @@ -402,8 +402,7 @@ sanity_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
> sanity_test_t test = { .data = data };
> igt_plane_t *primary;
> drmModeModeInfo *mode;
> - int i;
> - int expect = 0;
Could you keep this var?
> + int i, ret;
>
> igt_output_set_pipe(output, pipe);
> mode = igt_output_get_mode(output);
> @@ -418,43 +417,86 @@ sanity_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
>
> /*
> * Try to use universal plane API to set primary plane that
> - * doesn't cover CRTC (should fail on pre-gen9 and succeed on
> - * gen9+).
> + * doesn't cover CRTC.
> */
> igt_plane_set_fb(primary, &test.undersized_fb);
> - if (is_intel_device(data->drm_fd))
> - expect = (data->display_ver < 9) ? -EINVAL : 0;
> - igt_assert(igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL) == expect);
> + ret = igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL);
> +
> + if (is_intel_device(data->drm_fd)) {
> + /* should fail on pre-gen9 and succeed on gen9+: */
> + if (data->display_ver < 9)
> + igt_assert_eq(ret, -EINVAL);
> + else
> + igt_assert_eq(ret, 0);
> + } else if (is_msm_device(data->drm_fd)) {
> + /* Should be supported on all gens: */
> + igt_assert_eq(ret, 0);
> + } else {
> + igt_assert_eq(ret, 0);
> + }
The old code above should work on msm, why do you changed it?
>
> - /* Same as above, but different plane positioning. */
> + /* Same as above, but different plane positioning: */
Drop this change.
> igt_plane_set_position(primary, 100, 100);
> - igt_assert(igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL) == expect);
> + ret = igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL);
> +
> + if (is_intel_device(data->drm_fd)) {
> + /* should fail on pre-gen9 and succeed on gen9+: */
> + if (data->display_ver < 9)
> + igt_assert_eq(ret, -EINVAL);
> + else
> + igt_assert_eq(ret, 0);
> + } else if (is_msm_device(data->drm_fd)) {
> + /* Should be supported on all gens: */
> + igt_assert_eq(ret, 0);
> + } else {
> + igt_assert_eq(ret, 0);
> + }
Same here, old code should just work for msm.
>
> igt_plane_set_position(primary, 0, 0);
>
> - /* Try to use universal plane API to scale down (should fail on pre-gen9) */
> - if (is_intel_device(data->drm_fd))
> - expect = (data->display_ver < 9) ? -ERANGE : 0;
Please keep this if(...) expect = ...
for a reason which follows.
> - igt_assert(drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> - output->config.crtc->crtc_id,
> - test.oversized_fb.fb_id, 0,
> - 0, 0,
> - mode->hdisplay + 100,
> - mode->vdisplay + 100,
> - IGT_FIXED(0,0), IGT_FIXED(0,0),
> - IGT_FIXED(mode->hdisplay,0),
> - IGT_FIXED(mode->vdisplay,0)) == expect);
> + /* Try to use universal plane API to scale down */
> + ret = drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> + output->config.crtc->crtc_id,
> + test.oversized_fb.fb_id, 0,
> + 0, 0,
> + mode->hdisplay + 100,
> + mode->vdisplay + 100,
> + IGT_FIXED(0,0), IGT_FIXED(0,0),
> + IGT_FIXED(mode->hdisplay,0),
> + IGT_FIXED(mode->vdisplay,0));
> +
> + if (is_intel_device(data->drm_fd)) {
> + /* should fail on pre-gen9 and succeed on gen9+: */
> + if (data->display_ver < 9)
> + igt_assert_eq(ret, -ERANGE);
> + else
> + igt_assert_eq(ret, 0);
Or just simple
igt_assert_eq(ret, expect);
> + } else {
> + /* Could succeed or fail with -ERANGE depending on hw limits: */
> + igt_assert((ret == 0) || (ret == -ERANGE));
> + }
This is a real change, it relaxed checks for non-intel HW.
imho we should ask others, for example amd developers if that is ok
for them.
>
> /* Try to use universal plane API to scale up (should fail on pre-gen9) */
> - igt_assert(drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> - output->config.crtc->crtc_id,
> - test.oversized_fb.fb_id, 0,
> - 0, 0,
> - mode->hdisplay,
> - mode->vdisplay,
> - IGT_FIXED(0,0), IGT_FIXED(0,0),
> - IGT_FIXED(mode->hdisplay - 100,0),
> - IGT_FIXED(mode->vdisplay - 100,0)) == expect);
> + ret = drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> + output->config.crtc->crtc_id,
> + test.oversized_fb.fb_id, 0,
> + 0, 0,
> + mode->hdisplay,
> + mode->vdisplay,
> + IGT_FIXED(0,0), IGT_FIXED(0,0),
> + IGT_FIXED(mode->hdisplay - 100,0),
> + IGT_FIXED(mode->vdisplay - 100,0));
> +
> + if (is_intel_device(data->drm_fd)) {
> + /* should fail on pre-gen9 and succeed on gen9+: */
> + if (data->display_ver < 9)
> + igt_assert_eq(ret, -ERANGE);
> + else
> + igt_assert_eq(ret, 0);
Same here, just simple:
igt_assert_eq(ret, expect);
Regards,
Kamil
> + } else {
> + /* Could succeed or fail with -ERANGE depending on hw limits: */
> + igt_assert((ret == 0) || (ret == -ERANGE));
> + }
>
> /* Find other crtcs and try to program our primary plane on them */
> for (i = 0; i < test.moderes->count_crtcs; i++)
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tests/kms_universal_plane: Re-work expectations for non-intel hw
2024-01-11 13:36 ` [PATCH] " Kamil Konieczny
@ 2024-01-11 17:30 ` Rob Clark
2024-01-16 9:42 ` Kamil Konieczny
0 siblings, 1 reply; 8+ messages in thread
From: Rob Clark @ 2024-01-11 17:30 UTC (permalink / raw)
To: Kamil Konieczny, igt-dev, Rob Clark, Rob Clark
On Thu, Jan 11, 2024 at 5:37 AM Kamil Konieczny
<kamil.konieczny@linux.intel.com> wrote:
>
> Hi Rob,
> On 2024-01-10 at 12:04:34 -0800, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > The expectation assumptions do not fit for non-intel hw. So re-work the
> > expectations and add msm support.
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> > tests/kms_universal_plane.c | 102 +++++++++++++++++++++++++-----------
> > 1 file changed, 72 insertions(+), 30 deletions(-)
> >
> > diff --git a/tests/kms_universal_plane.c b/tests/kms_universal_plane.c
> > index 6a39f93cc5aa..3158ff816046 100644
> > --- a/tests/kms_universal_plane.c
> > +++ b/tests/kms_universal_plane.c
> > @@ -402,8 +402,7 @@ sanity_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
> > sanity_test_t test = { .data = data };
> > igt_plane_t *primary;
> > drmModeModeInfo *mode;
> > - int i;
> > - int expect = 0;
>
> Could you keep this var?
>
> > + int i, ret;
> >
> > igt_output_set_pipe(output, pipe);
> > mode = igt_output_get_mode(output);
> > @@ -418,43 +417,86 @@ sanity_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
> >
> > /*
> > * Try to use universal plane API to set primary plane that
> > - * doesn't cover CRTC (should fail on pre-gen9 and succeed on
> > - * gen9+).
> > + * doesn't cover CRTC.
> > */
> > igt_plane_set_fb(primary, &test.undersized_fb);
> > - if (is_intel_device(data->drm_fd))
> > - expect = (data->display_ver < 9) ? -EINVAL : 0;
> > - igt_assert(igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL) == expect);
> > + ret = igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL);
> > +
> > + if (is_intel_device(data->drm_fd)) {
> > + /* should fail on pre-gen9 and succeed on gen9+: */
> > + if (data->display_ver < 9)
> > + igt_assert_eq(ret, -EINVAL);
> > + else
> > + igt_assert_eq(ret, 0);
> > + } else if (is_msm_device(data->drm_fd)) {
> > + /* Should be supported on all gens: */
> > + igt_assert_eq(ret, 0);
> > + } else {
> > + igt_assert_eq(ret, 0);
> > + }
>
> The old code above should work on msm, why do you changed it?
The reason for this change, and several other similar but (for msm)
non-functionally-changing hunks was to document the expectations for
msm and to give folks working on other drivers a guide about where to
insert their specific expectations.
The alternative, I think, if we don't want to keep adding driver
specifics is to just change all cases to allow either success or the
correct error code.
BR,
-R
>
> >
> > - /* Same as above, but different plane positioning. */
> > + /* Same as above, but different plane positioning: */
>
> Drop this change.
>
> > igt_plane_set_position(primary, 100, 100);
> > - igt_assert(igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL) == expect);
> > + ret = igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL);
> > +
> > + if (is_intel_device(data->drm_fd)) {
> > + /* should fail on pre-gen9 and succeed on gen9+: */
> > + if (data->display_ver < 9)
> > + igt_assert_eq(ret, -EINVAL);
> > + else
> > + igt_assert_eq(ret, 0);
> > + } else if (is_msm_device(data->drm_fd)) {
> > + /* Should be supported on all gens: */
> > + igt_assert_eq(ret, 0);
> > + } else {
> > + igt_assert_eq(ret, 0);
> > + }
>
> Same here, old code should just work for msm.
>
> >
> > igt_plane_set_position(primary, 0, 0);
> >
> > - /* Try to use universal plane API to scale down (should fail on pre-gen9) */
> > - if (is_intel_device(data->drm_fd))
> > - expect = (data->display_ver < 9) ? -ERANGE : 0;
>
> Please keep this if(...) expect = ...
> for a reason which follows.
>
> > - igt_assert(drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> > - output->config.crtc->crtc_id,
> > - test.oversized_fb.fb_id, 0,
> > - 0, 0,
> > - mode->hdisplay + 100,
> > - mode->vdisplay + 100,
> > - IGT_FIXED(0,0), IGT_FIXED(0,0),
> > - IGT_FIXED(mode->hdisplay,0),
> > - IGT_FIXED(mode->vdisplay,0)) == expect);
> > + /* Try to use universal plane API to scale down */
> > + ret = drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> > + output->config.crtc->crtc_id,
> > + test.oversized_fb.fb_id, 0,
> > + 0, 0,
> > + mode->hdisplay + 100,
> > + mode->vdisplay + 100,
> > + IGT_FIXED(0,0), IGT_FIXED(0,0),
> > + IGT_FIXED(mode->hdisplay,0),
> > + IGT_FIXED(mode->vdisplay,0));
> > +
> > + if (is_intel_device(data->drm_fd)) {
> > + /* should fail on pre-gen9 and succeed on gen9+: */
> > + if (data->display_ver < 9)
> > + igt_assert_eq(ret, -ERANGE);
> > + else
> > + igt_assert_eq(ret, 0);
>
> Or just simple
> igt_assert_eq(ret, expect);
>
> > + } else {
> > + /* Could succeed or fail with -ERANGE depending on hw limits: */
> > + igt_assert((ret == 0) || (ret == -ERANGE));
> > + }
>
> This is a real change, it relaxed checks for non-intel HW.
> imho we should ask others, for example amd developers if that is ok
> for them.
>
> >
> > /* Try to use universal plane API to scale up (should fail on pre-gen9) */
> > - igt_assert(drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> > - output->config.crtc->crtc_id,
> > - test.oversized_fb.fb_id, 0,
> > - 0, 0,
> > - mode->hdisplay,
> > - mode->vdisplay,
> > - IGT_FIXED(0,0), IGT_FIXED(0,0),
> > - IGT_FIXED(mode->hdisplay - 100,0),
> > - IGT_FIXED(mode->vdisplay - 100,0)) == expect);
> > + ret = drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> > + output->config.crtc->crtc_id,
> > + test.oversized_fb.fb_id, 0,
> > + 0, 0,
> > + mode->hdisplay,
> > + mode->vdisplay,
> > + IGT_FIXED(0,0), IGT_FIXED(0,0),
> > + IGT_FIXED(mode->hdisplay - 100,0),
> > + IGT_FIXED(mode->vdisplay - 100,0));
> > +
> > + if (is_intel_device(data->drm_fd)) {
> > + /* should fail on pre-gen9 and succeed on gen9+: */
> > + if (data->display_ver < 9)
> > + igt_assert_eq(ret, -ERANGE);
> > + else
> > + igt_assert_eq(ret, 0);
>
> Same here, just simple:
> igt_assert_eq(ret, expect);
>
> Regards,
> Kamil
>
> > + } else {
> > + /* Could succeed or fail with -ERANGE depending on hw limits: */
> > + igt_assert((ret == 0) || (ret == -ERANGE));
> > + }
> >
> > /* Find other crtcs and try to program our primary plane on them */
> > for (i = 0; i < test.moderes->count_crtcs; i++)
> > --
> > 2.43.0
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tests/kms_universal_plane: Re-work expectations for non-intel hw
2024-01-11 17:30 ` Rob Clark
@ 2024-01-16 9:42 ` Kamil Konieczny
2024-01-16 15:55 ` Rob Clark
0 siblings, 1 reply; 8+ messages in thread
From: Kamil Konieczny @ 2024-01-16 9:42 UTC (permalink / raw)
To: igt-dev; +Cc: Rob Clark, juha-pekka.heikkila
Hi Rob,
On 2024-01-11 at 09:30:08 -0800, Rob Clark wrote:
> On Thu, Jan 11, 2024 at 5:37 AM Kamil Konieczny
> <kamil.konieczny@linux.intel.com> wrote:
> >
> > Hi Rob,
> > On 2024-01-10 at 12:04:34 -0800, Rob Clark wrote:
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > The expectation assumptions do not fit for non-intel hw. So re-work the
> > > expectations and add msm support.
> > >
> > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > ---
> > > tests/kms_universal_plane.c | 102 +++++++++++++++++++++++++-----------
> > > 1 file changed, 72 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/tests/kms_universal_plane.c b/tests/kms_universal_plane.c
> > > index 6a39f93cc5aa..3158ff816046 100644
> > > --- a/tests/kms_universal_plane.c
> > > +++ b/tests/kms_universal_plane.c
> > > @@ -402,8 +402,7 @@ sanity_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
> > > sanity_test_t test = { .data = data };
> > > igt_plane_t *primary;
> > > drmModeModeInfo *mode;
> > > - int i;
> > > - int expect = 0;
> >
> > Could you keep this var?
> >
> > > + int i, ret;
> > >
> > > igt_output_set_pipe(output, pipe);
> > > mode = igt_output_get_mode(output);
> > > @@ -418,43 +417,86 @@ sanity_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
> > >
> > > /*
> > > * Try to use universal plane API to set primary plane that
> > > - * doesn't cover CRTC (should fail on pre-gen9 and succeed on
> > > - * gen9+).
> > > + * doesn't cover CRTC.
> > > */
> > > igt_plane_set_fb(primary, &test.undersized_fb);
> > > - if (is_intel_device(data->drm_fd))
> > > - expect = (data->display_ver < 9) ? -EINVAL : 0;
> > > - igt_assert(igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL) == expect);
> > > + ret = igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL);
> > > +
> > > + if (is_intel_device(data->drm_fd)) {
> > > + /* should fail on pre-gen9 and succeed on gen9+: */
> > > + if (data->display_ver < 9)
> > > + igt_assert_eq(ret, -EINVAL);
> > > + else
> > > + igt_assert_eq(ret, 0);
> > > + } else if (is_msm_device(data->drm_fd)) {
> > > + /* Should be supported on all gens: */
> > > + igt_assert_eq(ret, 0);
> > > + } else {
> > > + igt_assert_eq(ret, 0);
> > > + }
> >
> > The old code above should work on msm, why do you changed it?
>
> The reason for this change, and several other similar but (for msm)
> non-functionally-changing hunks was to document the expectations for
> msm and to give folks working on other drivers a guide about where to
> insert their specific expectations.
>
> The alternative, I think, if we don't want to keep adding driver
> specifics is to just change all cases to allow either success or the
> correct error code.
>
> BR,
> -R
>
imho we should leave it as is and introduce changes when they will be
needed and keep lines of code low. If someone will have other error
it can be instroduced with before:
if (is_intel_device(data->drm_fd))
expect = (data->display_ver < 9) ? -EINVAL : 0;
else if (is_msm_device(data->drm_fd))
As this is display code I will add Juha-Pekka to Cc.
> > > - expect = (data->display_ver < 9) ? -EINVAL : 0;
> > > - igt_assert(igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL) == expect);
> > > + ret = igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL);
> >
> > >
> > > - /* Same as above, but different plane positioning. */
> > > + /* Same as above, but different plane positioning: */
> >
> > Drop this change.
> >
> > > igt_plane_set_position(primary, 100, 100);
> > > - igt_assert(igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL) == expect);
> > > + ret = igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL);
> > > +
> > > + if (is_intel_device(data->drm_fd)) {
> > > + /* should fail on pre-gen9 and succeed on gen9+: */
> > > + if (data->display_ver < 9)
> > > + igt_assert_eq(ret, -EINVAL);
> > > + else
> > > + igt_assert_eq(ret, 0);
> > > + } else if (is_msm_device(data->drm_fd)) {
> > > + /* Should be supported on all gens: */
> > > + igt_assert_eq(ret, 0);
> > > + } else {
> > > + igt_assert_eq(ret, 0);
> > > + }
> >
> > Same here, old code should just work for msm.
> >
> > >
> > > igt_plane_set_position(primary, 0, 0);
> > >
> > > - /* Try to use universal plane API to scale down (should fail on pre-gen9) */
> > > - if (is_intel_device(data->drm_fd))
> > > - expect = (data->display_ver < 9) ? -ERANGE : 0;
> >
> > Please keep this if(...) expect = ...
> > for a reason which follows.
> >
> > > - igt_assert(drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> > > - output->config.crtc->crtc_id,
> > > - test.oversized_fb.fb_id, 0,
> > > - 0, 0,
> > > - mode->hdisplay + 100,
> > > - mode->vdisplay + 100,
> > > - IGT_FIXED(0,0), IGT_FIXED(0,0),
> > > - IGT_FIXED(mode->hdisplay,0),
> > > - IGT_FIXED(mode->vdisplay,0)) == expect);
> > > + /* Try to use universal plane API to scale down */
> > > + ret = drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> > > + output->config.crtc->crtc_id,
> > > + test.oversized_fb.fb_id, 0,
> > > + 0, 0,
> > > + mode->hdisplay + 100,
> > > + mode->vdisplay + 100,
> > > + IGT_FIXED(0,0), IGT_FIXED(0,0),
> > > + IGT_FIXED(mode->hdisplay,0),
> > > + IGT_FIXED(mode->vdisplay,0));
> > > +
> > > + if (is_intel_device(data->drm_fd)) {
> > > + /* should fail on pre-gen9 and succeed on gen9+: */
> > > + if (data->display_ver < 9)
> > > + igt_assert_eq(ret, -ERANGE);
> > > + else
> > > + igt_assert_eq(ret, 0);
> >
> > Or just simple
> > igt_assert_eq(ret, expect);
> >
> > > + } else {
> > > + /* Could succeed or fail with -ERANGE depending on hw limits: */
> > > + igt_assert((ret == 0) || (ret == -ERANGE));
Could you read/discover those limits and check for actual
expected error?
Regards,
Kamil
> > > + }
> >
> > This is a real change, it relaxed checks for non-intel HW.
> > imho we should ask others, for example amd developers if that is ok
> > for them.
> >
> > >
> > > /* Try to use universal plane API to scale up (should fail on pre-gen9) */
> > > - igt_assert(drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> > > - output->config.crtc->crtc_id,
> > > - test.oversized_fb.fb_id, 0,
> > > - 0, 0,
> > > - mode->hdisplay,
> > > - mode->vdisplay,
> > > - IGT_FIXED(0,0), IGT_FIXED(0,0),
> > > - IGT_FIXED(mode->hdisplay - 100,0),
> > > - IGT_FIXED(mode->vdisplay - 100,0)) == expect);
> > > + ret = drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> > > + output->config.crtc->crtc_id,
> > > + test.oversized_fb.fb_id, 0,
> > > + 0, 0,
> > > + mode->hdisplay,
> > > + mode->vdisplay,
> > > + IGT_FIXED(0,0), IGT_FIXED(0,0),
> > > + IGT_FIXED(mode->hdisplay - 100,0),
> > > + IGT_FIXED(mode->vdisplay - 100,0));
> > > +
> > > + if (is_intel_device(data->drm_fd)) {
> > > + /* should fail on pre-gen9 and succeed on gen9+: */
> > > + if (data->display_ver < 9)
> > > + igt_assert_eq(ret, -ERANGE);
> > > + else
> > > + igt_assert_eq(ret, 0);
> >
> > Same here, just simple:
> > igt_assert_eq(ret, expect);
> >
> > Regards,
> > Kamil
> >
> > > + } else {
> > > + /* Could succeed or fail with -ERANGE depending on hw limits: */
> > > + igt_assert((ret == 0) || (ret == -ERANGE));
> > > + }
> > >
> > > /* Find other crtcs and try to program our primary plane on them */
> > > for (i = 0; i < test.moderes->count_crtcs; i++)
> > > --
> > > 2.43.0
> > >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tests/kms_universal_plane: Re-work expectations for non-intel hw
2024-01-16 9:42 ` Kamil Konieczny
@ 2024-01-16 15:55 ` Rob Clark
2024-01-18 11:18 ` Kamil Konieczny
0 siblings, 1 reply; 8+ messages in thread
From: Rob Clark @ 2024-01-16 15:55 UTC (permalink / raw)
To: Kamil Konieczny, igt-dev, Rob Clark, Rob Clark,
Juha-Pekka Heikkila, juha-pekka.heikkila
On Tue, Jan 16, 2024 at 1:43 AM Kamil Konieczny
<kamil.konieczny@linux.intel.com> wrote:
>
> Hi Rob,
> On 2024-01-11 at 09:30:08 -0800, Rob Clark wrote:
> > On Thu, Jan 11, 2024 at 5:37 AM Kamil Konieczny
> > <kamil.konieczny@linux.intel.com> wrote:
> > >
> > > Hi Rob,
> > > On 2024-01-10 at 12:04:34 -0800, Rob Clark wrote:
> > > > From: Rob Clark <robdclark@chromium.org>
> > > >
> > > > The expectation assumptions do not fit for non-intel hw. So re-work the
> > > > expectations and add msm support.
> > > >
> > > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > > ---
> > > > tests/kms_universal_plane.c | 102 +++++++++++++++++++++++++-----------
> > > > 1 file changed, 72 insertions(+), 30 deletions(-)
> > > >
> > > > diff --git a/tests/kms_universal_plane.c b/tests/kms_universal_plane.c
> > > > index 6a39f93cc5aa..3158ff816046 100644
> > > > --- a/tests/kms_universal_plane.c
> > > > +++ b/tests/kms_universal_plane.c
> > > > @@ -402,8 +402,7 @@ sanity_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
> > > > sanity_test_t test = { .data = data };
> > > > igt_plane_t *primary;
> > > > drmModeModeInfo *mode;
> > > > - int i;
> > > > - int expect = 0;
> > >
> > > Could you keep this var?
> > >
> > > > + int i, ret;
> > > >
> > > > igt_output_set_pipe(output, pipe);
> > > > mode = igt_output_get_mode(output);
> > > > @@ -418,43 +417,86 @@ sanity_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
> > > >
> > > > /*
> > > > * Try to use universal plane API to set primary plane that
> > > > - * doesn't cover CRTC (should fail on pre-gen9 and succeed on
> > > > - * gen9+).
> > > > + * doesn't cover CRTC.
> > > > */
> > > > igt_plane_set_fb(primary, &test.undersized_fb);
> > > > - if (is_intel_device(data->drm_fd))
> > > > - expect = (data->display_ver < 9) ? -EINVAL : 0;
> > > > - igt_assert(igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL) == expect);
> > > > + ret = igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL);
> > > > +
> > > > + if (is_intel_device(data->drm_fd)) {
> > > > + /* should fail on pre-gen9 and succeed on gen9+: */
> > > > + if (data->display_ver < 9)
> > > > + igt_assert_eq(ret, -EINVAL);
> > > > + else
> > > > + igt_assert_eq(ret, 0);
> > > > + } else if (is_msm_device(data->drm_fd)) {
> > > > + /* Should be supported on all gens: */
> > > > + igt_assert_eq(ret, 0);
> > > > + } else {
> > > > + igt_assert_eq(ret, 0);
> > > > + }
> > >
> > > The old code above should work on msm, why do you changed it?
> >
> > The reason for this change, and several other similar but (for msm)
> > non-functionally-changing hunks was to document the expectations for
> > msm and to give folks working on other drivers a guide about where to
> > insert their specific expectations.
> >
> > The alternative, I think, if we don't want to keep adding driver
> > specifics is to just change all cases to allow either success or the
> > correct error code.
> >
> > BR,
> > -R
> >
>
> imho we should leave it as is and introduce changes when they will be
> needed and keep lines of code low. If someone will have other error
> it can be instroduced with before:
>
> if (is_intel_device(data->drm_fd))
> expect = (data->display_ver < 9) ? -EINVAL : 0;
> else if (is_msm_device(data->drm_fd))
The problem with (and the reason I removed) the expect variable
approach is that it can't deal with two possible results. And short
of enumerating every single different device, I don't think there is a
good way to know the exact expected device. But changing things to
allow either success or checking for the expected errno would be
possible.
> As this is display code I will add Juha-Pekka to Cc.
>
> > > > - expect = (data->display_ver < 9) ? -EINVAL : 0;
> > > > - igt_assert(igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL) == expect);
> > > > + ret = igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL);
>
> > >
> > > >
> > > > - /* Same as above, but different plane positioning. */
> > > > + /* Same as above, but different plane positioning: */
> > >
> > > Drop this change.
> > >
> > > > igt_plane_set_position(primary, 100, 100);
> > > > - igt_assert(igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL) == expect);
> > > > + ret = igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL);
> > > > +
> > > > + if (is_intel_device(data->drm_fd)) {
> > > > + /* should fail on pre-gen9 and succeed on gen9+: */
> > > > + if (data->display_ver < 9)
> > > > + igt_assert_eq(ret, -EINVAL);
> > > > + else
> > > > + igt_assert_eq(ret, 0);
> > > > + } else if (is_msm_device(data->drm_fd)) {
> > > > + /* Should be supported on all gens: */
> > > > + igt_assert_eq(ret, 0);
> > > > + } else {
> > > > + igt_assert_eq(ret, 0);
> > > > + }
> > >
> > > Same here, old code should just work for msm.
> > >
> > > >
> > > > igt_plane_set_position(primary, 0, 0);
> > > >
> > > > - /* Try to use universal plane API to scale down (should fail on pre-gen9) */
> > > > - if (is_intel_device(data->drm_fd))
> > > > - expect = (data->display_ver < 9) ? -ERANGE : 0;
> > >
> > > Please keep this if(...) expect = ...
> > > for a reason which follows.
> > >
> > > > - igt_assert(drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> > > > - output->config.crtc->crtc_id,
> > > > - test.oversized_fb.fb_id, 0,
> > > > - 0, 0,
> > > > - mode->hdisplay + 100,
> > > > - mode->vdisplay + 100,
> > > > - IGT_FIXED(0,0), IGT_FIXED(0,0),
> > > > - IGT_FIXED(mode->hdisplay,0),
> > > > - IGT_FIXED(mode->vdisplay,0)) == expect);
> > > > + /* Try to use universal plane API to scale down */
> > > > + ret = drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> > > > + output->config.crtc->crtc_id,
> > > > + test.oversized_fb.fb_id, 0,
> > > > + 0, 0,
> > > > + mode->hdisplay + 100,
> > > > + mode->vdisplay + 100,
> > > > + IGT_FIXED(0,0), IGT_FIXED(0,0),
> > > > + IGT_FIXED(mode->hdisplay,0),
> > > > + IGT_FIXED(mode->vdisplay,0));
> > > > +
> > > > + if (is_intel_device(data->drm_fd)) {
> > > > + /* should fail on pre-gen9 and succeed on gen9+: */
> > > > + if (data->display_ver < 9)
> > > > + igt_assert_eq(ret, -ERANGE);
> > > > + else
> > > > + igt_assert_eq(ret, 0);
> > >
> > > Or just simple
> > > igt_assert_eq(ret, expect);
> > >
> > > > + } else {
> > > > + /* Could succeed or fail with -ERANGE depending on hw limits: */
> > > > + igt_assert((ret == 0) || (ret == -ERANGE));
>
> Could you read/discover those limits and check for actual
> expected error?
Not really, other than doing atomic-test commits. Because of the way
this all turns into atomic test+commit on the driver side, I'm not
sure if that really adds value compared to just allowing each of these
to return zero or expected error code.
BR,
-R
> Regards,
> Kamil
>
> > > > + }
> > >
> > > This is a real change, it relaxed checks for non-intel HW.
> > > imho we should ask others, for example amd developers if that is ok
> > > for them.
> > >
> > > >
> > > > /* Try to use universal plane API to scale up (should fail on pre-gen9) */
> > > > - igt_assert(drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> > > > - output->config.crtc->crtc_id,
> > > > - test.oversized_fb.fb_id, 0,
> > > > - 0, 0,
> > > > - mode->hdisplay,
> > > > - mode->vdisplay,
> > > > - IGT_FIXED(0,0), IGT_FIXED(0,0),
> > > > - IGT_FIXED(mode->hdisplay - 100,0),
> > > > - IGT_FIXED(mode->vdisplay - 100,0)) == expect);
> > > > + ret = drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> > > > + output->config.crtc->crtc_id,
> > > > + test.oversized_fb.fb_id, 0,
> > > > + 0, 0,
> > > > + mode->hdisplay,
> > > > + mode->vdisplay,
> > > > + IGT_FIXED(0,0), IGT_FIXED(0,0),
> > > > + IGT_FIXED(mode->hdisplay - 100,0),
> > > > + IGT_FIXED(mode->vdisplay - 100,0));
> > > > +
> > > > + if (is_intel_device(data->drm_fd)) {
> > > > + /* should fail on pre-gen9 and succeed on gen9+: */
> > > > + if (data->display_ver < 9)
> > > > + igt_assert_eq(ret, -ERANGE);
> > > > + else
> > > > + igt_assert_eq(ret, 0);
> > >
> > > Same here, just simple:
> > > igt_assert_eq(ret, expect);
> > >
> > > Regards,
> > > Kamil
> > >
> > > > + } else {
> > > > + /* Could succeed or fail with -ERANGE depending on hw limits: */
> > > > + igt_assert((ret == 0) || (ret == -ERANGE));
> > > > + }
> > > >
> > > > /* Find other crtcs and try to program our primary plane on them */
> > > > for (i = 0; i < test.moderes->count_crtcs; i++)
> > > > --
> > > > 2.43.0
> > > >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tests/kms_universal_plane: Re-work expectations for non-intel hw
2024-01-16 15:55 ` Rob Clark
@ 2024-01-18 11:18 ` Kamil Konieczny
0 siblings, 0 replies; 8+ messages in thread
From: Kamil Konieczny @ 2024-01-18 11:18 UTC (permalink / raw)
To: igt-dev; +Cc: Rob Clark, Rob Clark, juha-pekka.heikkila
Hi Rob,
On 2024-01-16 at 07:55:02 -0800, Rob Clark wrote:
> On Tue, Jan 16, 2024 at 1:43 AM Kamil Konieczny
> <kamil.konieczny@linux.intel.com> wrote:
> >
> > Hi Rob,
> > On 2024-01-11 at 09:30:08 -0800, Rob Clark wrote:
> > > On Thu, Jan 11, 2024 at 5:37 AM Kamil Konieczny
> > > <kamil.konieczny@linux.intel.com> wrote:
> > > >
> > > > Hi Rob,
> > > > On 2024-01-10 at 12:04:34 -0800, Rob Clark wrote:
> > > > > From: Rob Clark <robdclark@chromium.org>
> > > > >
> > > > > The expectation assumptions do not fit for non-intel hw. So re-work the
> > > > > expectations and add msm support.
> > > > >
> > > > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > > > ---
> > > > > tests/kms_universal_plane.c | 102 +++++++++++++++++++++++++-----------
> > > > > 1 file changed, 72 insertions(+), 30 deletions(-)
> > > > >
> > > > > diff --git a/tests/kms_universal_plane.c b/tests/kms_universal_plane.c
> > > > > index 6a39f93cc5aa..3158ff816046 100644
> > > > > --- a/tests/kms_universal_plane.c
> > > > > +++ b/tests/kms_universal_plane.c
> > > > > @@ -402,8 +402,7 @@ sanity_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
> > > > > sanity_test_t test = { .data = data };
> > > > > igt_plane_t *primary;
> > > > > drmModeModeInfo *mode;
> > > > > - int i;
> > > > > - int expect = 0;
> > > >
> > > > Could you keep this var?
> > > >
> > > > > + int i, ret;
> > > > >
> > > > > igt_output_set_pipe(output, pipe);
> > > > > mode = igt_output_get_mode(output);
> > > > > @@ -418,43 +417,86 @@ sanity_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
> > > > >
> > > > > /*
> > > > > * Try to use universal plane API to set primary plane that
> > > > > - * doesn't cover CRTC (should fail on pre-gen9 and succeed on
> > > > > - * gen9+).
> > > > > + * doesn't cover CRTC.
> > > > > */
> > > > > igt_plane_set_fb(primary, &test.undersized_fb);
> > > > > - if (is_intel_device(data->drm_fd))
> > > > > - expect = (data->display_ver < 9) ? -EINVAL : 0;
> > > > > - igt_assert(igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL) == expect);
> > > > > + ret = igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL);
> > > > > +
> > > > > + if (is_intel_device(data->drm_fd)) {
> > > > > + /* should fail on pre-gen9 and succeed on gen9+: */
> > > > > + if (data->display_ver < 9)
> > > > > + igt_assert_eq(ret, -EINVAL);
> > > > > + else
> > > > > + igt_assert_eq(ret, 0);
> > > > > + } else if (is_msm_device(data->drm_fd)) {
> > > > > + /* Should be supported on all gens: */
> > > > > + igt_assert_eq(ret, 0);
> > > > > + } else {
> > > > > + igt_assert_eq(ret, 0);
> > > > > + }
> > > >
> > > > The old code above should work on msm, why do you changed it?
> > >
> > > The reason for this change, and several other similar but (for msm)
> > > non-functionally-changing hunks was to document the expectations for
> > > msm and to give folks working on other drivers a guide about where to
> > > insert their specific expectations.
> > >
> > > The alternative, I think, if we don't want to keep adding driver
> > > specifics is to just change all cases to allow either success or the
> > > correct error code.
> > >
> > > BR,
> > > -R
> > >
> >
> > imho we should leave it as is and introduce changes when they will be
> > needed and keep lines of code low. If someone will have other error
> > it can be instroduced with before:
> >
> > if (is_intel_device(data->drm_fd))
> > expect = (data->display_ver < 9) ? -EINVAL : 0;
> > else if (is_msm_device(data->drm_fd))
>
> The problem with (and the reason I removed) the expect variable
> approach is that it can't deal with two possible results. And short
> of enumerating every single different device, I don't think there is a
> good way to know the exact expected device. But changing things to
> allow either success or checking for the expected errno would be
> possible.
One rule of thumb is to not touch code that works.
Second is to not to write for future, especially if that
means more code for write and read.
>
> > As this is display code I will add Juha-Pekka to Cc.
> >
> > > > > - expect = (data->display_ver < 9) ? -EINVAL : 0;
> > > > > - igt_assert(igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL) == expect);
> > > > > + ret = igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL);
> >
> > > >
> > > > >
> > > > > - /* Same as above, but different plane positioning. */
> > > > > + /* Same as above, but different plane positioning: */
> > > >
> > > > Drop this change.
> > > >
> > > > > igt_plane_set_position(primary, 100, 100);
> > > > > - igt_assert(igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL) == expect);
> > > > > + ret = igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL);
> > > > > +
> > > > > + if (is_intel_device(data->drm_fd)) {
> > > > > + /* should fail on pre-gen9 and succeed on gen9+: */
> > > > > + if (data->display_ver < 9)
> > > > > + igt_assert_eq(ret, -EINVAL);
> > > > > + else
> > > > > + igt_assert_eq(ret, 0);
> > > > > + } else if (is_msm_device(data->drm_fd)) {
> > > > > + /* Should be supported on all gens: */
> > > > > + igt_assert_eq(ret, 0);
> > > > > + } else {
> > > > > + igt_assert_eq(ret, 0);
> > > > > + }
> > > >
> > > > Same here, old code should just work for msm.
> > > >
> > > > >
> > > > > igt_plane_set_position(primary, 0, 0);
> > > > >
> > > > > - /* Try to use universal plane API to scale down (should fail on pre-gen9) */
> > > > > - if (is_intel_device(data->drm_fd))
> > > > > - expect = (data->display_ver < 9) ? -ERANGE : 0;
> > > >
> > > > Please keep this if(...) expect = ...
> > > > for a reason which follows.
> > > >
> > > > > - igt_assert(drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> > > > > - output->config.crtc->crtc_id,
> > > > > - test.oversized_fb.fb_id, 0,
> > > > > - 0, 0,
> > > > > - mode->hdisplay + 100,
> > > > > - mode->vdisplay + 100,
> > > > > - IGT_FIXED(0,0), IGT_FIXED(0,0),
> > > > > - IGT_FIXED(mode->hdisplay,0),
> > > > > - IGT_FIXED(mode->vdisplay,0)) == expect);
> > > > > + /* Try to use universal plane API to scale down */
> > > > > + ret = drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> > > > > + output->config.crtc->crtc_id,
> > > > > + test.oversized_fb.fb_id, 0,
> > > > > + 0, 0,
> > > > > + mode->hdisplay + 100,
> > > > > + mode->vdisplay + 100,
> > > > > + IGT_FIXED(0,0), IGT_FIXED(0,0),
> > > > > + IGT_FIXED(mode->hdisplay,0),
> > > > > + IGT_FIXED(mode->vdisplay,0));
> > > > > +
> > > > > + if (is_intel_device(data->drm_fd)) {
> > > > > + /* should fail on pre-gen9 and succeed on gen9+: */
> > > > > + if (data->display_ver < 9)
> > > > > + igt_assert_eq(ret, -ERANGE);
> > > > > + else
> > > > > + igt_assert_eq(ret, 0);
> > > >
> > > > Or just simple
> > > > igt_assert_eq(ret, expect);
> > > >
> > > > > + } else {
> > > > > + /* Could succeed or fail with -ERANGE depending on hw limits: */
> > > > > + igt_assert((ret == 0) || (ret == -ERANGE));
> >
> > Could you read/discover those limits and check for actual
> > expected error?
>
> Not really, other than doing atomic-test commits. Because of the way
> this all turns into atomic test+commit on the driver side, I'm not
> sure if that really adds value compared to just allowing each of these
> to return zero or expected error code.
>
> BR,
> -R
ok, so it fixes this for msm, so it should be adjusted like:
(spaces for keeping it in column):
if (is_intel_device(data->drm_fd)) {
expect = (data->display_ver < 9) ? -ERANGE : 0;
igt_assert_eq(ret, expect);
} else if (is_msm_device(data->drm_fd)) {
/* Could succeed or fail with -ERANGE depending on hw limits: */
igt_assert((ret == 0) || (ret == -ERANGE));
} else {
igt_assert(ret == 0);
}
This way you adjust it for msm, let other devs check if for their
drivers.
Regards,
Kamil
>
> > Regards,
> > Kamil
> >
> > > > > + }
> > > >
> > > > This is a real change, it relaxed checks for non-intel HW.
> > > > imho we should ask others, for example amd developers if that is ok
> > > > for them.
> > > >
> > > > >
> > > > > /* Try to use universal plane API to scale up (should fail on pre-gen9) */
> > > > > - igt_assert(drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> > > > > - output->config.crtc->crtc_id,
> > > > > - test.oversized_fb.fb_id, 0,
> > > > > - 0, 0,
> > > > > - mode->hdisplay,
> > > > > - mode->vdisplay,
> > > > > - IGT_FIXED(0,0), IGT_FIXED(0,0),
> > > > > - IGT_FIXED(mode->hdisplay - 100,0),
> > > > > - IGT_FIXED(mode->vdisplay - 100,0)) == expect);
> > > > > + ret = drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> > > > > + output->config.crtc->crtc_id,
> > > > > + test.oversized_fb.fb_id, 0,
> > > > > + 0, 0,
> > > > > + mode->hdisplay,
> > > > > + mode->vdisplay,
> > > > > + IGT_FIXED(0,0), IGT_FIXED(0,0),
> > > > > + IGT_FIXED(mode->hdisplay - 100,0),
> > > > > + IGT_FIXED(mode->vdisplay - 100,0));
> > > > > +
> > > > > + if (is_intel_device(data->drm_fd)) {
> > > > > + /* should fail on pre-gen9 and succeed on gen9+: */
> > > > > + if (data->display_ver < 9)
> > > > > + igt_assert_eq(ret, -ERANGE);
> > > > > + else
> > > > > + igt_assert_eq(ret, 0);
> > > >
> > > > Same here, just simple:
> > > > igt_assert_eq(ret, expect);
> > > >
> > > > Regards,
> > > > Kamil
> > > >
> > > > > + } else {
> > > > > + /* Could succeed or fail with -ERANGE depending on hw limits: */
> > > > > + igt_assert((ret == 0) || (ret == -ERANGE));
> > > > > + }
> > > > >
> > > > > /* Find other crtcs and try to program our primary plane on them */
> > > > > for (i = 0; i < test.moderes->count_crtcs; i++)
> > > > > --
> > > > > 2.43.0
> > > > >
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-01-18 11:19 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-10 20:04 [PATCH] tests/kms_universal_plane: Re-work expectations for non-intel hw Rob Clark
2024-01-10 20:59 ` ✗ Fi.CI.BAT: failure for " Patchwork
2024-01-10 22:06 ` ✓ CI.xeBAT: success " Patchwork
2024-01-11 13:36 ` [PATCH] " Kamil Konieczny
2024-01-11 17:30 ` Rob Clark
2024-01-16 9:42 ` Kamil Konieczny
2024-01-16 15:55 ` Rob Clark
2024-01-18 11:18 ` Kamil Konieczny
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox