Re: [PATCH i-g-t v3 1/3] lib/igt_sysfs: Implement dynamic adjustment of debug log level

["Modem, Bhanuprakash" <bhanuprakash.modem@intel.com>]

On 11-07-2024 09:43 am, Pranay Samala wrote:
> Adjust debug log levels dynamically to prevent machine
> disk overflow during excessive test debug logs.
> 
> Introduce function to modify log levels as needed,
> with an exit handler restoring default settings post-test.
> 
> v3:
> - Adding module drm_open_param function to open
>    the sysfs directory (Bhanu)
> - Reducing the current drm loglevel by step value
>    to make it more robust (Bhanu)

Please keep maintain the rev history (v2 is missing).

> 
> Signed-off-by: Pranay Samala <pranay.samala@intel.com>
> ---
>   lib/igt_sysfs.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++
>   lib/igt_sysfs.h |   5 +++
>   2 files changed, 114 insertions(+)
> 
> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> index ffeec1ca2..d29e0299c 100644
> --- a/lib/igt_sysfs.c
> +++ b/lib/igt_sysfs.c
> @@ -49,6 +49,8 @@
>   #include "igt_io.h"
>   #include "intel_chipset.h"
>   
> +#define PATH "/sys/module/drm/parameters/debug"

Unused declaration, please drop.

> +
>   /**
>    * SECTION:igt_sysfs
>    * @short_description: Support code for sysfs features
> @@ -412,6 +414,113 @@ int igt_sysfs_get_num_gt(int device)
>   	return num_gts;
>   }
>   
> +/**
> + * igt_sysfs_drm_module_params_open:
> + *
> + * This opens the sysfs directory corresponding to drm module
> + * parameters.
> + *
> + * Returns:
> + * The directory fd, or -1 on failure.
> + */
> +int igt_sysfs_drm_module_params_open(void)
> +{
> +	char path[] = "/sys/module/drm/parameters";
> +
> +	if (access(path, F_OK))
> +		return -1;
> +
> +	return open(path, O_RDONLY);
> +}
> +
> +static int log_level = -1;
> +
> +/**
> + * igt_drm_debug_level_get:
> + *
> + * This reads the current debug log level of the machine on
> + * which the test is currently executing.
> + *
> + * Returns:
> + * The current log level, or -1 on error.
> + */
> +

Please drop this extra new line.

> +int igt_drm_debug_level_get(int dir)
> +{
> +	char buf[20];
> +
> +	if (log_level >= 0)
> +		return log_level;
> +
> +	if (igt_sysfs_read(dir, "debug", buf, sizeof(buf) - 1) < 0)
> +		return -1;
> +
> +	return atoi(buf);
> +}
> +
> +/**
> + * igt_drm_debug_level_reset:
> + *
> + * This modifies the current debug log level of the machine
> + * to the default value post-test.
> + *
> + */
> +

Please drop this extra new line.

> +void igt_drm_debug_level_reset(void)
> +{
> +	char buf[20];
> +	int dir;
> +
> +	if (log_level < 0)
> +		return;
> +
> +	dir = igt_sysfs_drm_module_params_open();
> +	if (dir < 0)
> +		return;
> +
> +	igt_debug("Resetting DRM debug level to %d\n", log_level);
> +	snprintf(buf, sizeof(buf), "%d", log_level);
> +	igt_assert(igt_sysfs_set(dir, "debug", buf));
> +
> +	close(dir);
> +}
> +
> +static void igt_drm_debug_level_reset_exit_handler(int sig)
> +{
> +	igt_drm_debug_level_reset();
> +}
> +
> +/**
> + * igt_drm_debug_level_update:
> + * @debug_level: new debug level to set
> + *
> + * This modifies the current drm debug log level to the new value.
> + */
> +

Please drop this extra new line.

> +void igt_drm_debug_level_update(unsigned int step)
> +{
> +	char buf[220];
----------------^
Please reduce the buf size.

> +	int dir, new_log_level;
> +
> +	dir = igt_sysfs_drm_module_params_open();
> +	if (dir < 0)
> +		return;
> +
> +	log_level = igt_drm_debug_level_get(dir);
> +	if (log_level < 0) {
> +		close(dir);
> +		return;
> +	}
> +
> +	new_log_level = log_level - step;

This logic must be handled at caller. This API is supposed to update 
with the requested value.

> +	igt_debug("Setting DRM bebug level to %d\n", new_log_level);
-------------------------------^
Typo: s/bebug/debug/

- Bhanu

> +	snprintf(buf, sizeof(buf), "%d", new_log_level);
> +	igt_assert(igt_sysfs_set(dir, "debug", buf));
> +
> +	close(dir);
> +	igt_install_exit_handler(igt_drm_debug_level_reset_exit_handler);
> +}
> +
>   /**
>    * igt_sysfs_write:
>    * @dir: sysfs directory
> diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
> index 6c604d939..a276c6ed1 100644
> --- a/lib/igt_sysfs.h
> +++ b/lib/igt_sysfs.h
> @@ -138,6 +138,11 @@ void igt_sysfs_set_boolean(int dir, const char *attr, bool value);
>   void bind_fbcon(bool enable);
>   void fbcon_blink_enable(bool enable);
>   
> +void igt_drm_debug_level_update(unsigned int debug_level);
> +void igt_drm_debug_level_reset(void);
> +int igt_drm_debug_level_get(int dir);
> +int igt_sysfs_drm_module_params_open(void);
> +
>   /**
>    * igt_sysfs_rw_attr:
>    * @dir: file descriptor for parent directory