[igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: before going testing check how many planes are allowed
Juha-Pekka Heikkila
juhapekka.heikkila at gmail.com
Thu Apr 11 15:36:12 UTC 2019
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 at 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++;
More information about the igt-dev
mailing list