* [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition @ 2019-02-19 9:38 Stanislav Lisovskiy 2019-02-19 10:12 ` [igt-dev] ✓ Fi.CI.BAT: success for igt/tests: Fix error checking in kms_atomic_transition (rev4) Patchwork ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Stanislav Lisovskiy @ 2019-02-19 9:38 UTC (permalink / raw) To: igt-dev Cc: norkernel, ville.syrjala, martin.peres, stanislav.lisovskiy, juha-pekka.heikkila From: Stanislav Lisovskiy <stanislav.lisovskiy@gmail.com> There is no guarantee that error return value will be always EINVAL, made a check more general as it can be ERANGE, ENOSPC, EINVAL and probably others, which all mean the same in context of this test case: i.e this sprite size is not valid. v2: Added macro to make check look a bit nicer. v3: Removed redundant debug line. v4: Added assertion if error is not EINVAL as expected, other errors except EINVAL are considered now a failures. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109225 Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- tests/kms_atomic_transition.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c index 12bafe87..e1a2d4a0 100644 --- a/tests/kms_atomic_transition.c +++ b/tests/kms_atomic_transition.c @@ -187,6 +187,12 @@ static void set_sprite_wh(igt_display_t *display, enum pipe pipe, LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb); } +#define is_atomic_check_failure_errno(errno) \ + (errno != -EINVAL && errno != 0) + +#define is_atomic_check_plane_size_errno(errno) \ + (errno == -EINVAL) + static void setup_parms(igt_display_t *display, enum pipe pipe, const drmModeModeInfo *mode, struct igt_fb *primary_fb, @@ -251,8 +257,9 @@ retry: wm_setup_plane(display, pipe, (1 << n_planes) - 1, parms, false); ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); + igt_assert(!is_atomic_check_failure_errno(ret)); - if (ret == -EINVAL) { + if (is_atomic_check_plane_size_errno(ret)) { if (cursor_width == sprite_width && cursor_height == sprite_height) { igt_assert_f(alpha, @@ -442,7 +449,9 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output igt_pipe_request_out_fence(pipe_obj); ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); - if (ret != -EINVAL || pipe_obj->n_planes < 3) + igt_assert(!is_atomic_check_failure_errno(ret)); + + if (!is_atomic_check_plane_size_errno(ret) || pipe_obj->n_planes < 3) break; ret = 0; -- 2.17.1 _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [igt-dev] ✓ Fi.CI.BAT: success for igt/tests: Fix error checking in kms_atomic_transition (rev4) 2019-02-19 9:38 [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition Stanislav Lisovskiy @ 2019-02-19 10:12 ` Patchwork 2019-02-19 12:18 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork 2019-02-19 21:05 ` [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition Summers, Stuart 2 siblings, 0 replies; 10+ messages in thread From: Patchwork @ 2019-02-19 10:12 UTC (permalink / raw) To: igt-dev == Series Details == Series: igt/tests: Fix error checking in kms_atomic_transition (rev4) URL : https://patchwork.freedesktop.org/series/56617/ State : success == Summary == CI Bug Log - changes from IGT_4837 -> IGTPW_2442 ==================================================== Summary ------- **SUCCESS** No regressions found. External URL: https://patchwork.freedesktop.org/api/1.0/series/56617/revisions/4/mbox/ Known issues ------------ Here are the changes found in IGTPW_2442 that come from known issues: ### IGT changes ### #### Issues hit #### * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence: - fi-byt-clapper: PASS -> FAIL [fdo#103191] / [fdo#107362] #### Possible fixes #### * igt@i915_selftest@live_evict: - fi-bsw-kefka: DMESG-WARN [fdo#107709] -> PASS * igt@i915_selftest@live_execlists: - {fi-icl-y}: INCOMPLETE [fdo#109567] -> PASS * igt@kms_chamelium@hdmi-hpd-fast: - fi-kbl-7500u: FAIL [fdo#109485] -> PASS * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a: - fi-byt-clapper: FAIL [fdo#103191] / [fdo#107362] -> PASS +1 * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b: - fi-blb-e6850: INCOMPLETE [fdo#107718] -> PASS {name}: This element is suppressed. This means it is ignored when computing the status of the difference (SUCCESS, WARNING, or FAILURE). [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191 [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362 [fdo#107709]: https://bugs.freedesktop.org/show_bug.cgi?id=107709 [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718 [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271 [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278 [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485 [fdo#109567]: https://bugs.freedesktop.org/show_bug.cgi?id=109567 Participating hosts (43 -> 39) ------------------------------ Additional (2): fi-kbl-7560u fi-snb-2600 Missing (6): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-apl-guc fi-whl-u fi-gdg-551 Build changes ------------- * IGT: IGT_4837 -> IGTPW_2442 CI_DRM_5629: e5bd5364ce58650dc664ee250629aa902dfe64ac @ git://anongit.freedesktop.org/gfx-ci/linux IGTPW_2442: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2442/ IGT_4837: 368e76156f752e6ed6ac32ed9f400567aef7d3fc @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2442/ _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 10+ messages in thread
* [igt-dev] ✓ Fi.CI.IGT: success for igt/tests: Fix error checking in kms_atomic_transition (rev4) 2019-02-19 9:38 [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition Stanislav Lisovskiy 2019-02-19 10:12 ` [igt-dev] ✓ Fi.CI.BAT: success for igt/tests: Fix error checking in kms_atomic_transition (rev4) Patchwork @ 2019-02-19 12:18 ` Patchwork 2019-02-19 21:05 ` [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition Summers, Stuart 2 siblings, 0 replies; 10+ messages in thread From: Patchwork @ 2019-02-19 12:18 UTC (permalink / raw) To: igt-dev == Series Details == Series: igt/tests: Fix error checking in kms_atomic_transition (rev4) URL : https://patchwork.freedesktop.org/series/56617/ State : success == Summary == CI Bug Log - changes from IGT_4837_full -> IGTPW_2442_full ==================================================== Summary ------- **SUCCESS** No regressions found. External URL: https://patchwork.freedesktop.org/api/1.0/series/56617/revisions/4/mbox/ Known issues ------------ Here are the changes found in IGTPW_2442_full that come from known issues: ### IGT changes ### #### Issues hit #### * igt@gem_ctx_param@invalid-param-get: - shard-snb: NOTRUN -> FAIL [fdo#109559] * igt@gem_exec_schedule@out-order-vebox: - shard-glk: PASS -> FAIL [fdo#109662] * igt@kms_busy@extended-modeset-hang-newfb-render-b: - shard-glk: NOTRUN -> DMESG-WARN [fdo#107956] * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a: - shard-snb: NOTRUN -> DMESG-WARN [fdo#107956] * igt@kms_cursor_crc@cursor-128x128-suspend: - shard-apl: PASS -> FAIL [fdo#103191] / [fdo#103232] * igt@kms_cursor_crc@cursor-256x256-suspend: - shard-kbl: PASS -> FAIL [fdo#103191] / [fdo#103232] * igt@kms_cursor_crc@cursor-64x21-random: - shard-apl: PASS -> FAIL [fdo#103232] +5 - shard-kbl: PASS -> FAIL [fdo#103232] +2 * igt@kms_flip@flip-vs-expired-vblank: - shard-glk: PASS -> FAIL [fdo#102887] / [fdo#105363] * igt@kms_flip@flip-vs-expired-vblank-interruptible: - shard-glk: PASS -> FAIL [fdo#105363] * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-pwrite: - shard-kbl: PASS -> FAIL [fdo#103167] * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-render: - shard-apl: PASS -> FAIL [fdo#103167] +1 * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-cpu: - shard-glk: PASS -> FAIL [fdo#103167] +8 * igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb: - shard-glk: PASS -> FAIL [fdo#108145] * igt@kms_plane_alpha_blend@pipe-c-alpha-basic: - shard-glk: NOTRUN -> FAIL [fdo#108145] * igt@kms_plane_multiple@atomic-pipe-a-tiling-y: - shard-glk: PASS -> FAIL [fdo#103166] +4 * igt@kms_setmode@basic: - shard-apl: PASS -> FAIL [fdo#99912] * igt@kms_universal_plane@universal-plane-pipe-a-functional: - shard-apl: PASS -> FAIL [fdo#103166] * igt@kms_vblank@pipe-b-ts-continuation-suspend: - shard-kbl: PASS -> INCOMPLETE [fdo#103665] * igt@pm_rpm@gem-execbuf-stress: - shard-kbl: PASS -> INCOMPLETE [fdo#103665] / [fdo#107803] / [fdo#107807] * igt@testdisplay: - shard-apl: PASS -> INCOMPLETE [fdo#103927] #### Possible fixes #### * igt@kms_busy@extended-modeset-hang-newfb-render-b: - shard-hsw: DMESG-WARN [fdo#107956] -> PASS * igt@kms_ccs@pipe-a-crc-sprite-planes-basic: - shard-apl: FAIL [fdo#106510] / [fdo#108145] -> PASS +1 - shard-glk: FAIL [fdo#108145] -> PASS * igt@kms_ccs@pipe-b-crc-sprite-planes-basic: - shard-kbl: FAIL [fdo#107725] / [fdo#108145] -> PASS +1 * igt@kms_cursor_crc@cursor-128x42-onscreen: - shard-apl: FAIL [fdo#103232] -> PASS +5 * igt@kms_cursor_crc@cursor-64x21-onscreen: - shard-kbl: FAIL [fdo#103232] -> PASS * igt@kms_cursor_crc@cursor-alpha-opaque: - shard-apl: FAIL [fdo#109350] -> PASS * igt@kms_flip@flip-vs-dpms-interruptible: - shard-hsw: INCOMPLETE [fdo#103540] -> PASS * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-onoff: - shard-apl: FAIL [fdo#103167] -> PASS * igt@kms_frontbuffer_tracking@fbc-1p-rte: - shard-glk: FAIL [fdo#103167] / [fdo#105682] -> PASS * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-onoff: - shard-glk: FAIL [fdo#103167] -> PASS +5 * igt@kms_plane@pixel-format-pipe-c-planes-source-clamping: - shard-apl: FAIL [fdo#108948] -> PASS * igt@kms_plane_multiple@atomic-pipe-c-tiling-x: - shard-glk: FAIL [fdo#103166] -> PASS +2 * igt@kms_plane_multiple@atomic-pipe-c-tiling-yf: - shard-apl: FAIL [fdo#103166] -> PASS +5 * igt@kms_setmode@basic: - shard-hsw: FAIL [fdo#99912] -> PASS * igt@pm_rc6_residency@rc6-accuracy: - shard-snb: {SKIP} [fdo#109271] -> PASS #### Warnings #### * igt@kms_dp_dsc@basic-dsc-enable-edp: - shard-snb: {SKIP} [fdo#109271] -> INCOMPLETE [fdo#105411] / [fdo#107469] {name}: This element is suppressed. This means it is ignored when computing the status of the difference (SUCCESS, WARNING, or FAILURE). [fdo#102887]: https://bugs.freedesktop.org/show_bug.cgi?id=102887 [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166 [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167 [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191 [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232 [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540 [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665 [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927 [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363 [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411 [fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682 [fdo#106510]: https://bugs.freedesktop.org/show_bug.cgi?id=106510 [fdo#107469]: https://bugs.freedesktop.org/show_bug.cgi?id=107469 [fdo#107725]: https://bugs.freedesktop.org/show_bug.cgi?id=107725 [fdo#107803]: https://bugs.freedesktop.org/show_bug.cgi?id=107803 [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807 [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956 [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145 [fdo#108948]: https://bugs.freedesktop.org/show_bug.cgi?id=108948 [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271 [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278 [fdo#109350]: https://bugs.freedesktop.org/show_bug.cgi?id=109350 [fdo#109559]: https://bugs.freedesktop.org/show_bug.cgi?id=109559 [fdo#109662]: https://bugs.freedesktop.org/show_bug.cgi?id=109662 [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912 Participating hosts (7 -> 5) ------------------------------ Missing (2): shard-skl shard-iclb Build changes ------------- * IGT: IGT_4837 -> IGTPW_2442 CI_DRM_5629: e5bd5364ce58650dc664ee250629aa902dfe64ac @ git://anongit.freedesktop.org/gfx-ci/linux IGTPW_2442: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2442/ IGT_4837: 368e76156f752e6ed6ac32ed9f400567aef7d3fc @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2442/ _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition 2019-02-19 9:38 [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition Stanislav Lisovskiy 2019-02-19 10:12 ` [igt-dev] ✓ Fi.CI.BAT: success for igt/tests: Fix error checking in kms_atomic_transition (rev4) Patchwork 2019-02-19 12:18 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork @ 2019-02-19 21:05 ` Summers, Stuart 2019-03-06 12:56 ` Petri Latvala 2019-04-03 14:23 ` Daniel Vetter 2 siblings, 2 replies; 10+ messages in thread From: Summers, Stuart @ 2019-02-19 21:05 UTC (permalink / raw) To: igt-dev, Lisovskiy, Stanislav Cc: norkernel, Syrjala, Ville, Heikkila, Juha-pekka, Peres, Martin [-- Attachment #1.1: Type: text/plain, Size: 2576 bytes --] On Tue, 2019-02-19 at 11:38 +0200, Stanislav Lisovskiy wrote: > From: Stanislav Lisovskiy <stanislav.lisovskiy@gmail.com> > > There is no guarantee that error return value will be > always EINVAL, made a check more general as it can be > ERANGE, ENOSPC, EINVAL and probably others, which all > mean the same in context of this test case: i.e this sprite > size is not valid. > > v2: Added macro to make check look a bit nicer. > v3: Removed redundant debug line. > v4: Added assertion if error is not EINVAL as expected, > other errors except EINVAL are considered now a failures. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109225 > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > tests/kms_atomic_transition.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/tests/kms_atomic_transition.c > b/tests/kms_atomic_transition.c > index 12bafe87..e1a2d4a0 100644 > --- a/tests/kms_atomic_transition.c > +++ b/tests/kms_atomic_transition.c > @@ -187,6 +187,12 @@ static void set_sprite_wh(igt_display_t > *display, enum pipe pipe, > LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb); > } > > +#define is_atomic_check_failure_errno(errno) \ > + (errno != -EINVAL && errno != 0) > + > +#define is_atomic_check_plane_size_errno(errno) \ > + (errno == -EINVAL) > + > static void setup_parms(igt_display_t *display, enum pipe pipe, > const drmModeModeInfo *mode, > struct igt_fb *primary_fb, > @@ -251,8 +257,9 @@ retry: > > wm_setup_plane(display, pipe, (1 << n_planes) - 1, > parms, false); > ret = igt_display_try_commit_atomic(display, > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); > + igt_assert(!is_atomic_check_failure_errno(ret)); > > - if (ret == -EINVAL) { > + if (is_atomic_check_plane_size_errno(ret)) { > if (cursor_width == sprite_width && > cursor_height == sprite_height) { > igt_assert_f(alpha, > @@ -442,7 +449,9 @@ run_transition_test(igt_display_t *display, enum > pipe pipe, igt_output_t *output > igt_pipe_request_out_fence(pipe_obj); > > ret = igt_display_try_commit_atomic(display, > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); > - if (ret != -EINVAL || pipe_obj->n_planes < 3) > + igt_assert(!is_atomic_check_failure_errno(ret)); > + > + if (!is_atomic_check_plane_size_errno(ret) || pipe_obj- > >n_planes < 3) > break; > > ret = 0; LGTM Reviewed-by: Stuart Summers <stuart.summers@intel.com> [-- Attachment #1.2: smime.p7s --] [-- Type: application/x-pkcs7-signature, Size: 3270 bytes --] [-- Attachment #2: Type: text/plain, Size: 153 bytes --] _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition 2019-02-19 21:05 ` [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition Summers, Stuart @ 2019-03-06 12:56 ` Petri Latvala 2019-03-06 14:52 ` Lisovskiy, Stanislav 2019-04-03 14:23 ` Daniel Vetter 1 sibling, 1 reply; 10+ messages in thread From: Petri Latvala @ 2019-03-06 12:56 UTC (permalink / raw) To: Summers, Stuart, Lisovskiy, Stanislav Cc: igt-dev, norkernel, Syrjala, Ville, Heikkila, Juha-pekka, Peres, Martin On Tue, Feb 19, 2019 at 09:05:12PM +0000, Summers, Stuart wrote: > > Reviewed-by: Stuart Summers <stuart.summers@intel.com> Merged, thanks for the patch and review. Stan, do you have IGT commit access yet? -- Petri Latvala _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition 2019-03-06 12:56 ` Petri Latvala @ 2019-03-06 14:52 ` Lisovskiy, Stanislav 0 siblings, 0 replies; 10+ messages in thread From: Lisovskiy, Stanislav @ 2019-03-06 14:52 UTC (permalink / raw) To: Summers, Stuart, Latvala, Petri Cc: igt-dev, norkernel, Syrjala, Ville, Heikkila, Juha-pekka, Peres, Martin On Wed, 2019-03-06 at 14:56 +0200, Petri Latvala wrote: > On Tue, Feb 19, 2019 at 09:05:12PM +0000, Summers, Stuart wrote: > > > > Reviewed-by: Stuart Summers <stuart.summers@intel.com> > > > Merged, thanks for the patch and review. > > Stan, do you have IGT commit access yet? I didn't try to commit anything, but I guess not. Best Regards, Lisovskiy Stanislav > > _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition 2019-02-19 21:05 ` [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition Summers, Stuart 2019-03-06 12:56 ` Petri Latvala @ 2019-04-03 14:23 ` Daniel Vetter 2019-04-04 7:33 ` Lisovskiy, Stanislav 1 sibling, 1 reply; 10+ messages in thread From: Daniel Vetter @ 2019-04-03 14:23 UTC (permalink / raw) To: Summers, Stuart Cc: Lisovskiy, Stanislav, Syrjala, Ville, norkernel, Peres, Martin, igt-dev, Heikkila, Juha-pekka On Tue, Feb 19, 2019 at 09:05:12PM +0000, Summers, Stuart wrote: > On Tue, 2019-02-19 at 11:38 +0200, Stanislav Lisovskiy wrote: > > From: Stanislav Lisovskiy <stanislav.lisovskiy@gmail.com> > > > > There is no guarantee that error return value will be > > always EINVAL, made a check more general as it can be > > ERANGE, ENOSPC, EINVAL and probably others, which all > > mean the same in context of this test case: i.e this sprite > > size is not valid. > > > > v2: Added macro to make check look a bit nicer. > > v3: Removed redundant debug line. > > v4: Added assertion if error is not EINVAL as expected, > > other errors except EINVAL are considered now a failures. Uh, commit message says to make the error message more general, and then proceeds to make it not more general. This kind of inconsistencies should be caught in review. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109225 > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> And I also don't understand how this patch fixed anything really, we still just check for EINVAL. -Daniel > > --- > > tests/kms_atomic_transition.c | 13 +++++++++++-- > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/tests/kms_atomic_transition.c > > b/tests/kms_atomic_transition.c > > index 12bafe87..e1a2d4a0 100644 > > --- a/tests/kms_atomic_transition.c > > +++ b/tests/kms_atomic_transition.c > > @@ -187,6 +187,12 @@ static void set_sprite_wh(igt_display_t > > *display, enum pipe pipe, > > LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb); > > } > > > > +#define is_atomic_check_failure_errno(errno) \ > > + (errno != -EINVAL && errno != 0) > > + > > +#define is_atomic_check_plane_size_errno(errno) \ > > + (errno == -EINVAL) > > + > > static void setup_parms(igt_display_t *display, enum pipe pipe, > > const drmModeModeInfo *mode, > > struct igt_fb *primary_fb, > > @@ -251,8 +257,9 @@ retry: > > > > wm_setup_plane(display, pipe, (1 << n_planes) - 1, > > parms, false); > > ret = igt_display_try_commit_atomic(display, > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); > > + igt_assert(!is_atomic_check_failure_errno(ret)); > > > > - if (ret == -EINVAL) { > > + if (is_atomic_check_plane_size_errno(ret)) { > > if (cursor_width == sprite_width && > > cursor_height == sprite_height) { > > igt_assert_f(alpha, > > @@ -442,7 +449,9 @@ run_transition_test(igt_display_t *display, enum > > pipe pipe, igt_output_t *output > > igt_pipe_request_out_fence(pipe_obj); > > > > ret = igt_display_try_commit_atomic(display, > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); > > - if (ret != -EINVAL || pipe_obj->n_planes < 3) > > + igt_assert(!is_atomic_check_failure_errno(ret)); > > + > > + if (!is_atomic_check_plane_size_errno(ret) || pipe_obj- > > >n_planes < 3) > > break; > > > > ret = 0; > > LGTM > > Reviewed-by: Stuart Summers <stuart.summers@intel.com> > _______________________________________________ > igt-dev mailing list > igt-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/igt-dev -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition 2019-04-03 14:23 ` Daniel Vetter @ 2019-04-04 7:33 ` Lisovskiy, Stanislav 2019-04-04 8:10 ` Daniel Vetter 0 siblings, 1 reply; 10+ messages in thread From: Lisovskiy, Stanislav @ 2019-04-04 7:33 UTC (permalink / raw) To: daniel, Summers, Stuart Cc: igt-dev, norkernel, Syrjala, Ville, Heikkila, Juha-pekka, Peres, Martin On Wed, 2019-04-03 at 16:23 +0200, Daniel Vetter wrote: > On Tue, Feb 19, 2019 at 09:05:12PM +0000, Summers, Stuart wrote: > > On Tue, 2019-02-19 at 11:38 +0200, Stanislav Lisovskiy wrote: > > > From: Stanislav Lisovskiy <stanislav.lisovskiy@gmail.com> > > > > > > There is no guarantee that error return value will be > > > always EINVAL, made a check more general as it can be > > > ERANGE, ENOSPC, EINVAL and probably others, which all > > > mean the same in context of this test case: i.e this sprite > > > size is not valid. > > > > > > v2: Added macro to make check look a bit nicer. > > > v3: Removed redundant debug line. > > > v4: Added assertion if error is not EINVAL as expected, > > > other errors except EINVAL are considered now a failures. > > Uh, commit message says to make the error message more general, and > then > proceeds to make it not more general. This kind of inconsistencies > should > be caught in review. Hi Daniel, As the commit message says, previsouly we were expecting only EINVAL(which indicates for us that we had reached max size of a plane) or 0. Problem was that in some circumstances, if we get something else test case did not catch that and proceeded as if everything is great, failing afterwards, when trying already to commit or other place. The thing which this patch does is as said "Added assertion if error is not EINVAL as expected, other errors except EINVAL are considered now a failures". Otherwise if we got EINVAL, we proceed normally. There was actually no purpose to "fix" something by that, but rather make it fail in a proper place and proper way, otherwise error just went further. My initial idea actually was to catch all kind of problems here, so that we always get some kind of working plane configuration here. However it was not accepted and currently only EINVAL is treated as "normal" error. If you have any proposals, how this can be improved sure I can do this. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109225 > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com > > > > > > And I also don't understand how this patch fixed anything really, we > still > just check for EINVAL. > -Daniel > > > > --- > > > tests/kms_atomic_transition.c | 13 +++++++++++-- > > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > > > diff --git a/tests/kms_atomic_transition.c > > > b/tests/kms_atomic_transition.c > > > index 12bafe87..e1a2d4a0 100644 > > > --- a/tests/kms_atomic_transition.c > > > +++ b/tests/kms_atomic_transition.c > > > @@ -187,6 +187,12 @@ static void set_sprite_wh(igt_display_t > > > *display, enum pipe pipe, > > > LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb); > > > } > > > > > > +#define is_atomic_check_failure_errno(errno) \ > > > + (errno != -EINVAL && errno != 0) > > > + > > > +#define is_atomic_check_plane_size_errno(errno) \ > > > + (errno == -EINVAL) > > > + > > > static void setup_parms(igt_display_t *display, enum pipe pipe, > > > const drmModeModeInfo *mode, > > > struct igt_fb *primary_fb, > > > @@ -251,8 +257,9 @@ retry: > > > > > > wm_setup_plane(display, pipe, (1 << n_planes) - 1, > > > parms, false); > > > ret = igt_display_try_commit_atomic(display, > > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); > > > + igt_assert(!is_atomic_check_failure_errno(ret)); > > > > > > - if (ret == -EINVAL) { > > > + if (is_atomic_check_plane_size_errno(ret)) { > > > if (cursor_width == sprite_width && > > > cursor_height == sprite_height) { > > > igt_assert_f(alpha, > > > @@ -442,7 +449,9 @@ run_transition_test(igt_display_t *display, > > > enum > > > pipe pipe, igt_output_t *output > > > igt_pipe_request_out_fence(pipe_obj); > > > > > > ret = igt_display_try_commit_atomic(display, > > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); > > > - if (ret != -EINVAL || pipe_obj->n_planes < 3) > > > + igt_assert(!is_atomic_check_failure_errno(ret)); > > > + > > > + if (!is_atomic_check_plane_size_errno(ret) || pipe_obj- > > > > n_planes < 3) > > > > > > break; > > > > > > ret = 0; > > > > LGTM > > > > Reviewed-by: Stuart Summers <stuart.summers@intel.com> > > > > > _______________________________________________ > > igt-dev mailing list > > igt-dev@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/igt-dev > > _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition 2019-04-04 7:33 ` Lisovskiy, Stanislav @ 2019-04-04 8:10 ` Daniel Vetter 2019-04-04 8:35 ` Lisovskiy, Stanislav 0 siblings, 1 reply; 10+ messages in thread From: Daniel Vetter @ 2019-04-04 8:10 UTC (permalink / raw) To: Lisovskiy, Stanislav Cc: norkernel, Syrjala, Ville, igt-dev, Peres, Martin, Heikkila, Juha-pekka, daniel On Thu, Apr 04, 2019 at 07:33:31AM +0000, Lisovskiy, Stanislav wrote: > On Wed, 2019-04-03 at 16:23 +0200, Daniel Vetter wrote: > > On Tue, Feb 19, 2019 at 09:05:12PM +0000, Summers, Stuart wrote: > > > On Tue, 2019-02-19 at 11:38 +0200, Stanislav Lisovskiy wrote: > > > > From: Stanislav Lisovskiy <stanislav.lisovskiy@gmail.com> > > > > > > > > There is no guarantee that error return value will be > > > > always EINVAL, made a check more general as it can be > > > > ERANGE, ENOSPC, EINVAL and probably others, which all > > > > mean the same in context of this test case: i.e this sprite > > > > size is not valid. > > > > > > > > v2: Added macro to make check look a bit nicer. > > > > v3: Removed redundant debug line. > > > > v4: Added assertion if error is not EINVAL as expected, > > > > other errors except EINVAL are considered now a failures. > > > > Uh, commit message says to make the error message more general, and > > then > > proceeds to make it not more general. This kind of inconsistencies > > should > > be caught in review. > > Hi Daniel, > > As the commit message says, previsouly we were expecting only > EINVAL(which indicates for > us that we had reached max size of a plane) or 0. > > Problem was that in some circumstances, if we get something else > test case did not catch that and proceeded as if everything is > great, failing afterwards, when trying already to commit or other > place. > The thing which this patch does is as said "Added assertion if error is > not EINVAL as expected, other errors except EINVAL are considered now a > failures". Otherwise if we got EINVAL, we proceed normally. > > There was actually no purpose to "fix" something by that, but rather > make it fail in a proper place and proper way, otherwise error just > went further. > > My initial idea actually was to catch all kind of problems here, so > that we always get some kind of working plane configuration here. > However it was not accepted and currently only EINVAL is treated as > "normal" error. > > If you have any proposals, how this can be improved sure I can do this. Yeah I think the patch is fine, but the commit message isn't the clearest, since it explains what exactly an interim version did, and then amends that. Which is confusing. You're explanation above would have made a much better commit message, but oh well the patch is committed so can't fix that up anymore. But good to heed for next time around, review should also make sure the commit message captures everything discussed as well as possible. Cheers, Daniel > > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109225 > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com > > > > > > > > > And I also don't understand how this patch fixed anything really, we > > still > > just check for EINVAL. > > -Daniel > > > > > > --- > > > > tests/kms_atomic_transition.c | 13 +++++++++++-- > > > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/tests/kms_atomic_transition.c > > > > b/tests/kms_atomic_transition.c > > > > index 12bafe87..e1a2d4a0 100644 > > > > --- a/tests/kms_atomic_transition.c > > > > +++ b/tests/kms_atomic_transition.c > > > > @@ -187,6 +187,12 @@ static void set_sprite_wh(igt_display_t > > > > *display, enum pipe pipe, > > > > LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb); > > > > } > > > > > > > > +#define is_atomic_check_failure_errno(errno) \ > > > > + (errno != -EINVAL && errno != 0) > > > > + > > > > +#define is_atomic_check_plane_size_errno(errno) \ > > > > + (errno == -EINVAL) > > > > + > > > > static void setup_parms(igt_display_t *display, enum pipe pipe, > > > > const drmModeModeInfo *mode, > > > > struct igt_fb *primary_fb, > > > > @@ -251,8 +257,9 @@ retry: > > > > > > > > wm_setup_plane(display, pipe, (1 << n_planes) - 1, > > > > parms, false); > > > > ret = igt_display_try_commit_atomic(display, > > > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); > > > > + igt_assert(!is_atomic_check_failure_errno(ret)); > > > > > > > > - if (ret == -EINVAL) { > > > > + if (is_atomic_check_plane_size_errno(ret)) { > > > > if (cursor_width == sprite_width && > > > > cursor_height == sprite_height) { > > > > igt_assert_f(alpha, > > > > @@ -442,7 +449,9 @@ run_transition_test(igt_display_t *display, > > > > enum > > > > pipe pipe, igt_output_t *output > > > > igt_pipe_request_out_fence(pipe_obj); > > > > > > > > ret = igt_display_try_commit_atomic(display, > > > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); > > > > - if (ret != -EINVAL || pipe_obj->n_planes < 3) > > > > + igt_assert(!is_atomic_check_failure_errno(ret)); > > > > + > > > > + if (!is_atomic_check_plane_size_errno(ret) || pipe_obj- > > > > > n_planes < 3) > > > > > > > > break; > > > > > > > > ret = 0; > > > > > > LGTM > > > > > > Reviewed-by: Stuart Summers <stuart.summers@intel.com> > > > > > > > > > _______________________________________________ > > > igt-dev mailing list > > > igt-dev@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/igt-dev > > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition 2019-04-04 8:10 ` Daniel Vetter @ 2019-04-04 8:35 ` Lisovskiy, Stanislav 0 siblings, 0 replies; 10+ messages in thread From: Lisovskiy, Stanislav @ 2019-04-04 8:35 UTC (permalink / raw) To: daniel Cc: norkernel, Syrjala, Ville, igt-dev, Peres, Martin, Heikkila, Juha-pekka On Thu, 2019-04-04 at 10:10 +0200, Daniel Vetter wrote: > On Thu, Apr 04, 2019 at 07:33:31AM +0000, Lisovskiy, Stanislav wrote: > > On Wed, 2019-04-03 at 16:23 +0200, Daniel Vetter wrote: > > > On Tue, Feb 19, 2019 at 09:05:12PM +0000, Summers, Stuart wrote: > > > > On Tue, 2019-02-19 at 11:38 +0200, Stanislav Lisovskiy wrote: > > > > > From: Stanislav Lisovskiy <stanislav.lisovskiy@gmail.com> > > > > > > > > > > There is no guarantee that error return value will be > > > > > always EINVAL, made a check more general as it can be > > > > > ERANGE, ENOSPC, EINVAL and probably others, which all > > > > > mean the same in context of this test case: i.e this sprite > > > > > size is not valid. > > > > > > > > > > v2: Added macro to make check look a bit nicer. > > > > > v3: Removed redundant debug line. > > > > > v4: Added assertion if error is not EINVAL as expected, > > > > > other errors except EINVAL are considered now a failures. > > > > > > Uh, commit message says to make the error message more general, > > > and > > > then > > > proceeds to make it not more general. This kind of > > > inconsistencies > > > should > > > be caught in review. > > > > Hi Daniel, > > > > As the commit message says, previsouly we were expecting only > > EINVAL(which indicates for > > us that we had reached max size of a plane) or 0. > > > > Problem was that in some circumstances, if we get something else > > test case did not catch that and proceeded as if everything is > > great, failing afterwards, when trying already to commit or other > > place. > > The thing which this patch does is as said "Added assertion if > > error is > > not EINVAL as expected, other errors except EINVAL are considered > > now a > > failures". Otherwise if we got EINVAL, we proceed normally. > > > > There was actually no purpose to "fix" something by that, but > > rather > > make it fail in a proper place and proper way, otherwise error just > > went further. > > > > My initial idea actually was to catch all kind of problems here, so > > that we always get some kind of working plane configuration here. > > However it was not accepted and currently only EINVAL is treated as > > "normal" error. > > > > If you have any proposals, how this can be improved sure I can do > > this. > > Yeah I think the patch is fine, but the commit message isn't the > clearest, > since it explains what exactly an interim version did, and then > amends > that. Which is confusing. > > You're explanation above would have made a much better commit > message, but > oh well the patch is committed so can't fix that up anymore. But good > to > heed for next time around, review should also make sure the commit > message > captures everything discussed as well as possible. > > Cheers, Daniel Well, yeah v4 revision says something different from initial commit message :) I just thought that I need to keep it for history. To me it is still at least arguable, if we really need to fail in setup_parms. Anyway, if the function purpose was to find a maximum size for sprite plane, should it really matter what was the error( ENOSPC, ERANGE, EINVAL) then if we are still able to find max plane size which works? > > > > > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109225 > > > > > Signed-off-by: Stanislav Lisovskiy < > > > > > stanislav.lisovskiy@intel.com > > > > > > > > > > > > And I also don't understand how this patch fixed anything really, > > > we > > > still > > > just check for EINVAL. > > > -Daniel > > > > > > > > --- > > > > > tests/kms_atomic_transition.c | 13 +++++++++++-- > > > > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/tests/kms_atomic_transition.c > > > > > b/tests/kms_atomic_transition.c > > > > > index 12bafe87..e1a2d4a0 100644 > > > > > --- a/tests/kms_atomic_transition.c > > > > > +++ b/tests/kms_atomic_transition.c > > > > > @@ -187,6 +187,12 @@ static void set_sprite_wh(igt_display_t > > > > > *display, enum pipe pipe, > > > > > LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb); > > > > > } > > > > > > > > > > +#define is_atomic_check_failure_errno(errno) \ > > > > > + (errno != -EINVAL && errno != 0) > > > > > + > > > > > +#define is_atomic_check_plane_size_errno(errno) \ > > > > > + (errno == -EINVAL) > > > > > + > > > > > static void setup_parms(igt_display_t *display, enum pipe > > > > > pipe, > > > > > const drmModeModeInfo *mode, > > > > > struct igt_fb *primary_fb, > > > > > @@ -251,8 +257,9 @@ retry: > > > > > > > > > > wm_setup_plane(display, pipe, (1 << n_planes) - > > > > > 1, > > > > > parms, false); > > > > > ret = igt_display_try_commit_atomic(display, > > > > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > NULL); > > > > > + igt_assert(!is_atomic_check_failure_errno(ret)) > > > > > ; > > > > > > > > > > - if (ret == -EINVAL) { > > > > > + if (is_atomic_check_plane_size_errno(ret)) { > > > > > if (cursor_width == sprite_width && > > > > > cursor_height == sprite_height) { > > > > > igt_assert_f(alpha, > > > > > @@ -442,7 +449,9 @@ run_transition_test(igt_display_t > > > > > *display, > > > > > enum > > > > > pipe pipe, igt_output_t *output > > > > > igt_pipe_request_out_fence(pipe_obj); > > > > > > > > > > ret = igt_display_try_commit_atomic(display, > > > > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > NULL); > > > > > - if (ret != -EINVAL || pipe_obj->n_planes < 3) > > > > > + igt_assert(!is_atomic_check_failure_errno(ret)) > > > > > ; > > > > > + > > > > > + if (!is_atomic_check_plane_size_errno(ret) || > > > > > pipe_obj- > > > > > > n_planes < 3) > > > > > > > > > > break; > > > > > > > > > > ret = 0; > > > > > > > > LGTM > > > > > > > > Reviewed-by: Stuart Summers <stuart.summers@intel.com> > > > > > > > > > > > > > _______________________________________________ > > > > igt-dev mailing list > > > > igt-dev@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/igt-dev > > > > > > > > _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-04-04 8:35 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-19 9:38 [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition Stanislav Lisovskiy 2019-02-19 10:12 ` [igt-dev] ✓ Fi.CI.BAT: success for igt/tests: Fix error checking in kms_atomic_transition (rev4) Patchwork 2019-02-19 12:18 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork 2019-02-19 21:05 ` [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition Summers, Stuart 2019-03-06 12:56 ` Petri Latvala 2019-03-06 14:52 ` Lisovskiy, Stanislav 2019-04-03 14:23 ` Daniel Vetter 2019-04-04 7:33 ` Lisovskiy, Stanislav 2019-04-04 8:10 ` Daniel Vetter 2019-04-04 8:35 ` Lisovskiy, Stanislav
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.