Re: [PATCH i-g-t 1/2] lib/igt_sysfs: Update DRM debug severity handling to use bitmask for verbosity control

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

On Mon, 10 Mar 2025, Pranay Samala <pranay.samala@intel.com> wrote:
> This approach ensures precise control by modifying
> specific bits in the DRM debug mask.
>
> Updated the DRM debug severity logic to use bitwise
> operations to unset unwanted bits in the debug mask.
> The function now only sets the bits that are passed,
> allowing for more granular control over the logging output.

Please don't call it "severity" when drm debug is about "category".

You can't say one category is more "severe" than another.

>
> Cc: Leo Li <sunpeng.li@amd.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Kunal Joshi <kunal1.joshi@intel.com>
> Cc: Karthik B S <karthik.b.s@intel.com>
> Cc: Ramanaidu Naladala <ramanaidu.naladala@intel.com>
> Cc: Sameer Lattannavar <sameer.lattannavar@intel.com>
>
> Fixes: 56b91193b825 ("lib/igt_sysfs: Implement dynamic adjustment of debug log level")
> Signed-off-by: Pranay Samala <pranay.samala@intel.com>
> ---
>  lib/igt_sysfs.c | 54 ++++++++++++++++++++++++++-----------------------
>  lib/igt_sysfs.h | 19 ++++++++++++++---
>  2 files changed, 45 insertions(+), 28 deletions(-)
>
> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> index 2e4c2ee63..2a3cb68e4 100644
> --- a/lib/igt_sysfs.c
> +++ b/lib/igt_sysfs.c
> @@ -443,69 +443,71 @@ int igt_sysfs_drm_module_params_open(void)
>  	return open(path, O_RDONLY);
>  }
>  
> -static int log_level = -1;
> +static int log_severity = -1;
>  
>  /**
> - * igt_drm_debug_level_get:
> + * igt_drm_debug_severity_get:
>   *
> - * This reads the current debug log level of the machine on
> + * This reads the current debug log severity of the machine on
>   * which the test is currently executing.
>   *
>   * Returns:
> - * The current log level, or -1 on error.
> + * The current log severity, or -1 on error.
>   */
> -int igt_drm_debug_level_get(int dir)
> +int igt_drm_debug_severity_get(int dir)
>  {
>  	char buf[20];
>  
> -	if (log_level >= 0)
> -		return log_level;
> +	if (log_severity >= 0)
> +		return log_severity;
>  
> -	if (igt_sysfs_read(dir, "debug", buf, sizeof(buf) - 1) < 0)
> +	if (igt_sysfs_read(dir, "debug", buf, sizeof(buf)) < 0)
>  		return -1;
>  
>  	return atoi(buf);
>  }
>  
>  /**
> - * igt_drm_debug_level_reset:
> + * igt_drm_debug_severity_reset:
>   *
> - * This modifies the current debug log level of the machine
> + * This modifies the current debug log severity of the machine
>   * to the default value post-test.
>   *
>   */
> -void igt_drm_debug_level_reset(void)
> +void igt_drm_debug_severity_reset(void)
>  {
>  	char buf[20];
>  	int dir;
>  
> -	if (log_level < 0)
> +	if (log_severity < 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_debug("Resetting DRM debug severity to 0x%x\n", log_severity);
> +	snprintf(buf, sizeof(buf), "%d", log_severity);
>  	igt_assert(igt_sysfs_set(dir, "debug", buf));
>  
>  	close(dir);
>  }
>  
> -static void igt_drm_debug_level_reset_exit_handler(int sig)
> +static void igt_drm_debug_severity_reset_exit_handler(int sig)
>  {
> -	igt_drm_debug_level_reset();
> +	igt_drm_debug_severity_reset();
>  }
>  
>  /**
> - * igt_drm_debug_level_update:
> - * @debug_level: new debug level to set
> + * igt_drm_debug_severity_update:
> + * @debug_severity: new debug severity to set
>   *
> - * This modifies the current drm debug log level to the new value.
> + * This modifies the current drm debug log severity to the new value.
>   */
> -void igt_drm_debug_level_update(unsigned int new_log_level)
> +
> +void igt_drm_debug_severity_update(enum drm_debug_category category_to_set)

This should accept a mask instead of a single category, and the function
and parameter naming should reflect that.

>  {
> +	unsigned int new_log_severity;
>  	char buf[20];
>  	int dir;
>  
> @@ -513,14 +515,16 @@ void igt_drm_debug_level_update(unsigned int new_log_level)
>  	if (dir < 0)
>  		return;
>  
> -	log_level = igt_drm_debug_level_get(dir);
> -	if (log_level < 0) {
> +	log_severity = igt_drm_debug_severity_get(dir);
> +	if (log_severity < 0) {
>  		close(dir);
>  		return;
>  	}

This doesn't seem to handle nested updates. The reset will put this back
to the previous value, not the original value.

>  
> -	igt_debug("Setting DRM debug level to %d\n", new_log_level);
> -	snprintf(buf, sizeof(buf), "%d", new_log_level);
> +	new_log_severity = category_to_set;
> +
> +	igt_debug("Setting DRM debug severity to 0x%x\n", new_log_severity);
> +	snprintf(buf, sizeof(buf), "%d", new_log_severity);
>  	igt_assert(igt_sysfs_set(dir, "debug", buf));
>  
>  	close(dir);
> @@ -529,7 +533,7 @@ void igt_drm_debug_level_update(unsigned int new_log_level)
>  	 * TODO: Check whether multiple exit handlers will get installed,
>  	 * if we call this api multiple times
>  	 */
> -	igt_install_exit_handler(igt_drm_debug_level_reset_exit_handler);
> +	igt_install_exit_handler(igt_drm_debug_severity_reset_exit_handler);
>  }
>  
>  /**
> diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
> index 86345f3d1..c2e15320d 100644
> --- a/lib/igt_sysfs.h
> +++ b/lib/igt_sysfs.h
> @@ -143,9 +143,22 @@ 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 new_log_level);
> -void igt_drm_debug_level_reset(void);
> -int igt_drm_debug_level_get(int dir);
> +enum drm_debug_category {
> +	DRM_UT_CORE      = 1 << 0,
> +	DRM_UT_DRIVER    = 1 << 1,
> +	DRM_UT_KMS       = 1 << 2,
> +	DRM_UT_PRIME     = 1 << 3,
> +	DRM_UT_ATOMIC    = 1 << 4,
> +	DRM_UT_VBL       = 1 << 5,
> +	DRM_UT_STATE     = 1 << 6,
> +	DRM_UT_LEASE     = 1 << 7,
> +	DRM_UT_DP        = 1 << 8,
> +	DRM_UT_DRMRES    = 1 << 9,
> +};
> +
> +void igt_drm_debug_severity_update(enum drm_debug_category category_to_set);
> +void igt_drm_debug_severity_reset(void);
> +int igt_drm_debug_severity_get(int dir);
>  int igt_sysfs_drm_module_params_open(void);
>  
>  /**

-- 
Jani Nikula, Intel