Re: [PATCH v2 i-g-t 3/6] tests: Add core_debugfs_display_on_off

[Karthik B S <karthik.b.s@intel.com>]

On 6/13/2025 11:24 PM, Kamil Konieczny wrote:
> Hi Peter,
> On 2025-06-13 at 12:51:42 +0200, Peter Senna Tschudin wrote:
>
> this is not full review, I only have few nits, see below.
>
>> Introduce core_debugfs_display_on_off that is expected to work with any
>> GPU, not limited to i915 and Xe. The test powers on all available
>> displays before reading debugfs files, and then powers off all displays
>> before reading the files again.
>>
>> Cc: marcin.bernatowicz@intel.com
>> Cc: kamil.konieczny@linux.intel.com
>> Cc: katarzyna.piecielska@intel.com
>> Cc: zbigniew.kempczynski@intel.com
>> Cc: ewelina.musial@intel.com
>> Signed-off-by: Peter Senna Tschudin <peter.senna@linux.intel.com>
>> ---
>> v2:
>>   - changed style of comparison to NULL
>>   - moved to a separate patch
>>
>>   tests/core_debugfs_display_on_off.c | 171 ++++++++++++++++++++++++++++
>>   tests/meson.build                   |   1 +
>>   2 files changed, 172 insertions(+)
>>   create mode 100644 tests/core_debugfs_display_on_off.c
> imho test name should be shorter, like core_debugfs_display.c
>
>> diff --git a/tests/core_debugfs_display_on_off.c b/tests/core_debugfs_display_on_off.c
>> new file mode 100644
>> index 000000000..b7dd96208
>> --- /dev/null
>> +++ b/tests/core_debugfs_display_on_off.c
>> @@ -0,0 +1,171 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2025 Intel Corporation
>> + */
>> +
>> +#include "igt.h"
>> +#include "igt_debugfs.h"
>> +#include "igt_dir.h"
>> +
>> +/**
>> + * TEST: debugfs display on/off test
>> + * Description: Read entries from debugfs, and sysfs paths.
> Please also write about display in description.
> Is this for both debugfs and sysfs? Test name has only 'debugfs'
> so imho you should remove 'sysfs'
>
>> + * Category: Core
>> + * Mega feature: General Core features
>> + * Sub-category: uapi
>> + * Functionality: debugfs
>> + * Feature: core
>> + * Test category: uapi
>> + *
>> + * SUBTEST: debugfs-read-all-entries-display-off
> imho subtest name should be shorter, like: off-read-all
>
>> + * Description: Read all debugfs entries with display off.
>> + *
>> + * SUBTEST: debugfs-read-all-entries-display-on
> Same here, imho better: on-read-all
>
> Or it could go with 'display-' prefix:
>
> display-off-read-all
>
> display-on-read-all
>
> +cc Karthik, Juha-Pekka and Swati
>
> Cc: Karthik B S <karthik.b.s@intel.com>
> Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> Cc: Juha-Pekka Heikkila <juha-pekka.heikkila@intel.com>
> Cc: Swati Sharma <swati2.sharma@intel.com>
>
>> + * Description: Read all debugfs entries with display on.
>> + */
>> +
>> +/** bool igt_kms_all_displays_on: Try to turn on all displays
> imho:
>
> /**
>   * bool igt_kms_all_displays_on: Try to turn on all displays
>
>> + * @fd: file descriptor for the drm device
>> + *
>> + * Returns: void
>> + */
>> +static void igt_display_all_on(igt_display_t *display)
>> +{
>> +	struct igt_fb fb[IGT_MAX_PIPES];
>> +	enum pipe pipe;
>> +	int ret;
>> +
>> +	/* try to light all pipes */
>> +retry:
>> +	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;
>> +		}
>> +	}
>> +
>> +	if (display->is_atomic)
>> +		ret = igt_display_try_commit_atomic(display,
>> +				DRM_MODE_ATOMIC_TEST_ONLY |
>> +				DRM_MODE_ATOMIC_ALLOW_MODESET,
>> +				NULL);
>> +	else
>> +		ret = igt_display_try_commit2(display, COMMIT_LEGACY);
>> +
>> +	if (ret) {
>> +		igt_output_t *output;
>> +		bool found = igt_override_all_active_output_modes_to_fit_bw(display);
>> +
>> +		igt_require_f(found, "No valid mode combo found.\n");
> Why require here? We want just displays on, why break when
> not hi-res here? Karthik or Swati, please help with this.

Hi Kamil,

Sorry for getting back late on this one.

This break happens only if no valid combination of modes is found across 
all connected displays. So this require check is valid.

Regards,
Karthik.B.S
>
>> +
>> +		for_each_connected_output(display, output)
>> +			igt_output_set_pipe(output, PIPE_NONE);
>> +
>> +		goto retry;
>> +	}
>> +
>> +	igt_display_commit2(display, display->is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
>> +}
>> +
>> +/** bool igt_kms_all_displays_off: Try to turn off all displays
>> + * @fd: file descriptor for the drm device
>> + *
>> + * 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);
>> +}
>> +
>> +static void kms_tests(int fd, igt_dir_t *igt_dir)
>> +{
> Please move this code to igt_main below.
>
>> +	igt_display_t *display;
>> +	char test_name[64];
>> +
>> +	display = calloc(1, sizeof(*display));
>> +
>> +	igt_fixture
>> +		igt_display_require(display, fd);
>> +
>> +	snprintf(test_name, sizeof(test_name),
>> +		 "debugfs-read-all-entries-display-on");
>> +
>> +	igt_subtest(test_name) {
> Use directly name, why snprintf to test_name?
>
> 	igt_subtest("on-read-all") {
>
>> +		/* try to light all pipes */
>> +		igt_display_all_on(display);
>> +
>> +		igt_dir_scan_dirfd(igt_dir, -1);
>> +		igt_dir_process_files(igt_dir, NULL, NULL);
>> +	}
>> +
>> +	snprintf(test_name, sizeof(test_name),
>> +		 "debugfs-read-all-entries-display-off");
>> +
>> +	igt_subtest(test_name) {
> Same here:
> 	igt_subtest("off-read-all") {
>
>> +		igt_display_all_off(display);
>> +
>> +		igt_dir_scan_dirfd(igt_dir, -1);
>> +		igt_dir_process_files(igt_dir, NULL, NULL);
>> +	}
>> +
>> +	igt_fixture
>> +		igt_display_fini(display);
>> +}
>> +
>> +IGT_TEST_DESCRIPTION("Read entries from debugfs with display on/off.");
>> +
>> +igt_main
>> +{
>> +	int debugfs = -1;
>> +	int fd = -1;
>> +	igt_dir_t *igt_dir = NULL;
>> +
>> +	igt_fixture {
>> +		fd = drm_open_driver_master(DRIVER_ANY);
>> +		debugfs = igt_debugfs_dir(fd);
>> +		igt_require(debugfs >= 0);
>> +
>> +		igt_dir = igt_dir_create(debugfs);
>> +		igt_require(igt_dir);
>> +
>> +		kmstest_set_vt_graphics_mode();
>> +	}
>> +
>> +	igt_subtest_group
>> +		kms_tests(fd, igt_dir);
> No need for subtest group when only display is here.
>
>> +
>> +	igt_fixture {
> Should we turn on all displays here?
>
> Regards,
> Kamil
>
>> +		close(debugfs);
>> +		drm_close_driver(fd);
>> +	}
>> +}
>> diff --git a/tests/meson.build b/tests/meson.build
>> index fa62cbeb9..c05c023ef 100644
>> --- a/tests/meson.build
>> +++ b/tests/meson.build
>> @@ -1,6 +1,7 @@
>>   test_progs = [
>>   	'core_auth',
>>   	'core_debugfs',
>> +	'core_debugfs_display_on_off',
>>   	'core_getclient',
>>   	'core_getstats',
>>   	'core_getversion',
>> -- 
>> 2.43.0
>>