From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 62C7BD35179 for ; Wed, 1 Apr 2026 12:07:00 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 0CA4110E14E; Wed, 1 Apr 2026 12:07:00 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="KgEhPEVL"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.16]) by gabe.freedesktop.org (Postfix) with ESMTPS id C68AF10E369 for ; Wed, 1 Apr 2026 12:06:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1775045211; x=1806581211; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=nl4Ob09tTbJ4k6589e7hjD+8DDnNygR3qOc8/DU9Rxc=; b=KgEhPEVLFsqObT4qIpljo6tzGtETn4CBzcfIOL4Xt1/YohSZoKTbFoFM nDR2TT86svFfOP9/EKlnBVaGWAscwvqD0pYRPVI8hZSNto+d8JdfbvEIS 2clc4jhJFuWHoMswieEkMswUTUmg1hUeZikmsVrecxZ5Lw4DxsOfYhoID Pb19BB2W46FnN9oDlpf8uOW+khBMQ9B6nt+ybR9KYPeVUdeeCqG0t7MHc rBUSq/l7XbYVXtKXhXqkLzsjcyHdWjsDcmFtINlprwd3BHzSeO5m5OSdk DXW8hZeO/kggp28G9gWkVgdgnh+1/v86HV+kyCQ1DXEvk5Onqw0qp14l5 A==; X-CSE-ConnectionGUID: sPI7czQuR5SevVsUAFxJ4g== X-CSE-MsgGUID: LWBmpBkZROOEiiP3VEtKZw== X-IronPort-AV: E=McAfee;i="6800,10657,11745"; a="63630655" X-IronPort-AV: E=Sophos;i="6.23,153,1770624000"; d="scan'208";a="63630655" Received: from orviesa004.jf.intel.com ([10.64.159.144]) by fmvoesa110.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Apr 2026 05:06:51 -0700 X-CSE-ConnectionGUID: trkKpCQKRyKXN5dvv9n1Eg== X-CSE-MsgGUID: OHyaoc14QLWKGsHY6BuXhg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.23,153,1770624000"; d="scan'208";a="231071012" Received: from pgcooper-mobl3.ger.corp.intel.com (HELO localhost) ([10.245.244.199]) by orviesa004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Apr 2026 05:06:49 -0700 Date: Wed, 1 Apr 2026 15:06:46 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Jani Nikula Cc: Kunal Joshi , igt-dev@lists.freedesktop.org, Mohammed Thasleem Subject: Re: [PATCH i-g-t 6/6] tests/kms_pipe_crc_basic: Add MST suspend-resume test Message-ID: References: <20260401115542.1074290-1-kunal1.joshi@intel.com> <20260401115542.1074290-7-kunal1.joshi@intel.com> <996f8fca71b8e9afad16b0a4ca3331636bfb34ac@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <996f8fca71b8e9afad16b0a4ca3331636bfb34ac@intel.com> X-Patchwork-Hint: comment Organization: Intel Finland Oy - BIC 0357606-4 - c/o Alberga Business Park, 6 krs Bertel Jungin Aukio 5, 02600 Espoo, Finland X-BeenThere: igt-dev@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Development mailing list for IGT GPU Tools List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" On Wed, Apr 01, 2026 at 02:52:12PM +0300, Jani Nikula wrote: > On Wed, 01 Apr 2026, Kunal Joshi wrote: > > Add a new subtest mst-suspend-read-crc that verifies MST outputs > > preserve display content (via CRC comparison) across S3 suspend/resume. > > > > The test enumerates all connected DP MST topologies, and for each root > > connector exercises all non-empty subsets of the discovered MST outputs. > > For each subset it: > > - Assigns pipes and selects non-joiner modes > > - Fits modes within bandwidth constraints > > - Creates green FBs, commits, and collects pre-suspend CRCs > > - Suspends to memory and resumes > > - Collects post-suspend CRCs and asserts they match > > tests/kms_pipe_crc_basic.c is supposed to be the basic CRC test. Adding > MST to it is fine, I guess, I don't understand even the MST part. What does the "MST vs. not" have to do verifying that pipe CRCs work? Maybe the intention here is the opposite? As in verify that MST suspend/resume works, and implement that by *using* pipe CRCs. In that case this whole thing should be a completely different test and not shoehorned into kms_pipe_crc_basic. > but adding all the Intel specific joiner > stuff here seems questionable. > > IMO basically anything that needs to know anything about the concept of > "pipes" (as opposed to CRTCs and CRTC indexes) should be inside > tests/intel or minimized within checks for Intel devices. > > > BR, > Jani. > > > > > v2: > > - Add error handling for igt_pipe_crc_new (Thasleem) > > - Improve log format with index for clarity (Thasleem) > > - Order declarations in decreasing line length (Thasleem) > > - Add print message when no MST found (Thasleem) > > - Add bounds check for idx in subset loop (Thasleem) > > - Use igt_crtc_crc_new() instead of igt_pipe_crc_new() (Ville) > > > > v3: > > - framebuffers missing in commit (Bilal) > > > > v4: > > - Drop cleanup framework > > > > v5: > > - Use igt_debug instead of igt_info for CRC logging (Thasleem) > > - Remove comments (Suraj) > > - Remove double vblank waits before/after suspend (Suraj) > > - Rename variable (Thasleem) > > - Two-pass approach (Suraj) > > > > Signed-off-by: Kunal Joshi > > Reviewed-by: Mohammed Thasleem > > --- > > tests/kms_pipe_crc_basic.c | 318 +++++++++++++++++++++++++++++++++++++ > > tests/meson.build | 3 + > > 2 files changed, 321 insertions(+) > > > > diff --git a/tests/kms_pipe_crc_basic.c b/tests/kms_pipe_crc_basic.c > > index 42e141545..d8369807e 100644 > > --- a/tests/kms_pipe_crc_basic.c > > +++ b/tests/kms_pipe_crc_basic.c > > @@ -32,7 +32,10 @@ > > > > #include "igt.h" > > #include "igt_sysfs.h" > > +#include "intel/kms_mst_helper.h" > > +#include "intel/kms_joiner_helper.h" > > #include > > +#include > > #include > > #include > > #include > > @@ -77,6 +80,11 @@ > > * CRTC does not cause issues. > > */ > > > > +/** > > + * SUBTEST: mst-suspend-read-crc > > + * Description: MST suspend test for pipe CRC reads. > > + */ > > + > > static bool extended; > > static int active_crtcs[IGT_MAX_PIPES]; > > static uint32_t last_crtc_index; > > @@ -96,6 +104,310 @@ static struct { > > { .r = 0.0, .g = 1.0, .b = 1.0 }, > > }; > > > > +#define MAX_MST_OUTPUT 3 > > + > > +struct mst_root_info { > > + int root_id; > > + igt_output_t *mst_outputs[IGT_MAX_PIPES]; > > + int num_mst; > > +}; > > + > > +static void collect_single_crc(igt_crtc_t *crtc, igt_crc_t *crc) > > +{ > > + igt_pipe_crc_t *pipe_crc; > > + > > + pipe_crc = igt_crtc_crc_new(crtc, IGT_PIPE_CRC_SOURCE_AUTO); > > + igt_assert(pipe_crc); > > + igt_pipe_crc_collect_crc(pipe_crc, crc); > > + igt_pipe_crc_free(pipe_crc); > > +} > > + > > +static void collect_crc_for_active_outputs(igt_output_t **outputs, > > + int num_outputs, > > + igt_crc_t *crcs, > > + bool post_suspend) > > +{ > > + int i; > > + > > + for (i = 0; i < num_outputs; i++) { > > + igt_output_t *output = outputs[i]; > > + igt_crtc_t *crtc = igt_output_get_driving_crtc(output); > > + > > + if (post_suspend) > > + igt_require_f(crtc, > > + "POST-SUSPEND: no driving CRTC for %s (topology changed?)\n", > > + output->name); > > + else > > + igt_assert_f(crtc, > > + "PRE-SUSPEND: no driving CRTC for %s (bad pipe assignment?)\n", > > + output->name); > > + > > + collect_single_crc(crtc, &crcs[i]); > > + } > > +} > > + > > +static void wait_for_vblanks(igt_output_t **outputs, int num_outputs, > > + bool post_suspend) > > +{ > > + int i; > > + > > + for (i = 0; i < num_outputs; i++) { > > + igt_crtc_t *crtc = igt_output_get_driving_crtc(outputs[i]); > > + > > + if (post_suspend) > > + igt_require_f(crtc, > > + "POST-SUSPEND: no driving CRTC for %s during vblank wait (topology changed?)\n", > > + outputs[i]->name); > > + else > > + igt_assert_f(crtc, > > + "PRE-SUSPEND: no driving CRTC for %s during vblank wait (bad pipe assignment?)\n", > > + outputs[i]->name); > > + > > + igt_wait_for_vblank(crtc); > > + } > > +} > > + > > +static void log_crc_for_outputs(igt_output_t **outputs, int num_outputs, > > + igt_crc_t *crcs, const char *stage) > > +{ > > + int i; > > + > > + for (i = 0; i < num_outputs; i++) { > > + igt_output_t *o = outputs[i]; > > + drmModeModeInfo *m = igt_output_get_mode(o); > > + char *s = igt_crc_to_string(&crcs[i]); > > + igt_crtc_t *crtc = igt_output_get_driving_crtc(o); > > + > > + igt_debug("[%d] %s: output %s -> crtc %s, CRC %s, mode %dx%d@%d\n", > > + i, stage, o->name, > > + igt_crtc_name(crtc), > > + s, > > + m ? m->hdisplay : -1, > > + m ? m->vdisplay : -1, > > + m ? m->vrefresh : 0); > > + free(s); > > + } > > +} > > + > > +static void select_non_joiner_modes(int drm_fd, igt_output_t **outputs, > > + int num_outputs) > > +{ > > + int max_dotclock = igt_get_max_dotclock(drm_fd); > > + int i; > > + > > + if (max_dotclock <= 0) > > + max_dotclock = INT_MAX; > > + > > + for (i = 0; i < num_outputs; i++) { > > + igt_output_t *output = outputs[i]; > > + drmModeModeInfo non_joiner_mode; > > + > > + igt_require_f(max_non_joiner_mode_found(drm_fd, > > + output->config.connector, > > + max_dotclock, > > + &non_joiner_mode), > > + "No non-joiner mode found for %s\n", > > + output->name); > > + igt_output_override_mode(output, &non_joiner_mode); > > + } > > +} > > + > > +static void create_fbs_for_outputs(int drm_fd, igt_output_t **outputs, > > + int num_outputs, struct igt_fb *fbs) > > +{ > > + int i; > > + > > + for (i = 0; i < num_outputs; i++) { > > + igt_output_t *output = outputs[i]; > > + drmModeModeInfo *mode = igt_output_get_mode(output); > > + igt_plane_t *primary; > > + > > + igt_require_f(mode, "No mode available for output %s\n", output->name); > > + > > + primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); > > + > > + igt_create_color_fb(drm_fd, > > + mode->hdisplay, mode->vdisplay, > > + DRM_FORMAT_XRGB8888, > > + DRM_FORMAT_MOD_LINEAR, > > + 0.0, 1.0, 0.0, > > + &fbs[i]); > > + igt_plane_set_fb(primary, &fbs[i]); > > + } > > +} > > + > > +static void run_mst_subset_and_verify(igt_display_t *display, > > + igt_output_t **active_outputs, > > + int num_active, > > + int n_crtcs, > > + uint32_t master_pipes_mask, > > + uint32_t valid_pipes_mask) > > +{ > > + igt_crc_t pre_crcs[MAX_MST_OUTPUT]; > > + igt_crc_t post_crcs[MAX_MST_OUTPUT]; > > + struct igt_fb fbs[MAX_MST_OUTPUT]; > > + uint32_t used_pipes_mask = 0; > > + enum igt_suspend_test stest = SUSPEND_TEST_NONE; > > + int drm_fd = display->drm_fd; > > + int i; > > + > > + igt_require_f(num_active <= n_crtcs, > > + "Not enough crtcs for MST subset\n"); > > + > > + igt_require(igt_assign_pipes_for_outputs(drm_fd, > > + active_outputs, > > + num_active, > > + n_crtcs, > > + &used_pipes_mask, > > + master_pipes_mask, > > + valid_pipes_mask)); > > + > > + select_non_joiner_modes(drm_fd, active_outputs, num_active); > > + > > + igt_require_f(igt_fit_modes_in_bw(display), > > + "Unable to fit modes in bw\n"); > > + > > + create_fbs_for_outputs(drm_fd, active_outputs, num_active, fbs); > > + > > + igt_display_commit2(display, COMMIT_ATOMIC); > > + > > + /* rtcwake cmd is not supported on MTK devices */ > > + if (is_mtk_device(drm_fd)) > > + stest = SUSPEND_TEST_DEVICES; > > + > > + igt_info("MST subset: %d outputs, n_crtcs=%d\n", > > + num_active, n_crtcs); > > + > > + wait_for_vblanks(active_outputs, num_active, false); > > + > > + collect_crc_for_active_outputs(active_outputs, > > + num_active, > > + pre_crcs, false); > > + > > + log_crc_for_outputs(active_outputs, num_active, pre_crcs, "PRE-SUSPEND"); > > + > > + igt_system_suspend_autoresume(SUSPEND_STATE_MEM, stest); > > + > > + wait_for_vblanks(active_outputs, num_active, true); > > + > > + collect_crc_for_active_outputs(active_outputs, > > + num_active, > > + post_crcs, true); > > + > > + log_crc_for_outputs(active_outputs, num_active, post_crcs, "POST-SUSPEND"); > > + > > + for (i = 0; i < num_active; i++) > > + igt_assert_crc_equal(&pre_crcs[i], &post_crcs[i]); > > + > > + /* Detach FBs from planes and remove them to leave a clean state */ > > + for (i = 0; i < num_active; i++) { > > + igt_plane_t *primary = > > + igt_output_get_plane_type(active_outputs[i], > > + DRM_PLANE_TYPE_PRIMARY); > > + igt_plane_set_fb(primary, NULL); > > + igt_remove_fb(drm_fd, &fbs[i]); > > + } > > +} > > + > > +static int discover_mst_roots(igt_display_t *display, > > + struct mst_root_info *roots) > > +{ > > + igt_output_t *output; > > + int drm_fd = display->drm_fd; > > + int num_roots = 0; > > + int ret, i; > > + > > + for_each_connected_output(display, output) { > > + bool root_seen = false; > > + int root_id; > > + > > + root_id = igt_get_dp_mst_connector_id(output); > > + if (root_id < 0) > > + continue; > > + > > + for (i = 0; i < num_roots; i++) > > + if (roots[i].root_id == root_id) > > + root_seen = true; > > + > > + if (root_seen) > > + continue; > > + > > + if (num_roots >= IGT_MAX_PIPES) > > + break; > > + > > + roots[num_roots].root_id = root_id; > > + roots[num_roots].num_mst = 0; > > + ret = igt_find_all_mst_output_in_topology(drm_fd, > > + display, output, > > + roots[num_roots].mst_outputs, > > + &roots[num_roots].num_mst); > > + igt_require(ret == 0); > > + > > + if (roots[num_roots].num_mst == 0) > > + continue; > > + > > + if (roots[num_roots].num_mst > MAX_MST_OUTPUT) > > + roots[num_roots].num_mst = MAX_MST_OUTPUT; > > + > > + num_roots++; > > + } > > + > > + return num_roots; > > +} > > + > > +static void mst_suspend_read_crc(igt_display_t *display) > > +{ > > + struct mst_root_info roots[IGT_MAX_PIPES]; > > + igt_output_t *active_outputs[MAX_MST_OUTPUT]; > > + uint32_t valid_pipes_mask = 0; > > + uint32_t master_pipes_mask; > > + int n_crtcs = igt_display_n_crtcs(display); > > + int num_roots, num_active, i; > > + igt_crtc_t *crtc; > > + > > + for_each_crtc(display, crtc) > > + valid_pipes_mask |= BIT(crtc->pipe); > > + > > + igt_set_all_master_pipes_for_platform(display, &master_pipes_mask); > > + > > + num_roots = discover_mst_roots(display, roots); > > + igt_require_f(num_roots > 0, "No MST roots found\n"); > > + > > + for (i = 0; i < num_roots; i++) { > > + igt_dynamic_f("mst-root-%d", roots[i].root_id) { > > + int mask; > > + > > + for (mask = 1; mask < (1 << roots[i].num_mst); mask++) { > > + int bit, idx = 0; > > + > > + for (bit = 0; bit < roots[i].num_mst; bit++) { > > + if (!(mask & (1 << bit))) > > + continue; > > + > > + if (idx >= MAX_MST_OUTPUT) > > + break; > > + > > + active_outputs[idx++] = > > + roots[i].mst_outputs[bit]; > > + } > > + > > + num_active = idx; > > + if (!num_active) > > + continue; > > + > > + igt_display_reset(display); > > + > > + run_mst_subset_and_verify(display, > > + active_outputs, > > + num_active, > > + n_crtcs, > > + master_pipes_mask, > > + valid_pipes_mask); > > + } > > + } > > + } > > +} > > + > > static bool simulation_constraint(igt_crtc_t *crtc) > > { > > if (igt_run_in_simulation() && !extended && > > @@ -525,6 +837,12 @@ int igt_main_args("e", NULL, help_str, opt_handler, NULL) > > } > > } > > > > + igt_describe("MST suspend test for pipe CRC reads."); > > + igt_subtest_with_dynamic("mst-suspend-read-crc") { > > + igt_require(igt_display_has_mst_output(&data.display)); > > + mst_suspend_read_crc(&data.display); > > + } > > + > > igt_fixture() { > > igt_display_fini(&data.display); > > drm_close_driver(data.drm_fd); > > diff --git a/tests/meson.build b/tests/meson.build > > index cecb4a8ae..a1e42755e 100644 > > --- a/tests/meson.build > > +++ b/tests/meson.build > > @@ -402,6 +402,9 @@ extra_sources = { > > join_paths ('intel', 'kms_joiner_helper.c') ], > > 'kms_dsc': [ join_paths ('intel', 'kms_dsc_helper.c') ], > > 'kms_joiner': [ join_paths ('intel', 'kms_joiner_helper.c') ], > > + 'kms_pipe_crc_basic': [ > > + join_paths ('intel', 'kms_mst_helper.c'), > > + join_paths ('intel', 'kms_joiner_helper.c') ], > > 'kms_psr2_sf': [ join_paths ('intel', 'kms_dsc_helper.c') ], > > } > > -- > Jani Nikula, Intel -- Ville Syrjälä Intel