Re: [PATCH v3 11/23] drm/xe/log: Extend components list with hardware items

On 12-08-2026 14:25, Michal Wajdeczko wrote:
>
> On 8/12/2026 8:55 AM, Tauro, Riana wrote:
>> On 04-08-2026 11:35, Mallesh, Koujalagi wrote:
>>> On 30-07-2026 08:51 pm, Michal Wajdeczko wrote:
>>>> Update our super macro with new top level hardware components.
>>>>
>>>> Signed-off-by: Michal Wajdeczko <[email protected]>
>>> LGTM,
>>> Reviewed-by: Mallesh Koujalagi <[email protected]>
>>>> Cc: Rodrigo Vivi <[email protected]>
>>>> Cc: Aravind Iddamsetty <[email protected]>
>>>> ---
>>>> v2: follow enum xe_ras_component values (Aravind)
>>>> ---
>>>>    drivers/gpu/drm/xe/abi/xe_log_abi.h | 12 ++++++++++++
>>>>    1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/abi/xe_log_abi.h b/drivers/gpu/drm/xe/abi/xe_log_abi.h
>>>> index 4861a5b58c10..b3373d4b29d5 100644
>>>> --- a/drivers/gpu/drm/xe/abi/xe_log_abi.h
>>>> +++ b/drivers/gpu/drm/xe/abi/xe_log_abi.h
>>>> @@ -129,6 +129,10 @@ enum xe_log_location_bits {
>>>>     *     @SIGID is the default xe_sigid for the component (without the XE_SIGID_ prefix)
>>>>     */
>>>>    #define DEFINE_XE_LOG_COMPONENTS(define) \
>>>> +    DEFINE_XE_LOG_SOFTWARE_COMPONENTS(define) \
>>>> +    DEFINE_XE_LOG_HARDWARE_COMPONENTS(define)
>>>> +
>>>> +#define DEFINE_XE_LOG_SOFTWARE_COMPONENTS(define) \
>>>>        /* */                                    \
>>>>        define(SYSTEM, 1, PCI, SW, "Linux PCI Subsystem")            \
>>>>        define(SYSTEM, 2, DRM, SW, "DRM")                    \
>>>> @@ -156,6 +160,14 @@ enum xe_log_location_bits {
>>>>        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")    \
>>>> +
>>>> +#define DEFINE_XE_LOG_HARDWARE_COMPONENTS(define) \
>>>> +    define(HARDWARE, 1, DEVMEM, DEVICE_MEMORY, "Device Memory")        \
>> DEVICE_MEMORY
> do you mean s/DEVMEM/DEVICE_MEMORY ?
>
>>>> +    define(HARDWARE, 2, HWCORE, CORE_COMPUTE, "Core Compute")        \
>> Core_compute
> and here s/HWCORE/CORE_COMPUTE ?

yes. We have the same names in uapi for netlink and same names are used 
in firmware and L0.
Adding different naming will unnecessary cause confusion.

>
>>>> +    /*     HARDWARE, 3, RESERVED */                        \
>>>> +    define(HARDWARE, 4, PCIE, PCIE, "PCIe Interface")            \
>>>> +    define(HARDWARE, 5, FABRIC, FABRIC, "Fabric")                \
>>>> +    define(HARDWARE, 6, SOC, SOC_INTERNAL, "SoC Internal")            \
>> SOC_INTERNAL
> and here s/SOC/SOC_INTERNAL ?
>
> (btw, is there a plan for SOC_EXTERNAL ?
> if no, why do we want to keep _INTERNAL suffix ?
>> Let's retain the names same as UAPI to avoid unnecessary confusion while collecting CPER logs.
> but those listed here are our local component names
> and just today there is 1:1 relation between those
> HW components and HW SIGIDs, but maybe one day there
> will be more components that would map into existing
> SIGIDs (compare COMP_GUC/HUC --> SIGID_RUNTIME_FW)

For hardware errors, any other error would be part of existing class or 
a new class added.
We don't expect the names to change as it is already part of UAPI for 
netlink and
used by userspace.

>
> the SIGID enum names are intact
> 	XE_SIGID_DEVICE_MEMORY
> 	XE_SIGID_SOC_INTERNAL
> 	..
> and btw I'm not sure that we ever convert SIGID enum values
> (202, 205) into any user facing friendly names
>
> and the idea for having a shorter component names was that
> we are doing dmesg decoration and print friendly component
> and/or location name (except SIGID, which is a raw integer)
>
> 	[drm] ERROR SIGID=104 (-EPROTO) Tile0: GT0: GUC: blah
> 	[drm] ERROR SIGID=202 (123456789012) Tile0: DEVMEM: blah
> 	[drm] ERROR SIGID=205 (123456789012) Tile1: SOC: blah

Okay for software errors. But for hardware errors it would be good to 
have consistency


Thanks
Riana

>
> having those lengthy names does not look nice:
>
> 	[drm] ERROR SIGID=202 (123456789012) Tile0: DEVICE_MEMORY: blah
> 	[drm] ERROR SIGID=205 (123456789012) Tile1: SOC_INTERNAL: blah
>
> besides, in dmesg only "SIGID=%u" chunk is part of our contract
>
> in CPER, all SIGID and component and location will be stored
> as plain numbers and how those numbers will be decoded is beyond us
>
> btw, maybe component decoration should be before location?
>
> 	[drm] ERROR SIGID=104 (-EPROTO) GUC: Tile0: GT0: blah
> 	[drm] ERROR SIGID=202 (123456789012) DEVICE_MEMORY: Tile1: blah
> 	[drm] ERROR SIGID=205 (123456789012) SOC_INTERNAL: Tile1: blah
>
> (see patch 4/23)
>
>> These names were asked to be retained similar to actual hardware error classification even for uapi.
>>
>> ++@Aravind
>>
>> Thanks
>> Riana
>>
>>
>>>>        /* eod */
>>>>      /**