[PATCH v6 2/6] drm/xe/guc: Add LFD related abi definitions

[John Harrison]

On 7/21/2025 6:35 PM, Zhanjun Dong wrote:
> Add GuC LFD (Log Format Descriptors) related ABI definitions.
>
> Signed-off-by: Zhanjun Dong <zhanjun.dong at intel.com>
> ---
>   drivers/gpu/drm/xe/abi/guc_lfd_abi.h | 282 +++++++++++++++++++++++++++
>   1 file changed, 282 insertions(+)
>   create mode 100644 drivers/gpu/drm/xe/abi/guc_lfd_abi.h
>
> diff --git a/drivers/gpu/drm/xe/abi/guc_lfd_abi.h b/drivers/gpu/drm/xe/abi/guc_lfd_abi.h
> new file mode 100644
> index 000000000000..b6888ab47e25
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/abi/guc_lfd_abi.h
> @@ -0,0 +1,282 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2025 Intel Corporation
> + */
> +
> +#ifndef _ABI_GUC_LFD_ABI_H_
> +#define _ABI_GUC_LFD_ABI_H_
> +
> +#include <linux/types.h>
> +
> +#include "guc_lic_abi.h"
> +
> +/* Magic keys define */
> +#define GUC_LFD_DRIVER_KEY_STREAMING		0x8086AAAA474C5346
> +#define GUC_LFD_LOG_BUFFER_MARKER_2		0xDEADFEED
> +#define GUC_LFD_CRASH_DUMP_BUFFER_MARKER_2	0x8086DEAD
> +#define GUC_LFD_STATE_CAPTURE_BUFFER_MARKER_2	0xBEEFFEED
> +#define GUC_LFD_LOG_BUFFER_MARKER_1V2		0xCABBA9E6
> +#define GUC_LFD_STATE_CAPTURE_BUFFER_MARKER_1V2	0xCABBA9F7
> +#define GUC_LFD_DATA_HEADER_MAGIC		0x8086
These are not LFD defines. They are the in-memory buffer header magic words.

> +
> +/* The current major version of GuC-Log-File format. */
> +#define GUC_LFD_FORMAT_VERSION_MAJOR		0x0001
> +/* The current minor version of GuC-Log-File format. */
> +#define GUC_LFD_FORMAT_VERSION_MINOR		0x0000
> +
> +/** enum guc_lfd_type - Log format descriptor type */
> +enum guc_lfd_type {
> +	/**
> +	 * @GUC_LFD_TYPE_FW_REQUIRED_RANGE_START: Start of range for
> +	 * required LFDs from GuC
> +	 */
> +	GUC_LFD_TYPE_FW_REQUIRED_RANGE_START = 0x1,
> +	/**
> +	 * @GUC_LFD_TYPE_FW_VERSION: GuC Firmware Version structure. LFDs
> +	 * payload is guc_lfd_data_fw_version
> +	 */
> +	GUC_LFD_TYPE_FW_VERSION = GUC_LIC_TYPE_GUC_SW_VERSION,
This equality is coincidental not a definition. Indeed, the original 
spec had different numbering for the two enums. It only change to match 
due to an implementation bug! So you should not define them as being 
related and should not make any assumptions in the code about them being 
related.


> +	/**
> +	 * @GUC_LFD_TYPE_GUC_DEVICE_ID: GuC microcontroller device ID.
> +	 * LFDs payload is guc_lfd_data_guc_devid
> +	 */
> +	GUC_LFD_TYPE_GUC_DEVICE_ID = GUC_LIC_TYPE_GUC_DEVICE_ID,
> +	/**
> +	 * @GUC_LFD_TYPE_TSC_FREQUENCY: Frequency of GuC timestamps. LFDs
> +	 * payload is guc_lfd_data_tsc_freq
> +	 */
> +	GUC_LFD_TYPE_TSC_FREQUENCY = GUC_LIC_TYPE_TSC_FREQUENCY,
> +	/**
> +	 * @GUC_LFD_TYPE_GMD_ID: HW GMD ID. LFDs payload is
> +	 * guc_lfd_data_gmdid
> +	 */
> +	GUC_LFD_TYPE_GMD_ID = GUC_LIC_TYPE_GMD_ID,
> +	/**
> +	 * @GUC_LFD_TYPE_BUILD_PLATFORM_ID: GuC build platform ID. LFDs
> +	 * payload is guc_lfd_data_build_platformid
> +	 */
> +	GUC_LFD_TYPE_BUILD_PLATFORM_ID = GUC_LIC_TYPE_BUILD_PLATFORM_ID,
> +	/** @GUC_LFD_TYPE_FW_REQUIRED_RANGE_END: End of this range */
> +	GUC_LFD_TYPE_FW_REQUIRED_RANGE_END = 0x1FFF,
> +	/**
> +	 * @GUC_LFD_TYPE_FW_OPTIONAL_RANGE_START: Start of range for
> +	 * required LFDs from GuC
> +	 */
> +	GUC_LFD_TYPE_FW_OPTIONAL_RANGE_START = 0x2000,
> +	/**
> +	 * @GUC_LFD_TYPE_LOG_EVENTS_BUFFER: Log-event-entries buffer. LFDs
> +	 * payload is guc_lfd_data_log_events_buf
> +	 */
> +	GUC_LFD_TYPE_LOG_EVENTS_BUFFER = 0x2000,
> +	/**
> +	 * @GUC_LFD_TYPE_FW_CRASH_DUMP: GuC generated crash-dump blob.
> +	 * LFDs payload is guc_lfd_data_fw_crashdump
> +	 */
> +	GUC_LFD_TYPE_FW_CRASH_DUMP = 0x2001,
> +	/** @GUC_LFD_TYPE_FW_OPTIONAL_RANGE_END: End of this range */
> +	GUC_LFD_TYPE_FW_OPTIONAL_RANGE_END = 0x3FFF,
> +	/**
> +	 * @GUC_LFD_TYPE_KMD_REQUIRED_RANGE_START: Start of range for
> +	 * required KMD LFDs
> +	 */
> +	GUC_LFD_TYPE_KMD_REQUIRED_RANGE_START = 0x4000,
> +	/**
> +	 * @GUC_LFD_TYPE_OS_ID: An identifier for the OS. LFDs payload is
> +	 * guc_lfd_data_os_id
> +	 */
> +	GUC_LFD_TYPE_OS_ID = 0x4000,
> +	/** @GUC_LFD_TYPE_KMD_REQUIRED_RANGE_END: End of this range */
> +	GUC_LFD_TYPE_KMD_REQUIRED_RANGE_END = 0x5FFF,
> +	/**
> +	 * @GUC_LFD_TYPE_KMD_OPTIONAL_RANGE_START: Start of range for
> +	 * optional KMD LFDs
> +	 */
> +	GUC_LFD_TYPE_KMD_OPTIONAL_RANGE_START = 0x6000,
> +	/**
> +	 * @GUC_LFD_TYPE_BINARY_SCHEMA_FORMAT: Binary representation of
> +	 * GuC log-events schema. LFDs TLV payload is
> +	 * guc_lfd_data_binary_schema
> +	 */
> +	GUC_LFD_TYPE_BINARY_SCHEMA_FORMAT = 0x6000,
> +	/**
> +	 * @GUC_LFD_TYPE_HOST_COMMENT: ASCII string containing comments
> +	 * from the host/KMD. LFDs TLV payload is
> +	 * guc_lfd_data_host_comment
> +	 */
> +	GUC_LFD_TYPE_HOST_COMMENT = 0x6001,
> +	/** @GUC_LFD_TYPE_KMD_OPTIONAL_RANGE_END: End of this range */
> +	GUC_LFD_TYPE_KMD_OPTIONAL_RANGE_END = 0x7FFF,
> +	/** @GUC_LFD_TYPE_RESERVED_RANGE_START: Start of reserved range */
> +	GUC_LFD_TYPE_RESERVED_RANGE_START = 0x8000,
> +	/** @GUC_LFD_TYPE_RESERVED_RANGE_END: End of this range */
> +	GUC_LFD_TYPE_RESERVED_RANGE_END = 0xFFFF,
> +};
Personally, I find the above impossible to read at a glance. The 
comments add nothing - GUC_LFD_TYPE_KMD_REQUIRED_RANGE_START is 
described as "Start of range for required KMD LFDs". Well, yeah, I can 
get that from the name. I think this whole section would be much easier 
to read if was just the enums:

+	GUC_LFD_TYPE_KMD_REQUIRED_RANGE_START = 0x4000,
+	GUC_LFD_TYPE_OS_ID = 0x4000,
+	GUC_LFD_TYPE_KMD_REQUIRED_RANGE_END = 0x5FFF,

The structure definitions and such for the individual entries can refer 
back to the define they are associated with. But this section would be 
more use if it is a simple list of enum values that is quick and easy to 
scan through.

> +
> +/** enum guc_lfd_os_type - OS Type LFD-ID */
> +enum guc_lfd_os_type {
> +	/** @GUC_LFD_OS_TYPE_OSID_WIN: Windows OS */
> +	GUC_LFD_OS_TYPE_OSID_WIN = 0x1,
> +	/** @GUC_LFD_OS_TYPE_OSID_LIN: Linux OS */
> +	GUC_LFD_OS_TYPE_OSID_LIN = 0x2,
> +	/** @GUC_LFD_OS_TYPE_OSID_VMW: VMWare OS */
> +	GUC_LFD_OS_TYPE_OSID_VMW = 0x3,
> +	/** @GUC_LFD_OS_TYPE_OSID_OTHER: Other */
> +	GUC_LFD_OS_TYPE_OSID_OTHER = 0x4,
> +};
> +
> +/**
> + * struct guc_lfd_data - Log format descriptor (LFD).
> + * A type of KLV with custom field-sizes + magic numbers.
This description tells me nothing that I can't immediately see from 
looking at the contents. Maybe just say "A generic header structure for 
all LFD blocks".

> + */
> +struct guc_lfd_data {
> +	/** @dw0: A 32 bits dword, contains multiple bit fields */
> +	u32 dw0;
'header' would be better than 'dw0'.

> +	/*
> +	 * Expected value: GUC_LFD_DATA_HEADER_MAGIC.
> +	 * Helpful in detecting file errors.
Why 'expected value'? Indeed, I don't think this comment is worth having 
at all.
> +	 */
> +#define GUC_LFD_DATA_MAGIC		GENMASK(15, 0)
Calling this 'DATA_MAGIC' implies it is related to the 'data' field 
below. Assuming that 'dw0' is renamed to 'header' then this shld be 
'GUC_LFD_HEADER_MAGIC'.

> +	/*
> +	 * 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
Again, this is a redundant comment. Also, the KMD uses KLV not TLV.

> +	 */
> +#define GUC_LFD_DATA_DESC_TYPE		GENMASK(31, 16)
Again, GUC_LFD_HEADER_TYPE would be a better name.

> +	/** @dw_size: Number of dwords the `data` field contains. */
> +	u32 dw_size;
data_size

> +	/** @data: Data defined by File descriptor type. */
Defined by GUC_LFD_HEADER_TYPE.

> +	u32 data[] __counted_by(dw_size);
> +} __packed;
> +
> +/**
> + * struct guc_lfd_version - GuC Log File Format Version.
LFD is Log File Descriptor not Log File Format.

> + * Major-Minor is not a fractional number (i.e. Ver 1.3 would be older
> + * than 1.12)
> + */
> +struct guc_lfd_version {
> +	/** @dw0: A 32 bits dword, contains multiple bit fields */
> +	u32 dw0;
> +	/*
> +	 * Guc-Log-File Format minor version. Must be
> +	 * GUC_LOG_FILE_FORMAT_VERSION_MINOR
> +	 */
> +#define GUC_LFD_VERSION_MINOR	GENMASK(15, 0)
> +	/*
> +	 * Guc-Log-File Format major version. Must be
> +	 * GUC_LOG_FILE_FORMAT_VERSION_MAJOR
> +	 */
> +#define GUC_LFD_VERSION_MAJOR	GENMASK(31, 16)
> +} __packed;
Again, no point in having a structure defined for a single u32. Just the 
bit field definitions is all we need.

> +
> +/**
> + * struct guc_lfd_data_guc_devid - GuC Device ID.
> + * This is mandatory fw LFD data
"fw" meaning firmware? But this is the LFD as created entirely by the 
KMD, the firmware is not involved. Should just say "This is a mandatory 
LFD block."

> + */
> +struct guc_lfd_data_guc_devid {
> +	/**
> +	 * @guc_devid: GuC microcontroller device ID defined as described
> +	 * in GUC_LIC_TYPE_GUC_DEVICE_ID
> +	 */
> +	u32 guc_devid;
> +} __packed;
> +
> +/**
> + * struct guc_lfd_data_tsc_freq - GuC TSC Fequency.
> + * This is mandatory fw LFD data
> + */
> +struct guc_lfd_data_tsc_freq {
> +	/**
> +	 * @tsc_freq: GuC timestamp counter frequency as described in
> +	 * GUC_LIC_TYPE_TSC_FREQUENCY
> +	 */
> +	u32 tsc_freq;
> +} __packed;
> +
> +/** struct guc_lfd_data_gmdid - GMD ID. */
> +struct guc_lfd_data_gmdid {
> +	/** @gmd_id: GMD ID as described in GUC_LIC_TYPE_GMD_ID */
> +	u32 gmd_id;
> +} __packed;
> +
> +/** struct guc_lfd_data_build_platformid - GuC build platform ID. */
> +struct guc_lfd_data_build_platformid {
> +	/**
> +	 * @platform_build_id: GuC build platform ID as described in
> +	 * GUC_LIC_TYPE_BUILD_PLATFORM_ID
> +	 */
> +	u32 platform_build_id;
> +} __packed;
Again, not sure these four are worth having structure definitions for. A 
comment that says LFD_DEVID maps to LIC_DEVID, etc. would be sufficient.

> +
> +/**
> + * struct guc_lfd_data_log_events_buf - GuC Log Events Buffer.
> + * This is optional fw LFD data
As above, this is not a firmware generated data stream. Should just say 
"This is an optional LFD block.".

> + */
> +struct guc_lfd_data_log_events_buf {
> +	/**
> +	 * @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. If `log_format_version` == 2, array
> +	 * of guc_log_format_version_2. Dword size determined by
> +	 * guc_logfile_lfd.`desc_dw_size` - 1
This does not match the name you used in the parent LFD header 
structure. Should just say "Size in dwords is LFD block size - 1".

> +	 */
> +	u32 log_format_buf[];
> +} __packed;
> +
> +/**
> + * struct guc_lfd_data_os_info - OS Version Information.
> + * This is mandatory host LFD data
As above, this is all being created by the host so no need to specify.

> + */
> +struct guc_lfd_data_os_info {
> +	/**
> +	 * @os_id: enum values to identify the OS brand (1=Windows,
> +	 * 2=Linux, etc..). See guc_lfd_os_type for the range of types
Should not repeat a subset of the enum definition values here. Just 
reference the enum name.
> +	 */
> +	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.`desc_dw_size` - 1
As above about naming.

> +	 */
> +	char build_version[];
> +} __packed;
> +
> +/**
> + * struct guc_lfd_data_fw_version - GuC FW Version.
> + * This is mandatory fw LFD data
> + */
> +struct guc_lfd_data_fw_version {
> +	/**
> +	 * @guc_sw_version: The full version of the GuC microkernel that
> +	 * generated the logs as described in
> +	 * GUC_LIC_TYPE_GUC_SW_VERSION.
Except that the log data block is optional and so not necessarily 
present. Should just say "The full version of the GuC firmware.".

> +	 */
> +	struct guc_sw_version guc_sw_version;
> +} __packed;
Although, again this is a single u32 structure, which seems pointless. A 
comment description would be sufficient.

> +
> +/**
> + * struct guc_logfile_header - Header of GuC Log Streaming-LFD-File Format.
> + * This structure encapsulates the layout of the guc-log-file format
> + */
> +struct guc_lfd_file_header {
> +	/**
> +	 * @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: GUC_LFD_DRIVER_KEY_STREAMING
> +	 */
> +	u64 magic;
> +	/**
> +	 * @version: Version of this file format layout as per
> +	 * guc_lfd_version
> +	 */
> +	struct guc_lfd_version version;
> +	/**
> +	 * @lfd_stream: A stream of one or more guc_lfd_data LFD data
"LFD blocks" is a better way to describe them than "LFD data". Data can 
mean pretty much anything, whereas block implies a discrete unit.

> +	 */
> +	struct guc_lfd_data lfd_stream;
Technically, this is just a single LFD block, not a stream. And you 
can't really do 'lfd_stream[]' because blocks are variable in size. 
Would be better to just say "u32 lfd_stream[];".

John.


> +} __packed;
> +
> +#endif