[PATCH v4 1/6] drm/xe/guc: Add log init config abi definitions

Mon Jul 14 21:44:20 UTC 2025

Please see my comments inline below.

Regards,
Zhanjun Dong

On 2025-06-28 8:32 a.m., Michal Wajdeczko wrote:
> 
> 
> On 28.06.2025 14:19, Michal Wajdeczko wrote:
>>
>>
>> On 23.04.2025 23:58, Zhanjun Dong wrote:
>>> Add GuC log init config (LIC) ABI definitions.
>>>
>>> Signed-off-by: Zhanjun Dong <zhanjun.dong at intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/abi/guc_log_lic_abi.h | 117 +++++++++++++++++++++++
>>
>> since LIC already refers to LOG (LOG-init-config) then maybe names
>> should follow GUC_LIC naming? here: guc_lic_abi.h
Sure

>>>   1 file changed, 117 insertions(+)
>>>   create mode 100644 drivers/gpu/drm/xe/abi/guc_log_lic_abi.h
>>>
>>> diff --git a/drivers/gpu/drm/xe/abi/guc_log_lic_abi.h b/drivers/gpu/drm/xe/abi/guc_log_lic_abi.h
>>> new file mode 100644
>>> index 000000000000..48302bfcab2b
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/xe/abi/guc_log_lic_abi.h
>>> @@ -0,0 +1,117 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Copyright © 2025 Intel Corporation
>>> + */
>>> +
>>> +#ifndef _ABI_GUC_LOG_LIC_ABI_H_
>>> +#define _ABI_GUC_LOG_LIC_ABI_H_
>>> +
>>> +#include <linux/types.h>
>>> +
>>> +#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
>>
>> are above definitions really related to the LIC?
Right, that's not part of ABI, should be moved out

>>
>>> +
>>> +#define GUC_LOG_INIT_CONFIG_LIC_MAGIC			0x8086900D
>>
>> move closer to guc_log_init_config
>>
>>> +#define GUC_LOG_INIT_CONFIG_FORMAT_VERSION_MAJOR	0x0001
>>> +#define GUC_LOG_INIT_CONFIG_FORMAT_VERSION_MINOR	0x0000
>>
>> move closer to guc_lic_format_version and make them 1u and 0u
>>
>>> +
>>> +/** enum guc_log_lic_type - Log Init Config KLV IDs. */
>>
>> if those are really KLVs, not TLVs as wrongly(?) described once in the
Let me find out, and I will get back to you once got confirmed.

>> spec, then maybe this enum shall be named as guc_log_lic_klv_id with:
>>
>> GUC_LOG_LIC_KLV_GUC_SW_VERSION
>> GUC_LOG_LIC_KLV_GUC_DEVICE_ID
>> GUC_LOG_LIC_KLV_TSC_FREQUENCY
>> GUC_LOG_LIC_KLV_GMD_ID

If it is KLV and since LIC already refers to LOG (LOG-init-config), it 
could be:
GUC_LIC_KLV_GUC_SW_VERSION
GUC_LIC_KLV_GUC_DEVICE_ID
GUC_LIC_KLV_TSC_FREQUENCY
GUC_LIC_KLV_GMD_ID

If it is TLV, then it would be:
GUC_LIC_TYPE_xxx

>>
>>> +enum guc_log_lic_type {
>>> +	/**
>>> +	 * @GUC_LOG_LIC_TYPE_GUC_SW_VERSION: GuC firmware version. Value
>>> +	 * is a 32 bit number represented by guc_sw_version.
>>> +	 */
>>> +	GUC_LOG_LIC_TYPE_GUC_SW_VERSION = 0x1,
>>> +	/**
>>> +	 * @GUC_LOG_LIC_TYPE_GUC_DEVICE_ID: GuC device id. Value is a 32
>>> +	 * bit.
>>> +	 */
>>> +	GUC_LOG_LIC_TYPE_GUC_DEVICE_ID = 0x2,
>>> +	/**
>>> +	 * @GUC_LOG_LIC_TYPE_TSC_FREQUENCY : GuC timestamp counter
>>> +	 * frequency. Value is a 32 bit number representing frequency in
>>> +	 * kilohertz. This timestamp is utilized in log entries, timer and
>>
>> s/kilohertz/kHz
>>
>>> +	 * for engine utilization tracking.
>>> +	 */
>>> +	GUC_LOG_LIC_TYPE_TSC_FREQUENCY = 0x3,
>>> +	/**
>>> +	 * @GUC_LOG_LIC_TYPE_GMD_ID: 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_LOG_LIC_TYPE_BUILD_PLATFORM_ID: GuC build platform ID.
>>> +	 * Value is 32 bits.
>>> +	 */
>>> +	GUC_LOG_LIC_TYPE_BUILD_PLATFORM_ID = 0x5,
>>> +};
>>> +
>>> +/**
>>> + * struct guc_sw_version - This structure describes the full version of a
>>> + * software component.
>>> + */
>>> +struct guc_sw_version {
>>> +	/** @dw0: A 32 bits dword, contains multiple bit fields */
>>> +	u32 dw0;
>>> +	/*  Patch version */
>>> +#define GUC_SW_VERSION_PATCH_VERSION		GENMASK(7, 0)
>>> +	/*  Minor version */
>>> +#define GUC_SW_VERSION_MINOR_VERSION		GENMASK(15, 8)
>>> +	/*  Major version */
>>> +#define GUC_SW_VERSION_MAJOR_VERSION		GENMASK(23, 16)
>>> +	/*  Branch ID */
>>> +#define GUC_SW_VERSION_BRANCH_ID		GENMASK(31, 24)
>>
>> drop useless comments (patch major ...
sure
 >>
>>> +} __packed;
>>> +
>>> +/**
>>> + * struct guc_lic_format_version - 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 {
>>> +	/** @dw0: A 32 bits dword, contains multiple bit fields */
>>> +	u32 dw0;
>>> +	/*
>>> +	 *  Log-Init-Config structure minor version. Must be
>>> +	 * GUC_LOG_INIT_CONFIG_FORMAT_VERSION_MINOR
>>> +	 */
>>> +#define GUC_LIC_FORMAT_VERSION_MINOR_VERSION		GENMASK(15, 0)
>>> +	/*
>>> +	 *  Log-Init-Config structure major version. Must be
>>> +	 * GUC_LOG_INIT_CONFIG_FORMAT_VERSION_MAJOR
>>> +	 */
>>> +#define GUC_LIC_FORMAT_VERSION_MAJOR_VERSION		GENMASK(31, 16)
>>> +} __packed;
>>> +
>>> +/**
>>> + * struct guc_log_init_config - 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 {
> 
> missed to add - there is a lot of inconsistency in the naming (likely
> due to a spec):
> 
> guc_log_init_config
>          ^no lic
> 
> guc_lic_format_version
>     ^no log
> 
> guc_log_lic_type
> 
> GUC_LOG_INIT_CONFIG_LIC_MAGIC
>          ^full name  ^
>                      ^then lic again
> GUC_LOG_INIT_CONFIG_FORMAT_VERSION_MINOR
>          ^full name, no lic

> so maybe:
> 
> guc_log_lic_buffer
> guc_log_lic_version
> guc_log_lic_klv_id
> 
> GUC_LOG_LIC_MAGIC
> GUC_LOG_LIC_VERSION_MINOR

As discussed above, since LIC already refers to LOG (LOG-init-config),
I will do
s/_log_lic/_lic
s/_log_init_config_lic/_lic
s/_log_init_config/_lic

> 
> maybe the spec needs some tuning/updates?

Agree, I will clean up all inconsistency in this series, and will work 
with other team to arrange further cleanup on spec

> 
>>> +	/**
>>> +	 * @magic: A magic number set by GuC to identify that this
>>> +	 * structure contains valid information: magic = 0x8086900D.
>>
>> nit: you may want to add reference to GUC_LOG_INIT_CONFIG_LIC_MAGIC
>>
>>> +	 * Used to verify the information in this structure is valid.
>>> +	 */
>>> +	u32 magic;
>>
>> maybe GUC_LOG_INIT_CONFIG_LIC_MAGIC definition can be put here?
>>
>>> +	/**
>>> +	 * @ver: The version of the this structure. Detail description
>>> +	 * is guc_lic_format_version
>>
>> drop 'Detail ...' since this struct is documented>>
>>> +	 */
>>> +	struct guc_lic_format_version ver;
>>
>> this could be named in full 'version'

suresure>>
>>> +	/** @dw_size: Number of Dws the `data` array contains. */
>>> +	u32 dw_size;
>>> +	/**
>>> +	 * @data: Array of dwords representing a list of LIC KLVs of
>>> +	 * type guc_klv_generic with keys represented by guc_log_lic_type
>>> +	 */
>>> +	u32 data[] __counted_by(dw_size);
>>> +} __packed;
>>> +
>>> +#endif
>>
> 