Re: [PATCH v3 03/23] drm/xe/log: Introduce structured component/location identifiers

On 8/3/2026 10:00 AM, Mallesh, Koujalagi wrote:
> 
> On 30-07-2026 08:50 pm, Michal Wajdeczko wrote:
>> Introduce structured identifiers for each component type that
>> could emit a SIGID log entry and for their locations. We plan
>> to store those IDs in the CPER records for better filtering.
>> Define also structured identifiers for the supported locations.
>>
>> Signed-off-by: Michal Wajdeczko <[email protected]>
>> Cc: Rodrigo Vivi <[email protected]>
>> Reviewed-by: Rodrigo Vivi <[email protected]>
>> ---
>> Cc: Aravind Iddamsetty <[email protected]>
>> Cc: Mallesh Koujalagi <[email protected]>
>> Cc: Raag Jadav <[email protected]>
>> Cc: Riana Tauro <[email protected]>
>> ---
>> v2: fix typo, define reserved ids (Michal)
>> v3: fix kernel-doc to match code (Sashiko)
>> ---
>>   drivers/gpu/drm/xe/abi/xe_log_abi.h | 186 ++++++++++++++++++++++++++++
>>   1 file changed, 186 insertions(+)
>>   create mode 100644 drivers/gpu/drm/xe/abi/xe_log_abi.h
>>
>> diff --git a/drivers/gpu/drm/xe/abi/xe_log_abi.h b/drivers/gpu/drm/xe/abi/xe_log_abi.h
>> new file mode 100644
>> index 000000000000..4861a5b58c10
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/abi/xe_log_abi.h
>> @@ -0,0 +1,186 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2026 Intel Corporation
>> + */
>> +
>> +#ifndef _ABI_XE_LOG_ABI_H_
>> +#define _ABI_XE_LOG_ABI_H_
>> +
>> +#include <linux/bits.h>
>> +#include <linux/bitfield.h>
>> +
>> +#include "abi/xe_sigid_abi.h"
>> +
>> +/**
>> + * enum xe_log_component_bits - bits for components structure definitions
>> + *
>> + * Component identifiers are structured based on::
>> + *
>> + *     COMPONENT = CLASS(8b).TYPE(8b)
>> + *
> 
> Are 8b sufficient for CLASS and TYPE?  In future we need to increase that.

we can make it 16b and 16b (as component parameter is already u32)

but ...

are you sure that we will define anytime soon more than 255 component types per class, or have more than 255 classes?

> 
>> + * and the structure looks like this::
>> + *
>> + *     ├── SYSTEM(0)
>> + *     │   └── ...
>> + *     ├── DRIVER(1)
>> + *     │   └── ...
>> + *     ├── FEATURE(2)
>> + *     │   └── ...
>> + *     ├── FIRMWARE(4)
>> + *     │   └── ...
>> + *     └── HARDWARE(8)
>> + *         └── ...
>> + *
>> + * Examples::
>> + *
>> + *     COMPONENT(0.type) = SYSTEM.type = system component
>> + *     COMPONENT(1.type) = DRIVER.type = driver core component
>> + *     COMPONENT(3.type) = DRIVER_FEATURE.type = driver feature
>> + *     COMPONENT(5.type) = DRIVER_FIRMWARE.type = firmware driver component
>> + *     COMPONENT(9.type) = DRIVER_HARDWARE.type = hardware driver component
>> + *
>> + */
>> +enum xe_log_component_bits {
>> +    /* private: */
>> +    XE_LOG_COMPONENT_CLASS_MASK = GENMASK_U16(7, 0),
>> +    XE_LOG_COMPONENT_TYPE_MASK = GENMASK_U16(15, 8),
>> +    /* private: component classes */
>> +    XE_LOG_COMPONENT_CLASS_SYSTEM = 0u,
>> +    XE_LOG_COMPONENT_CLASS_DRIVER = 1u,
>> +    XE_LOG_COMPONENT_CLASS_FEATURE = 2u,
>> +    XE_LOG_COMPONENT_CLASS_FIRMWARE = 4u,
>> +    XE_LOG_COMPONENT_CLASS_HARDWARE = 8u,
>> +    XE_LOG_COMPONENT_CLASS_DRIVER_FEATURE = XE_LOG_COMPONENT_CLASS_DRIVER |
>> +                        XE_LOG_COMPONENT_CLASS_FEATURE,
>> +    XE_LOG_COMPONENT_CLASS_DRIVER_FIRMWARE = XE_LOG_COMPONENT_CLASS_DRIVER |
>> +                         XE_LOG_COMPONENT_CLASS_FIRMWARE,
>> +    XE_LOG_COMPONENT_CLASS_DRIVER_HARDWARE = XE_LOG_COMPONENT_CLASS_DRIVER |
>> +                         XE_LOG_COMPONENT_CLASS_HARDWARE,
>> +    /* private: reserved identifiers */
>> +    XE_LOG_COMPONENT_NONE = 0u,
>> +};
>> +
>> +#define MAKE_XE_LOG_COMPONENT(_CLASS, type) \
>> +    (FIELD_PREP_CONST(XE_LOG_COMPONENT_CLASS_MASK, \
>> +              XE_LOG_COMPONENT_CLASS_##_CLASS) | \
>> +     FIELD_PREP_CONST(XE_LOG_COMPONENT_TYPE_MASK, (type)))
>> +
>> +/**
>> + * enum xe_log_location_bits - bits for location structure definitions
>> + *
>> + * Location identifiers are structured based on::
>> + *
>> + *     LOCATION = TYPE(8b).ID(8b)
>> + *
> 
> Are 8b sufficient for Type and ID?

we can make it 16b & 16b (as location parameter is already u32)

but ...

do you have any new location candidates in mind that would require more than 255 IDs or that we would need to define more than 255 location types?

currently we have:
	TILE	max ID = XE_MAX_TILES_PER_DEVICE(2) = 2
	GT	max ID = XE_MAX_GT_PER_TILE(2) * XE_MAX_TILES_PER_DEVICE(2) = 4

even if we add:
	VF	max ID = 63

still everything < 255

unless we would like to use someday:
	PASID	-> 20b 

but then 16b/16b wont work either

I can change location bits to TYPE(8b) and ID(24b) if you think it is required now

> 
> Everything else looks good.
> 
> Reviewed-by: Mallesh Koujalagi <[email protected]>
> 
>> + * and the structure looks like this::
>> + *
>> + *     ├── DEVICE(0)
>> + *     │   └── MBZ(0)
>> + *     ├── TILE(1)
>> + *     │   ├── Tile0(0)
>> + *     │   ├── ...
>> + *     │   └── TileN(n)
>> + *     ├── GT(1)
>> + *     │   ├── GT0(0)
>> + *     │   ├── ...
>> + *     │   └── GTn(n)
>> + *     └── ...
>> + *
>> + * Examples::
>> + *
>> + *     LOCATION(0.0) = NONE
>> + *     LOCATION(1.0) = DEVICE.0 = "Device"
>> + *     LOCATION(2.1) = TILE.1 = "Tile1"
>> + *     LOCATION(3.2) = GT.2 = "GT2"
>> + *
>> + */
>> +enum xe_log_location_bits {
>> +    /* private: */
>> +    XE_LOG_LOCATION_TYPE_MASK = GENMASK_U16(7, 0),
>> +    XE_LOG_LOCATION_ID_MASK = GENMASK_U16(15, 8),
>> +    /* private: location types */
>> +    XE_LOG_LOCATION_TYPE_DEVICE = 1u,
>> +    XE_LOG_LOCATION_TYPE_TILE = 2u,
>> +    XE_LOG_LOCATION_TYPE_GT = 3u,
>> +    /* private: reserved identifiers */
>> +    XE_LOG_LOCATION_NONE = 0u,
>> +};
>> +
>> +#define PREP_XE_LOG_LOCATION(type, id) \
>> +    (FIELD_PREP(XE_LOG_LOCATION_TYPE_MASK, (type)) | \
>> +     FIELD_PREP(XE_LOG_LOCATION_ID_MASK, (id)))
>> +
>> +#define MAKE_XE_LOG_LOCATION(_TYPE, id) \
>> +    PREP_XE_LOG_LOCATION(XE_LOG_LOCATION_TYPE_##_TYPE, (id))
>> +
>> +/**
>> + * DEFINE_XE_LOG_COMPONENTS() - Define log components.
>> + * @define: name of the inner macro to expand.
>> + *
>> + * Use this super macro to define custom code for the log components.
>> + * The following parameters are available for each component::
>> + *
>> + *     define(CLASS, ID, TAG, SIGID, NAME)
>> + *
>> + * where:
>> + *
>> + *     @ID is the unique component identifier within CLASS.SUBCLASS.CATEGORY
>> + *     @TAG is unique component tag (across all components)
>> + *     @SIGID is the default xe_sigid for the component (without the XE_SIGID_ prefix)
>> + */
>> +#define DEFINE_XE_LOG_COMPONENTS(define) \
>> +    /* */                                    \
>> +    define(SYSTEM, 1, PCI, SW, "Linux PCI Subsystem")            \
>> +    define(SYSTEM, 2, DRM, SW, "DRM")                    \
>> +    /* */                                    \
>> +    define(DRIVER, 1, XE, SW, "Xe Driver")                    \
>> +    define(DRIVER, 2, PROBE, PROBE, "Driver Initialization")        \
>> +    define(DRIVER, 3, WEDGED, WEDGED, "Device Malfunction")            \
>> +    define(DRIVER, 4, RTP, SW, "Register Table Processing")            \
>> +    define(DRIVER, 5, WA, SW, "Workarounds")                \
>> +    define(DRIVER, 6, PAGEFAULT, MEM_FAULT, "Page Fault")            \
>> +    /* */                                    \
>> +    define(DRIVER_HARDWARE, 1, REGS, IO_BUS, "Registers")            \
>> +    define(DRIVER_HARDWARE, 2, GGTT, IO_BUS, "Global GTT")            \
>> +    define(DRIVER_HARDWARE, 3, GT, GT_TDR, "Graphics Technology")        \
>> +    define(DRIVER_HARDWARE, 4, LMTT, IO_BUS, "LMEM Translation Table")    \
>> +    define(DRIVER_HARDWARE, 5, MEMIRQ, IO_BUS, "Memory Based IRQ")        \
>> +    /* */                                    \
>> +    define(DRIVER_FEATURE, 1, PF, SW, "SR-IOV Physical Function")        \
>> +    define(DRIVER_FEATURE, 2, VF, SW, "SR-IOV Virtual Function")        \
>> +    define(DRIVER_FEATURE, 3, SURVIVABILITY, SURVIVABILITY, "Survivability") \
>> +    define(DRIVER_FEATURE, 4, RAS, SW, "Reliability, Accessibility, Serviceability") \
>> +    /* */                                    \
>> +    define(DRIVER_FIRMWARE, 1, GUC, RUNTIME_FW, "GuC")            \
>> +    define(DRIVER_FIRMWARE, 2, HUC, RUNTIME_FW, "HuC")            \
>> +    define(DRIVER_FIRMWARE, 3, GSC, RUNTIME_FW, "GSC")            \
>> +    define(DRIVER_FIRMWARE, 16, PCODE, DEVICE_FW, "PCode")            \
>> +    define(DRIVER_FIRMWARE, 17, SYSCTRL, DEVICE_FW, "System Controller")    \
>> +    /* eod */
>> +
>> +/**
>> + * enum xe_log_component_tags - TAGs of all supported components
>> + */
>> +enum xe_log_component_tags {
>> +    /* private: */
>> +#define MAKE_XE_LOG_COMPONENT_ENUM(_CLASS, _ID, _TAG, _SIG, _NAME) \
>> +    XE_LOG_COMPONENT_##_TAG = MAKE_XE_LOG_COMPONENT(_CLASS, (_ID)), \
>> +    XE_LOG_COMPONENT_##_CLASS##_##_ID = XE_LOG_COMPONENT_##_TAG, \
>> +    /* eod */
>> +    DEFINE_XE_LOG_COMPONENTS(MAKE_XE_LOG_COMPONENT_ENUM)
>> +#undef MAKE_XE_LOG_COMPONENT_ENUM
>> +};
>> +
>> +/**
>> + * enum xe_log_component_sigids - SIGIDs of all supported components
>> + */
>> +enum xe_log_component_sigids {
>> +    /* private: */
>> +#define MAKE_XE_LOG_COMPONENT_SIGID(_CLASS, _ID, _TAG, _SIG, _NAME) \
>> +    XE_LOG_COMPONENT_##_TAG##_SIGID = XE_SIGID_##_SIG, \
>> +    /* eod */
>> +    DEFINE_XE_LOG_COMPONENTS(MAKE_XE_LOG_COMPONENT_SIGID)
>> +#undef MAKE_XE_LOG_COMPONENT_SIGID
>> +};
>> +
>> +#endif