Re: [PATCH v6 i-g-t 3/6] tests: Add kms_debugfs

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Jul 16, 2025 at 11:36:51AM +0200, Peter Senna Tschudin wrote:
> Introduce kms_debugfs that is expected to work with any GPU, not limited
> to i915 and Xe. The test powers off all available displays before
> reading debugfs files, and then powers on all displays before reading
> the files again.
> 
> Cc: kamil.konieczny@linux.intel.com
> Cc: katarzyna.piecielska@intel.com
> Cc: zbigniew.kempczynski@intel.com
> Cc: michal.wajdeczko@intel.com
> Cc: karthik.b.s@intel.com
> Cc: ewelina.musial@intel.com
> Signed-off-by: Peter Senna Tschudin <peter.senna@linux.intel.com>
> ---
> v6:
>  - use the new igt_fit_modes_in_bw()
> 
> v5:
>  - use igt_dir_process_files_simple()
> 
> v4:
>  - change test name to kms_debugfs
>  - use the wrapper function igt_fit_modes_in_bw()
>  - use igt_display_require_output() to ensure there is at least one display
> 
> v3:
>  - renamed the test
>  - Removed reference to sysfs from comments  (thanks Kamil)
>  - Updated description to match the display part (thanks Kamil)
>  - Moved from "display" to "heads". Our CI uses "headless" to refer
>    to a DUT without display and it is shorter
>  - renamed subtests for shorter names (thanks Kamil)
>  - fixed comments style (thanks Kamil)
>  - updated CC list
>  - changed the order to test first with all heads off then with all heads on
>    to prevent the need from powering on the heads at the end of the test
>    (thanks Kamil)
>  - removed snprintf from test names (thanks Kamil)
>  - removed subtest group (thanks Kamil)
>  - deleted kms_tests() and moved the code to igt_main (thanks Kamil)
> 
> v2:
>  - changed style of comparison to NULL
>  - moved to a separate patch
> 
>  tests/kms_debugfs.c | 133 ++++++++++++++++++++++++++++++++++++++++++++
>  tests/meson.build   |   1 +
>  2 files changed, 134 insertions(+)
>  create mode 100644 tests/kms_debugfs.c
> 
> diff --git a/tests/kms_debugfs.c b/tests/kms_debugfs.c
> new file mode 100644
> index 000000000..f8a3360e6
> --- /dev/null
> +++ b/tests/kms_debugfs.c
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2025 Intel Corporation
> + */
> +
> +#include "igt.h"
> +#include "igt_debugfs.h"
> +#include "igt_dir.h"
> +
> +/**
> + * TEST: kms debugfs test
> + * Description: Read entries from debugfs with all displays on and with
> + *		all displays off.
> + *
> + * Category: Core
> + * Mega feature: General Core features
> + * Sub-category: uapi
> + * Functionality: debugfs
> + * Feature: core
> + * Test category: uapi
> + *
> + * SUBTEST: display-off-read-all
> + * Description: Read all debugfs entries with display off.
> + *
> + * SUBTEST: display-on-read-all
> + * Description: Read all debugfs entries with display on.
> + */
> +
> +/**
> + * igt_display_all_on: Try to turn on all displays
> + * @display: pointer to the igt_display structure
> + *
> + * Returns: void
> + */
> +static void igt_display_all_on(igt_display_t *display)
> +{
> +	struct igt_fb fb[IGT_MAX_PIPES];
> +	enum pipe pipe;
> +
> +	/* try to light all pipes */
> +	for_each_pipe(display, pipe) {
> +		igt_output_t *output;
> +
> +		for_each_valid_output_on_pipe(display, pipe, output) {
> +			igt_plane_t *primary;
> +			drmModeModeInfo *mode;
> +
> +			if (output->pending_pipe != PIPE_NONE)
> +				continue;
> +
> +			igt_output_set_pipe(output, pipe);
> +			primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
> +			mode = igt_output_get_mode(output);
> +			igt_create_pattern_fb(display->drm_fd,
> +					      mode->hdisplay, mode->vdisplay,
> +					      DRM_FORMAT_XRGB8888,
> +					      DRM_FORMAT_MOD_LINEAR, &fb[pipe]);
> +
> +			/* Set a valid fb as some debugfs like to
> +			 * inspect it on a active pipe
> +			 */
> +			igt_plane_set_fb(primary, &fb[pipe]);
> +			break;
> +		}
> +	}
> +
> +	/* Skip if bandwidth is insufficient for all simultaneous displays */
> +	igt_require(igt_fit_modes_in_bw(display));
> +}
> +
> +/**
> + * igt_display_all_off: Try to turn off all displays
> + * @display: pointer to the igt_display structure
> + *
> + * Returns: void
> + */
> +static void igt_display_all_off(igt_display_t *display)
> +{
> +	enum pipe pipe;
> +	igt_output_t *output;
> +	igt_plane_t *plane;
> +
> +	for_each_connected_output(display, output)
> +		igt_output_set_pipe(output, PIPE_NONE);
> +
> +	for_each_pipe(display, pipe)
> +		for_each_plane_on_pipe(display, pipe, plane)
> +			igt_plane_set_fb(plane, NULL);
> +
> +	igt_display_commit2(display, display->is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);

I wonder if something here would be different then doing dpms off like xe_pm does...
perhaps we need 2 different cases?

anyway, this patch looks correct to me:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

since series is entirely reviewed now, I'm about to push this as is and if we find out
that this and dpms off are worth exploring we can add as a follow up.

> +}
> +
> +IGT_TEST_DESCRIPTION("Read entries from debugfs with display on/off.");
> +
> +igt_main
> +{
> +	int debugfs = -1;
> +	igt_display_t *display;
> +	int fd = -1;
> +
> +	igt_fixture {
> +		fd = drm_open_driver_master(DRIVER_ANY);
> +		debugfs = igt_debugfs_dir(fd);
> +		igt_require(debugfs >= 0);
> +
> +		kmstest_set_vt_graphics_mode();
> +
> +		display = calloc(1, sizeof(*display));
> +		igt_display_require(display, fd);
> +
> +		/* Make sure we have at least one output connected */
> +		igt_display_require_output(display);
> +	}
> +
> +	igt_subtest("display-off-read-all") {
> +		igt_display_all_off(display);
> +
> +		igt_dir_process_files_simple(debugfs);
> +	}
> +
> +	igt_subtest("display-on-read-all") {
> +		/* try to light all pipes */
> +		igt_display_all_on(display);
> +
> +		igt_dir_process_files_simple(debugfs);
> +	}
> +
> +	igt_fixture {
> +		igt_display_fini(display);
> +		close(debugfs);
> +		drm_close_driver(fd);
> +	}
> +}
> diff --git a/tests/meson.build b/tests/meson.build
> index 16c699bba..568e7db00 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -29,6 +29,7 @@ test_progs = [
>  	'kms_cursor_crc',
>  	'kms_cursor_edge_walk',
>  	'kms_cursor_legacy',
> +	'kms_debugfs',
>  	'kms_dither',
>  	'kms_display_modes',
>  	'kms_dp_aux_dev',
> -- 
> 2.43.0
>