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

[Dong, Zhanjun]

On 2025-07-29 3:08 p.m., John Harrison wrote:
> 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.
Sure, to be moved out from abi header.

> 
>> +
>> +/* 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.
Got it, to be changed to numbers.

> 
> 
>> +    /**
>> +     * @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.
Good idea, comments for each items make it hard to read here.
We might still want to have kernel docs, so maybe I can change each 
range like:

/**
  *  @GUC_LFD_TYPE_KMD_REQUIRED_RANGE_START : Start of range for
  *  required KMD LFDs
  *  @GUC_LFD_TYPE_OS_ID: An identifier for the OS
  *  @GUC_LFD_TYPE_KMD_REQUIRED_RANGE_END : End of range for
  *  required KMD LFDs
  */

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



> 
>> +
>> +/** 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".
Will follow.

> 
>> + */
>> +struct guc_lfd_data {
>> +    /** @dw0: A 32 bits dword, contains multiple bit fields */
>> +    u32 dw0;
> 'header' would be better than 'dw0'.
sure>
>> +    /*
>> +     * 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.
To be removed.>> +     */
>> +#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'.
Will do a cleanup among this series, to rename all macros names for 
GENMASK to be
STRUCTURE_NAME_XXX_MASK_YYY
XXX is structure field name
YYY is bit field name
for example:
	GUC_LFD_DATA_HEADER_MASK_TYPE
	GUC_LFD_DATA_HEADER_MASK_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.
Need to confirm TLV or KLV
>> +     */
>> +#define GUC_LFD_DATA_DESC_TYPE        GENMASK(31, 16)
> Again, GUC_LFD_HEADER_TYPE would be a better name.
Will be GUC_LFD_DATA_HEADER_MASK_TYPE

> 
>> +    /** @dw_size: Number of dwords the `data` field contains. */
>> +    u32 dw_size;
> data_size
size might mislead to byte size, will use data_count
>> +    /** @data: Data defined by File descriptor type. */
> Defined by GUC_LFD_HEADER_TYPE.
Sure
>> +    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.
Struct to be removed
>> + * 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.
Yes, will do that way>
>> +
>> +/**
>> + * 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."
will do>
>> + */
>> +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.
Agree, not worth to declare a structure, especially a single u32.

There is an difference between LFD and LIC values, The payload(or value) 
for above is one u32, and for LIC, it is KLV, which has extra length 
field, make it possible to more than 1 u32. There is no conflict at this 
moment, but some assertion is needed to prevent further mismatch.
>> +
>> +/**
>> + * 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".
sure
 >
>> +     */
>> +    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.
To be removed.>
>> + */
>> +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.
Sure
 >> +     */
>> +    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.
Sure
>> +     */
>> +    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.
Yes, no need for single u32, To be removed>
>> +
>> +/**
>> + * 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.
will be LFD blocks>
>> +     */
>> +    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[];".
Sounds good
> John.
> 
> 
>> +} __packed;
>> +
>> +#endif
>