[PATCH 2/2] tools/intel_error_decode: Enable support for xe driver in the tool
Ch, Sai Gowtham
sai.gowtham.ch at intel.com
Tue Feb 11 05:37:19 UTC 2025
>-----Original Message-----
>From: Vivi, Rodrigo <rodrigo.vivi at intel.com>
>Sent: Thursday, February 6, 2025 9:55 PM
>To: Ch, Sai Gowtham <sai.gowtham.ch at intel.com>
>Cc: igt-dev at lists.freedesktop.org
>Subject: Re: [PATCH 2/2] tools/intel_error_decode: Enable support for xe driver in
>the tool
>
>On Fri, Jan 31, 2025 at 08:29:40PM +0000, sai.gowtham.ch at intel.com wrote:
>> From: Sai Gowtham Ch <sai.gowtham.ch at 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 at 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...
>
Thanks for your comments Rodrigo, will keep these comments in mind and make changes according.
Thanks,
Gowtham
>> + free(line);
>> fclose(file);
>>
>> if (filename != path)
>> --
>> 2.34.1
>>
More information about the igt-dev
mailing list