[PATCH v5,resend i-g-t 3/6] tests: Add kms_debugfs
Peter Senna Tschudin
peter.senna at linux.intel.com
Thu Jul 10 19:06:13 UTC 2025
Hi Karthik,
Thank you for your reply.
On 7/10/2025 6:09 PM, Karthik B S wrote:
> Hi Peter,
>
> On 7/10/2025 3:43 PM, Kamil Konieczny wrote:
>> Hi Peter,
>> On 2025-07-08 at 16:40:35 +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:lucas.demarchi at intel.com
>>> Cc:rodrigo.vivi at intel.com
>>> Cc:kamil.konieczny at linux.intel.com
>>> Cc:katarzyna.piecielska at intel.com
>>> Cc:zbigniew.kempczynski at intel.com
>>> Cc:michal.wajdeczko at intel.com
>>> Cc:karthik.b.s at intel.com
>>> Signed-off-by: Peter Senna Tschudin<peter.senna at linux.intel.com>
>> +Cc: Vitaly Prosyak<vitaly.prosyak at amd.com>
>>
>> Vitaly, could you help us and forward this to your CI testing
>> team and ask them to test this on amdgpu + display connected?
>> This should pass as it is a simple debugfs reading.
>>
>>> ---
>>> 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 | 151 ++++++++++++++++++++++++++++++++++++++++++++
>>> tests/meson.build | 1 +
>>> 2 files changed, 152 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..7e10c4bca
>>> --- /dev/null
>>> +++ b/tests/kms_debugfs.c
>>> @@ -0,0 +1,151 @@
>>> +// 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;
>>> + 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,
>> Align here.
>>
>>> + DRM_FORMAT_XRGB8888,
>> Same.
>>
>>> + DRM_FORMAT_MOD_LINEAR, &fb[pipe]);
>> Same.
>>
>>> +
>>> + /* 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) {
>>> + bool found = igt_fit_modes_in_bw(display);
>>> +
>>> + ret = found ? 0 : -1;
>>> + igt_require_f(found, "No valid mode combo found.\n");
>>> + } else
>>> + ret = igt_display_try_commit2(display, COMMIT_LEGACY);
>> Add braces for 'else' here:
>> } else {
>> ret = igt_display_try_commit2(display, COMMIT_LEGACY);
>> }
>>
>> You could see it yourself with checkpatch.pl with options from
>> CONTRIBUTING.md:
>>
>> 0003-tests-Add-kms_debugfs.patch:99: CHECK:BRACES: braces {} should be
>> used on all arms of this statement
>> #99: FILE: tests/kms_debugfs.c:69:
>> + if (display->is_atomic) {
>> [...]
>> + } else
>> [...]
>>
>> 0003-tests-Add-kms_debugfs.patch:104: CHECK:BRACES: Unbalanced braces
>> around else statement
>> #104: FILE: tests/kms_debugfs.c:74:
>> + } else
>>
>>> +
>>> + if (ret) {
>>> + igt_output_t *output;
>>> +
>>> + for_each_connected_output(display, output)
>>> + igt_output_set_pipe(output, PIPE_NONE);
>>> +
>> Please add some counter and abort on reaching max_retry
>> to prevent infinite loop here. Something like:
>>
>> ++cnt_retry;
>> igt_assert_f(cnt_retry < MAX_IGT_PIPES, "Too much retries
>> %d\n", cnt_retry);
>>
>> Karthik or Swati: I am not sure if this is needed, could you comment
>> on this here?
>
> |I think we don't need a retry at all here. If there is a fail with the
> first commit just try if we're able to go ahead after using
> 'igt_fit_modes_in_bw' and if it doesn't allow just skip the test here.
> That was my motivation to suggest to use the wrapper on the previous
> revision of the patch.|
>
> |The only issue currently I see(that I just observed) is in
> 'igt_fit_modes_in_bw' the try_commit we're doing is using atomic commit
> by default, although there is no reason to have this restriction. So
> ideally we would want to fix that in lib and then just have something
> like below in this test.|
This is getting a bit out of hand for me because I don't know much about
displays. First you asked me to use a wrapper, now you want me to fix
the wrapper. I will pass fixing the wrapper as this may lead to yet more
unrelated work.
Should I revert back to not using igt_fit_modes_in_bw()?
>
> |if (atomic)|
>
> | ret = igt_display_try_atomic_commit|
>
> |else|
>
> | ret = igt_display_try_commit2(legacy)|
>
> |if (ret)|
>
> | igt_require(||igt_fit_modes_in_bw(display))|
>
> |igt_display_commit2(display, display->is_atomic ? COMMIT_ATOMIC :
> COMMIT_LEGACY);|
I am not sure I follow what you are suggesting here.
Thank you,
Peter
>
> |Regards,|
> |Karthik.B.S|
>>
>> Overall code looks good.
>>
>> Regards,
>> Kamil
>>
>>> + goto retry;
>>> + }
>>> +
>>> + igt_display_commit2(display, display->is_atomic ?
>>> COMMIT_ATOMIC : COMMIT_LEGACY);
>>> +}
>>> +
>>> +/**
>>> + * 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);
>>> +}
>>> +
>>> +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 99d7d95be..1009bef2e 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
>>>
More information about the igt-dev
mailing list