* [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: Before going testing check how many planes are allowed
@ 2019-04-02 14:42 Juha-Pekka Heikkila
2019-04-02 17:12 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
` (3 more replies)
0 siblings, 4 replies; 18+ messages in thread
From: Juha-Pekka Heikkila @ 2019-04-02 14:42 UTC (permalink / raw)
To: igt-dev; +Cc: Lisovskiy, Stanislav
Before start testing try out how many planes kernel will allow
simultaneously to be used.
Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
---
tests/kms_plane_multiple.c | 32 ++++++++++++++++++++++++++------
1 file changed, 26 insertions(+), 6 deletions(-)
diff --git a/tests/kms_plane_multiple.c b/tests/kms_plane_multiple.c
index bfaeede..5107d9d 100644
--- a/tests/kms_plane_multiple.c
+++ b/tests/kms_plane_multiple.c
@@ -260,7 +260,9 @@ test_plane_position_with_output(data_t *data, enum pipe pipe,
{
color_t blue = { 0.0f, 0.0f, 1.0f };
igt_crc_t crc;
+ igt_plane_t *plane;
int i;
+ int err, c = 0;
int iterations = opt.iterations < 1 ? 1 : opt.iterations;
bool loop_forever;
char info[256];
@@ -274,17 +276,35 @@ test_plane_position_with_output(data_t *data, enum pipe pipe,
iterations, iterations > 1 ? "iterations" : "iteration");
}
- igt_info("Testing connector %s using pipe %s with %d planes %s with seed %d\n",
- igt_output_name(output), kmstest_pipe_name(pipe), n_planes,
- info, opt.seed);
-
test_init(data, pipe, n_planes);
test_grab_crc(data, output, pipe, &blue, tiling);
+ /*
+ * Find out how many planes are allowed simultaneously
+ */
+ do {
+ c++;
+ prepare_planes(data, pipe, &blue, tiling, c, output);
+ err = igt_display_try_commit2(&data->display, COMMIT_ATOMIC);
+ } while (!err && c < n_planes);
+
+ if(err)
+ c--;
+
+ /*
+ * clean up failed state.
+ */
+ for_each_plane_on_pipe(&data->display, pipe, plane)
+ igt_plane_set_fb(plane, NULL);
+
+ igt_info("Testing connector %s using pipe %s with %d planes %s with seed %d\n",
+ igt_output_name(output), kmstest_pipe_name(pipe), c,
+ info, opt.seed);
+
i = 0;
- while (i < iterations || loop_forever) {
- prepare_planes(data, pipe, &blue, tiling, n_planes, output);
+ while ((i < iterations || loop_forever) && c > 0) {
+ prepare_planes(data, pipe, &blue, tiling, c, output);
igt_display_commit2(&data->display, COMMIT_ATOMIC);
--
2.7.4
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_plane_multiple: Before going testing check how many planes are allowed
2019-04-02 14:42 [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: Before going testing check how many planes are allowed Juha-Pekka Heikkila
@ 2019-04-02 17:12 ` Patchwork
2019-04-02 18:22 ` [igt-dev] [PATCH i-g-t] " Souza, Jose
` (2 subsequent siblings)
3 siblings, 0 replies; 18+ messages in thread
From: Patchwork @ 2019-04-02 17:12 UTC (permalink / raw)
To: Juha-Pekka Heikkila; +Cc: igt-dev
== Series Details ==
Series: tests/kms_plane_multiple: Before going testing check how many planes are allowed
URL : https://patchwork.freedesktop.org/series/58875/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_5856 -> IGTPW_2761
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/58875/revisions/1/mbox/
Known issues
------------
Here are the changes found in IGTPW_2761 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@amdgpu/amd_cs_nop@fork-compute0:
- fi-icl-y: NOTRUN -> SKIP [fdo#109315] +17
* igt@amdgpu/amd_cs_nop@fork-gfx0:
- fi-icl-u2: NOTRUN -> SKIP [fdo#109315] +17
* igt@gem_exec_basic@basic-bsd2:
- fi-kbl-7500u: NOTRUN -> SKIP [fdo#109271] +9
- fi-icl-y: NOTRUN -> SKIP [fdo#109276] +7
* igt@gem_exec_basic@readonly-bsd1:
- fi-icl-u2: NOTRUN -> SKIP [fdo#109276] +7
* igt@gem_exec_parse@basic-allowed:
- fi-icl-u2: NOTRUN -> SKIP [fdo#109289] +1
* igt@gem_exec_parse@basic-rejected:
- fi-icl-y: NOTRUN -> SKIP [fdo#109289] +1
* igt@i915_module_load@reload:
- fi-blb-e6850: PASS -> INCOMPLETE [fdo#107718]
* igt@i915_selftest@live_contexts:
- fi-icl-u2: NOTRUN -> DMESG-FAIL [fdo#108569]
- fi-icl-y: NOTRUN -> DMESG-FAIL [fdo#108569]
* igt@i915_selftest@live_execlists:
- fi-apl-guc: NOTRUN -> INCOMPLETE [fdo#103927] / [fdo#109720]
* igt@kms_chamelium@dp-crc-fast:
- fi-kbl-7500u: NOTRUN -> DMESG-WARN [fdo#103841]
- fi-icl-y: NOTRUN -> SKIP [fdo#109284] +8
* igt@kms_chamelium@dp-edid-read:
- fi-icl-u2: NOTRUN -> SKIP [fdo#109316] +2
* igt@kms_chamelium@vga-hpd-fast:
- fi-icl-u2: NOTRUN -> SKIP [fdo#109309] +1
* igt@kms_force_connector_basic@force-load-detect:
- fi-icl-y: NOTRUN -> SKIP [fdo#109285] +3
* igt@kms_force_connector_basic@prune-stale-modes:
- fi-icl-u2: NOTRUN -> SKIP [fdo#109285] +3
* igt@kms_frontbuffer_tracking@basic:
- fi-icl-u2: NOTRUN -> FAIL [fdo#103167]
* igt@kms_psr@primary_mmap_gtt:
- fi-icl-y: NOTRUN -> SKIP [fdo#110189] +3
- fi-byt-clapper: NOTRUN -> SKIP [fdo#109271] +23
* igt@kms_psr@primary_page_flip:
- fi-apl-guc: NOTRUN -> SKIP [fdo#109271] +50
* igt@prime_vgem@basic-fence-flip:
- fi-icl-y: NOTRUN -> SKIP [fdo#109294]
* igt@runner@aborted:
- fi-kbl-7500u: NOTRUN -> FAIL [fdo#103841]
- fi-apl-guc: NOTRUN -> FAIL [fdo#108622] / [fdo#109720]
#### Possible fixes ####
* igt@i915_selftest@live_contexts:
- fi-bdw-gvtdvm: DMESG-FAIL [fdo#110235 ] -> PASS
- fi-skl-gvtdvm: DMESG-FAIL [fdo#110235 ] -> PASS
* igt@i915_selftest@live_uncore:
- fi-skl-gvtdvm: DMESG-FAIL [fdo#110210] -> PASS
* igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
- fi-byt-clapper: FAIL [fdo#107362] -> PASS
* igt@kms_pipe_crc_basic@read-crc-pipe-b-frame-sequence:
- fi-byt-clapper: FAIL [fdo#103191] / [fdo#107362] -> PASS
* igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
- fi-byt-clapper: INCOMPLETE [fdo#102657] -> PASS
#### Warnings ####
* igt@i915_selftest@live_contexts:
- fi-icl-u3: INCOMPLETE [fdo#108569] -> DMESG-FAIL [fdo#108569]
[fdo#102657]: https://bugs.freedesktop.org/show_bug.cgi?id=102657
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
[fdo#103841]: https://bugs.freedesktop.org/show_bug.cgi?id=103841
[fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
[fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
[fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
[fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
[fdo#108622]: https://bugs.freedesktop.org/show_bug.cgi?id=108622
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
[fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
[fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
[fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
[fdo#109294]: https://bugs.freedesktop.org/show_bug.cgi?id=109294
[fdo#109309]: https://bugs.freedesktop.org/show_bug.cgi?id=109309
[fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
[fdo#109316]: https://bugs.freedesktop.org/show_bug.cgi?id=109316
[fdo#109720]: https://bugs.freedesktop.org/show_bug.cgi?id=109720
[fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
[fdo#110210]: https://bugs.freedesktop.org/show_bug.cgi?id=110210
[fdo#110235 ]: https://bugs.freedesktop.org/show_bug.cgi?id=110235
Participating hosts (43 -> 44)
------------------------------
Additional (4): fi-icl-y fi-icl-u2 fi-apl-guc fi-kbl-7500u
Missing (3): fi-ilk-m540 fi-byt-squawks fi-bdw-samus
Build changes
-------------
* IGT: IGT_4922 -> IGTPW_2761
CI_DRM_5856: 55074bd825098a71779cf65a69786547f0eccbe9 @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_2761: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2761/
IGT_4922: e941e4a29438c7130554492e4daf52afbc99ffdf @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2761/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: Before going testing check how many planes are allowed
2019-04-02 14:42 [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: Before going testing check how many planes are allowed Juha-Pekka Heikkila
2019-04-02 17:12 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
@ 2019-04-02 18:22 ` Souza, Jose
2019-04-03 11:26 ` Juha-Pekka Heikkila
2019-04-03 4:25 ` [igt-dev] ✓ Fi.CI.IGT: success for " Patchwork
2019-04-03 8:38 ` [igt-dev] [PATCH i-g-t] " Daniel Vetter
3 siblings, 1 reply; 18+ messages in thread
From: Souza, Jose @ 2019-04-02 18:22 UTC (permalink / raw)
To: juhapekka.heikkila, igt-dev; +Cc: Lisovskiy, Stanislav
[-- Attachment #1.1: Type: text/plain, Size: 3153 bytes --]
On Tue, 2019-04-02 at 17:42 +0300, Juha-Pekka Heikkila wrote:
> Before start testing try out how many planes kernel will allow
> simultaneously to be used.
>
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> ---
> tests/kms_plane_multiple.c | 32 ++++++++++++++++++++++++++------
> 1 file changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/tests/kms_plane_multiple.c b/tests/kms_plane_multiple.c
> index bfaeede..5107d9d 100644
> --- a/tests/kms_plane_multiple.c
> +++ b/tests/kms_plane_multiple.c
> @@ -260,7 +260,9 @@ test_plane_position_with_output(data_t *data,
> enum pipe pipe,
> {
> color_t blue = { 0.0f, 0.0f, 1.0f };
> igt_crc_t crc;
> + igt_plane_t *plane;
> int i;
> + int err, c = 0;
> int iterations = opt.iterations < 1 ? 1 : opt.iterations;
> bool loop_forever;
> char info[256];
> @@ -274,17 +276,35 @@ test_plane_position_with_output(data_t *data,
> enum pipe pipe,
> iterations, iterations > 1 ? "iterations" :
> "iteration");
> }
>
> - igt_info("Testing connector %s using pipe %s with %d planes %s
> with seed %d\n",
> - igt_output_name(output), kmstest_pipe_name(pipe),
> n_planes,
> - info, opt.seed);
> -
> test_init(data, pipe, n_planes);
>
> test_grab_crc(data, output, pipe, &blue, tiling);
>
> + /*
> + * Find out how many planes are allowed simultaneously
> + */
> + do {
> + c++;
> + prepare_planes(data, pipe, &blue, tiling, c, output);
> + err = igt_display_try_commit2(&data->display,
> COMMIT_ATOMIC);
> + } while (!err && c < n_planes);
> +
> + if(err)
> + c--;
"do {} while()" does the job but "for()" would be better suitable:
for (planes_allowed = 0; planes_allowed < n_planes; planes_allowed++) {
int err;
prepare_planes(data, pipe, &blue, tiling, planes_allowed,
output);
err = igt_display_try_commit2(&data->display, COMMIT_ATOMIC);
if (err)
break;
}
Also you are leaking a bunch o memory here, each call to
prepare_planes() will leak max_planes framebuffers, this could cause
test to fail due no GPU memory available.
Actually this test is already leaking a bunch as test_fini() is only
freeing the first framebuffer, there is other leaks here too like:
data->fb, prepare_planes().x, prepare_planes().y and
prepare_planes().size.
Can you take care of that too?
> +
> + /*
> + * clean up failed state.
> + */
> + for_each_plane_on_pipe(&data->display, pipe, plane)
> + igt_plane_set_fb(plane, NULL);
> +
> + igt_info("Testing connector %s using pipe %s with %d planes %s
> with seed %d\n",
> + igt_output_name(output), kmstest_pipe_name(pipe), c,
> + info, opt.seed);
> +
> i = 0;
> - while (i < iterations || loop_forever) {
> - prepare_planes(data, pipe, &blue, tiling, n_planes,
> output);
> + while ((i < iterations || loop_forever) && c > 0) {
We actually want to fail the test if it can not even test one plane, so
add this before this while()
igt_assert_lt(0, planes_allowed);
> + prepare_planes(data, pipe, &blue, tiling, c, output);
>
> igt_display_commit2(&data->display, COMMIT_ATOMIC);
>
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 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] 18+ messages in thread
* [igt-dev] ✓ Fi.CI.IGT: success for tests/kms_plane_multiple: Before going testing check how many planes are allowed
2019-04-02 14:42 [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: Before going testing check how many planes are allowed Juha-Pekka Heikkila
2019-04-02 17:12 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
2019-04-02 18:22 ` [igt-dev] [PATCH i-g-t] " Souza, Jose
@ 2019-04-03 4:25 ` Patchwork
2019-04-03 8:38 ` [igt-dev] [PATCH i-g-t] " Daniel Vetter
3 siblings, 0 replies; 18+ messages in thread
From: Patchwork @ 2019-04-03 4:25 UTC (permalink / raw)
To: Juha-Pekka Heikkila; +Cc: igt-dev
== Series Details ==
Series: tests/kms_plane_multiple: Before going testing check how many planes are allowed
URL : https://patchwork.freedesktop.org/series/58875/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_5856_full -> IGTPW_2761_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/58875/revisions/1/mbox/
Known issues
------------
Here are the changes found in IGTPW_2761_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_exec_store@cachelines-bsd2:
- shard-glk: NOTRUN -> SKIP [fdo#109271] +19
* igt@kms_atomic_transition@1x-modeset-transitions-nonblocking:
- shard-apl: PASS -> FAIL [fdo#109660]
* igt@kms_atomic_transition@3x-modeset-transitions:
- shard-hsw: NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +3
* igt@kms_available_modes_crc@available_mode_test_crc:
- shard-snb: NOTRUN -> FAIL [fdo#106641]
* igt@kms_busy@basic-modeset-e:
- shard-kbl: NOTRUN -> SKIP [fdo#109271] / [fdo#109278]
* igt@kms_busy@extended-modeset-hang-newfb-render-a:
- shard-hsw: PASS -> DMESG-WARN [fdo#110222] +2
* igt@kms_chamelium@common-hpd-after-suspend:
- shard-hsw: NOTRUN -> SKIP [fdo#109271] +20
* igt@kms_cursor_crc@cursor-64x64-suspend:
- shard-apl: PASS -> DMESG-FAIL [fdo#103232]
- shard-kbl: PASS -> FAIL [fdo#103191] / [fdo#103232]
* igt@kms_flip@2x-wf_vblank-ts-check:
- shard-snb: NOTRUN -> SKIP [fdo#109271] +69
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-gtt:
- shard-apl: PASS -> FAIL [fdo#103167]
- shard-kbl: PASS -> FAIL [fdo#103167]
* igt@kms_frontbuffer_tracking@fbc-2p-primscrn-pri-shrfb-draw-mmap-cpu:
- shard-glk: PASS -> FAIL [fdo#103167]
* igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-cur-indfb-draw-mmap-wc:
- shard-kbl: NOTRUN -> SKIP [fdo#109271] +24
* igt@kms_lease@atomic_implicit_crtc:
- shard-kbl: NOTRUN -> FAIL [fdo#110279]
* igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
- shard-apl: PASS -> DMESG-WARN [fdo#108566] +1
* igt@kms_plane_alpha_blend@pipe-a-alpha-7efc:
- shard-kbl: NOTRUN -> FAIL [fdo#108145] / [fdo#108590]
* igt@kms_plane_scaling@pipe-a-scaler-with-clipping-clamping:
- shard-glk: PASS -> SKIP [fdo#109271] / [fdo#109278] +1
* igt@kms_plane_scaling@pipe-b-scaler-with-clipping-clamping:
- shard-glk: NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +1
* igt@kms_setmode@basic:
- shard-apl: PASS -> FAIL [fdo#99912]
- shard-kbl: PASS -> FAIL [fdo#99912]
* igt@kms_vblank@pipe-a-ts-continuation-dpms-rpm:
- shard-snb: NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +9
* igt@kms_vblank@pipe-b-ts-continuation-modeset-hang:
- shard-apl: PASS -> FAIL [fdo#104894]
#### Possible fixes ####
* igt@gem_create@create-clear:
- shard-snb: INCOMPLETE [fdo#105411] -> PASS
* igt@kms_cursor_legacy@2x-long-nonblocking-modeset-vs-cursor-atomic:
- shard-glk: FAIL [fdo#106509] / [fdo#107409] -> PASS
* igt@kms_plane_alpha_blend@pipe-c-alpha-opaque-fb:
- shard-apl: FAIL [fdo#108145] -> PASS
* igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend:
- shard-kbl: INCOMPLETE [fdo#103665] -> PASS
* igt@kms_vblank@pipe-c-ts-continuation-dpms-suspend:
- shard-apl: FAIL [fdo#104894] -> PASS
#### Warnings ####
* igt@kms_frontbuffer_tracking@psr-2p-primscrn-pri-shrfb-draw-render:
- shard-hsw: INCOMPLETE [fdo#103540] -> SKIP [fdo#109271]
* igt@perf_pmu@semaphore-wait-bcs0:
- shard-hsw: SKIP [fdo#109271] -> INCOMPLETE [fdo#103540]
[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#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894
[fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
[fdo#106509]: https://bugs.freedesktop.org/show_bug.cgi?id=106509
[fdo#106641]: https://bugs.freedesktop.org/show_bug.cgi?id=106641
[fdo#107409]: https://bugs.freedesktop.org/show_bug.cgi?id=107409
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
[fdo#108590]: https://bugs.freedesktop.org/show_bug.cgi?id=108590
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
[fdo#109660]: https://bugs.freedesktop.org/show_bug.cgi?id=109660
[fdo#110222]: https://bugs.freedesktop.org/show_bug.cgi?id=110222
[fdo#110279]: https://bugs.freedesktop.org/show_bug.cgi?id=110279
[fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
Participating hosts (10 -> 5)
------------------------------
Missing (5): shard-skl pig-hsw-4770r pig-glk-j5005 shard-iclb pig-skl-6260u
Build changes
-------------
* IGT: IGT_4922 -> IGTPW_2761
* Piglit: piglit_4509 -> None
CI_DRM_5856: 55074bd825098a71779cf65a69786547f0eccbe9 @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_2761: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2761/
IGT_4922: e941e4a29438c7130554492e4daf52afbc99ffdf @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2761/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: Before going testing check how many planes are allowed
2019-04-02 14:42 [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: Before going testing check how many planes are allowed Juha-Pekka Heikkila
` (2 preceding siblings ...)
2019-04-03 4:25 ` [igt-dev] ✓ Fi.CI.IGT: success for " Patchwork
@ 2019-04-03 8:38 ` Daniel Vetter
2019-04-03 12:23 ` Juha-Pekka Heikkila
3 siblings, 1 reply; 18+ messages in thread
From: Daniel Vetter @ 2019-04-03 8:38 UTC (permalink / raw)
To: Juha-Pekka Heikkila; +Cc: igt-dev, Lisovskiy, Stanislav
On Tue, Apr 02, 2019 at 05:42:30PM +0300, Juha-Pekka Heikkila wrote:
> Before start testing try out how many planes kernel will allow
> simultaneously to be used.
This isn't enough, kernel might have other limits than just total number
of planes. In the future this might depend upon where they are actually
placed, how big, and all that stuff.
Also, we need to randomize which planes we pick, so needs an
igt_permute_array somewhere to avoid always using the same first planes.
-Daniel
>
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> ---
> tests/kms_plane_multiple.c | 32 ++++++++++++++++++++++++++------
> 1 file changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/tests/kms_plane_multiple.c b/tests/kms_plane_multiple.c
> index bfaeede..5107d9d 100644
> --- a/tests/kms_plane_multiple.c
> +++ b/tests/kms_plane_multiple.c
> @@ -260,7 +260,9 @@ test_plane_position_with_output(data_t *data, enum pipe pipe,
> {
> color_t blue = { 0.0f, 0.0f, 1.0f };
> igt_crc_t crc;
> + igt_plane_t *plane;
> int i;
> + int err, c = 0;
> int iterations = opt.iterations < 1 ? 1 : opt.iterations;
> bool loop_forever;
> char info[256];
> @@ -274,17 +276,35 @@ test_plane_position_with_output(data_t *data, enum pipe pipe,
> iterations, iterations > 1 ? "iterations" : "iteration");
> }
>
> - igt_info("Testing connector %s using pipe %s with %d planes %s with seed %d\n",
> - igt_output_name(output), kmstest_pipe_name(pipe), n_planes,
> - info, opt.seed);
> -
> test_init(data, pipe, n_planes);
>
> test_grab_crc(data, output, pipe, &blue, tiling);
>
> + /*
> + * Find out how many planes are allowed simultaneously
> + */
> + do {
> + c++;
> + prepare_planes(data, pipe, &blue, tiling, c, output);
> + err = igt_display_try_commit2(&data->display, COMMIT_ATOMIC);
> + } while (!err && c < n_planes);
> +
> + if(err)
> + c--;
> +
> + /*
> + * clean up failed state.
> + */
> + for_each_plane_on_pipe(&data->display, pipe, plane)
> + igt_plane_set_fb(plane, NULL);
> +
> + igt_info("Testing connector %s using pipe %s with %d planes %s with seed %d\n",
> + igt_output_name(output), kmstest_pipe_name(pipe), c,
> + info, opt.seed);
> +
> i = 0;
> - while (i < iterations || loop_forever) {
> - prepare_planes(data, pipe, &blue, tiling, n_planes, output);
> + while ((i < iterations || loop_forever) && c > 0) {
> + prepare_planes(data, pipe, &blue, tiling, c, output);
>
> igt_display_commit2(&data->display, COMMIT_ATOMIC);
>
> --
> 2.7.4
>
> _______________________________________________
> 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] 18+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: Before going testing check how many planes are allowed
2019-04-02 18:22 ` [igt-dev] [PATCH i-g-t] " Souza, Jose
@ 2019-04-03 11:26 ` Juha-Pekka Heikkila
0 siblings, 0 replies; 18+ messages in thread
From: Juha-Pekka Heikkila @ 2019-04-03 11:26 UTC (permalink / raw)
To: Souza, Jose, igt-dev; +Cc: Lisovskiy, Stanislav
On 2.4.2019 21.22, Souza, Jose wrote:
> On Tue, 2019-04-02 at 17:42 +0300, Juha-Pekka Heikkila wrote:
>> Before start testing try out how many planes kernel will allow
>> simultaneously to be used.
>>
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>> ---
>> tests/kms_plane_multiple.c | 32 ++++++++++++++++++++++++++------
>> 1 file changed, 26 insertions(+), 6 deletions(-)
>>
>> diff --git a/tests/kms_plane_multiple.c b/tests/kms_plane_multiple.c
>> index bfaeede..5107d9d 100644
>> --- a/tests/kms_plane_multiple.c
>> +++ b/tests/kms_plane_multiple.c
>> @@ -260,7 +260,9 @@ test_plane_position_with_output(data_t *data,
>> enum pipe pipe,
>> {
>> color_t blue = { 0.0f, 0.0f, 1.0f };
>> igt_crc_t crc;
>> + igt_plane_t *plane;
>> int i;
>> + int err, c = 0;
>> int iterations = opt.iterations < 1 ? 1 : opt.iterations;
>> bool loop_forever;
>> char info[256];
>> @@ -274,17 +276,35 @@ test_plane_position_with_output(data_t *data,
>> enum pipe pipe,
>> iterations, iterations > 1 ? "iterations" :
>> "iteration");
>> }
>>
>> - igt_info("Testing connector %s using pipe %s with %d planes %s
>> with seed %d\n",
>> - igt_output_name(output), kmstest_pipe_name(pipe),
>> n_planes,
>> - info, opt.seed);
>> -
>> test_init(data, pipe, n_planes);
>>
>> test_grab_crc(data, output, pipe, &blue, tiling);
>>
>> + /*
>> + * Find out how many planes are allowed simultaneously
>> + */
>> + do {
>> + c++;
>> + prepare_planes(data, pipe, &blue, tiling, c, output);
>> + err = igt_display_try_commit2(&data->display,
>> COMMIT_ATOMIC);
>> + } while (!err && c < n_planes);
>> +
>> + if(err)
>> + c--;
>
> "do {} while()" does the job but "for()" would be better suitable:
>
> for (planes_allowed = 0; planes_allowed < n_planes; planes_allowed++) {
> int err;
>
> prepare_planes(data, pipe, &blue, tiling, planes_allowed,
> output);
> err = igt_display_try_commit2(&data->display, COMMIT_ATOMIC);
> if (err)
> break;
> }
I don't have strong preference on this issue. For such loops I normally
use do{}..while() to make it certain loop is ran at least once in any
situation and underline it for 'the next guy'.
Anyway your for loop wouldn't do the trick. I think it will become
somewhat more complex and need different handling for 'all planes did
work' and 'not all planes did work'. Loop cannot start with zero planes,
then if n_planes is just one it will never be tested unless specially
taken care of.
>
>
> Also you are leaking a bunch o memory here, each call to
> prepare_planes() will leak max_planes framebuffers, this could cause
> test to fail due no GPU memory available.
>
> Actually this test is already leaking a bunch as test_fini() is only
> freeing the first framebuffer, there is other leaks here too like:
> data->fb, prepare_planes().x, prepare_planes().y and
> prepare_planes().size.
>
> Can you take care of that too?
>
yea, I did see those in Valgrind already while writing this patch.
Reason why I didn't go after them yet is below 'clean up failed state'
is something I shouldn't need to do. Last commit in my loop can fail and
IGT has mechanism to do cleaning, for some reason IGT mechanism misses
it here. I want to find out how IGT lost the cleaning information and
this test can show it to me.
>
>> +
>> + /*
>> + * clean up failed state.
>> + */
>> + for_each_plane_on_pipe(&data->display, pipe, plane)
>> + igt_plane_set_fb(plane, NULL);
>> +
>> + igt_info("Testing connector %s using pipe %s with %d planes %s
>> with seed %d\n",
>> + igt_output_name(output), kmstest_pipe_name(pipe), c,
>> + info, opt.seed);
>> +
>> i = 0;
>> - while (i < iterations || loop_forever) {
>> - prepare_planes(data, pipe, &blue, tiling, n_planes,
>> output);
>> + while ((i < iterations || loop_forever) && c > 0) {
>
> We actually want to fail the test if it can not even test one plane, so
> add this before this while()
>
> igt_assert_lt(0, planes_allowed);
On after thought checking here for zero is useless, I'll throw it away.
If there was not a single plane this test would've asserted already
during setup where it get reference crc.
>
>> + prepare_planes(data, pipe, &blue, tiling, c, output);
>>
>> igt_display_commit2(&data->display, COMMIT_ATOMIC);
>>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: Before going testing check how many planes are allowed
2019-04-03 8:38 ` [igt-dev] [PATCH i-g-t] " Daniel Vetter
@ 2019-04-03 12:23 ` Juha-Pekka Heikkila
0 siblings, 0 replies; 18+ messages in thread
From: Juha-Pekka Heikkila @ 2019-04-03 12:23 UTC (permalink / raw)
To: Daniel Vetter; +Cc: igt-dev, Lisovskiy, Stanislav
On 3.4.2019 11.38, Daniel Vetter wrote:
> On Tue, Apr 02, 2019 at 05:42:30PM +0300, Juha-Pekka Heikkila wrote:
>> Before start testing try out how many planes kernel will allow
>> simultaneously to be used.
>
> This isn't enough, kernel might have other limits than just total number
> of planes. In the future this might depend upon where they are actually
> placed, how big, and all that stuff.
I think that sound like new subtest for this test. This test does
randomize placing of planes already though.
>
> Also, we need to randomize which planes we pick, so needs an
> igt_permute_array somewhere to avoid always using the same first planes.
that sound like good idea, I'll include it into v2.
> -Daniel
>
>>
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>> ---
>> tests/kms_plane_multiple.c | 32 ++++++++++++++++++++++++++------
>> 1 file changed, 26 insertions(+), 6 deletions(-)
>>
>> diff --git a/tests/kms_plane_multiple.c b/tests/kms_plane_multiple.c
>> index bfaeede..5107d9d 100644
>> --- a/tests/kms_plane_multiple.c
>> +++ b/tests/kms_plane_multiple.c
>> @@ -260,7 +260,9 @@ test_plane_position_with_output(data_t *data, enum pipe pipe,
>> {
>> color_t blue = { 0.0f, 0.0f, 1.0f };
>> igt_crc_t crc;
>> + igt_plane_t *plane;
>> int i;
>> + int err, c = 0;
>> int iterations = opt.iterations < 1 ? 1 : opt.iterations;
>> bool loop_forever;
>> char info[256];
>> @@ -274,17 +276,35 @@ test_plane_position_with_output(data_t *data, enum pipe pipe,
>> iterations, iterations > 1 ? "iterations" : "iteration");
>> }
>>
>> - igt_info("Testing connector %s using pipe %s with %d planes %s with seed %d\n",
>> - igt_output_name(output), kmstest_pipe_name(pipe), n_planes,
>> - info, opt.seed);
>> -
>> test_init(data, pipe, n_planes);
>>
>> test_grab_crc(data, output, pipe, &blue, tiling);
>>
>> + /*
>> + * Find out how many planes are allowed simultaneously
>> + */
>> + do {
>> + c++;
>> + prepare_planes(data, pipe, &blue, tiling, c, output);
>> + err = igt_display_try_commit2(&data->display, COMMIT_ATOMIC);
>> + } while (!err && c < n_planes);
>> +
>> + if(err)
>> + c--;
>> +
>> + /*
>> + * clean up failed state.
>> + */
>> + for_each_plane_on_pipe(&data->display, pipe, plane)
>> + igt_plane_set_fb(plane, NULL);
>> +
>> + igt_info("Testing connector %s using pipe %s with %d planes %s with seed %d\n",
>> + igt_output_name(output), kmstest_pipe_name(pipe), c,
>> + info, opt.seed);
>> +
>> i = 0;
>> - while (i < iterations || loop_forever) {
>> - prepare_planes(data, pipe, &blue, tiling, n_planes, output);
>> + while ((i < iterations || loop_forever) && c > 0) {
>> + prepare_planes(data, pipe, &blue, tiling, c, output);
>>
>> igt_display_commit2(&data->display, COMMIT_ATOMIC);
>>
>> --
>> 2.7.4
>>
>> _______________________________________________
>> 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] 18+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: before going testing check how many planes are allowed
2019-05-29 13:39 ` Lisovskiy, Stanislav
@ 2019-06-04 10:26 ` Kahola, Mika
0 siblings, 0 replies; 18+ messages in thread
From: Kahola, Mika @ 2019-06-04 10:26 UTC (permalink / raw)
To: juhapekka.heikkila, igt-dev, Lisovskiy, Stanislav
On Wed, 2019-05-29 at 13:39 +0000, Lisovskiy, Stanislav wrote:
> On Fri, 2019-04-05 at 17:55 +0300, Juha-Pekka Heikkila wrote:
>
>
> With current failures we need this fixed.
>
> Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Patch is now merged to IGT. Thanks for the patch and a review!
Cheers,
Mika
>
> > before start testing try out how many planes kernel will
> > allow simultaneously to be used.
> >
> > v2 (Jose Souza, Daniel Vetter): Fix resource leaks. Randomize
> > used planes.
> >
> > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > ---
> > tests/kms_plane_multiple.c | 88
> > +++++++++++++++++++++++++++++++++++++---------
> > 1 file changed, 71 insertions(+), 17 deletions(-)
> >
> > diff --git a/tests/kms_plane_multiple.c
> > b/tests/kms_plane_multiple.c
> > index bfaeede..f2707dc 100644
> > --- a/tests/kms_plane_multiple.c
> > +++ b/tests/kms_plane_multiple.c
> > @@ -80,16 +80,6 @@ static void test_fini(data_t *data, igt_output_t
> > *output, int n_planes)
> > {
> > igt_pipe_crc_stop(data->pipe_crc);
> >
> > - for (int i = 0; i < n_planes; i++) {
> > - igt_plane_t *plane = data->plane[i];
> > - if (!plane)
> > - continue;
> > - if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > - continue;
> > - igt_plane_set_fb(plane, NULL);
> > - data->plane[i] = NULL;
> > - }
> > -
> > /* reset the constraint on the pipe */
> > igt_output_set_pipe(output, PIPE_ANY);
> >
> > @@ -99,7 +89,8 @@ static void test_fini(data_t *data, igt_output_t
> > *output, int n_planes)
> > free(data->plane);
> > data->plane = NULL;
> >
> > - igt_remove_fb(data->drm_fd, data->fb);
> > + free(data->fb);
> > + data->fb = NULL;
> >
> > igt_display_reset(&data->display);
> > }
> > @@ -195,6 +186,7 @@ prepare_planes(data_t *data, enum pipe pipe_id,
> > color_t *color,
> > int *y;
> > int *size;
> > int i;
> > + int* suffle;
> >
> > igt_output_set_pipe(output, pipe_id);
> > primary = igt_output_get_plane_type(output,
> > DRM_PLANE_TYPE_PRIMARY);
> > @@ -206,6 +198,34 @@ prepare_planes(data_t *data, enum pipe
> > pipe_id,
> > color_t *color,
> > igt_assert_f(y, "Failed to allocate %ld bytes for variable
> > y\n", (long int) (pipe->n_planes * sizeof(*y)));
> > size = malloc(pipe->n_planes * sizeof(*size));
> > igt_assert_f(size, "Failed to allocate %ld bytes for variable
> > size\n", (long int) (pipe->n_planes * sizeof(*size)));
> > + suffle = malloc(pipe->n_planes * sizeof(*suffle));
> > + igt_assert_f(suffle, "Failed to allocate %ld bytes for variable
> > size\n", (long int) (pipe->n_planes * sizeof(*suffle)));
> > +
> > + for (i = 0; i < pipe->n_planes; i++)
> > + suffle[i] = i;
> > +
> > + /*
> > + * suffle table for planes. using rand() should keep it
> > + * 'randomized in expected way'
> > + */
> > + for (i = 0; i < 256; i++) {
> > + int n, m;
> > + int a, b;
> > +
> > + n = rand() % (pipe->n_planes-1);
> > + m = rand() % (pipe->n_planes-1);
> > +
> > + /*
> > + * keep primary plane at its place for test's sake.
> > + */
> > + if(n == primary->index || m == primary->index)
> > + continue;
> > +
> > + a = suffle[n];
> > + b = suffle[m];
> > + suffle[n] = b;
> > + suffle[m] = a;
> > + }
> >
> > mode = igt_output_get_mode(output);
> >
> > @@ -213,7 +233,11 @@ prepare_planes(data_t *data, enum pipe
> > pipe_id,
> > color_t *color,
> > x[primary->index] = 0;
> > y[primary->index] = 0;
> > for (i = 0; i < max_planes; i++) {
> > - igt_plane_t *plane = igt_output_get_plane(output, i);
> > + /*
> > + * Here is made assumption primary plane will have
> > + * index zero.
> > + */
> > + igt_plane_t *plane = igt_output_get_plane(output,
> > suffle[i]);
> > uint32_t plane_format;
> > uint64_t plane_tiling;
> >
> > @@ -251,6 +275,10 @@ prepare_planes(data_t *data, enum pipe
> > pipe_id,
> > color_t *color,
> > create_fb_for_mode_position(data, output, mode, color, x, y,
> > size, size, tiling, max_planes);
> > igt_plane_set_fb(data->plane[primary->index], &data-
> > > fb[primary->index]);
> >
> > + free((void*)x);
> > + free((void*)y);
> > + free((void*)size);
> > + free((void*)suffle);
> > }
> >
> > static void
> > @@ -260,7 +288,9 @@ test_plane_position_with_output(data_t *data,
> > enum pipe pipe,
> > {
> > color_t blue = { 0.0f, 0.0f, 1.0f };
> > igt_crc_t crc;
> > + igt_plane_t *plane;
> > int i;
> > + int err, c = 0;
> > int iterations = opt.iterations < 1 ? 1 : opt.iterations;
> > bool loop_forever;
> > char info[256];
> > @@ -274,22 +304,46 @@ test_plane_position_with_output(data_t *data,
> > enum pipe pipe,
> > iterations, iterations > 1 ? "iterations" :
> > "iteration");
> > }
> >
> > - igt_info("Testing connector %s using pipe %s with %d planes %s
> > with seed %d\n",
> > - igt_output_name(output), kmstest_pipe_name(pipe),
> > n_planes,
> > - info, opt.seed);
> > -
> > test_init(data, pipe, n_planes);
> >
> > test_grab_crc(data, output, pipe, &blue, tiling);
> >
> > + /*
> > + * Find out how many planes are allowed simultaneously
> > + */
> > + do {
> > + c++;
> > + prepare_planes(data, pipe, &blue, tiling, c, output);
> > + err = igt_display_try_commit2(&data->display,
> > COMMIT_ATOMIC);
> > +
> > + for_each_plane_on_pipe(&data->display, pipe, plane)
> > + igt_plane_set_fb(plane, NULL);
> > +
> > + for (int x = 0; x < c; x++)
> > + igt_remove_fb(data->drm_fd, &data->fb[x]);
> > + } while (!err && c < n_planes);
> > +
> > + if(err)
> > + c--;
> > +
> > + igt_info("Testing connector %s using pipe %s with %d planes %s
> > with seed %d\n",
> > + igt_output_name(output), kmstest_pipe_name(pipe), c,
> > + info, opt.seed);
> > +
> > i = 0;
> > while (i < iterations || loop_forever) {
> > - prepare_planes(data, pipe, &blue, tiling, n_planes,
> > output);
> > + prepare_planes(data, pipe, &blue, tiling, c, output);
> >
> > igt_display_commit2(&data->display, COMMIT_ATOMIC);
> >
> > igt_pipe_crc_get_current(data->display.drm_fd, data-
> > > pipe_crc, &crc);
> >
> >
> > + for_each_plane_on_pipe(&data->display, pipe, plane)
> > + igt_plane_set_fb(plane, NULL);
> > +
> > + for (int x = 0; x < c; x++)
> > + igt_remove_fb(data->drm_fd, &data->fb[x]);
> > +
> > igt_assert_crc_equal(&data->ref_crc, &crc);
> >
> > i++;
>
> _______________________________________________
> 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] 18+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: before going testing check how many planes are allowed
2019-04-05 14:55 [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: before " Juha-Pekka Heikkila
2019-04-09 1:29 ` Souza, Jose
@ 2019-05-29 13:39 ` Lisovskiy, Stanislav
2019-06-04 10:26 ` Kahola, Mika
1 sibling, 1 reply; 18+ messages in thread
From: Lisovskiy, Stanislav @ 2019-05-29 13:39 UTC (permalink / raw)
To: juhapekka.heikkila, igt-dev
On Fri, 2019-04-05 at 17:55 +0300, Juha-Pekka Heikkila wrote:
With current failures we need this fixed.
Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> before start testing try out how many planes kernel will
> allow simultaneously to be used.
>
> v2 (Jose Souza, Daniel Vetter): Fix resource leaks. Randomize
> used planes.
>
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> ---
> tests/kms_plane_multiple.c | 88
> +++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 71 insertions(+), 17 deletions(-)
>
> diff --git a/tests/kms_plane_multiple.c b/tests/kms_plane_multiple.c
> index bfaeede..f2707dc 100644
> --- a/tests/kms_plane_multiple.c
> +++ b/tests/kms_plane_multiple.c
> @@ -80,16 +80,6 @@ static void test_fini(data_t *data, igt_output_t
> *output, int n_planes)
> {
> igt_pipe_crc_stop(data->pipe_crc);
>
> - for (int i = 0; i < n_planes; i++) {
> - igt_plane_t *plane = data->plane[i];
> - if (!plane)
> - continue;
> - if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> - continue;
> - igt_plane_set_fb(plane, NULL);
> - data->plane[i] = NULL;
> - }
> -
> /* reset the constraint on the pipe */
> igt_output_set_pipe(output, PIPE_ANY);
>
> @@ -99,7 +89,8 @@ static void test_fini(data_t *data, igt_output_t
> *output, int n_planes)
> free(data->plane);
> data->plane = NULL;
>
> - igt_remove_fb(data->drm_fd, data->fb);
> + free(data->fb);
> + data->fb = NULL;
>
> igt_display_reset(&data->display);
> }
> @@ -195,6 +186,7 @@ prepare_planes(data_t *data, enum pipe pipe_id,
> color_t *color,
> int *y;
> int *size;
> int i;
> + int* suffle;
>
> igt_output_set_pipe(output, pipe_id);
> primary = igt_output_get_plane_type(output,
> DRM_PLANE_TYPE_PRIMARY);
> @@ -206,6 +198,34 @@ prepare_planes(data_t *data, enum pipe pipe_id,
> color_t *color,
> igt_assert_f(y, "Failed to allocate %ld bytes for variable
> y\n", (long int) (pipe->n_planes * sizeof(*y)));
> size = malloc(pipe->n_planes * sizeof(*size));
> igt_assert_f(size, "Failed to allocate %ld bytes for variable
> size\n", (long int) (pipe->n_planes * sizeof(*size)));
> + suffle = malloc(pipe->n_planes * sizeof(*suffle));
> + igt_assert_f(suffle, "Failed to allocate %ld bytes for variable
> size\n", (long int) (pipe->n_planes * sizeof(*suffle)));
> +
> + for (i = 0; i < pipe->n_planes; i++)
> + suffle[i] = i;
> +
> + /*
> + * suffle table for planes. using rand() should keep it
> + * 'randomized in expected way'
> + */
> + for (i = 0; i < 256; i++) {
> + int n, m;
> + int a, b;
> +
> + n = rand() % (pipe->n_planes-1);
> + m = rand() % (pipe->n_planes-1);
> +
> + /*
> + * keep primary plane at its place for test's sake.
> + */
> + if(n == primary->index || m == primary->index)
> + continue;
> +
> + a = suffle[n];
> + b = suffle[m];
> + suffle[n] = b;
> + suffle[m] = a;
> + }
>
> mode = igt_output_get_mode(output);
>
> @@ -213,7 +233,11 @@ prepare_planes(data_t *data, enum pipe pipe_id,
> color_t *color,
> x[primary->index] = 0;
> y[primary->index] = 0;
> for (i = 0; i < max_planes; i++) {
> - igt_plane_t *plane = igt_output_get_plane(output, i);
> + /*
> + * Here is made assumption primary plane will have
> + * index zero.
> + */
> + igt_plane_t *plane = igt_output_get_plane(output,
> suffle[i]);
> uint32_t plane_format;
> uint64_t plane_tiling;
>
> @@ -251,6 +275,10 @@ prepare_planes(data_t *data, enum pipe pipe_id,
> color_t *color,
> create_fb_for_mode_position(data, output, mode, color, x, y,
> size, size, tiling, max_planes);
> igt_plane_set_fb(data->plane[primary->index], &data-
> >fb[primary->index]);
> + free((void*)x);
> + free((void*)y);
> + free((void*)size);
> + free((void*)suffle);
> }
>
> static void
> @@ -260,7 +288,9 @@ test_plane_position_with_output(data_t *data,
> enum pipe pipe,
> {
> color_t blue = { 0.0f, 0.0f, 1.0f };
> igt_crc_t crc;
> + igt_plane_t *plane;
> int i;
> + int err, c = 0;
> int iterations = opt.iterations < 1 ? 1 : opt.iterations;
> bool loop_forever;
> char info[256];
> @@ -274,22 +304,46 @@ test_plane_position_with_output(data_t *data,
> enum pipe pipe,
> iterations, iterations > 1 ? "iterations" :
> "iteration");
> }
>
> - igt_info("Testing connector %s using pipe %s with %d planes %s
> with seed %d\n",
> - igt_output_name(output), kmstest_pipe_name(pipe),
> n_planes,
> - info, opt.seed);
> -
> test_init(data, pipe, n_planes);
>
> test_grab_crc(data, output, pipe, &blue, tiling);
>
> + /*
> + * Find out how many planes are allowed simultaneously
> + */
> + do {
> + c++;
> + prepare_planes(data, pipe, &blue, tiling, c, output);
> + err = igt_display_try_commit2(&data->display,
> COMMIT_ATOMIC);
> +
> + for_each_plane_on_pipe(&data->display, pipe, plane)
> + igt_plane_set_fb(plane, NULL);
> +
> + for (int x = 0; x < c; x++)
> + igt_remove_fb(data->drm_fd, &data->fb[x]);
> + } while (!err && c < n_planes);
> +
> + if(err)
> + c--;
> +
> + igt_info("Testing connector %s using pipe %s with %d planes %s
> with seed %d\n",
> + igt_output_name(output), kmstest_pipe_name(pipe), c,
> + info, opt.seed);
> +
> i = 0;
> while (i < iterations || loop_forever) {
> - prepare_planes(data, pipe, &blue, tiling, n_planes,
> output);
> + prepare_planes(data, pipe, &blue, tiling, c, output);
>
> igt_display_commit2(&data->display, COMMIT_ATOMIC);
>
> igt_pipe_crc_get_current(data->display.drm_fd, data-
> >pipe_crc, &crc);
>
> + for_each_plane_on_pipe(&data->display, pipe, plane)
> + igt_plane_set_fb(plane, NULL);
> +
> + for (int x = 0; x < c; x++)
> + igt_remove_fb(data->drm_fd, &data->fb[x]);
> +
> igt_assert_crc_equal(&data->ref_crc, &crc);
>
> i++;
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: before going testing check how many planes are allowed
2019-04-10 20:06 ` Souza, Jose
@ 2019-04-11 15:36 ` Juha-Pekka Heikkila
0 siblings, 0 replies; 18+ messages in thread
From: Juha-Pekka Heikkila @ 2019-04-11 15:36 UTC (permalink / raw)
To: Souza, Jose, igt-dev; +Cc: Lisovskiy, Stanislav
On 10.4.2019 23.06, Souza, Jose wrote:
> On Tue, 2019-04-09 at 14:47 +0300, Juha-Pekka Heikkila wrote:
>> On 9.4.2019 4.29, Souza, Jose wrote:
>>> On Fri, 2019-04-05 at 17:55 +0300, Juha-Pekka Heikkila wrote:
>>>> before start testing try out how many planes kernel will
>>>> allow simultaneously to be used.
>>>>
>>>> v2 (Jose Souza, Daniel Vetter): Fix resource leaks. Randomize
>>>> used planes.
>>>>
>>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>>>> ---
>>>> tests/kms_plane_multiple.c | 88
>>>> +++++++++++++++++++++++++++++++++++++---------
>>>> 1 file changed, 71 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/tests/kms_plane_multiple.c
>>>> b/tests/kms_plane_multiple.c
>>>> index bfaeede..f2707dc 100644
>>>> --- a/tests/kms_plane_multiple.c
>>>> +++ b/tests/kms_plane_multiple.c
>>>> @@ -80,16 +80,6 @@ static void test_fini(data_t *data,
>>>> igt_output_t
>>>> *output, int n_planes)
>>>> {
>>>> igt_pipe_crc_stop(data->pipe_crc);
>>>>
>>>> - for (int i = 0; i < n_planes; i++) {
>>>> - igt_plane_t *plane = data->plane[i];
>>>> - if (!plane)
>>>> - continue;
>>>> - if (plane->type == DRM_PLANE_TYPE_PRIMARY)
>>>> - continue;
>>>> - igt_plane_set_fb(plane, NULL);
>>>> - data->plane[i] = NULL;
>>>> - }
>>>> -
>>>> /* reset the constraint on the pipe */
>>>> igt_output_set_pipe(output, PIPE_ANY);
>>>>
>>>> @@ -99,7 +89,8 @@ static void test_fini(data_t *data,
>>>> igt_output_t
>>>> *output, int n_planes)
>>>> free(data->plane);
>>>> data->plane = NULL;
>>>>
>>>> - igt_remove_fb(data->drm_fd, data->fb);
>>>> + free(data->fb);
>>>> + data->fb = NULL;
>>>>
>>>> igt_display_reset(&data->display);
>>>> }
>>>> @@ -195,6 +186,7 @@ prepare_planes(data_t *data, enum pipe
>>>> pipe_id,
>>>> color_t *color,
>>>> int *y;
>>>> int *size;
>>>> int i;
>>>> + int* suffle;
>>>>
>>>> igt_output_set_pipe(output, pipe_id);
>>>> primary = igt_output_get_plane_type(output,
>>>> DRM_PLANE_TYPE_PRIMARY);
>>>> @@ -206,6 +198,34 @@ prepare_planes(data_t *data, enum pipe
>>>> pipe_id,
>>>> color_t *color,
>>>> igt_assert_f(y, "Failed to allocate %ld bytes for
>>>> variable
>>>> y\n", (long int) (pipe->n_planes * sizeof(*y)));
>>>> size = malloc(pipe->n_planes * sizeof(*size));
>>>> igt_assert_f(size, "Failed to allocate %ld bytes for
>>>> variable
>>>> size\n", (long int) (pipe->n_planes * sizeof(*size)));
>>>> + suffle = malloc(pipe->n_planes * sizeof(*suffle));
>>>> + igt_assert_f(suffle, "Failed to allocate %ld bytes for variable
>>>> size\n", (long int) (pipe->n_planes * sizeof(*suffle)));
>>>> +
>>>> + for (i = 0; i < pipe->n_planes; i++)
>>>> + suffle[i] = i;
>>>> +
>>>> + /*
>>>> + * suffle table for planes. using rand() should keep it
>>>> + * 'randomized in expected way'
>>>> + */
>>>> + for (i = 0; i < 256; i++) {
>>>
>>> Not beautiful at all
>>>
>>>> + int n, m;
>>>> + int a, b;
>>>> +
>>>> + n = rand() % (pipe->n_planes-1);
>>>> + m = rand() % (pipe->n_planes-1);
>>>> +
>>>> + /*
>>>> + * keep primary plane at its place for test's sake.
>>>> + */
>>>> + if(n == primary->index || m == primary->index)
>>>> + continue;
>>>> +
>>>> + a = suffle[n];
>>>> + b = suffle[m];
>>>> + suffle[n] = b;
>>>> + suffle[m] = a;
>>>> + }
>>>>
>>>> mode = igt_output_get_mode(output);
>>>>
>>>> @@ -213,7 +233,11 @@ prepare_planes(data_t *data, enum pipe
>>>> pipe_id,
>>>> color_t *color,
>>>> x[primary->index] = 0;
>>>> y[primary->index] = 0;
>>>> for (i = 0; i < max_planes; i++) {
>>>> - igt_plane_t *plane = igt_output_get_plane(output, i);
>>>> + /*
>>>> + * Here is made assumption primary plane will have
>>>> + * index zero.
>>>> + */
>>>> + igt_plane_t *plane = igt_output_get_plane(output,
>>>> suffle[i]);
>>>> uint32_t plane_format;
>>>> uint64_t plane_tiling;
>>>>
>>>> @@ -251,6 +275,10 @@ prepare_planes(data_t *data, enum pipe
>>>> pipe_id,
>>>> color_t *color,
>>>> create_fb_for_mode_position(data, output, mode, color,
>>>> x, y,
>>>> size, size, tiling,
>>>> max_planes);
>>>> igt_plane_set_fb(data->plane[primary->index], &data-
>>>>> fb[primary->index]);
>>>> + free((void*)x);
>>>> + free((void*)y);
>>>> + free((void*)size);
>>>> + free((void*)suffle);
>>>> }
>>>>
>>>> static void
>>>> @@ -260,7 +288,9 @@ test_plane_position_with_output(data_t *data,
>>>> enum pipe pipe,
>>>> {
>>>> color_t blue = { 0.0f, 0.0f, 1.0f };
>>>> igt_crc_t crc;
>>>> + igt_plane_t *plane;
>>>> int i;
>>>> + int err, c = 0;
>>>> int iterations = opt.iterations < 1 ? 1 :
>>>> opt.iterations;
>>>> bool loop_forever;
>>>> char info[256];
>>>> @@ -274,22 +304,46 @@ test_plane_position_with_output(data_t
>>>> *data,
>>>> enum pipe pipe,
>>>> iterations, iterations > 1 ?
>>>> "iterations" :
>>>> "iteration");
>>>> }
>>>>
>>>> - igt_info("Testing connector %s using pipe %s with %d planes %s
>>>> with seed %d\n",
>>>> - igt_output_name(output), kmstest_pipe_name(pipe),
>>>> n_planes,
>>>> - info, opt.seed);
>>>> -
>>>> test_init(data, pipe, n_planes);
>>>>
>>>> test_grab_crc(data, output, pipe, &blue, tiling);
>>>>
>>>> + /*
>>>> + * Find out how many planes are allowed simultaneously
>>>> + */
>>>> + do {
>>>> + c++;
>>>
>>> Adding one plane at time is a waste of CI time in platforms without
>>> a
>>> high number of planes(anything besides ICL), better start with max
>>> and
>>> decreasing until commit is accepted.
>>>
>>
>> I'd rather not try to hammer in configurations which may not work,
>> that's not what we're testing here. Time anyways is less than 6
>> frames
>> per loop so I don't think we are really wasting time.
>
> Platforms with 3 planes will do 3 iterations with current approach
> while it could be done with just one and ICL will will do 5
> iterations(my ICL setup support 5 planes at the same time) while it
> could be done in 3 iterations.
>
> Now takes this additional iterations * 4 tests per pipe * number of
> pipes and you have a considerable waste of CI time.
I still don't think it to be 'considerable waste of time' when the
difference is small enough to be lost in noise. On my box with this test
execution time fluctuate for one subtest between 0.561s .. 1.671s as the
test report itself. Though, I admit I was wrong about how long this loop
takes, its just one frame, not over five frames.
TBH if one actually was to care about time spent I'd go after setting
those fbs to null but this patch is not about that. Then again I don't
even understand why are you bothering about possible 0.09s lost in CI?
>
>
>>
>>>> + prepare_planes(data, pipe, &blue, tiling, c, output);
>>>> + err = igt_display_try_commit2(&data->display,
>>>> COMMIT_ATOMIC);
>>>
>>> No need to do a real commit here, you can only test with:
>>>
>>> err = igt_display_try_commit_atomic(&data->display,
>>> DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
>>>
>>>
>>> Also if a err different than -EINVAL is returned you should handle
>>> like
>>> a error.
>>>
>>
>> We're just enabling planes in the commit, which error would you
>> expect?
>
> -ENOMEM is one example that I have without looking at the libdrm and
> kernel code.
>
You do realize we'd be wasting time on checking that? Anyway if you're
into wasting time and consider we got this far and suddenly get any
random error, still what's the point in why should handling this error
be included here? Wouldn't it give us failure on the actual loop anyway
and fail this test? Why make it intentionally more complex?
>>
>>>> +
>>>> + for_each_plane_on_pipe(&data->display, pipe, plane)
>>>> + igt_plane_set_fb(plane, NULL);
>>>> +
>>>> + for (int x = 0; x < c; x++)
>>>> + igt_remove_fb(data->drm_fd, &data->fb[x]);
>>>
>>> This is still leaking, example:
>>>
>>> c = 3
>>> plane 0, plane 1 and plane 3 enabled
>>>
>>> Other leak, the framebuffer created in test_grab_crc()
>>>
>>>
>>
>> Huh? hmm... Your comment doesn't make sense. Maybe you have confused
>> idea of what plane vs framebuffer is or something?
>
> Yeah, I checked again and you are not leaking, sorry about that.
>
>>
>>>> + } while (!err && c < n_planes);
>>>> +
>>>> + if(err)
>>>> + c--;
>>>> +
>>>> + igt_info("Testing connector %s using pipe %s with %d planes %s
>>>> with seed %d\n",
>>>> + igt_output_name(output), kmstest_pipe_name(pipe), c,
>>>> + info, opt.seed);
>>>> +
>>>> i = 0;
>>>> while (i < iterations || loop_forever) {
>>>> - prepare_planes(data, pipe, &blue, tiling, n_planes,
>>>> output);
>>>> + prepare_planes(data, pipe, &blue, tiling, c, output);
>>>>
>>>> igt_display_commit2(&data->display,
>>>> COMMIT_ATOMIC);
>>>>
>>>> igt_pipe_crc_get_current(data->display.drm_fd,
>>>> data-
>>>>> pipe_crc, &crc);
>>>>
>>>> + for_each_plane_on_pipe(&data->display, pipe, plane)
>>>> + igt_plane_set_fb(plane, NULL);
>>>> +
>>>> + for (int x = 0; x < c; x++)
>>>> + igt_remove_fb(data->drm_fd, &data->fb[x]);
>>>
>>> move this cleanup to function and call it here and where you test
>>> the
>>> number of planes that can be enabled
>>>
>>>> +
>>>> igt_assert_crc_equal(&data->ref_crc, &crc);
>>>>
>>>> i++;
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: before going testing check how many planes are allowed
2019-04-09 11:47 ` Juha-Pekka Heikkila
2019-04-10 20:06 ` Souza, Jose
@ 2019-04-11 8:35 ` Maarten Lankhorst
1 sibling, 0 replies; 18+ messages in thread
From: Maarten Lankhorst @ 2019-04-11 8:35 UTC (permalink / raw)
To: juhapekka.heikkila, Souza, Jose, igt-dev; +Cc: Lisovskiy, Stanislav
Op 09-04-2019 om 13:47 schreef Juha-Pekka Heikkila:
> On 9.4.2019 4.29, Souza, Jose wrote:
>> On Fri, 2019-04-05 at 17:55 +0300, Juha-Pekka Heikkila wrote:
>>> before start testing try out how many planes kernel will
>>> allow simultaneously to be used.
>>>
>>> v2 (Jose Souza, Daniel Vetter): Fix resource leaks. Randomize
>>> used planes.
>>>
>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>>> ---
>>> tests/kms_plane_multiple.c | 88
>>> +++++++++++++++++++++++++++++++++++++---------
>>> 1 file changed, 71 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/tests/kms_plane_multiple.c b/tests/kms_plane_multiple.c
>>> index bfaeede..f2707dc 100644
>>> --- a/tests/kms_plane_multiple.c
>>> +++ b/tests/kms_plane_multiple.c
>>> @@ -80,16 +80,6 @@ static void test_fini(data_t *data, igt_output_t
>>> *output, int n_planes)
>>> {
>>> igt_pipe_crc_stop(data->pipe_crc);
>>> - for (int i = 0; i < n_planes; i++) {
>>> - igt_plane_t *plane = data->plane[i];
>>> - if (!plane)
>>> - continue;
>>> - if (plane->type == DRM_PLANE_TYPE_PRIMARY)
>>> - continue;
>>> - igt_plane_set_fb(plane, NULL);
>>> - data->plane[i] = NULL;
>>> - }
>>> -
>>> /* reset the constraint on the pipe */
>>> igt_output_set_pipe(output, PIPE_ANY);
>>> @@ -99,7 +89,8 @@ static void test_fini(data_t *data, igt_output_t
>>> *output, int n_planes)
>>> free(data->plane);
>>> data->plane = NULL;
>>> - igt_remove_fb(data->drm_fd, data->fb);
>>> + free(data->fb);
>>> + data->fb = NULL;
>>> igt_display_reset(&data->display);
>>> }
>>> @@ -195,6 +186,7 @@ prepare_planes(data_t *data, enum pipe pipe_id,
>>> color_t *color,
>>> int *y;
>>> int *size;
>>> int i;
>>> + int* suffle;
>>> igt_output_set_pipe(output, pipe_id);
>>> primary = igt_output_get_plane_type(output,
>>> DRM_PLANE_TYPE_PRIMARY);
>>> @@ -206,6 +198,34 @@ prepare_planes(data_t *data, enum pipe pipe_id,
>>> color_t *color,
>>> igt_assert_f(y, "Failed to allocate %ld bytes for variable
>>> y\n", (long int) (pipe->n_planes * sizeof(*y)));
>>> size = malloc(pipe->n_planes * sizeof(*size));
>>> igt_assert_f(size, "Failed to allocate %ld bytes for variable
>>> size\n", (long int) (pipe->n_planes * sizeof(*size)));
>>> + suffle = malloc(pipe->n_planes * sizeof(*suffle));
>>> + igt_assert_f(suffle, "Failed to allocate %ld bytes for variable
>>> size\n", (long int) (pipe->n_planes * sizeof(*suffle)));
>>> +
>>> + for (i = 0; i < pipe->n_planes; i++)
>>> + suffle[i] = i;
>>> +
>>> + /*
>>> + * suffle table for planes. using rand() should keep it
>>> + * 'randomized in expected way'
>>> + */
>>> + for (i = 0; i < 256; i++) {
>>
>> Not beautiful at all
>>
>>> + int n, m;
>>> + int a, b;
>>> +
>>> + n = rand() % (pipe->n_planes-1);
>>> + m = rand() % (pipe->n_planes-1);
>>> +
>>> + /*
>>> + * keep primary plane at its place for test's sake.
>>> + */
>>> + if(n == primary->index || m == primary->index)
>>> + continue;
>>> +
>>> + a = suffle[n];
>>> + b = suffle[m];
>>> + suffle[n] = b;
>>> + suffle[m] = a;
>>> + }
>>> mode = igt_output_get_mode(output);
>>> @@ -213,7 +233,11 @@ prepare_planes(data_t *data, enum pipe pipe_id,
>>> color_t *color,
>>> x[primary->index] = 0;
>>> y[primary->index] = 0;
>>> for (i = 0; i < max_planes; i++) {
>>> - igt_plane_t *plane = igt_output_get_plane(output, i);
>>> + /*
>>> + * Here is made assumption primary plane will have
>>> + * index zero.
>>> + */
>>> + igt_plane_t *plane = igt_output_get_plane(output,
>>> suffle[i]);
>>> uint32_t plane_format;
>>> uint64_t plane_tiling;
>>> @@ -251,6 +275,10 @@ prepare_planes(data_t *data, enum pipe pipe_id,
>>> color_t *color,
>>> create_fb_for_mode_position(data, output, mode, color, x, y,
>>> size, size, tiling, max_planes);
>>> igt_plane_set_fb(data->plane[primary->index], &data-
>>>> fb[primary->index]);
>>> + free((void*)x);
>>> + free((void*)y);
>>> + free((void*)size);
>>> + free((void*)suffle);
>>> }
>>> static void
>>> @@ -260,7 +288,9 @@ test_plane_position_with_output(data_t *data,
>>> enum pipe pipe,
>>> {
>>> color_t blue = { 0.0f, 0.0f, 1.0f };
>>> igt_crc_t crc;
>>> + igt_plane_t *plane;
>>> int i;
>>> + int err, c = 0;
>>> int iterations = opt.iterations < 1 ? 1 : opt.iterations;
>>> bool loop_forever;
>>> char info[256];
>>> @@ -274,22 +304,46 @@ test_plane_position_with_output(data_t *data,
>>> enum pipe pipe,
>>> iterations, iterations > 1 ? "iterations" :
>>> "iteration");
>>> }
>>> - igt_info("Testing connector %s using pipe %s with %d planes %s
>>> with seed %d\n",
>>> - igt_output_name(output), kmstest_pipe_name(pipe),
>>> n_planes,
>>> - info, opt.seed);
>>> -
>>> test_init(data, pipe, n_planes);
>>> test_grab_crc(data, output, pipe, &blue, tiling);
>>> + /*
>>> + * Find out how many planes are allowed simultaneously
>>> + */
>>> + do {
>>> + c++;
>>
>> Adding one plane at time is a waste of CI time in platforms without a
>> high number of planes(anything besides ICL), better start with max and
>> decreasing until commit is accepted.
>>
>
> I'd rather not try to hammer in configurations which may not work, that's not what we're testing here. Time anyways is less than 6 frames per loop so I don't think we are really wasting time.
This is the whole point of atomic though. With the try_commit_atomic(TEST_ONLY | ALLOW_MODESET) mentioned by Jose you're not wasting any time in finding a configuration that works.. We don't need to commit it to HW, only have to test in sw. :)
~Maarten
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: before going testing check how many planes are allowed
2019-04-10 19:55 ` Souza, Jose
@ 2019-04-11 6:38 ` Lisovskiy, Stanislav
0 siblings, 0 replies; 18+ messages in thread
From: Lisovskiy, Stanislav @ 2019-04-11 6:38 UTC (permalink / raw)
To: juhapekka.heikkila, igt-dev, Souza, Jose
On Wed, 2019-04-10 at 12:55 -0700, Souza, Jose wrote:
> On Tue, 2019-04-09 at 13:47 +0100, Lisovskiy, Stanislav wrote:
> > On Mon, 2019-04-08 at 18:29 -0700, Souza, Jose wrote:
> > > On Fri, 2019-04-05 at 17:55 +0300, Juha-Pekka Heikkila wrote:
> > > > before start testing try out how many planes kernel will
> > > > allow simultaneously to be used.
> > > >
> > > > v2 (Jose Souza, Daniel Vetter): Fix resource leaks. Randomize
> > > > used planes.
> > > >
> > > > Signed-off-by: Juha-Pekka Heikkila <
> > > > juhapekka.heikkila@gmail.com>
> > > > ---
> > > > tests/kms_plane_multiple.c | 88
> > > > +++++++++++++++++++++++++++++++++++++---------
> > > > 1 file changed, 71 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/tests/kms_plane_multiple.c
> > > > b/tests/kms_plane_multiple.c
> > > > index bfaeede..f2707dc 100644
> > > > --- a/tests/kms_plane_multiple.c
> > > > +++ b/tests/kms_plane_multiple.c
> > > > @@ -80,16 +80,6 @@ static void test_fini(data_t *data,
> > > > igt_output_t
> > > > *output, int n_planes)
> > > > {
> > > > igt_pipe_crc_stop(data->pipe_crc);
> > > >
> > > > - for (int i = 0; i < n_planes; i++) {
> > > > - igt_plane_t *plane = data->plane[i];
> > > > - if (!plane)
> > > > - continue;
> > > > - if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > > > - continue;
> > > > - igt_plane_set_fb(plane, NULL);
> > > > - data->plane[i] = NULL;
> > > > - }
> > > > -
> > > > /* reset the constraint on the pipe */
> > > > igt_output_set_pipe(output, PIPE_ANY);
> > > >
> > > > @@ -99,7 +89,8 @@ static void test_fini(data_t *data,
> > > > igt_output_t
> > > > *output, int n_planes)
> > > > free(data->plane);
> > > > data->plane = NULL;
> > > >
> > > > - igt_remove_fb(data->drm_fd, data->fb);
> > > > + free(data->fb);
> > > > + data->fb = NULL;
> > > >
> > > > igt_display_reset(&data->display);
> > > > }
> > > > @@ -195,6 +186,7 @@ prepare_planes(data_t *data, enum pipe
> > > > pipe_id,
> > > > color_t *color,
> > > > int *y;
> > > > int *size;
> > > > int i;
> > > > + int* suffle;
> > > >
> > > > igt_output_set_pipe(output, pipe_id);
> > > > primary = igt_output_get_plane_type(output,
> > > > DRM_PLANE_TYPE_PRIMARY);
> > > > @@ -206,6 +198,34 @@ prepare_planes(data_t *data, enum pipe
> > > > pipe_id,
> > > > color_t *color,
> > > > igt_assert_f(y, "Failed to allocate %ld bytes for
> > > > variable
> > > > y\n", (long int) (pipe->n_planes * sizeof(*y)));
> > > > size = malloc(pipe->n_planes * sizeof(*size));
> > > > igt_assert_f(size, "Failed to allocate %ld bytes for
> > > > variable
> > > > size\n", (long int) (pipe->n_planes * sizeof(*size)));
> > > > + suffle = malloc(pipe->n_planes * sizeof(*suffle));
> > > > + igt_assert_f(suffle, "Failed to allocate %ld bytes for
> > > > variable
> > > > size\n", (long int) (pipe->n_planes * sizeof(*suffle)));
> > > > +
> > > > + for (i = 0; i < pipe->n_planes; i++)
> > > > + suffle[i] = i;
> > > > +
> > > > + /*
> > > > + * suffle table for planes. using rand() should keep it
> > > > + * 'randomized in expected way'
> > > > + */
> > > > + for (i = 0; i < 256; i++) {
> > >
> > > Not beautiful at all
> > >
> > > > + int n, m;
> > > > + int a, b;
> > > > +
> > > > + n = rand() % (pipe->n_planes-1);
> > > > + m = rand() % (pipe->n_planes-1);
> > > > +
> > > > + /*
> > > > + * keep primary plane at its place for test's
> > > > sake.
> > > > + */
> > > > + if(n == primary->index || m == primary->index)
> > > > + continue;
> > > > +
> > > > + a = suffle[n];
> > > > + b = suffle[m];
> > > > + suffle[n] = b;
> > > > + suffle[m] = a;
> > > > + }
> > > >
> > > > mode = igt_output_get_mode(output);
> > > >
> > > > @@ -213,7 +233,11 @@ prepare_planes(data_t *data, enum pipe
> > > > pipe_id,
> > > > color_t *color,
> > > > x[primary->index] = 0;
> > > > y[primary->index] = 0;
> > > > for (i = 0; i < max_planes; i++) {
> > > > - igt_plane_t *plane =
> > > > igt_output_get_plane(output, i);
> > > > + /*
> > > > + * Here is made assumption primary plane will
> > > > have
> > > > + * index zero.
> > > > + */
> > > > + igt_plane_t *plane =
> > > > igt_output_get_plane(output,
> > > > suffle[i]);
> > > > uint32_t plane_format;
> > > > uint64_t plane_tiling;
> > > >
> > > > @@ -251,6 +275,10 @@ prepare_planes(data_t *data, enum pipe
> > > > pipe_id,
> > > > color_t *color,
> > > > create_fb_for_mode_position(data, output, mode, color,
> > > > x, y,
> > > > size, size, tiling,
> > > > max_planes);
> > > > igt_plane_set_fb(data->plane[primary->index], &data-
> > > > > fb[primary->index]);
> > > >
> > > > + free((void*)x);
> > > > + free((void*)y);
> > > > + free((void*)size);
> > > > + free((void*)suffle);
> > > > }
> > > >
> > > > static void
> > > > @@ -260,7 +288,9 @@ test_plane_position_with_output(data_t
> > > > *data,
> > > > enum pipe pipe,
> > > > {
> > > > color_t blue = { 0.0f, 0.0f, 1.0f };
> > > > igt_crc_t crc;
> > > > + igt_plane_t *plane;
> > > > int i;
> > > > + int err, c = 0;
> > > > int iterations = opt.iterations < 1 ? 1 :
> > > > opt.iterations;
> > > > bool loop_forever;
> > > > char info[256];
> > > > @@ -274,22 +304,46 @@ test_plane_position_with_output(data_t
> > > > *data,
> > > > enum pipe pipe,
> > > > iterations, iterations > 1 ?
> > > > "iterations" :
> > > > "iteration");
> > > > }
> > > >
> > > > - igt_info("Testing connector %s using pipe %s with %d
> > > > planes %s
> > > > with seed %d\n",
> > > > - igt_output_name(output),
> > > > kmstest_pipe_name(pipe),
> > > > n_planes,
> > > > - info, opt.seed);
> > > > -
> > > > test_init(data, pipe, n_planes);
> > > >
> > > > test_grab_crc(data, output, pipe, &blue, tiling);
> > > >
> > > > + /*
> > > > + * Find out how many planes are allowed simultaneously
> > > > + */
> > > > + do {
> > > > + c++;
> > >
> > > Adding one plane at time is a waste of CI time in platforms
> > > without
> > > a
> > > high number of planes(anything besides ICL), better start with
> > > max
> > > and
> > > decreasing until commit is accepted.
> >
> > It is actually opposite. Platforms with high amount of planes would
> > be slower obviously, as you will have to increment more and faster
> > for systems with less amount of planes. Consider 0->7 and 0-
> > > 3(platforms with less amount of planes) - so where is more waste
> >
> > of time?
>
> Platforms with 3 planes will do 3 iterations with current approach
> while it could be done with just one and ICL will will do 5
> iterations(my ICL setup support 5 planes at the same time) while it
> could be done in 3 iterations.
>
> Now takes this additional iterations * 4 tests per pipe * number of
> pipes and you have a considerable waste of CI time.
That is exactly what I said. "Platforms with high amount of planes
would be slower obviously". However in reality I'm currently testing
it with bandwidth patches: the number of allowed planes can be quite
different, based on tiling and other resources,
so it is really hard to predict from which plane it is beneficial to
start. For example: on my ICL setup it can sometimes tolerate 8(yeah)
planes on one pipe, however if second pipe is used simultaneously, it
is allowed to have only 1(!) plane.
>
> >
> > > > + prepare_planes(data, pipe, &blue, tiling, c,
> > > > output);
> > > > + err = igt_display_try_commit2(&data->display,
> > > > COMMIT_ATOMIC);
> > >
> > > No need to do a real commit here, you can only test with:
> >
> > This is igt_display_TRY_commit2.
>
> The TRY here means that the test will not fail in case the commit is
> not accepted/valid, it will not call drmModeAtomicCommit() with
> DRM_MODE_ATOMIC_TEST_ONLY, this will save some time when the commit
> is
> valid/accepted.
I agree here, naming confused me - there is a try commit2 function with
quite similar name..
>
> >
> > > err = igt_display_try_commit_atomic(&data->display,
> > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> > >
> > >
> > > Also if a err different than -EINVAL is returned you should
> > > handle
> > > like
> > > a error.
> > >
> > > > +
> > > > + for_each_plane_on_pipe(&data->display, pipe,
> > > > plane)
> > > > + igt_plane_set_fb(plane, NULL);
> > > > +
> > > > + for (int x = 0; x < c; x++)
> > > > + igt_remove_fb(data->drm_fd, &data-
> > > > >fb[x]);
> > >
> > > This is still leaking, example:
> > >
> > > c = 3
> > > plane 0, plane 1 and plane 3 enabled
> >
> > There should be no leak here. He uses suffle[i]
> > for index indirection in prepare_planes like igt_plane_t *plane =
> > igt_output_get_plane(output, suffle[i]), so that suffle[0]=0,
> > suffle[1]=1, suffle[2]=3, meanwhile correspondent data->fb[i]
> > remains contiguous, so that plane 0 corresponds to data->fb[0],
> > plane 1 -> data->fb[1] and plane 3 -> data->fb[2].
> > So all the igt_remove_fb calls target correct data->fb[] through
> > suffle[i] used as indirection layer.
>
> Okay
>
> >
> > > Other leak, the framebuffer created in test_grab_crc()
> > >
> > >
> > > > + } while (!err && c < n_planes);
> > > > +
> > > > + if(err)
> > > > + c--;
> > > > +
> > > > + igt_info("Testing connector %s using pipe %s with %d
> > > > planes %s
> > > > with seed %d\n",
> > > > + igt_output_name(output),
> > > > kmstest_pipe_name(pipe), c,
> > > > + info, opt.seed);
> > > > +
> > > > i = 0;
> > > > while (i < iterations || loop_forever) {
> > > > - prepare_planes(data, pipe, &blue, tiling,
> > > > n_planes,
> > > > output);
> > > > + prepare_planes(data, pipe, &blue, tiling, c,
> > > > output);
> > > >
> > > > igt_display_commit2(&data->display,
> > > > COMMIT_ATOMIC);
> > > >
> > > > igt_pipe_crc_get_current(data->display.drm_fd,
> > > > data-
> > > > > pipe_crc, &crc);
> > > >
> > > >
> > > > + for_each_plane_on_pipe(&data->display, pipe,
> > > > plane)
> > > > + igt_plane_set_fb(plane, NULL);
> > > > +
> > > > + for (int x = 0; x < c; x++)
> > > > + igt_remove_fb(data->drm_fd, &data-
> > > > >fb[x]);
> > >
> > > move this cleanup to function and call it here and where you test
> > > the
> > > number of planes that can be enabled
> > >
> > > > +
> > > > igt_assert_crc_equal(&data->ref_crc, &crc);
> > > >
> > > > i++;
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: before going testing check how many planes are allowed
2019-04-09 11:47 ` Juha-Pekka Heikkila
@ 2019-04-10 20:06 ` Souza, Jose
2019-04-11 15:36 ` Juha-Pekka Heikkila
2019-04-11 8:35 ` Maarten Lankhorst
1 sibling, 1 reply; 18+ messages in thread
From: Souza, Jose @ 2019-04-10 20:06 UTC (permalink / raw)
To: juhapekka.heikkila, igt-dev; +Cc: Lisovskiy, Stanislav
[-- Attachment #1.1: Type: text/plain, Size: 8737 bytes --]
On Tue, 2019-04-09 at 14:47 +0300, Juha-Pekka Heikkila wrote:
> On 9.4.2019 4.29, Souza, Jose wrote:
> > On Fri, 2019-04-05 at 17:55 +0300, Juha-Pekka Heikkila wrote:
> > > before start testing try out how many planes kernel will
> > > allow simultaneously to be used.
> > >
> > > v2 (Jose Souza, Daniel Vetter): Fix resource leaks. Randomize
> > > used planes.
> > >
> > > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > > ---
> > > tests/kms_plane_multiple.c | 88
> > > +++++++++++++++++++++++++++++++++++++---------
> > > 1 file changed, 71 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/tests/kms_plane_multiple.c
> > > b/tests/kms_plane_multiple.c
> > > index bfaeede..f2707dc 100644
> > > --- a/tests/kms_plane_multiple.c
> > > +++ b/tests/kms_plane_multiple.c
> > > @@ -80,16 +80,6 @@ static void test_fini(data_t *data,
> > > igt_output_t
> > > *output, int n_planes)
> > > {
> > > igt_pipe_crc_stop(data->pipe_crc);
> > >
> > > - for (int i = 0; i < n_planes; i++) {
> > > - igt_plane_t *plane = data->plane[i];
> > > - if (!plane)
> > > - continue;
> > > - if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > > - continue;
> > > - igt_plane_set_fb(plane, NULL);
> > > - data->plane[i] = NULL;
> > > - }
> > > -
> > > /* reset the constraint on the pipe */
> > > igt_output_set_pipe(output, PIPE_ANY);
> > >
> > > @@ -99,7 +89,8 @@ static void test_fini(data_t *data,
> > > igt_output_t
> > > *output, int n_planes)
> > > free(data->plane);
> > > data->plane = NULL;
> > >
> > > - igt_remove_fb(data->drm_fd, data->fb);
> > > + free(data->fb);
> > > + data->fb = NULL;
> > >
> > > igt_display_reset(&data->display);
> > > }
> > > @@ -195,6 +186,7 @@ prepare_planes(data_t *data, enum pipe
> > > pipe_id,
> > > color_t *color,
> > > int *y;
> > > int *size;
> > > int i;
> > > + int* suffle;
> > >
> > > igt_output_set_pipe(output, pipe_id);
> > > primary = igt_output_get_plane_type(output,
> > > DRM_PLANE_TYPE_PRIMARY);
> > > @@ -206,6 +198,34 @@ prepare_planes(data_t *data, enum pipe
> > > pipe_id,
> > > color_t *color,
> > > igt_assert_f(y, "Failed to allocate %ld bytes for
> > > variable
> > > y\n", (long int) (pipe->n_planes * sizeof(*y)));
> > > size = malloc(pipe->n_planes * sizeof(*size));
> > > igt_assert_f(size, "Failed to allocate %ld bytes for
> > > variable
> > > size\n", (long int) (pipe->n_planes * sizeof(*size)));
> > > + suffle = malloc(pipe->n_planes * sizeof(*suffle));
> > > + igt_assert_f(suffle, "Failed to allocate %ld bytes for variable
> > > size\n", (long int) (pipe->n_planes * sizeof(*suffle)));
> > > +
> > > + for (i = 0; i < pipe->n_planes; i++)
> > > + suffle[i] = i;
> > > +
> > > + /*
> > > + * suffle table for planes. using rand() should keep it
> > > + * 'randomized in expected way'
> > > + */
> > > + for (i = 0; i < 256; i++) {
> >
> > Not beautiful at all
> >
> > > + int n, m;
> > > + int a, b;
> > > +
> > > + n = rand() % (pipe->n_planes-1);
> > > + m = rand() % (pipe->n_planes-1);
> > > +
> > > + /*
> > > + * keep primary plane at its place for test's sake.
> > > + */
> > > + if(n == primary->index || m == primary->index)
> > > + continue;
> > > +
> > > + a = suffle[n];
> > > + b = suffle[m];
> > > + suffle[n] = b;
> > > + suffle[m] = a;
> > > + }
> > >
> > > mode = igt_output_get_mode(output);
> > >
> > > @@ -213,7 +233,11 @@ prepare_planes(data_t *data, enum pipe
> > > pipe_id,
> > > color_t *color,
> > > x[primary->index] = 0;
> > > y[primary->index] = 0;
> > > for (i = 0; i < max_planes; i++) {
> > > - igt_plane_t *plane = igt_output_get_plane(output, i);
> > > + /*
> > > + * Here is made assumption primary plane will have
> > > + * index zero.
> > > + */
> > > + igt_plane_t *plane = igt_output_get_plane(output,
> > > suffle[i]);
> > > uint32_t plane_format;
> > > uint64_t plane_tiling;
> > >
> > > @@ -251,6 +275,10 @@ prepare_planes(data_t *data, enum pipe
> > > pipe_id,
> > > color_t *color,
> > > create_fb_for_mode_position(data, output, mode, color,
> > > x, y,
> > > size, size, tiling,
> > > max_planes);
> > > igt_plane_set_fb(data->plane[primary->index], &data-
> > > > fb[primary->index]);
> > > + free((void*)x);
> > > + free((void*)y);
> > > + free((void*)size);
> > > + free((void*)suffle);
> > > }
> > >
> > > static void
> > > @@ -260,7 +288,9 @@ test_plane_position_with_output(data_t *data,
> > > enum pipe pipe,
> > > {
> > > color_t blue = { 0.0f, 0.0f, 1.0f };
> > > igt_crc_t crc;
> > > + igt_plane_t *plane;
> > > int i;
> > > + int err, c = 0;
> > > int iterations = opt.iterations < 1 ? 1 :
> > > opt.iterations;
> > > bool loop_forever;
> > > char info[256];
> > > @@ -274,22 +304,46 @@ test_plane_position_with_output(data_t
> > > *data,
> > > enum pipe pipe,
> > > iterations, iterations > 1 ?
> > > "iterations" :
> > > "iteration");
> > > }
> > >
> > > - igt_info("Testing connector %s using pipe %s with %d planes %s
> > > with seed %d\n",
> > > - igt_output_name(output), kmstest_pipe_name(pipe),
> > > n_planes,
> > > - info, opt.seed);
> > > -
> > > test_init(data, pipe, n_planes);
> > >
> > > test_grab_crc(data, output, pipe, &blue, tiling);
> > >
> > > + /*
> > > + * Find out how many planes are allowed simultaneously
> > > + */
> > > + do {
> > > + c++;
> >
> > Adding one plane at time is a waste of CI time in platforms without
> > a
> > high number of planes(anything besides ICL), better start with max
> > and
> > decreasing until commit is accepted.
> >
>
> I'd rather not try to hammer in configurations which may not work,
> that's not what we're testing here. Time anyways is less than 6
> frames
> per loop so I don't think we are really wasting time.
Platforms with 3 planes will do 3 iterations with current approach
while it could be done with just one and ICL will will do 5
iterations(my ICL setup support 5 planes at the same time) while it
could be done in 3 iterations.
Now takes this additional iterations * 4 tests per pipe * number of
pipes and you have a considerable waste of CI time.
>
> > > + prepare_planes(data, pipe, &blue, tiling, c, output);
> > > + err = igt_display_try_commit2(&data->display,
> > > COMMIT_ATOMIC);
> >
> > No need to do a real commit here, you can only test with:
> >
> > err = igt_display_try_commit_atomic(&data->display,
> > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> >
> >
> > Also if a err different than -EINVAL is returned you should handle
> > like
> > a error.
> >
>
> We're just enabling planes in the commit, which error would you
> expect?
-ENOMEM is one example that I have without looking at the libdrm and
kernel code.
>
> > > +
> > > + for_each_plane_on_pipe(&data->display, pipe, plane)
> > > + igt_plane_set_fb(plane, NULL);
> > > +
> > > + for (int x = 0; x < c; x++)
> > > + igt_remove_fb(data->drm_fd, &data->fb[x]);
> >
> > This is still leaking, example:
> >
> > c = 3
> > plane 0, plane 1 and plane 3 enabled
> >
> > Other leak, the framebuffer created in test_grab_crc()
> >
> >
>
> Huh? hmm... Your comment doesn't make sense. Maybe you have confused
> idea of what plane vs framebuffer is or something?
Yeah, I checked again and you are not leaking, sorry about that.
>
> > > + } while (!err && c < n_planes);
> > > +
> > > + if(err)
> > > + c--;
> > > +
> > > + igt_info("Testing connector %s using pipe %s with %d planes %s
> > > with seed %d\n",
> > > + igt_output_name(output), kmstest_pipe_name(pipe), c,
> > > + info, opt.seed);
> > > +
> > > i = 0;
> > > while (i < iterations || loop_forever) {
> > > - prepare_planes(data, pipe, &blue, tiling, n_planes,
> > > output);
> > > + prepare_planes(data, pipe, &blue, tiling, c, output);
> > >
> > > igt_display_commit2(&data->display,
> > > COMMIT_ATOMIC);
> > >
> > > igt_pipe_crc_get_current(data->display.drm_fd,
> > > data-
> > > > pipe_crc, &crc);
> > >
> > > + for_each_plane_on_pipe(&data->display, pipe, plane)
> > > + igt_plane_set_fb(plane, NULL);
> > > +
> > > + for (int x = 0; x < c; x++)
> > > + igt_remove_fb(data->drm_fd, &data->fb[x]);
> >
> > move this cleanup to function and call it here and where you test
> > the
> > number of planes that can be enabled
> >
> > > +
> > > igt_assert_crc_equal(&data->ref_crc, &crc);
> > >
> > > i++;
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 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] 18+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: before going testing check how many planes are allowed
2019-04-09 12:47 ` Lisovskiy, Stanislav
@ 2019-04-10 19:55 ` Souza, Jose
2019-04-11 6:38 ` Lisovskiy, Stanislav
0 siblings, 1 reply; 18+ messages in thread
From: Souza, Jose @ 2019-04-10 19:55 UTC (permalink / raw)
To: juhapekka.heikkila, igt-dev, Lisovskiy, Stanislav
[-- Attachment #1.1: Type: text/plain, Size: 9107 bytes --]
On Tue, 2019-04-09 at 13:47 +0100, Lisovskiy, Stanislav wrote:
> On Mon, 2019-04-08 at 18:29 -0700, Souza, Jose wrote:
> > On Fri, 2019-04-05 at 17:55 +0300, Juha-Pekka Heikkila wrote:
> > > before start testing try out how many planes kernel will
> > > allow simultaneously to be used.
> > >
> > > v2 (Jose Souza, Daniel Vetter): Fix resource leaks. Randomize
> > > used planes.
> > >
> > > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > > ---
> > > tests/kms_plane_multiple.c | 88
> > > +++++++++++++++++++++++++++++++++++++---------
> > > 1 file changed, 71 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/tests/kms_plane_multiple.c
> > > b/tests/kms_plane_multiple.c
> > > index bfaeede..f2707dc 100644
> > > --- a/tests/kms_plane_multiple.c
> > > +++ b/tests/kms_plane_multiple.c
> > > @@ -80,16 +80,6 @@ static void test_fini(data_t *data,
> > > igt_output_t
> > > *output, int n_planes)
> > > {
> > > igt_pipe_crc_stop(data->pipe_crc);
> > >
> > > - for (int i = 0; i < n_planes; i++) {
> > > - igt_plane_t *plane = data->plane[i];
> > > - if (!plane)
> > > - continue;
> > > - if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > > - continue;
> > > - igt_plane_set_fb(plane, NULL);
> > > - data->plane[i] = NULL;
> > > - }
> > > -
> > > /* reset the constraint on the pipe */
> > > igt_output_set_pipe(output, PIPE_ANY);
> > >
> > > @@ -99,7 +89,8 @@ static void test_fini(data_t *data,
> > > igt_output_t
> > > *output, int n_planes)
> > > free(data->plane);
> > > data->plane = NULL;
> > >
> > > - igt_remove_fb(data->drm_fd, data->fb);
> > > + free(data->fb);
> > > + data->fb = NULL;
> > >
> > > igt_display_reset(&data->display);
> > > }
> > > @@ -195,6 +186,7 @@ prepare_planes(data_t *data, enum pipe
> > > pipe_id,
> > > color_t *color,
> > > int *y;
> > > int *size;
> > > int i;
> > > + int* suffle;
> > >
> > > igt_output_set_pipe(output, pipe_id);
> > > primary = igt_output_get_plane_type(output,
> > > DRM_PLANE_TYPE_PRIMARY);
> > > @@ -206,6 +198,34 @@ prepare_planes(data_t *data, enum pipe
> > > pipe_id,
> > > color_t *color,
> > > igt_assert_f(y, "Failed to allocate %ld bytes for variable
> > > y\n", (long int) (pipe->n_planes * sizeof(*y)));
> > > size = malloc(pipe->n_planes * sizeof(*size));
> > > igt_assert_f(size, "Failed to allocate %ld bytes for variable
> > > size\n", (long int) (pipe->n_planes * sizeof(*size)));
> > > + suffle = malloc(pipe->n_planes * sizeof(*suffle));
> > > + igt_assert_f(suffle, "Failed to allocate %ld bytes for variable
> > > size\n", (long int) (pipe->n_planes * sizeof(*suffle)));
> > > +
> > > + for (i = 0; i < pipe->n_planes; i++)
> > > + suffle[i] = i;
> > > +
> > > + /*
> > > + * suffle table for planes. using rand() should keep it
> > > + * 'randomized in expected way'
> > > + */
> > > + for (i = 0; i < 256; i++) {
> >
> > Not beautiful at all
> >
> > > + int n, m;
> > > + int a, b;
> > > +
> > > + n = rand() % (pipe->n_planes-1);
> > > + m = rand() % (pipe->n_planes-1);
> > > +
> > > + /*
> > > + * keep primary plane at its place for test's sake.
> > > + */
> > > + if(n == primary->index || m == primary->index)
> > > + continue;
> > > +
> > > + a = suffle[n];
> > > + b = suffle[m];
> > > + suffle[n] = b;
> > > + suffle[m] = a;
> > > + }
> > >
> > > mode = igt_output_get_mode(output);
> > >
> > > @@ -213,7 +233,11 @@ prepare_planes(data_t *data, enum pipe
> > > pipe_id,
> > > color_t *color,
> > > x[primary->index] = 0;
> > > y[primary->index] = 0;
> > > for (i = 0; i < max_planes; i++) {
> > > - igt_plane_t *plane = igt_output_get_plane(output, i);
> > > + /*
> > > + * Here is made assumption primary plane will have
> > > + * index zero.
> > > + */
> > > + igt_plane_t *plane = igt_output_get_plane(output,
> > > suffle[i]);
> > > uint32_t plane_format;
> > > uint64_t plane_tiling;
> > >
> > > @@ -251,6 +275,10 @@ prepare_planes(data_t *data, enum pipe
> > > pipe_id,
> > > color_t *color,
> > > create_fb_for_mode_position(data, output, mode, color, x, y,
> > > size, size, tiling, max_planes);
> > > igt_plane_set_fb(data->plane[primary->index], &data-
> > > > fb[primary->index]);
> > >
> > > + free((void*)x);
> > > + free((void*)y);
> > > + free((void*)size);
> > > + free((void*)suffle);
> > > }
> > >
> > > static void
> > > @@ -260,7 +288,9 @@ test_plane_position_with_output(data_t *data,
> > > enum pipe pipe,
> > > {
> > > color_t blue = { 0.0f, 0.0f, 1.0f };
> > > igt_crc_t crc;
> > > + igt_plane_t *plane;
> > > int i;
> > > + int err, c = 0;
> > > int iterations = opt.iterations < 1 ? 1 : opt.iterations;
> > > bool loop_forever;
> > > char info[256];
> > > @@ -274,22 +304,46 @@ test_plane_position_with_output(data_t
> > > *data,
> > > enum pipe pipe,
> > > iterations, iterations > 1 ? "iterations" :
> > > "iteration");
> > > }
> > >
> > > - igt_info("Testing connector %s using pipe %s with %d planes %s
> > > with seed %d\n",
> > > - igt_output_name(output), kmstest_pipe_name(pipe),
> > > n_planes,
> > > - info, opt.seed);
> > > -
> > > test_init(data, pipe, n_planes);
> > >
> > > test_grab_crc(data, output, pipe, &blue, tiling);
> > >
> > > + /*
> > > + * Find out how many planes are allowed simultaneously
> > > + */
> > > + do {
> > > + c++;
> >
> > Adding one plane at time is a waste of CI time in platforms without
> > a
> > high number of planes(anything besides ICL), better start with max
> > and
> > decreasing until commit is accepted.
>
> It is actually opposite. Platforms with high amount of planes would
> be slower obviously, as you will have to increment more and faster
> for systems with less amount of planes. Consider 0->7 and 0-
> > 3(platforms with less amount of planes) - so where is more waste
> of time?
Platforms with 3 planes will do 3 iterations with current approach
while it could be done with just one and ICL will will do 5
iterations(my ICL setup support 5 planes at the same time) while it
could be done in 3 iterations.
Now takes this additional iterations * 4 tests per pipe * number of
pipes and you have a considerable waste of CI time.
>
> > > + prepare_planes(data, pipe, &blue, tiling, c, output);
> > > + err = igt_display_try_commit2(&data->display,
> > > COMMIT_ATOMIC);
> >
> > No need to do a real commit here, you can only test with:
>
> This is igt_display_TRY_commit2.
The TRY here means that the test will not fail in case the commit is
not accepted/valid, it will not call drmModeAtomicCommit() with
DRM_MODE_ATOMIC_TEST_ONLY, this will save some time when the commit is
valid/accepted.
>
> > err = igt_display_try_commit_atomic(&data->display,
> > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> >
> >
> > Also if a err different than -EINVAL is returned you should handle
> > like
> > a error.
> >
> > > +
> > > + for_each_plane_on_pipe(&data->display, pipe, plane)
> > > + igt_plane_set_fb(plane, NULL);
> > > +
> > > + for (int x = 0; x < c; x++)
> > > + igt_remove_fb(data->drm_fd, &data->fb[x]);
> >
> > This is still leaking, example:
> >
> > c = 3
> > plane 0, plane 1 and plane 3 enabled
>
> There should be no leak here. He uses suffle[i]
> for index indirection in prepare_planes like igt_plane_t *plane =
> igt_output_get_plane(output, suffle[i]), so that suffle[0]=0,
> suffle[1]=1, suffle[2]=3, meanwhile correspondent data->fb[i]
> remains contiguous, so that plane 0 corresponds to data->fb[0],
> plane 1 -> data->fb[1] and plane 3 -> data->fb[2].
> So all the igt_remove_fb calls target correct data->fb[] through
> suffle[i] used as indirection layer.
Okay
>
> > Other leak, the framebuffer created in test_grab_crc()
> >
> >
> > > + } while (!err && c < n_planes);
> > > +
> > > + if(err)
> > > + c--;
> > > +
> > > + igt_info("Testing connector %s using pipe %s with %d planes %s
> > > with seed %d\n",
> > > + igt_output_name(output), kmstest_pipe_name(pipe), c,
> > > + info, opt.seed);
> > > +
> > > i = 0;
> > > while (i < iterations || loop_forever) {
> > > - prepare_planes(data, pipe, &blue, tiling, n_planes,
> > > output);
> > > + prepare_planes(data, pipe, &blue, tiling, c, output);
> > >
> > > igt_display_commit2(&data->display, COMMIT_ATOMIC);
> > >
> > > igt_pipe_crc_get_current(data->display.drm_fd, data-
> > > > pipe_crc, &crc);
> > >
> > >
> > > + for_each_plane_on_pipe(&data->display, pipe, plane)
> > > + igt_plane_set_fb(plane, NULL);
> > > +
> > > + for (int x = 0; x < c; x++)
> > > + igt_remove_fb(data->drm_fd, &data->fb[x]);
> >
> > move this cleanup to function and call it here and where you test
> > the
> > number of planes that can be enabled
> >
> > > +
> > > igt_assert_crc_equal(&data->ref_crc, &crc);
> > >
> > > i++;
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 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] 18+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: before going testing check how many planes are allowed
2019-04-09 1:29 ` Souza, Jose
2019-04-09 11:47 ` Juha-Pekka Heikkila
@ 2019-04-09 12:47 ` Lisovskiy, Stanislav
2019-04-10 19:55 ` Souza, Jose
1 sibling, 1 reply; 18+ messages in thread
From: Lisovskiy, Stanislav @ 2019-04-09 12:47 UTC (permalink / raw)
To: juhapekka.heikkila, igt-dev, Souza, Jose
On Mon, 2019-04-08 at 18:29 -0700, Souza, Jose wrote:
> On Fri, 2019-04-05 at 17:55 +0300, Juha-Pekka Heikkila wrote:
> > before start testing try out how many planes kernel will
> > allow simultaneously to be used.
> >
> > v2 (Jose Souza, Daniel Vetter): Fix resource leaks. Randomize
> > used planes.
> >
> > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > ---
> > tests/kms_plane_multiple.c | 88
> > +++++++++++++++++++++++++++++++++++++---------
> > 1 file changed, 71 insertions(+), 17 deletions(-)
> >
> > diff --git a/tests/kms_plane_multiple.c
> > b/tests/kms_plane_multiple.c
> > index bfaeede..f2707dc 100644
> > --- a/tests/kms_plane_multiple.c
> > +++ b/tests/kms_plane_multiple.c
> > @@ -80,16 +80,6 @@ static void test_fini(data_t *data, igt_output_t
> > *output, int n_planes)
> > {
> > igt_pipe_crc_stop(data->pipe_crc);
> >
> > - for (int i = 0; i < n_planes; i++) {
> > - igt_plane_t *plane = data->plane[i];
> > - if (!plane)
> > - continue;
> > - if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > - continue;
> > - igt_plane_set_fb(plane, NULL);
> > - data->plane[i] = NULL;
> > - }
> > -
> > /* reset the constraint on the pipe */
> > igt_output_set_pipe(output, PIPE_ANY);
> >
> > @@ -99,7 +89,8 @@ static void test_fini(data_t *data, igt_output_t
> > *output, int n_planes)
> > free(data->plane);
> > data->plane = NULL;
> >
> > - igt_remove_fb(data->drm_fd, data->fb);
> > + free(data->fb);
> > + data->fb = NULL;
> >
> > igt_display_reset(&data->display);
> > }
> > @@ -195,6 +186,7 @@ prepare_planes(data_t *data, enum pipe pipe_id,
> > color_t *color,
> > int *y;
> > int *size;
> > int i;
> > + int* suffle;
> >
> > igt_output_set_pipe(output, pipe_id);
> > primary = igt_output_get_plane_type(output,
> > DRM_PLANE_TYPE_PRIMARY);
> > @@ -206,6 +198,34 @@ prepare_planes(data_t *data, enum pipe
> > pipe_id,
> > color_t *color,
> > igt_assert_f(y, "Failed to allocate %ld bytes for variable
> > y\n", (long int) (pipe->n_planes * sizeof(*y)));
> > size = malloc(pipe->n_planes * sizeof(*size));
> > igt_assert_f(size, "Failed to allocate %ld bytes for variable
> > size\n", (long int) (pipe->n_planes * sizeof(*size)));
> > + suffle = malloc(pipe->n_planes * sizeof(*suffle));
> > + igt_assert_f(suffle, "Failed to allocate %ld bytes for variable
> > size\n", (long int) (pipe->n_planes * sizeof(*suffle)));
> > +
> > + for (i = 0; i < pipe->n_planes; i++)
> > + suffle[i] = i;
> > +
> > + /*
> > + * suffle table for planes. using rand() should keep it
> > + * 'randomized in expected way'
> > + */
> > + for (i = 0; i < 256; i++) {
>
> Not beautiful at all
>
> > + int n, m;
> > + int a, b;
> > +
> > + n = rand() % (pipe->n_planes-1);
> > + m = rand() % (pipe->n_planes-1);
> > +
> > + /*
> > + * keep primary plane at its place for test's sake.
> > + */
> > + if(n == primary->index || m == primary->index)
> > + continue;
> > +
> > + a = suffle[n];
> > + b = suffle[m];
> > + suffle[n] = b;
> > + suffle[m] = a;
> > + }
> >
> > mode = igt_output_get_mode(output);
> >
> > @@ -213,7 +233,11 @@ prepare_planes(data_t *data, enum pipe
> > pipe_id,
> > color_t *color,
> > x[primary->index] = 0;
> > y[primary->index] = 0;
> > for (i = 0; i < max_planes; i++) {
> > - igt_plane_t *plane = igt_output_get_plane(output, i);
> > + /*
> > + * Here is made assumption primary plane will have
> > + * index zero.
> > + */
> > + igt_plane_t *plane = igt_output_get_plane(output,
> > suffle[i]);
> > uint32_t plane_format;
> > uint64_t plane_tiling;
> >
> > @@ -251,6 +275,10 @@ prepare_planes(data_t *data, enum pipe
> > pipe_id,
> > color_t *color,
> > create_fb_for_mode_position(data, output, mode, color, x, y,
> > size, size, tiling, max_planes);
> > igt_plane_set_fb(data->plane[primary->index], &data-
> > > fb[primary->index]);
> >
> > + free((void*)x);
> > + free((void*)y);
> > + free((void*)size);
> > + free((void*)suffle);
> > }
> >
> > static void
> > @@ -260,7 +288,9 @@ test_plane_position_with_output(data_t *data,
> > enum pipe pipe,
> > {
> > color_t blue = { 0.0f, 0.0f, 1.0f };
> > igt_crc_t crc;
> > + igt_plane_t *plane;
> > int i;
> > + int err, c = 0;
> > int iterations = opt.iterations < 1 ? 1 : opt.iterations;
> > bool loop_forever;
> > char info[256];
> > @@ -274,22 +304,46 @@ test_plane_position_with_output(data_t *data,
> > enum pipe pipe,
> > iterations, iterations > 1 ? "iterations" :
> > "iteration");
> > }
> >
> > - igt_info("Testing connector %s using pipe %s with %d planes %s
> > with seed %d\n",
> > - igt_output_name(output), kmstest_pipe_name(pipe),
> > n_planes,
> > - info, opt.seed);
> > -
> > test_init(data, pipe, n_planes);
> >
> > test_grab_crc(data, output, pipe, &blue, tiling);
> >
> > + /*
> > + * Find out how many planes are allowed simultaneously
> > + */
> > + do {
> > + c++;
>
> Adding one plane at time is a waste of CI time in platforms without a
> high number of planes(anything besides ICL), better start with max
> and
> decreasing until commit is accepted.
It is actually opposite. Platforms with high amount of planes would
be slower obviously, as you will have to increment more and faster
for systems with less amount of planes. Consider 0->7 and 0-
>3(platforms with less amount of planes) - so where is more waste
of time?
>
> > + prepare_planes(data, pipe, &blue, tiling, c, output);
> > + err = igt_display_try_commit2(&data->display,
> > COMMIT_ATOMIC);
>
> No need to do a real commit here, you can only test with:
This is igt_display_TRY_commit2.
>
> err = igt_display_try_commit_atomic(&data->display,
> DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
>
>
> Also if a err different than -EINVAL is returned you should handle
> like
> a error.
>
> > +
> > + for_each_plane_on_pipe(&data->display, pipe, plane)
> > + igt_plane_set_fb(plane, NULL);
> > +
> > + for (int x = 0; x < c; x++)
> > + igt_remove_fb(data->drm_fd, &data->fb[x]);
>
> This is still leaking, example:
>
> c = 3
> plane 0, plane 1 and plane 3 enabled
There should be no leak here. He uses suffle[i]
for index indirection in prepare_planes like igt_plane_t *plane =
igt_output_get_plane(output, suffle[i]), so that suffle[0]=0,
suffle[1]=1, suffle[2]=3, meanwhile correspondent data->fb[i]
remains contiguous, so that plane 0 corresponds to data->fb[0],
plane 1 -> data->fb[1] and plane 3 -> data->fb[2].
So all the igt_remove_fb calls target correct data->fb[] through
suffle[i] used as indirection layer.
>
> Other leak, the framebuffer created in test_grab_crc()
>
>
> > + } while (!err && c < n_planes);
> > +
> > + if(err)
> > + c--;
> > +
> > + igt_info("Testing connector %s using pipe %s with %d planes %s
> > with seed %d\n",
> > + igt_output_name(output), kmstest_pipe_name(pipe), c,
> > + info, opt.seed);
> > +
> > i = 0;
> > while (i < iterations || loop_forever) {
> > - prepare_planes(data, pipe, &blue, tiling, n_planes,
> > output);
> > + prepare_planes(data, pipe, &blue, tiling, c, output);
> >
> > igt_display_commit2(&data->display, COMMIT_ATOMIC);
> >
> > igt_pipe_crc_get_current(data->display.drm_fd, data-
> > > pipe_crc, &crc);
> >
> >
> > + for_each_plane_on_pipe(&data->display, pipe, plane)
> > + igt_plane_set_fb(plane, NULL);
> > +
> > + for (int x = 0; x < c; x++)
> > + igt_remove_fb(data->drm_fd, &data->fb[x]);
>
> move this cleanup to function and call it here and where you test the
> number of planes that can be enabled
>
> > +
> > igt_assert_crc_equal(&data->ref_crc, &crc);
> >
> > i++;
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: before going testing check how many planes are allowed
2019-04-09 1:29 ` Souza, Jose
@ 2019-04-09 11:47 ` Juha-Pekka Heikkila
2019-04-10 20:06 ` Souza, Jose
2019-04-11 8:35 ` Maarten Lankhorst
2019-04-09 12:47 ` Lisovskiy, Stanislav
1 sibling, 2 replies; 18+ messages in thread
From: Juha-Pekka Heikkila @ 2019-04-09 11:47 UTC (permalink / raw)
To: Souza, Jose, igt-dev; +Cc: Lisovskiy, Stanislav
On 9.4.2019 4.29, Souza, Jose wrote:
> On Fri, 2019-04-05 at 17:55 +0300, Juha-Pekka Heikkila wrote:
>> before start testing try out how many planes kernel will
>> allow simultaneously to be used.
>>
>> v2 (Jose Souza, Daniel Vetter): Fix resource leaks. Randomize
>> used planes.
>>
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>> ---
>> tests/kms_plane_multiple.c | 88
>> +++++++++++++++++++++++++++++++++++++---------
>> 1 file changed, 71 insertions(+), 17 deletions(-)
>>
>> diff --git a/tests/kms_plane_multiple.c b/tests/kms_plane_multiple.c
>> index bfaeede..f2707dc 100644
>> --- a/tests/kms_plane_multiple.c
>> +++ b/tests/kms_plane_multiple.c
>> @@ -80,16 +80,6 @@ static void test_fini(data_t *data, igt_output_t
>> *output, int n_planes)
>> {
>> igt_pipe_crc_stop(data->pipe_crc);
>>
>> - for (int i = 0; i < n_planes; i++) {
>> - igt_plane_t *plane = data->plane[i];
>> - if (!plane)
>> - continue;
>> - if (plane->type == DRM_PLANE_TYPE_PRIMARY)
>> - continue;
>> - igt_plane_set_fb(plane, NULL);
>> - data->plane[i] = NULL;
>> - }
>> -
>> /* reset the constraint on the pipe */
>> igt_output_set_pipe(output, PIPE_ANY);
>>
>> @@ -99,7 +89,8 @@ static void test_fini(data_t *data, igt_output_t
>> *output, int n_planes)
>> free(data->plane);
>> data->plane = NULL;
>>
>> - igt_remove_fb(data->drm_fd, data->fb);
>> + free(data->fb);
>> + data->fb = NULL;
>>
>> igt_display_reset(&data->display);
>> }
>> @@ -195,6 +186,7 @@ prepare_planes(data_t *data, enum pipe pipe_id,
>> color_t *color,
>> int *y;
>> int *size;
>> int i;
>> + int* suffle;
>>
>> igt_output_set_pipe(output, pipe_id);
>> primary = igt_output_get_plane_type(output,
>> DRM_PLANE_TYPE_PRIMARY);
>> @@ -206,6 +198,34 @@ prepare_planes(data_t *data, enum pipe pipe_id,
>> color_t *color,
>> igt_assert_f(y, "Failed to allocate %ld bytes for variable
>> y\n", (long int) (pipe->n_planes * sizeof(*y)));
>> size = malloc(pipe->n_planes * sizeof(*size));
>> igt_assert_f(size, "Failed to allocate %ld bytes for variable
>> size\n", (long int) (pipe->n_planes * sizeof(*size)));
>> + suffle = malloc(pipe->n_planes * sizeof(*suffle));
>> + igt_assert_f(suffle, "Failed to allocate %ld bytes for variable
>> size\n", (long int) (pipe->n_planes * sizeof(*suffle)));
>> +
>> + for (i = 0; i < pipe->n_planes; i++)
>> + suffle[i] = i;
>> +
>> + /*
>> + * suffle table for planes. using rand() should keep it
>> + * 'randomized in expected way'
>> + */
>> + for (i = 0; i < 256; i++) {
>
> Not beautiful at all
>
>> + int n, m;
>> + int a, b;
>> +
>> + n = rand() % (pipe->n_planes-1);
>> + m = rand() % (pipe->n_planes-1);
>> +
>> + /*
>> + * keep primary plane at its place for test's sake.
>> + */
>> + if(n == primary->index || m == primary->index)
>> + continue;
>> +
>> + a = suffle[n];
>> + b = suffle[m];
>> + suffle[n] = b;
>> + suffle[m] = a;
>> + }
>>
>> mode = igt_output_get_mode(output);
>>
>> @@ -213,7 +233,11 @@ prepare_planes(data_t *data, enum pipe pipe_id,
>> color_t *color,
>> x[primary->index] = 0;
>> y[primary->index] = 0;
>> for (i = 0; i < max_planes; i++) {
>> - igt_plane_t *plane = igt_output_get_plane(output, i);
>> + /*
>> + * Here is made assumption primary plane will have
>> + * index zero.
>> + */
>> + igt_plane_t *plane = igt_output_get_plane(output,
>> suffle[i]);
>> uint32_t plane_format;
>> uint64_t plane_tiling;
>>
>> @@ -251,6 +275,10 @@ prepare_planes(data_t *data, enum pipe pipe_id,
>> color_t *color,
>> create_fb_for_mode_position(data, output, mode, color, x, y,
>> size, size, tiling, max_planes);
>> igt_plane_set_fb(data->plane[primary->index], &data-
>>> fb[primary->index]);
>> + free((void*)x);
>> + free((void*)y);
>> + free((void*)size);
>> + free((void*)suffle);
>> }
>>
>> static void
>> @@ -260,7 +288,9 @@ test_plane_position_with_output(data_t *data,
>> enum pipe pipe,
>> {
>> color_t blue = { 0.0f, 0.0f, 1.0f };
>> igt_crc_t crc;
>> + igt_plane_t *plane;
>> int i;
>> + int err, c = 0;
>> int iterations = opt.iterations < 1 ? 1 : opt.iterations;
>> bool loop_forever;
>> char info[256];
>> @@ -274,22 +304,46 @@ test_plane_position_with_output(data_t *data,
>> enum pipe pipe,
>> iterations, iterations > 1 ? "iterations" :
>> "iteration");
>> }
>>
>> - igt_info("Testing connector %s using pipe %s with %d planes %s
>> with seed %d\n",
>> - igt_output_name(output), kmstest_pipe_name(pipe),
>> n_planes,
>> - info, opt.seed);
>> -
>> test_init(data, pipe, n_planes);
>>
>> test_grab_crc(data, output, pipe, &blue, tiling);
>>
>> + /*
>> + * Find out how many planes are allowed simultaneously
>> + */
>> + do {
>> + c++;
>
> Adding one plane at time is a waste of CI time in platforms without a
> high number of planes(anything besides ICL), better start with max and
> decreasing until commit is accepted.
>
I'd rather not try to hammer in configurations which may not work,
that's not what we're testing here. Time anyways is less than 6 frames
per loop so I don't think we are really wasting time.
>> + prepare_planes(data, pipe, &blue, tiling, c, output);
>> + err = igt_display_try_commit2(&data->display,
>> COMMIT_ATOMIC);
>
> No need to do a real commit here, you can only test with:
>
> err = igt_display_try_commit_atomic(&data->display,
> DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
>
>
> Also if a err different than -EINVAL is returned you should handle like
> a error.
>
We're just enabling planes in the commit, which error would you expect?
>> +
>> + for_each_plane_on_pipe(&data->display, pipe, plane)
>> + igt_plane_set_fb(plane, NULL);
>> +
>> + for (int x = 0; x < c; x++)
>> + igt_remove_fb(data->drm_fd, &data->fb[x]);
>
> This is still leaking, example:
>
> c = 3
> plane 0, plane 1 and plane 3 enabled
>
> Other leak, the framebuffer created in test_grab_crc()
>
>
Huh? hmm... Your comment doesn't make sense. Maybe you have confused
idea of what plane vs framebuffer is or something?
>> + } while (!err && c < n_planes);
>> +
>> + if(err)
>> + c--;
>> +
>> + igt_info("Testing connector %s using pipe %s with %d planes %s
>> with seed %d\n",
>> + igt_output_name(output), kmstest_pipe_name(pipe), c,
>> + info, opt.seed);
>> +
>> i = 0;
>> while (i < iterations || loop_forever) {
>> - prepare_planes(data, pipe, &blue, tiling, n_planes,
>> output);
>> + prepare_planes(data, pipe, &blue, tiling, c, output);
>>
>> igt_display_commit2(&data->display, COMMIT_ATOMIC);
>>
>> igt_pipe_crc_get_current(data->display.drm_fd, data-
>>> pipe_crc, &crc);
>>
>> + for_each_plane_on_pipe(&data->display, pipe, plane)
>> + igt_plane_set_fb(plane, NULL);
>> +
>> + for (int x = 0; x < c; x++)
>> + igt_remove_fb(data->drm_fd, &data->fb[x]);
>
> move this cleanup to function and call it here and where you test the
> number of planes that can be enabled
>
>> +
>> igt_assert_crc_equal(&data->ref_crc, &crc);
>>
>> i++;
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: before going testing check how many planes are allowed
2019-04-05 14:55 [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: before " Juha-Pekka Heikkila
@ 2019-04-09 1:29 ` Souza, Jose
2019-04-09 11:47 ` Juha-Pekka Heikkila
2019-04-09 12:47 ` Lisovskiy, Stanislav
2019-05-29 13:39 ` Lisovskiy, Stanislav
1 sibling, 2 replies; 18+ messages in thread
From: Souza, Jose @ 2019-04-09 1:29 UTC (permalink / raw)
To: juhapekka.heikkila, igt-dev; +Cc: Lisovskiy, Stanislav
[-- Attachment #1.1: Type: text/plain, Size: 6585 bytes --]
On Fri, 2019-04-05 at 17:55 +0300, Juha-Pekka Heikkila wrote:
> before start testing try out how many planes kernel will
> allow simultaneously to be used.
>
> v2 (Jose Souza, Daniel Vetter): Fix resource leaks. Randomize
> used planes.
>
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> ---
> tests/kms_plane_multiple.c | 88
> +++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 71 insertions(+), 17 deletions(-)
>
> diff --git a/tests/kms_plane_multiple.c b/tests/kms_plane_multiple.c
> index bfaeede..f2707dc 100644
> --- a/tests/kms_plane_multiple.c
> +++ b/tests/kms_plane_multiple.c
> @@ -80,16 +80,6 @@ static void test_fini(data_t *data, igt_output_t
> *output, int n_planes)
> {
> igt_pipe_crc_stop(data->pipe_crc);
>
> - for (int i = 0; i < n_planes; i++) {
> - igt_plane_t *plane = data->plane[i];
> - if (!plane)
> - continue;
> - if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> - continue;
> - igt_plane_set_fb(plane, NULL);
> - data->plane[i] = NULL;
> - }
> -
> /* reset the constraint on the pipe */
> igt_output_set_pipe(output, PIPE_ANY);
>
> @@ -99,7 +89,8 @@ static void test_fini(data_t *data, igt_output_t
> *output, int n_planes)
> free(data->plane);
> data->plane = NULL;
>
> - igt_remove_fb(data->drm_fd, data->fb);
> + free(data->fb);
> + data->fb = NULL;
>
> igt_display_reset(&data->display);
> }
> @@ -195,6 +186,7 @@ prepare_planes(data_t *data, enum pipe pipe_id,
> color_t *color,
> int *y;
> int *size;
> int i;
> + int* suffle;
>
> igt_output_set_pipe(output, pipe_id);
> primary = igt_output_get_plane_type(output,
> DRM_PLANE_TYPE_PRIMARY);
> @@ -206,6 +198,34 @@ prepare_planes(data_t *data, enum pipe pipe_id,
> color_t *color,
> igt_assert_f(y, "Failed to allocate %ld bytes for variable
> y\n", (long int) (pipe->n_planes * sizeof(*y)));
> size = malloc(pipe->n_planes * sizeof(*size));
> igt_assert_f(size, "Failed to allocate %ld bytes for variable
> size\n", (long int) (pipe->n_planes * sizeof(*size)));
> + suffle = malloc(pipe->n_planes * sizeof(*suffle));
> + igt_assert_f(suffle, "Failed to allocate %ld bytes for variable
> size\n", (long int) (pipe->n_planes * sizeof(*suffle)));
> +
> + for (i = 0; i < pipe->n_planes; i++)
> + suffle[i] = i;
> +
> + /*
> + * suffle table for planes. using rand() should keep it
> + * 'randomized in expected way'
> + */
> + for (i = 0; i < 256; i++) {
Not beautiful at all
> + int n, m;
> + int a, b;
> +
> + n = rand() % (pipe->n_planes-1);
> + m = rand() % (pipe->n_planes-1);
> +
> + /*
> + * keep primary plane at its place for test's sake.
> + */
> + if(n == primary->index || m == primary->index)
> + continue;
> +
> + a = suffle[n];
> + b = suffle[m];
> + suffle[n] = b;
> + suffle[m] = a;
> + }
>
> mode = igt_output_get_mode(output);
>
> @@ -213,7 +233,11 @@ prepare_planes(data_t *data, enum pipe pipe_id,
> color_t *color,
> x[primary->index] = 0;
> y[primary->index] = 0;
> for (i = 0; i < max_planes; i++) {
> - igt_plane_t *plane = igt_output_get_plane(output, i);
> + /*
> + * Here is made assumption primary plane will have
> + * index zero.
> + */
> + igt_plane_t *plane = igt_output_get_plane(output,
> suffle[i]);
> uint32_t plane_format;
> uint64_t plane_tiling;
>
> @@ -251,6 +275,10 @@ prepare_planes(data_t *data, enum pipe pipe_id,
> color_t *color,
> create_fb_for_mode_position(data, output, mode, color, x, y,
> size, size, tiling, max_planes);
> igt_plane_set_fb(data->plane[primary->index], &data-
> >fb[primary->index]);
> + free((void*)x);
> + free((void*)y);
> + free((void*)size);
> + free((void*)suffle);
> }
>
> static void
> @@ -260,7 +288,9 @@ test_plane_position_with_output(data_t *data,
> enum pipe pipe,
> {
> color_t blue = { 0.0f, 0.0f, 1.0f };
> igt_crc_t crc;
> + igt_plane_t *plane;
> int i;
> + int err, c = 0;
> int iterations = opt.iterations < 1 ? 1 : opt.iterations;
> bool loop_forever;
> char info[256];
> @@ -274,22 +304,46 @@ test_plane_position_with_output(data_t *data,
> enum pipe pipe,
> iterations, iterations > 1 ? "iterations" :
> "iteration");
> }
>
> - igt_info("Testing connector %s using pipe %s with %d planes %s
> with seed %d\n",
> - igt_output_name(output), kmstest_pipe_name(pipe),
> n_planes,
> - info, opt.seed);
> -
> test_init(data, pipe, n_planes);
>
> test_grab_crc(data, output, pipe, &blue, tiling);
>
> + /*
> + * Find out how many planes are allowed simultaneously
> + */
> + do {
> + c++;
Adding one plane at time is a waste of CI time in platforms without a
high number of planes(anything besides ICL), better start with max and
decreasing until commit is accepted.
> + prepare_planes(data, pipe, &blue, tiling, c, output);
> + err = igt_display_try_commit2(&data->display,
> COMMIT_ATOMIC);
No need to do a real commit here, you can only test with:
err = igt_display_try_commit_atomic(&data->display,
DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
Also if a err different than -EINVAL is returned you should handle like
a error.
> +
> + for_each_plane_on_pipe(&data->display, pipe, plane)
> + igt_plane_set_fb(plane, NULL);
> +
> + for (int x = 0; x < c; x++)
> + igt_remove_fb(data->drm_fd, &data->fb[x]);
This is still leaking, example:
c = 3
plane 0, plane 1 and plane 3 enabled
Other leak, the framebuffer created in test_grab_crc()
> + } while (!err && c < n_planes);
> +
> + if(err)
> + c--;
> +
> + igt_info("Testing connector %s using pipe %s with %d planes %s
> with seed %d\n",
> + igt_output_name(output), kmstest_pipe_name(pipe), c,
> + info, opt.seed);
> +
> i = 0;
> while (i < iterations || loop_forever) {
> - prepare_planes(data, pipe, &blue, tiling, n_planes,
> output);
> + prepare_planes(data, pipe, &blue, tiling, c, output);
>
> igt_display_commit2(&data->display, COMMIT_ATOMIC);
>
> igt_pipe_crc_get_current(data->display.drm_fd, data-
> >pipe_crc, &crc);
>
> + for_each_plane_on_pipe(&data->display, pipe, plane)
> + igt_plane_set_fb(plane, NULL);
> +
> + for (int x = 0; x < c; x++)
> + igt_remove_fb(data->drm_fd, &data->fb[x]);
move this cleanup to function and call it here and where you test the
number of planes that can be enabled
> +
> igt_assert_crc_equal(&data->ref_crc, &crc);
>
> i++;
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 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] 18+ messages in thread
* [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: before going testing check how many planes are allowed
@ 2019-04-05 14:55 Juha-Pekka Heikkila
2019-04-09 1:29 ` Souza, Jose
2019-05-29 13:39 ` Lisovskiy, Stanislav
0 siblings, 2 replies; 18+ messages in thread
From: Juha-Pekka Heikkila @ 2019-04-05 14:55 UTC (permalink / raw)
To: igt-dev; +Cc: Lisovskiy, Stanislav
before start testing try out how many planes kernel will
allow simultaneously to be used.
v2 (Jose Souza, Daniel Vetter): Fix resource leaks. Randomize
used planes.
Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
---
tests/kms_plane_multiple.c | 88 +++++++++++++++++++++++++++++++++++++---------
1 file changed, 71 insertions(+), 17 deletions(-)
diff --git a/tests/kms_plane_multiple.c b/tests/kms_plane_multiple.c
index bfaeede..f2707dc 100644
--- a/tests/kms_plane_multiple.c
+++ b/tests/kms_plane_multiple.c
@@ -80,16 +80,6 @@ static void test_fini(data_t *data, igt_output_t *output, int n_planes)
{
igt_pipe_crc_stop(data->pipe_crc);
- for (int i = 0; i < n_planes; i++) {
- igt_plane_t *plane = data->plane[i];
- if (!plane)
- continue;
- if (plane->type == DRM_PLANE_TYPE_PRIMARY)
- continue;
- igt_plane_set_fb(plane, NULL);
- data->plane[i] = NULL;
- }
-
/* reset the constraint on the pipe */
igt_output_set_pipe(output, PIPE_ANY);
@@ -99,7 +89,8 @@ static void test_fini(data_t *data, igt_output_t *output, int n_planes)
free(data->plane);
data->plane = NULL;
- igt_remove_fb(data->drm_fd, data->fb);
+ free(data->fb);
+ data->fb = NULL;
igt_display_reset(&data->display);
}
@@ -195,6 +186,7 @@ prepare_planes(data_t *data, enum pipe pipe_id, color_t *color,
int *y;
int *size;
int i;
+ int* suffle;
igt_output_set_pipe(output, pipe_id);
primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
@@ -206,6 +198,34 @@ prepare_planes(data_t *data, enum pipe pipe_id, color_t *color,
igt_assert_f(y, "Failed to allocate %ld bytes for variable y\n", (long int) (pipe->n_planes * sizeof(*y)));
size = malloc(pipe->n_planes * sizeof(*size));
igt_assert_f(size, "Failed to allocate %ld bytes for variable size\n", (long int) (pipe->n_planes * sizeof(*size)));
+ suffle = malloc(pipe->n_planes * sizeof(*suffle));
+ igt_assert_f(suffle, "Failed to allocate %ld bytes for variable size\n", (long int) (pipe->n_planes * sizeof(*suffle)));
+
+ for (i = 0; i < pipe->n_planes; i++)
+ suffle[i] = i;
+
+ /*
+ * suffle table for planes. using rand() should keep it
+ * 'randomized in expected way'
+ */
+ for (i = 0; i < 256; i++) {
+ int n, m;
+ int a, b;
+
+ n = rand() % (pipe->n_planes-1);
+ m = rand() % (pipe->n_planes-1);
+
+ /*
+ * keep primary plane at its place for test's sake.
+ */
+ if(n == primary->index || m == primary->index)
+ continue;
+
+ a = suffle[n];
+ b = suffle[m];
+ suffle[n] = b;
+ suffle[m] = a;
+ }
mode = igt_output_get_mode(output);
@@ -213,7 +233,11 @@ prepare_planes(data_t *data, enum pipe pipe_id, color_t *color,
x[primary->index] = 0;
y[primary->index] = 0;
for (i = 0; i < max_planes; i++) {
- igt_plane_t *plane = igt_output_get_plane(output, i);
+ /*
+ * Here is made assumption primary plane will have
+ * index zero.
+ */
+ igt_plane_t *plane = igt_output_get_plane(output, suffle[i]);
uint32_t plane_format;
uint64_t plane_tiling;
@@ -251,6 +275,10 @@ prepare_planes(data_t *data, enum pipe pipe_id, color_t *color,
create_fb_for_mode_position(data, output, mode, color, x, y,
size, size, tiling, max_planes);
igt_plane_set_fb(data->plane[primary->index], &data->fb[primary->index]);
+ free((void*)x);
+ free((void*)y);
+ free((void*)size);
+ free((void*)suffle);
}
static void
@@ -260,7 +288,9 @@ test_plane_position_with_output(data_t *data, enum pipe pipe,
{
color_t blue = { 0.0f, 0.0f, 1.0f };
igt_crc_t crc;
+ igt_plane_t *plane;
int i;
+ int err, c = 0;
int iterations = opt.iterations < 1 ? 1 : opt.iterations;
bool loop_forever;
char info[256];
@@ -274,22 +304,46 @@ test_plane_position_with_output(data_t *data, enum pipe pipe,
iterations, iterations > 1 ? "iterations" : "iteration");
}
- igt_info("Testing connector %s using pipe %s with %d planes %s with seed %d\n",
- igt_output_name(output), kmstest_pipe_name(pipe), n_planes,
- info, opt.seed);
-
test_init(data, pipe, n_planes);
test_grab_crc(data, output, pipe, &blue, tiling);
+ /*
+ * Find out how many planes are allowed simultaneously
+ */
+ do {
+ c++;
+ prepare_planes(data, pipe, &blue, tiling, c, output);
+ err = igt_display_try_commit2(&data->display, COMMIT_ATOMIC);
+
+ for_each_plane_on_pipe(&data->display, pipe, plane)
+ igt_plane_set_fb(plane, NULL);
+
+ for (int x = 0; x < c; x++)
+ igt_remove_fb(data->drm_fd, &data->fb[x]);
+ } while (!err && c < n_planes);
+
+ if(err)
+ c--;
+
+ igt_info("Testing connector %s using pipe %s with %d planes %s with seed %d\n",
+ igt_output_name(output), kmstest_pipe_name(pipe), c,
+ info, opt.seed);
+
i = 0;
while (i < iterations || loop_forever) {
- prepare_planes(data, pipe, &blue, tiling, n_planes, output);
+ prepare_planes(data, pipe, &blue, tiling, c, output);
igt_display_commit2(&data->display, COMMIT_ATOMIC);
igt_pipe_crc_get_current(data->display.drm_fd, data->pipe_crc, &crc);
+ for_each_plane_on_pipe(&data->display, pipe, plane)
+ igt_plane_set_fb(plane, NULL);
+
+ for (int x = 0; x < c; x++)
+ igt_remove_fb(data->drm_fd, &data->fb[x]);
+
igt_assert_crc_equal(&data->ref_crc, &crc);
i++;
--
2.7.4
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-06-04 10:26 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-02 14:42 [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: Before going testing check how many planes are allowed Juha-Pekka Heikkila
2019-04-02 17:12 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
2019-04-02 18:22 ` [igt-dev] [PATCH i-g-t] " Souza, Jose
2019-04-03 11:26 ` Juha-Pekka Heikkila
2019-04-03 4:25 ` [igt-dev] ✓ Fi.CI.IGT: success for " Patchwork
2019-04-03 8:38 ` [igt-dev] [PATCH i-g-t] " Daniel Vetter
2019-04-03 12:23 ` Juha-Pekka Heikkila
2019-04-05 14:55 [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: before " Juha-Pekka Heikkila
2019-04-09 1:29 ` Souza, Jose
2019-04-09 11:47 ` Juha-Pekka Heikkila
2019-04-10 20:06 ` Souza, Jose
2019-04-11 15:36 ` Juha-Pekka Heikkila
2019-04-11 8:35 ` Maarten Lankhorst
2019-04-09 12:47 ` Lisovskiy, Stanislav
2019-04-10 19:55 ` Souza, Jose
2019-04-11 6:38 ` Lisovskiy, Stanislav
2019-05-29 13:39 ` Lisovskiy, Stanislav
2019-06-04 10:26 ` Kahola, Mika
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.