Re: [PATCH i-g-t] tests/kms_cursor_legacy: Reduce debug loglevel dynamically

[Jani Nikula <jani.nikula@intel.com>]

On Mon, 10 Feb 2025, Leo Li <sunpeng.li@amd.com> wrote:
> Hi Pranay,
>
> This change can also increase log verbosity; we observe this with amdgpu.
> Details inline.
>
> On 2025-01-28 00:28, Pranay Samala wrote:
>> This test is getting killed due to exceeding disk usage
>> limit on CI environment because its logging to many logs.
>> 
>> So dynamically reducing the debug log level to 10.
>> 
>> Signed-off-by: Pranay Samala <pranay.samala@intel.com>
>> ---
>>   tests/kms_cursor_legacy.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>> 
>> diff --git a/tests/kms_cursor_legacy.c b/tests/kms_cursor_legacy.c
>> index 1f082df2d..533972cd4 100644
>> --- a/tests/kms_cursor_legacy.c
>> +++ b/tests/kms_cursor_legacy.c
>> @@ -40,6 +40,7 @@
>>   #include "igt_psr.h"
>>   #include "igt_rand.h"
>>   #include "igt_stats.h"
>> +#include "igt_sysfs.h"
>>   
>>   /**
>>    * SUBTEST: %s-%s
>> @@ -1840,6 +1841,7 @@ igt_main
>>   	};
>>   
>>   	igt_fixture {
>> +		int dir, current_log_level;
>>   		display.drm_fd = drm_open_driver_master(DRIVER_ANY);
>>   		kmstest_set_vt_graphics_mode();
>>   
>> @@ -1850,6 +1852,15 @@ igt_main
>>   		 * fetch is enabled, so switching PSR1 for this whole test.
>>   		 */
>>   		intel_psr2_restore = i915_psr2_sel_fetch_to_psr1(display.drm_fd, NULL);
>> +
>> +		dir = igt_sysfs_drm_module_params_open();
>> +		if (dir >= 0) {
>> +			current_log_level = igt_drm_debug_level_get(dir);
>> +			close(dir);
>> +
>> +			if (current_log_level > 10)
>> +				igt_drm_debug_level_update(10);
>
> Isn't the DRM debug mask a bitmask?
> https://elixir.bootlin.com/linux/v6.13.1/source/include/drm/drm_print.h#L97
>
> So setting 10 isn't guaranteed to reduce verbosity, depending on how the DRM
> driver uses the various drm_* debug prints.

Ugh. Yes.

> What is guaranteed to reduce verbosity is to unset unwanted bits. Maybe that is
> the better approach?
>
> For example, instead of
> `if (current_log_level > 10)`,
>
> Do
>
> `igt_drm_debug_level_update(current_log_level & !(DRM_UT_CORE | ...);`

Ditto for commit a2ab0ec12ef4 ("tests/kms_atomic_transition: Reducing
debug loglevel dynamically"). It got to v6 and nobody noticed?

Ditto for commit 56b91193b825 ("lib/igt_sysfs: Implement dynamic
adjustment of debug log level") adding the whole thing. It should not be
referred to as "level" because it has nothing to do with "level". It
also makes no sense to print it out in decimal; hex makes more sense.


BR,
Jani.


>
> Thanks,
> Leo
>
>> +		}
>>   	}
>>   
>>   	igt_describe("Test checks how many cursor updates we can fit between vblanks "
>

-- 
Jani Nikula, Intel