Re: [PATCH 2/2] tools/intel_error_decode: Enable support for xe driver in the tool

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Jan 31, 2025 at 08:29:40PM +0000, sai.gowtham.ch@intel.com wrote:
> From: Sai Gowtham Ch <sai.gowtham.ch@intel.com>
> 
> Enables error decode support for xe dumps. which uses intel_error_decode_xe
> lib.
> 
> Signed-off-by: Sai Gowtham Ch <sai.gowtham.ch@intel.com>
> ---
>  tools/intel_error_decode.c | 156 +++++++++++++++++++++++++------------
>  1 file changed, 107 insertions(+), 49 deletions(-)
> 
> diff --git a/tools/intel_error_decode.c b/tools/intel_error_decode.c
> index 451608826..a722621e2 100644
> --- a/tools/intel_error_decode.c
> +++ b/tools/intel_error_decode.c
> @@ -57,7 +57,11 @@
>  #include "instdone.h"
>  #include "intel_reg.h"
>  #include "drmtest.h"
> +#include "drm-uapi/xe_drm.h"
>  #include "i915/intel_decode.h"

From this one we can see that the right place for the lib is under:
igt/lib/xe/

> +#include "xe/intel_error_decode_xe_lib.h"

also, what about
devcoredump_decode.h ?

> +
> +#define XE_KMD_ERROR_DUMP_IDENTIFIER "**** Xe Device Coredump ****"

what about having this in the lib?

>  
>  static uint32_t
>  print_head(unsigned int reg)
> @@ -793,14 +797,86 @@ static void setup_pager(void)
>  	}
>  }
>  
> +static FILE *
> +try_open_file(const char *format, ...)
> +{
> +	int ret;
> +	char *filename;
> +	FILE *file;
> +	va_list args;
> +
> +	va_start(args, format);
> +	ret = vasprintf(&filename, format, args);
> +	va_end(args);
> +
> +	igt_assert(ret > 0);
> +	file = fopen(filename, "r");
> +	free(filename);
> +
> +	return file;
> +}
> +
> +static FILE *
> +open_i915_core_dump(const char *path)
> +{
> +	FILE *file = NULL;
> +	struct stat st;
> +
> +	if (stat(path, &st))
> +		return NULL;
> +
> +	if (S_ISDIR(st.st_mode)) {
> +		file = try_open_file("%s/i915_error_state", path);
> +		if (!file) {
> +			int minor;
> +			for (minor = 0; minor < 64; minor++) {
> +				file = try_open_file("%s/%d/i915_error_state", path, minor);
> +				if (file)
> +					break;
> +			}
> +		}
> +	} else {
> +		file = fopen(path, "r");
> +	}
> +
> +	return file;
> +}
> +
> +static FILE *
> +open_xe_core_dump(const char *path)
> +{
> +	FILE *file = NULL;
> +
> +	if (path) {
> +		struct stat st;
> +
> +		if (stat(path, &st))
> +			return NULL;
> +
> +		if (S_ISDIR(st.st_mode)) {
> +			file = try_open_file("%s/data", path);
> +		} else {
> +			file = fopen(path, "r");
> +		}
> +	} else {
> +		for (int minor = 0; minor < 64; minor++) {
> +			file = try_open_file("/sys/class/drm/card%i/device/devcoredump/data", minor);
> +			if (file)
> +				break;
> +		}
> +	}
> +
> +	return file;
> +}
> +
>  int
>  main(int argc, char *argv[])
>  {
>  	FILE *file;
>  	const char *path;
>  	char *filename = NULL;
> -	struct stat st;
> -	int error;
> +	char *line = NULL;
> +	size_t line_size;
>  
>  	if (argc > 2) {
>  		fprintf(stderr,
> @@ -823,69 +899,51 @@ main(int argc, char *argv[])
>  
>  	if (argc == 1) {
>  		if (isatty(0)) {
> -			path = "/sys/class/drm/card0/error";
> -			error = stat(path, &st);
> -			if (error != 0) {
> -				path = "/debug/dri";
> -				error = stat(path, &st);
> +			file = open_i915_core_dump("/sys/class/drm/card0/error");
> +			if (!file) {
> +				file  = open_i915_core_dump("/debug/dri");
>  			}
> -			if (error != 0) {
> -				path = "/sys/kernel/debug/dri";
> -				error = stat(path, &st);
> +			if (!file) {
> +				file = open_i915_core_dump("/sys/kernel/debug/dri");

This i915 vs xe detection looks complex.
We should keep these i915 possibilities inside the i915 function
and here have a simple is i915 call i915 function, elif xe call xe function else error below...

>  			}
> -			if (error != 0) {
> +			if (!file) {
> +				file = open_xe_core_dump(NULL);
> +			}
> +			if (file == NULL) {
>  				errx(1,
> -				     "Couldn't find i915 debugfs directory.\n\n"
> +				     "Couldn't find i915 or xe debugfs directory.\n\n"
>  				     "Is debugfs mounted? You might try mounting it with a command such as:\n\n"
>  				     "\tsudo mount -t debugfs debugfs /sys/kernel/debug\n");
>  			}
>  		} else {
> -			read_data_file(stdin);
> -			exit(0);
> +			file = stdin;
>  		}
>  	} else {
>  		path = argv[1];
> -		error = stat(path, &st);
> -		if (error != 0) {
> -			fprintf(stderr, "Error opening %s: %s\n",
> -					path, strerror(errno));
> -			exit(1);
> -		}
> -	}
> +		if (strcmp(path, "-") == 0) {
> +				file = stdin;
> +		} else {
> +			FILE *i915, *xe;
>  
> -	if (S_ISDIR(st.st_mode)) {
> -		int ret;
> +			i915 = open_i915_core_dump(path);
> +			xe = open_xe_core_dump(path);
>  
> -		ret = asprintf(&filename, "%s/i915_error_state", path);
> -		assert(ret > 0);
> -		file = fopen(filename, "r");
> -		if (!file) {
> -			int minor;
> -			for (minor = 0; minor < 64; minor++) {
> -				free(filename);
> -				ret = asprintf(&filename, "%s/%d/i915_error_state", path, minor);
> -				assert(ret > 0);
> -
> -				file = fopen(filename, "r");
> -				if (file)
> -					break;
> +			if (i915 == NULL && xe == NULL) {
> +				fprintf(stderr, "Error opening %s: %s\n", path, strerror(errno));
> +				exit(1);
>  			}
> -		}
> -		if (!file) {
> -			fprintf(stderr, "Failed to find i915_error_state beneath %s\n",
> -					path);
> -			exit (1);
> -		}
> -	} else {
> -		file = fopen(path, "r");
> -		if (!file) {
> -			fprintf(stderr, "Failed to open %s: %s\n",
> -					path, strerror(errno));
> -			exit (1);
> +
> +			file = i915 ? i915 : xe;
>  		}
>  	}
>  
> -	read_data_file(file);
> +	getline(&line, &line_size, file);
> +	rewind(file);
> +	if (strncmp(line, XE_KMD_ERROR_DUMP_IDENTIFIER, strlen(XE_KMD_ERROR_DUMP_IDENTIFIER)) == 0)
> +		read_xe_data_file(file);

xe_read_data_file() is a better name for this function

> +	else
> +		read_data_file(file);

and probably good to rename this to

i915_read_data_file()

in other words, add i915_ prefix to all current functions exported in lib/i915/intel_decode.h
and even rename the file to i915_error_decode.h and in a separate patch...

> +	free(line);
>  	fclose(file);
>  
>  	if (filename != path)
> -- 
> 2.34.1
>