[PATCH v3 1/4] drm/xe/guc: Add LFD related abi definitions
Cavitt, Jonathan
jonathan.cavitt at intel.com
Thu Apr 10 22:29:12 UTC 2025
-----Original Message-----
From: Dong, Zhanjun <zhanjun.dong at intel.com>
Sent: Thursday, April 10, 2025 3:20 PM
To: Cavitt, Jonathan <jonathan.cavitt at intel.com>; intel-xe at lists.freedesktop.org; Wajdeczko, Michal <Michal.Wajdeczko at intel.com>
Subject: Re: [PATCH v3 1/4] drm/xe/guc: Add LFD related abi definitions
>
> Thanks for take time to review.
> Please see my inline comments below.
Yeah, those make sense. My Reviewed-by still stands, though I also still recommend getting
a second opinion on the last point, as I'm not the best authority on that kind of thing.
-Jonathan Cavitt
>
> Regards,
> Zhanjun Dong
>
> On 2025-04-10 1:50 p.m., Cavitt, Jonathan wrote:
> > -----Original Message-----
> > From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Zhanjun Dong
> > Sent: Thursday, April 10, 2025 8:59 AM
> > To: intel-xe at lists.freedesktop.org
> > Cc: Dong, Zhanjun <zhanjun.dong at intel.com>
> > Subject: [PATCH v3 1/4] drm/xe/guc: Add LFD related abi definitions
> >>
> >> Add GuC LFD(Log Format Descriptors) related ABI definition.
> >> Add GuC log init config(LIC) ABI definition.
> >>
> >> Signed-off-by: Zhanjun Dong <zhanjun.dong at intel.com>
> >
> > This probably needs a second opinion, but I'm willing to tentatively
> > provide my RB for this patch. Just don't apply the series quite yet and
> > wait for additional reviews.
> >
> > Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> >
> >> ---
> >> drivers/gpu/drm/xe/abi/guc_log_abi.h | 116 ++++++++++
> >> drivers/gpu/drm/xe/abi/guc_log_lfd_abi.h | 265 +++++++++++++++++++++++
> >> 2 files changed, 381 insertions(+)
> >> create mode 100644 drivers/gpu/drm/xe/abi/guc_log_lfd_abi.h
> >>
> >> diff --git a/drivers/gpu/drm/xe/abi/guc_log_abi.h b/drivers/gpu/drm/xe/abi/guc_log_abi.h
> >> index 554630b7ccd9..0b0c5631da93 100644
> >> --- a/drivers/gpu/drm/xe/abi/guc_log_abi.h
> >> +++ b/drivers/gpu/drm/xe/abi/guc_log_abi.h
> >> @@ -17,6 +17,122 @@ enum guc_log_buffer_type {
> >>
> >> #define GUC_LOG_BUFFER_TYPE_MAX 3
> >>
> >> +#define GUC_LOG_BUFFER_STATE_HEADER_LENGTH 4096
> >> +#define GUC_LOG_BUFFER_STATE_HEADER_ENTRY_LOG 0
> >> +#define GUC_LOG_BUFFER_STATE_HEADER_ENTRY_CRASH 1
> >> +#define GUC_LOG_BUFFER_STATE_HEADER_ENTRY_CAPTURE 2
> >> +#define GUC_LOG_BUFFER_STATE_HEADER_ENTRY_INIT 3
> >> +#define GUC_LOG_BUFFER_STATE_HEADER_ENTRY_COUNT 4
> >> +
> >> +#define GUC_LOG_INIT_CONFIG_LIC_MAGIC 0x8086900D
> >> +#define GUC_LOG_INIT_CONFIG_FORMAT_VERSION_MAJOR 0x0001
> >> +#define GUC_LOG_INIT_CONFIG_FORMAT_VERSION_MINOR 0x0000
> >> +
> >> +/** Log Init Config KLV IDs. */
> >> +enum guc_log_lic_type_t {
> >> + /**
> >> + * GuC firmware version. Value is a 32 bit number represented by
> >> + * guc_sw_version_t.
> >> + */
> >> + GUC_LOG_LIC_TYPE_GUC_SW_VERSION = 0x1,
> >> + /**
> >> + * GuC device id. Value is a 32 bit. Refer BSpec symbol
> >> + * GUC_DEVICEID
> >> + */
> >> + GUC_LOG_LIC_TYPE_GUC_DEVICE_ID = 0x2,
> >> + /**
> >> + * GuC timestamp counter frequency. Value is a 32 bit number
> >> + * representing frequency in kilohertz. This timestamp is utilized
> >> + * in log entries, timer and for engine utilization tracking.
> >> + */
> >> + GUC_LOG_LIC_TYPE_TSC_FREQUENCY = 0x3,
> >> + /**
> >> + * HW GMD ID. Value is a 32 bit number representing graphics,
> >> + * media and display HW architecture IDs.
> >> + */
> >> + GUC_LOG_LIC_TYPE_GMD_ID = 0x4,
> >> + /** GuC build platform ID. Value is 32 bits. */
> >> + GUC_LOG_LIC_TYPE_BUILD_PLATFORM_ID = 0x5,
> >> +};
> >> +
> >> +#define GUC_LOG_LIC_TYPE_FIRST (GUC_LOG_LIC_TYPE_GUC_SW_VERSION)
> >> +#define GUC_LOG_LIC_TYPE_LAST (GUC_LOG_LIC_TYPE_BUILD_PLATFORM_ID + 1)
> >> +
> >> +/** struct guc_klv_generic_t - KLV with multiple DWs in an array */
> >> +struct guc_klv_generic_t {
> >> + /** @length: Length in Dwords of data. */
> >> + u16 length;
> >> + /** @key: Key value */
> >> + u16 key;
> >> + /** @value: Value for this key */
> >> + u32 value[] __counted_by(length);
> >> +} __packed;
> >> +
> >> +/**
> >> + * struct guc_sw_version_t - This structure describes the full version of
> >> + * a software component.
> >> + */
> >> +struct guc_sw_version_t {
> >> + /** @dw0: A 32 bits dword, contains multiple bit fields */
> >> + u32 dw0;
> >> + /* BR[7:0] Patch version */
> >> + #define GUC_SW_VERSION_T_PATCH_VERSION GENMASK(7, 0)
> >> + /* BR[15:8] Minor version */
> >> + #define GUC_SW_VERSION_T_MINOR_VERSION GENMASK(15, 8)
> >> + /* BR[23:16] Major version */
> >> + #define GUC_SW_VERSION_T_MAJOR_VERSION GENMASK(23, 16)
> >> + /* BR[31:24] Branch ID */
> >> + #define GUC_SW_VERSION_T_BRANCH_ID GENMASK(31, 24)
> >
> > Non-blocking:
> > Perhaps we should split the dw0 into the 4 components
> > it's comprised of? Say:
> > """
> > #define GUC_SW_VERSION_T_PATCH_VERSION GENMASK(7, 0)
> > #define GUC_SW_VERSION_T_MINIOR_VERSION GENMASK(15,8)
> > #define GUC_SW_VERSION_T_MAJOR_VERSION GENMASK(23, 16)
> > #define GUC_SW_VERSION_T_BRANCH_ID GENMASK(31, 24)
> > struct guc_sw_version_t {
> > /** @patch_version: Patch version */
> > u8 patch_version;
> > /** @minor_version: Minor version */
> > u8 minor_version;
> > /** @major_version: Major version */
> > u8 major_version;
> > /** @branch_id: branch ID */
> > u8 branch_id;
> > } __packed;
> > """
> > Or is that not possible due to some architectural reason?
> Yes, for big and little endian, the byte order is reversed, that's why
> Mattew suggest to swtich from bit field to bitmask to get it works for
> both endian mode.>
> > Same logic above applies to guc_lic_format_version_t, guc_logfile_lfd_t, and
> > guc_logfile_fmt_ver_t below.>
> >> +} __packed;
> >> +
> >> +/**
> >> + * struct guc_lic_format_version_t - Log Init Config Structure Version.
> >> + * Major-Minor is not a fractional number (i.e. Ver 1.3 would be older
> >> + * than 1.12)
> >> + */
> >> +struct guc_lic_format_version_t {
> >> + /** @dw0: A 32 bits dword, contains multiple bit fields */
> >> + u32 dw0;
> >> + /*
> >> + * BR[15:0] Log-Init-Config structure minor version. Must be
> >> + * GUC_LOG_INIT_CONFIG_FORMAT_VERSION_MINOR
> >> + */
> >> + #define GUC_LIC_FORMAT_VERSION_T_MINOR_VERSION GENMASK(15, 0)
> >> + /*
> >> + * BR[31:16] Log-Init-Config structure major version. Must be
> >> + * GUC_LOG_INIT_CONFIG_FORMAT_VERSION_MAJOR
> >> + */
> >> + #define GUC_LIC_FORMAT_VERSION_T_MAJOR_VERSION GENMASK(31, 16)
> >> +} __packed;
> >> +
> >> +/**
> >> + * struct guc_log_init_config_t - GuC Log-Init-Config structure.
> >> + * This is populated by the GUC at log init time and is located in the log
> >> + * buffer as per the Log Buffer Layout (In Memory). The array of guc log
> >> + * buffer states plus this structure must not exceed 4KB
> >> + */
> >> +struct guc_log_init_config_t {
> >> + /**
> >> + * @lic_magic: A magic number set by GuC to identify that this
> >> + * structure contains valid information: lic_magic = 0x8086900D.
> >> + * Used to verify the information in this structure is valid.
> >> + */
> >> + u32 lic_magic;
> >> + /**
> >> + * @lic_ver: The version of the this structure. Detail description
> >> + * is guc_lic_format_version_t
> >> + */
> >> + struct guc_lic_format_version_t lic_ver;
> >> + /** @lic_dw_size: Number of Dws the `lic_data` array contains. */
> >> + u32 lic_dw_size;
> >> + /**
> >> + * @lic_data: Array of dwords representing a list of LIC KLVs of
> >> + * type guc_klv_generic_t with keys represented by
> >> + * guc_log_lic_type_t
> >> + */
> >> + u32 lic_data[] __counted_by(desc_dw_size);
> >> +} __packed;
> >> +
> >> /**
> >> * struct guc_log_buffer_state - GuC log buffer state
> >> *
> >> diff --git a/drivers/gpu/drm/xe/abi/guc_log_lfd_abi.h b/drivers/gpu/drm/xe/abi/guc_log_lfd_abi.h
> >> new file mode 100644
> >> index 000000000000..b4002f099fd5
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/xe/abi/guc_log_lfd_abi.h
> >> @@ -0,0 +1,265 @@
> >> +/* SPDX-License-Identifier: MIT */
> >> +/*
> >> + * Copyright (c) 2025 Intel Corporation
> >> + */
> >> +
> >> +#ifndef _ABI_GUC_LOG_LFD_ABI_H_
> >> +#define _ABI_GUC_LOG_LFD_ABI_H_
> >> +
> >> +#include <linux/types.h>
> >> +
> >> +#include "abi/guc_log_abi.h"
> >> +
> >> +/* Magic keys define */
> >> +#define LFD_DRIVER_KEY_STREAMING 0x8086AAAA474C5346
> >> +#define LFD_LOG_BUFFER_MARKER_2 0xDEADFEED
> >> +#define LFD_CRASH_DUMP_BUFFER_MARKER_2 0x8086DEAD
> >
> > Non-blocking:
> > These two magic keys should probably be aligned with the other keys below,
> > but I'm willing to concede that they may be out of alignment because of my
> > choice in mail viewer and not because they're also out of alignment in the
> > code itself.
> You are right, it is aligned in code.
>
> >
> >> +#define LFD_STATE_CAPTURE_BUFFER_MARKER_2 0xBEEFFEED
> >> +#define LFD_LOG_BUFFER_MARKER_1V2 0xCABBA9E6
> >> +#define LFD_STATE_CAPTURE_BUFFER_MARKER_1V2 0xCABBA9F7
> >> +
> >> +#define GUC_LOGFILE_LFD_MAGIC upper_16_bits(LFD_CRASH_DUMP_BUFFER_MARKER_2)
> >> +
> >> +/** The current major version of GuC-Log-File format. */
> >> +#define GUC_LOG_FILE_FORMAT_VERSION_MAJOR 0x0001
> >> +/** The current minor version of GuC-Log-File format. */
> >> +#define GUC_LOG_FILE_FORMAT_VERSION_MINOR 0x0000
> >> +
> >> +/** Log format descriptor type */
> >> +enum guc_lfd_type_t {
> >> + /** Start of range for required LFDs from GuC */
> >> + GUC_LFD_TYPE_FW_REQUIRED_RANGE_START = 0x1,
> >> + /**
> >> + * GuC Firmware Version structure. LFDs payload is
> >> + * guc_lfd_data_fw_version_t
> >> + */
> >> + GUC_LFD_TYPE_FW_VERSION = 0x1,
> >
> > Non-blocking:
> > Enum values don't generally repeat themselves. You might
> > want to convert this into a series of defines, instead of trying
> > to fit this into an enum list.
> >
> > Same with guc_lfd_os_type to a lesser extent, as well as all the
> > other enums in this patch.
> This ABI header is generated from GuC interface spec, the spec is
> designed for single platform, so some part might not followes typical
> tradition for Linux.
> Due to cross-platform support, I would leave it as is for easy to find
> macro/enum in code vs spec.>
> >> + /**
> >> + * GuC microcontroller device ID. LFDs payload is
> >> + * guc_lfd_data_guc_devid_t
> >> + */
> >> + GUC_LFD_TYPE_GUC_DEVICE_ID = 0x2,
> >> + /**
> >> + * Frequency of GuC timestamps. LFDs payload is
> >> + * guc_lfd_data_tsc_freq_t
> >> + */
> >> + GUC_LFD_TYPE_TSC_FREQUENCY = 0x3,
> >> + /** HW GMD ID. LFDs payload is guc_lfd_data_gmdid_t */
> >> + GUC_LFD_TYPE_GMD_ID = 0x4,
> >> + /**
> >> + * GuC build platform ID. LFDs payload is
> >> + * guc_lfd_data_build_platformid_t
> >> + */
> >> + GUC_LFD_TYPE_BUILD_PLATFORM_ID = 0x5,
> >> + /** End of this range */
> >> + GUC_LFD_TYPE_FW_REQUIRED_RANGE_END = 0x1FFF,
> >> + /** Start of range for required LFDs from GuC */
> >> + GUC_LFD_TYPE_FW_OPTIONAL_RANGE_START = 0x2000,
> >> + /**
> >> + * Log-event-entries buffer. LFDs payload is
> >> + * guc_lfd_data_log_events_buf_t
> >> + */
> >> + GUC_LFD_TYPE_LOG_EVENTS_BUFFER = 0x2000,
> >> + /**
> >> + * GuC generated crash-dump blob. LFDs payload is
> >> + * guc_lfd_data_fw_crashdump_t
> >> + */
> >> + GUC_LFD_TYPE_FW_CRASH_DUMP = 0x2001,
> >> + /** End of this range */
> >> + GUC_LFD_TYPE_FW_OPTIONAL_RANGE_END = 0x3FFF,
> >> + /** Start of range for required KMD LFDs */
> >> + GUC_LFD_TYPE_KMD_REQUIRED_RANGE_START = 0x4000,
> >> + /**
> >> + * An identifier for the OS. LFDs payload is guc_lfd_data_os_id_t
> >> + */
> >> + GUC_LFD_TYPE_OS_ID = 0x4000,
> >> + /** End of this range */
> >> + GUC_LFD_TYPE_KMD_REQUIRED_RANGE_END = 0x5FFF,
> >> + /** Start of range for optional KMD LFDs */
> >> + GUC_LFD_TYPE_KMD_OPTIONAL_RANGE_START = 0x6000,
> >> + /**
> >> + * Binary representation of GuC log-events schema. LFDs TLV
> >> + * payload is guc_lfd_data_binary_schema_t
> >> + */
> >> + GUC_LFD_TYPE_BINARY_SCHEMA_FORMAT = 0x6000,
> >> + /**
> >> + * ASCII string containing comments from the host/KMD. LFDs TLV
> >> + * payload is guc_lfd_data_host_comment_t
> >> + */
> >> + GUC_LFD_TYPE_HOST_COMMENT = 0x6001,
> >> + /** End of this range */
> >> + GUC_LFD_TYPE_KMD_OPTIONAL_RANGE_END = 0x7FFF,
> >> + /** Start of reserved range */
> >> + GUC_LFD_TYPE_RESERVED_RANGE_START = 0x8000,
> >> + /** End of this range */
> >> + GUC_LFD_TYPE_RESERVED_RANGE_END = 0xFFFF,
> >> +};
> >> +
> >> +/** OS Type LFD-ID */
> >> +enum guc_lfd_os_type_t {
> >> + /** Windows OS */
> >> + GUC_LFD_OS_TYPE_OSID_WIN = 0x1,
> >> + /** Linux OS */
> >> + GUC_LFD_OS_TYPE_OSID_LIN = 0x2,
> >> + /** VMWare OS */
> >> + GUC_LFD_OS_TYPE_OSID_VMW = 0x3,
> >> + /** Other */
> >> + GUC_LFD_OS_TYPE_OSID_OTHER = 0x4,
> >> +};
> >
> > All of the above defines and enums should probably be a part
> > of a separate xe/reg file. Say, xe_guc_log_lfd_desc.h?
> This OS type enum is part of the GuC interface spec.>
> > Though I'd recommend getting a second opinion on this, as I
> > just recall it being a requirement for something I was working
> > on earlier. Though it might be a different case?
> I didn't find similar defines (same os/value pair) from code right now.
> Let me know if you found something.>
> > @Wajdeczko, Michal might be a better authority on this.
> >
> > -Jonathan Cavitt
> >
> >> +
> >> +/**
> >> + * struct guc_logfile_lfd_t - Log format descriptor (LFD).
> >> + * A type of KLV with custom field-sizes + magic numbers.
> >> + */
> >> +struct guc_logfile_lfd_t {
> >> + /** @dw0: A 32 bits dword, contains multiple bit fields */
> >> + u32 dw0;
> >> + /*
> >> + * BR[15:0] Expected value: 0x8086. Helpful in detecting file
> >> + * errors.
> >> + */
> >> + #define GUC_LOGFILE_LFD_T_MAGIC GENMASK(15, 0)
> >> + /*
> >> + * BR[31:16] File descriptor type (the 'T' in TLV) is used to
> >> + * identify how to interpret `data` below. For the range of types,
> >> + * see guc_lfd_type_t
> >> + */
> >> + #define GUC_LOGFILE_LFD_T_DESC_TYPE GENMASK(31, 16)
> >> + /** @desc_dw_size: Number of dwords the `data` field contains. */
> >> + u32 desc_dw_size;
> >> + /** @data: Data defined by File descriptor type. */
> >> + u32 data[] __counted_by(desc_dw_size);
> >> +} __packed;
> >> +
> >> +/**
> >> + * struct guc_logfile_fmt_ver_t - GuC Log File Format Version.
> >> + * Major-Minor is not a fractional number (i.e. Ver 1.3 would be older
> >> + * than 1.12)
> >> + */
> >> +struct guc_logfile_fmt_ver_t {
> >> + /** @dw0: A 32 bits dword, contains multiple bit fields */
> >> + u32 dw0;
> >> + /*
> >> + * BR[15:0] Guc-Log-File Format minor version. Must be
> >> + * GUC_LOG_FILE_FORMAT_VERSION_MINOR
> >> + */
> >> + #define GUC_LOGFILE_FMT_VER_T_MINOR_VERSION GENMASK(15, 0)
> >> + /*
> >> + * BR[31:16] Guc-Log-File Format major version. Must be
> >> + * GUC_LOG_FILE_FORMAT_VERSION_MAJOR
> >> + */
> >> + #define GUC_LOGFILE_FMT_VER_T_MAJOR_VERSION GENMASK(31, 16)
> >> +} __packed;
> >> +
> >> +/**
> >> + * struct guc_lfd_data_guc_devid_t - GuC Device ID.
> >> + * This is mandatory fw LFD data
> >> + */
> >> +struct guc_lfd_data_guc_devid_t {
> >> + /**
> >> + * @guc_devid: GuC microcontroller device ID defined as described
> >> + * in GUC_LOG_LIC_TYPE_GUC_DEVICE_ID
> >> + */
> >> + u32 guc_devid;
> >> +} __packed;
> >> +
> >> +/**
> >> + * struct guc_lfd_data_tsc_freq_t - GuC TSC Fequency.
> >> + * This is mandatory fw LFD data
> >> + */
> >> +struct guc_lfd_data_tsc_freq_t {
> >> + /**
> >> + * @tsc_freq: GuC timestamp counter frequency as described in
> >> + * GUC_LOG_LIC_TYPE_TSC_FREQUENCY
> >> + */
> >> + u32 tsc_freq;
> >> +} __packed;
> >> +
> >> +/** struct guc_lfd_data_gmdid_t - GMD ID. */
> >> +struct guc_lfd_data_gmdid_t {
> >> + /** @gmd_id: GMD ID as described in GUC_LOG_LIC_TYPE_GMD_ID */
> >> + u32 gmd_id;
> >> +} __packed;
> >> +
> >> +/** struct guc_lfd_data_build_platformid_t - GuC build platform ID. */
> >> +struct guc_lfd_data_build_platformid_t {
> >> + /**
> >> + * @platform_build_id: GuC build platform ID as described in
> >> + * GUC_LOG_LIC_TYPE_BUILD_PLATFORM_ID
> >> + */
> >> + u32 platform_build_id;
> >> +} __packed;
> >> +
> >> +/**
> >> + * struct guc_lfd_data_log_events_buf_t - GuC Log Events Buffer.
> >> + * This is optional fw LFD data
> >> + */
> >> +struct guc_lfd_data_log_events_buf_t {
> >> + /**
> >> + * @log_events_format_version: version of GuC log format of buffer
> >> + */
> >> + u32 log_events_format_version;
> >> + /**
> >> + * @log_format_buf: If `log_format_version` == 1, array of
> >> + * guc_log_format_version_1_t. If `log_format_version` == 2, array
> >> + * of guc_log_format_version_2_t. Dword size determined by
> >> + * guc_logfile_lfd_t.`desc_dw_size` - 1
> >> + */
> >> + u32 log_format_buf[];
> >> +} __packed;
> >> +
> >> +/**
> >> + * struct guc_lfd_data_os_id_t - OS Version Information.
> >> + * This is mandatory host LFD data
> >> + */
> >> +struct guc_lfd_data_os_id_t {
> >> + /**
> >> + * @os_id: enum values to identify the OS brand (1=Windows,
> >> + * 2=Linux, etc..). See guc_lfd_os_type_t for the range of types
> >> + */
> >> + u32 os_id;
> >> + /**
> >> + * @build_version: ASCII string containing OS build version
> >> + * information based on os_id. String is padded with null
> >> + * characters to ensure its DWORD aligned. Dword size determined
> >> + * by guc_logfile_lfd_t.`desc_dw_size` - 1
> >> + */
> >> + char build_version[];
> >> +} __packed;
> >> +
> >> +/**
> >> + * struct guc_logfile_t - GuC Log Streaming-LFD-File Format.
> >> + * This structure encapsulates the layout of the guc-log-file format
> >> + */
> >> +struct guc_logfile_t {
> >> + /**
> >> + * @magic: A magic number set by producer of a GuC log file to
> >> + * identify that file is a valid guc-log-file containing a stream
> >> + * of LFDs: Expected value: 0x8086AAAA474C5346.
> >> + */
> >> + u64 magic;
> >> + /**
> >> + * @version: Version of this file format layout as per
> >> + * guc_logfile_fmt_ver_t
> >> + */
> >> + struct guc_logfile_fmt_ver_t version;
> >> + /**
> >> + * @lfd_stream: A stream of one or more guc_logfile_lfd_t LFD data
> >> + */
> >> + struct guc_logfile_lfd_t lfd_stream;
> >> +} __packed;
> >> +
> >> +/**
> >> + * struct guc_lfd_data_fw_version_t - GuC FW Version.
> >> + * This is mandatory fw LFD data
> >> + */
> >> +struct guc_lfd_data_fw_version_t {
> >> + /**
> >> + * @guc_sw_version: The full version of the GuC microkernel that
> >> + * generated the logs as described in
> >> + * GUC_LOG_LIC_TYPE_GUC_SW_VERSION.
> >> + */
> >> + struct guc_sw_version_t guc_sw_version;
> >> +} __packed;
> >> +
> >> +#endif
> >> --
> >> 2.34.1
> >>
> >>
>
>
More information about the Intel-xe
mailing list