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

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

Hi Pranay,

On 07-07-2024 08:34 pm, 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.
> 
> Signed-off-by: Pranay Samala <pranay.samala@intel.com>
> ---
>   lib/igt_sysfs.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>   lib/igt_sysfs.h |  3 +++
>   2 files changed, 48 insertions(+)
> 
> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> index ffeec1ca2..c256f0ae7 100644
> --- a/lib/igt_sysfs.c
> +++ b/lib/igt_sysfs.c
> @@ -412,6 +412,51 @@ int igt_sysfs_get_num_gt(int device)
>   	return num_gts;
>   }
>   
> +void igt_debug_reset(int drm_fd)

Better to use proper name to represent it's functionality, ex: 
igt_drm_debug_level_reset().

Also, documentation is mandatory for all non-static functions inside the 
lib.

> +{
> +	int fd = igt_sysfs_open(drm_fd);

Why do we need this check?
Basically, it'll search for the sysfs: "/sys/dev/char/<major>:<minor>".

> +
> +	if (fd < 0)
> +		return;
> +	const char *buf = "14";

We can't assume "14" as a default debug level, instead we must read the 
initial value and preserve it.

> +
> +	const char *path = "/sys/module/drm/parameters/debug";
> +	int debug_dir = openat(fd, path, O_WRONLY);
> +
> +	igt_debug("Resetting Debug value\n");
> +	igt_assert_eq(write(debug_dir, buf, strlen(buf)), strlen(buf));

For better handling, always prefer to use igt_sysfs_write() or 
igt_written() apis.

> +	close(fd);
> +}
> +
> +static void igt_debug_exit_handler(int sig)
> +{
> +	int fd = drm_open_driver(DRIVER_INTEL | DRIVER_XE);

Why do you need drm fd, as you are directly playing the full path of 
sysfs file?

If drm fd is really required, then why it is restricted to Intel only?

> +
> +	/* Here we assume that only one i915 device will be ever present */
> +	igt_debug_reset(fd);
> +	close(fd);
> +}
> +
> +void igt_set_debug_value(int drm_fd, unsigned int value)

Please check above comments.

- Bhanu

> +{
> +	char buf[16];
> +	int fd = igt_sysfs_open(drm_fd);
> +
> +	if (fd < 0)
> +		return;
> +
> +	const char *path = "/sys/module/drm/parameters/debug";
> +	int debug_dir = openat(fd, path, O_WRONLY);
> +
> +	igt_debug("Setting Debug value to %d\n", value);
> +	snprintf(buf, sizeof(buf), "%d", value);
> +	igt_assert_eq(write(debug_dir, buf, strlen(buf)), strlen(buf));
> +
> +	close(fd);
> +	close(debug_dir);
> +	igt_install_exit_handler(igt_debug_exit_handler);
> +}
> +
>   /**
>    * igt_sysfs_write:
>    * @dir: sysfs directory
> diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
> index 6c604d939..abc9c1739 100644
> --- a/lib/igt_sysfs.h
> +++ b/lib/igt_sysfs.h
> @@ -138,6 +138,9 @@ 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_set_debug_value(int dir, unsigned int value);
> +void igt_debug_reset(int dir);
> +
>   /**
>    * igt_sysfs_rw_attr:
>    * @dir: file descriptor for parent directory