From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by gabe.freedesktop.org (Postfix) with ESMTPS id 8756089ACC for ; Mon, 27 Apr 2020 09:31:39 +0000 (UTC) References: <20200424070528.15104-1-karthik.b.s@intel.com> <20200424091225.GK9497@platvala-desk.ger.corp.intel.com> <4cb973d9-5c8b-b00d-72fc-d1d9e436466e@intel.com> From: "Nautiyal, Ankit K" Message-ID: Date: Mon, 27 Apr 2020 15:01:36 +0530 MIME-Version: 1.0 In-Reply-To: <4cb973d9-5c8b-b00d-72fc-d1d9e436466e@intel.com> Subject: Re: [igt-dev] [PATCH i-g-t v2] tests/kms_multipipe_modeset: Add test to validate max pipe configuration List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="windows-1252"; Format="flowed" Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: Petri Latvala , Karthik B S Cc: igt-dev@lists.freedesktop.org List-ID: 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 >>> --- >>> 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 =3D \ >>> 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 =A9 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 >>> + */ >>> + >>> +#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] =3D { 0 }; >>> + igt_crc_t ref_crcs[IGT_MAX_PIPES], new_crcs[IGT_MAX_PIPES]; >>> + igt_display_t *display =3D &data->display; >>> + int width =3D 0, height =3D 0, i, count =3D 0; >>> + int output_to_pipe[valid_outputs]; >>> + igt_pipe_t *pipe; >>> + igt_plane_t *plane; >>> + drmModeModeInfo *mode; >>> + >>> + for_each_connected_output(display, output) { >>> + mode =3D igt_output_get_mode(output); >>> + igt_assert(mode); >>> + >>> + igt_output_set_pipe(output, PIPE_NONE); >>> + >>> + width =3D max(width, mode->hdisplay); >>> + height =3D 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 =3D 0; >>> + for_each_connected_output(display, output) { >>> + if (igt_pipe_connector_valid(i, output) && >>> + output->pending_pipe =3D=3D PIPE_NONE) { >>> + igt_output_set_pipe(output, i); >>> + output_to_pipe[count] =3D 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 =3D &display->pipes[i]; >>> + plane =3D igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY= ); >>> + >>> + mode =3D NULL; >>> + >>> + pipe_crcs[i] =3D igt_pipe_crc_new(display->drm_fd, i, >>> + INTEL_PIPE_CRC_SOURCE_AUTO); >>> + >>> + count =3D 0; >>> + for_each_connected_output(display, output) { >>> + if (output_to_pipe[count] =3D=3D pipe->pipe) { >>> + igt_output_set_pipe(output, i); >>> + mode =3D 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 =3D &display->pipes[i]; >>> + plane =3D igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY= ); >>> + >>> + mode =3D NULL; >>> + >>> + count =3D 0; >>> + for_each_connected_output(display, output) { >>> + if (output_to_pipe[count] =3D=3D pipe->pipe) { >>> + igt_output_set_pipe(output, i); >>> + mode =3D 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 =3D 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 =3D 0, num_pipes; >>> + >>> + num_pipes =3D igt_display_get_n_pipes(&data->display); >>> + for_each_connected_output(&data->display, output) >>> + valid_outputs++; >>> + >>> + igt_require_f(valid_outputs =3D=3D 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 =3D 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 =3D 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 >> >> >>> + >>> + 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 =3D [ >>> '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 >>> > _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev