[igt-dev] [PATCH i-g-t v2] tests/kms_multipipe_modeset: Add test to validate max pipe configuration
Nautiyal, Ankit K
ankit.k.nautiyal at intel.com
Mon Apr 27 09:31:36 UTC 2020
Hi Karthik,
I think I was mistaken. Since the modes which the platform cannot set,
will be pruned and since you are using a single plane on each pipe,
without tiling, scaling or any thing that might exceed Dbuf/watermark
requirement, there might not be any bandwidth issue.
You can discard my last comment.
I have a few suggestions given inline.
On 4/24/2020 3:12 PM, Nautiyal, Ankit K wrote:
> Hi Karthik,
>
> Thanks for the patch. Overall the patch looks good to me.
>
> Only a small concern I have is that when the commit fails due to not
> sufficient bandwidth, due to perhaps, high resolution displays
>
> connected. So it might be that the 'n' displays are supported, but
> perhaps not with the given resolutions.
>
> The test in that case might fail due to "EINVAL".
>
> Trying with try_commit by changing resolution is a solution but might
> be an overkill.
>
> Perhaps trying with lowest mode would be better, if we just want to
> check if all pipes are coming up simultaneously.
>
> Again this is just a specific scenario with high resolution displays
> connected, which the platform might not support.
>
> Regards,
>
> Ankit
>
> On 4/24/2020 2:42 PM, Petri Latvala wrote:
>> On Fri, Apr 24, 2020 at 12:35:28PM +0530, Karthik B S wrote:
>>> Added test to validate max pipe configuration for a platform.
>>> In the test, the reference CRC is collected first by doing modeset
>>> on all the outputs individually.Then a simultaneous modeset is
>>> done on all the outputs and the CRC is collected. This is compared
>>> with the reference CRC.
>>>
>>> If the number of outputs connected is less than the number of pipes
>>> supported by the platform, then the test skips.
>>>
>>> This test is added to verify if the max pipe configuration for a
>>> given platform is working fine. Even though there are other tests
>>> which test multipipe configuration, they test some other
>>> functionalities
>>> as well together with multipipe. This is a stand alone test that
>>> intends to only verify simultaneous modeset at the max pipe
>>> configuration.
>>>
>>> v2: -Fix Typo in comment (Petri)
>>> -Use igt_require_pipe_crc() to prevent crash in non i915 case
>>> (Petri)
>>> -Print the number of outputs and pipes in igt_require() for
>>> debugging aid (Petri)
>>>
>>> Signed-off-by: Karthik B S <karthik.b.s at intel.com>
>>> ---
>>> tests/Makefile.sources | 1 +
>>> tests/kms_multipipe_modeset.c | 181
>>> ++++++++++++++++++++++++++++++++++
>>> tests/meson.build | 1 +
>>> 3 files changed, 183 insertions(+)
>>> create mode 100644 tests/kms_multipipe_modeset.c
>>>
>>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
>>> index 32cbbf4f..c450fa0e 100644
>>> --- a/tests/Makefile.sources
>>> +++ b/tests/Makefile.sources
>>> @@ -61,6 +61,7 @@ TESTS_progs = \
>>> kms_lease \
>>> kms_legacy_colorkey \
>>> kms_mmap_write_crc \
>>> + kms_multipipe_modeset \
>>> kms_panel_fitting \
>>> kms_pipe_b_c_ivb \
>>> kms_pipe_crc_basic \
>>> diff --git a/tests/kms_multipipe_modeset.c
>>> b/tests/kms_multipipe_modeset.c
>>> new file mode 100644
>>> index 00000000..e1d292ee
>>> --- /dev/null
>>> +++ b/tests/kms_multipipe_modeset.c
>>> @@ -0,0 +1,181 @@
>>> +/*
>>> + * Copyright © 2020 Intel Corporation
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person
>>> obtaining a
>>> + * copy of this software and associated documentation files (the
>>> "Software"),
>>> + * to deal in the Software without restriction, including without
>>> limitation
>>> + * the rights to use, copy, modify, merge, publish, distribute,
>>> sublicense,
>>> + * and/or sell copies of the Software, and to permit persons to
>>> whom the
>>> + * Software is furnished to do so, subject to the following
>>> conditions:
>>> + *
>>> + * The above copyright notice and this permission notice (including
>>> the next
>>> + * paragraph) shall be included in all copies or substantial
>>> portions of the
>>> + * Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>>> EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>> MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
>>> EVENT SHALL
>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
>>> DAMAGES OR OTHER
>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>>> ARISING
>>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>>> OTHER DEALINGS
>>> + * IN THE SOFTWARE.
>>> + *
>>> + * Author:
>>> + * Karthik B S <karthik.b.s at intel.com>
>>> + */
>>> +
>>> +#include "igt.h"
>>> +
>>> +IGT_TEST_DESCRIPTION("Test simultaneous modeset on all the
>>> supported pipes");
>>> +
>>> +typedef struct {
>>> + int drm_fd;
>>> + igt_display_t display;
>>> + struct igt_fb fb;
>>> +} data_t;
>>> +
>>> +static void run_test(data_t *data, int valid_outputs)
>>> +{
>>> + igt_output_t *output;
>>> + igt_pipe_crc_t *pipe_crcs[IGT_MAX_PIPES] = { 0 };
>>> + igt_crc_t ref_crcs[IGT_MAX_PIPES], new_crcs[IGT_MAX_PIPES];
>>> + igt_display_t *display = &data->display;
>>> + int width = 0, height = 0, i, count = 0;
>>> + int output_to_pipe[valid_outputs];
>>> + igt_pipe_t *pipe;
>>> + igt_plane_t *plane;
>>> + drmModeModeInfo *mode;
>>> +
>>> + for_each_connected_output(display, output) {
>>> + mode = igt_output_get_mode(output);
>>> + igt_assert(mode);
>>> +
>>> + igt_output_set_pipe(output, PIPE_NONE);
>>> +
>>> + width = max(width, mode->hdisplay);
>>> + height = max(height, mode->vdisplay);
>>> + }
>>> +
>>> + igt_create_pattern_fb(data->drm_fd, width, height,
>>> DRM_FORMAT_XRGB8888,
>>> + LOCAL_DRM_FORMAT_MOD_NONE, &data->fb);
>>> +
>>> + /* Make pipe-output mapping which will be used unchanged across
>>> the test */
>>> + for_each_pipe(display, i) {
>>> + count = 0;
>>> + for_each_connected_output(display, output) {
>>> + if (igt_pipe_connector_valid(i, output) &&
>>> + output->pending_pipe == PIPE_NONE) {
>>> + igt_output_set_pipe(output, i);
>>> + output_to_pipe[count] = i;
>>> + break;
>>> + }
>>> + count++;
>>> + }
>>> + }
>>> +
In the above loop we are enabling all the pipes, and later we are
enabling one and disabling the rest.
Instead, after disabling all the pipes, loop for each valid connector
-Map it to an available pipe to it
-Do a modeset
-Collect CRC
-unset the pipe
This will avoid multiple loops below and need of output to pipe mapping,
as mapping is fixed:
PIPEA for first valid connected output, PIPEB for second and so on.
Regards,
Ankit
>>> + /* Collect reference CRC by Committing individually on all
>>> outputs*/
>>> + for_each_pipe(display, i) {
>>> + pipe = &display->pipes[i];
>>> + plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
>>> +
>>> + mode = NULL;
>>> +
>>> + pipe_crcs[i] = igt_pipe_crc_new(display->drm_fd, i,
>>> + INTEL_PIPE_CRC_SOURCE_AUTO);
>>> +
>>> + count = 0;
>>> + for_each_connected_output(display, output) {
>>> + if (output_to_pipe[count] == pipe->pipe) {
>>> + igt_output_set_pipe(output, i);
>>> + mode = igt_output_get_mode(output);
>>> + igt_assert(mode);
>>> + } else
>>> + igt_output_set_pipe(output, PIPE_NONE);
>>> + count++;
>>> + }
>>> +
>>> + igt_plane_set_fb(plane, &data->fb);
>>> + igt_fb_set_size(&data->fb, plane, mode->hdisplay,
>>> mode->vdisplay);
>>> + igt_plane_set_size(plane, mode->hdisplay, mode->vdisplay);
>>> +
>>> + igt_display_commit2(display, COMMIT_ATOMIC);
>>> + igt_pipe_crc_collect_crc(pipe_crcs[i], &ref_crcs[i]);
>>> + }
>>> +
>>> + /* Simultaneously commit on all outputs */
>>> + for_each_pipe(display, i) {
>>> + pipe = &display->pipes[i];
>>> + plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
>>> +
>>> + mode = NULL;
>>> +
>>> + count = 0;
>>> + for_each_connected_output(display, output) {
>>> + if (output_to_pipe[count] == pipe->pipe) {
>>> + igt_output_set_pipe(output, i);
>>> + mode = igt_output_get_mode(output);
>>> + igt_assert(mode);
>>> + break;
>>> + }
>>> + count++;
>>> + }
>>> +
>>> + igt_plane_set_fb(plane, &data->fb);
>>> + igt_fb_set_size(&data->fb, plane, mode->hdisplay,
>>> mode->vdisplay);
>>> + igt_plane_set_size(plane, mode->hdisplay, mode->vdisplay);
>>> + }
>>> +
>>> + igt_display_commit2(display, COMMIT_ATOMIC);
>>> +
>>> + /* CRC Verification */
>>> + for (count = 0; count < valid_outputs; count++) {
>>> + igt_pipe_crc_collect_crc(pipe_crcs[count], &new_crcs[count]);
>>> + igt_assert_crc_equal(&ref_crcs[count], &new_crcs[count]);
>>> + }
>>> +
>>> + igt_plane_set_fb(plane, NULL);
>>> + igt_remove_fb(data->drm_fd, &data->fb);
>>> +}
>>> +
>>> +static void test_multipipe(data_t *data)
>>> +{
>>> + igt_output_t *output;
>>> + int valid_outputs = 0, num_pipes;
>>> +
>>> + num_pipes = igt_display_get_n_pipes(&data->display);
>>> + for_each_connected_output(&data->display, output)
>>> + valid_outputs++;
>>> +
>>> + igt_require_f(valid_outputs == num_pipes,
>>> + "Number of connected outputs(%d) not equal to the "
>>> + "number of pipes supported(%d)\n", valid_outputs,
>>> num_pipes);
>>> +
>>> + run_test(data, valid_outputs);
>>> +}
>>> +
>>> +igt_main
>>> +{
>>> + data_t data;
>>> + drmModeResPtr res;
>>> +
>>> + igt_fixture {
>>> + data.drm_fd = drm_open_driver_master(DRIVER_ANY);
>>> + kmstest_set_vt_graphics_mode();
>>> +
>>> + igt_require_pipe_crc(data.drm_fd);
>>> + igt_display_require(&data.display, data.drm_fd);
>>> +
>>> + res = drmModeGetResources(data.drm_fd);
>> Hmm, assigning to a local variable in an igt_fixture... But it's not
>> read outside it, this is fine.
>>
>>> + igt_assert(res);
>>> +
>>> + kmstest_unset_all_crtcs(data.drm_fd, res);
>>> + }
>>> +
>>> + igt_describe("Verify if simultaneous modesets on all the
>>> supported "
>>> + "pipes is successful. Validate using CRC verification");
>>> + igt_subtest("multipipe-modeset")
>>> + test_multipipe(&data);
>> This test binary is already called kms_multipipe_modeset, I wonder if
>> we can find a better name for it, considering we might want to add
>> more later.
>>
>> "basic-crc-check" ?
>>
>> Otherwise the patch LGTM.
>> Reviewed-by: Petri Latvala <petri.latvala at intel.com>
>>
>>
>>> +
>>> + igt_fixture
>>> + igt_display_fini(&data.display);
>>> +}
>>> diff --git a/tests/meson.build b/tests/meson.build
>>> index 0bdcfbe4..88e4875b 100644
>>> --- a/tests/meson.build
>>> +++ b/tests/meson.build
>>> @@ -45,6 +45,7 @@ test_progs = [
>>> 'kms_lease',
>>> 'kms_legacy_colorkey',
>>> 'kms_mmap_write_crc',
>>> + 'kms_multipipe_modeset',
>>> 'kms_panel_fitting',
>>> 'kms_pipe_b_c_ivb',
>>> 'kms_pipe_crc_basic',
>>> --
>>> 2.22.0
>>>
>
More information about the igt-dev
mailing list