From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTPS id 7BB046F3CB for ; Fri, 26 Mar 2021 12:48:57 +0000 (UTC) From: "Modem, Bhanuprakash" Date: Fri, 26 Mar 2021 12:48:51 +0000 Message-ID: References: <20210310210354.9335-1-bhanuprakash.modem@intel.com> In-Reply-To: Content-Language: en-US MIME-Version: 1.0 Subject: Re: [igt-dev] [v3 i-g-t] tests/kms_busy: Limit the execution to two pipes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: "Latvala, Petri" Cc: "igt-dev@lists.freedesktop.org" , "Kunche, Kishore" List-ID: > From: Latvala, Petri > Sent: Friday, March 26, 2021 5:38 PM > To: Modem, Bhanuprakash > Cc: igt-dev@lists.freedesktop.org; Kunche, Kishore > > Subject: Re: [igt-dev] [v3 i-g-t] tests/kms_busy: Limit the execution to > two pipes > > On Thu, Mar 11, 2021 at 02:33:54AM +0530, Bhanuprakash Modem wrote: > > As all pipes are symmetric, restrict the execution to two pipes > > can save lot of CI time. > > > > If we want to execute on all pipes, we need to pass an extra > > argument "-e" indicates extended. > > > > Example: ./build/tests/kms_busy -e --r basic > > > > V2, V3: > > * Fix the typo in args handler (Petri) > > > > Cc: Karthik B S > > Cc: Latvala Petri > > Cc: Kunche Kishore > > Signed-off-by: Bhanuprakash Modem > > --- > > tests/kms_busy.c | 138 ++++++++++++++++++++++++++--------------------- > > 1 file changed, 77 insertions(+), 61 deletions(-) > > > > diff --git a/tests/kms_busy.c b/tests/kms_busy.c > > index df1f8e11a..9ced75742 100644 > > --- a/tests/kms_busy.c > > +++ b/tests/kms_busy.c > > @@ -30,6 +30,15 @@ > > > > IGT_TEST_DESCRIPTION("Basic check of KMS ABI with busy framebuffers."); > > > > +/* restricted pipe count */ > > +#define CRTC_RESTRICT_CNT 2 > > + > > +static bool all_pipes = false; > > + > > +#define for_each_pipe_with_valid_output_limited(display, pipe, output, > pipe_count) \ > > + for_each_pipe_with_valid_output(display, pipe, output) \ > > + for_each_if(pipe_count-- > 0) > > + > > > I don't like the semantics of a loop like this (as I think I've > commented on another patch): The pipe_count parameter needs to be an > lvalue, the caller needs to remember to reset it (below code doesn't > do it)... Thanks Petri, I'll take care of this in next version. > > > > > static igt_output_t * > > set_fb_on_crtc(igt_display_t *dpy, int pipe, struct igt_fb *fb) > > { > > @@ -287,10 +296,41 @@ static void > test_pageflip_modeset_hang(igt_display_t *dpy, enum pipe pipe) > > igt_remove_fb(dpy->drm_fd, &fb); > > } > > > > -igt_main > > +static int opt_handler(int opt, int opt_index, void *data) > > +{ > > + switch (opt) { > > + case 'e': > > + all_pipes = true; > > + break; > > + default: > > + return IGT_OPT_HANDLER_ERROR; > > + } > > + > > + return IGT_OPT_HANDLER_SUCCESS; > > +} > > + > > +const char *help_str = > > + " -e \tRun on all pipes. (By default subtests will run on two > pipes)\n"; > > + > > +igt_main_args("e", NULL, help_str, opt_handler, NULL) > > { > > igt_display_t display = { .drm_fd = -1, .n_pipes = IGT_MAX_PIPES }; > > - enum pipe n; > > + > > + int crtc_count; > > + int i; > > + struct { > > + const char *name; > > + bool modeset; > > + bool hang_newfb; > > + bool reset; > > + } tests[] = { > > + { "extended-pageflip-hang-oldfb", false, false, false }, > > + { "extended-pageflip-hang-newfb", false, true, false }, > > + { "extended-modeset-hang-oldfb", true, false, false }, > > + { "extended-modeset-hang-newfb", true, true, false }, > > + { "extended-modeset-hang-oldfb-with-reset", true, false, true > }, > > + { "extended-modeset-hang-newfb-with-reset", true, true, true }, > > + }; > > > > igt_fixture { > > int fd = drm_open_driver_master(DRIVER_INTEL); > > @@ -305,84 +345,60 @@ igt_main > > > > /* XXX Extend to cover atomic rendering tests to all planes + legacy > */ > > > > - igt_subtest_with_dynamic("basic") { /* just run on the first pipe */ > > + igt_subtest_with_dynamic("basic") { > > enum pipe pipe; > > igt_output_t *output; > > + crtc_count = (all_pipes)? display.n_pipes : CRTC_RESTRICT_CNT; > > > > - for_each_pipe_with_valid_output(&display, pipe, output) { > > - igt_dynamic("flip") > > + for_each_pipe_with_valid_output_limited(&display, pipe, output, > crtc_count) { > > + igt_dynamic_f("flip-pipe-%s", kmstest_pipe_name(pipe)) > > test_flip(&display, pipe, false); > > - igt_dynamic("modeset") > > + igt_dynamic_f("modeset-pipe-%s", kmstest_pipe_name(pipe)) > > test_flip(&display, pipe, true); > > - break; > > Commit message says this limits the amount of pipes used, but the > original code only executes on the first pipe. > Without this patch These basic tests are without hang, running on single pipe: igt_subtest_with_dynamic("basic") { igt_dynamic("flip"); igt_dynamic("modeset"); break; } And these tests are with hang, running on all pipes: for_each_pipe_static(pipe) { Allow hang; igt_subtest_f("basic-flip-pipe-%s"); igt_subtest_f("basic-modeset-pipe-%s"); } With this patch it became: igt_subtest_with_dynamic("basic") { igt_dynamic("flip"); igt_dynamic("modeset"); if (pipe_count >= 2) break; } And we lost the basic tests with hang. I'll float a new version by addressing this. Thanks for the review. > > > -- > Petri Latvala > > > > } > > } > > > > - for_each_pipe_static(n) igt_subtest_group { > > - igt_hang_t hang; > > - > > - errno = 0; > > - > > - igt_fixture { > > - igt_display_require_output_on_pipe(&display, n); > > - } > > - > > - igt_subtest_f("basic-flip-pipe-%s", kmstest_pipe_name(n)) { > > - test_flip(&display, n, false); > > - } > > - igt_subtest_f("basic-modeset-pipe-%s", kmstest_pipe_name(n)) { > > + igt_subtest_with_dynamic("extended-pageflip-modeset-hang-oldfb") { > > + enum pipe pipe; > > + igt_output_t *output; > > + crtc_count = (all_pipes)? display.n_pipes : CRTC_RESTRICT_CNT; > > > > - test_flip(&display, n, true); > > - } > > + for_each_pipe_with_valid_output_limited(&display, pipe, output, > crtc_count) { > > + igt_hang_t hang = igt_allow_hang(display.drm_fd, 0, 0); > > + errno = 0; > > > > - igt_fixture { > > - hang = igt_allow_hang(display.drm_fd, 0, 0); > > - } > > + igt_dynamic_f("pipe-%s", kmstest_pipe_name(pipe)) > > + test_pageflip_modeset_hang(&display, pipe); > > > > - igt_subtest_f("extended-pageflip-modeset-hang-oldfb-pipe-%s", > > - kmstest_pipe_name(n)) { > > - test_pageflip_modeset_hang(&display, n); > > + igt_disallow_hang(display.drm_fd, hang); > > } > > + } > > > > - igt_fixture > > - igt_require(display.is_atomic); > > - > > - igt_subtest_f("extended-pageflip-hang-oldfb-pipe-%s", > > - kmstest_pipe_name(n)) > > - test_hang(&display, n, false, false); > > - > > - igt_subtest_f("extended-pageflip-hang-newfb-pipe-%s", > > - kmstest_pipe_name(n)) > > - test_hang(&display, n, false, true); > > - > > - igt_subtest_f("extended-modeset-hang-oldfb-pipe-%s", > > - kmstest_pipe_name(n)) > > - test_hang(&display, n, true, false); > > - > > - igt_subtest_f("extended-modeset-hang-newfb-pipe-%s", > > - kmstest_pipe_name(n)) > > - test_hang(&display, n, true, true); > > + for (i = 0; i < sizeof(tests) / sizeof (tests[0]); i++) { > > + igt_subtest_with_dynamic(tests[i].name) { > > + enum pipe pipe; > > + igt_output_t *output; > > + crtc_count = (all_pipes)? display.n_pipes : > CRTC_RESTRICT_CNT; > > > > - igt_subtest_f("extended-modeset-hang-oldfb-with-reset-pipe-%s", > > - kmstest_pipe_name(n)) { > > - igt_set_module_param_int(display.drm_fd, > "force_reset_modeset_test", 1); > > + for_each_pipe_with_valid_output_limited(&display, pipe, > output, crtc_count) { > > + igt_hang_t hang; > > + errno = 0; > > > > - test_hang(&display, n, true, false); > > + igt_require(display.is_atomic); > > + hang = igt_allow_hang(display.drm_fd, 0, 0); > > > > - igt_set_module_param_int(display.drm_fd, > "force_reset_modeset_test", 0); > > - } > > + igt_dynamic_f("pipe-%s", kmstest_pipe_name(pipe)) { > > + if (tests[i].reset) > > + igt_set_module_param_int(display.drm_fd, > "force_reset_modeset_test", 1); > > > > - igt_subtest_f("extended-modeset-hang-newfb-with-reset-pipe-%s", > > - kmstest_pipe_name(n)) { > > - igt_set_module_param_int(display.drm_fd, > "force_reset_modeset_test", 1); > > + test_hang(&display, pipe, tests[i].modeset, > tests[i].hang_newfb); > > > > - test_hang(&display, n, true, true); > > + if (tests[i].reset) > > + igt_set_module_param_int(display.drm_fd, > "force_reset_modeset_test", 0); > > + } > > > > - igt_set_module_param_int(display.drm_fd, > "force_reset_modeset_test", 0); > > - } > > - > > - igt_fixture { > > - igt_disallow_hang(display.drm_fd, hang); > > + igt_disallow_hang(display.drm_fd, hang); > > + } > > } > > } > > > > -- > > 2.20.1 > > > > _______________________________________________ > > 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