Re: [PATCH v4 02/32] drm/xe/log: Add structured SIGID error logging infrastructure

On 8/14/2026 3:05 PM, Nilawar, Badal wrote:
> On 14-08-2026 00:30, Rodrigo Vivi wrote:
>> On Thu, Aug 13, 2026 at 07:12:41PM +0530, Nilawar, Badal wrote:
>>> Hi Michal,
>>>
>>> Couple of comments, on rate-limiting and XE_SIGID_WEDGED, from v3 https://patchwork.freedesktop.org/patch/743308/?series=171022&rev=3#comment_1373861
>>>
>>> are not answered/addressed.
>> Hi Badal, could you please be more specific about the questions and repeat them here?
>> I couldn't find from the provided url...
>>
>> But as we chatted, this series doesn't change the behavior of that and
>> the comment of rate-limit is exactly to ensure this is documented. To
>> ensure it is clear dmesg is not an ABI. It should not be reliable in
>> any way or shape. It is simpler a helper.
> Ok.
>>
>> On the wedge, the resolution buckets were removed, but I'm not sure
>> if there were more questions around that.
> 
> My concern is about statement "Pick per *report site*, not per incident", XE_SIGID_WEDGED is not site specific SIGID. It can be reported from multiple sites.

the XE_SIGID_WEDGED is specific to xe_device_declare_wedged()
which we treated here as a separate "site"

and whoever triggers the wedge state, should use its own SIGID
to reports its own errors, should not use the SIGID_WEDGED

> 
>>
>>>> + * Examples::
>>>> + *
>>>> + *   <3> xe 0000:03:00.0: [drm] *ERROR* SIGID=104 FATAL (-EPROTO) Invalid GuC reply
>>>> + *   <3> xe 0000:03:00.0: [drm] *ERROR* SIGID=106 (-ETIMEDOUT) Engine 'rcs0' hung
>>>> + *   <6> xe 0000:03:00.0: [drm] SIGID=103 In survivability mode
>>>> + */
>>>> +void xe_log_emit(struct pci_dev *pdev, int cper_sev, enum xe_sigid sigid,
>>>> +         u32 component, u32 location, const void *data, size_t len,
>>>> +         const char *fmt, ...)
>>>> +{
>>>> +    struct va_format vaf;
>>>> +    va_list args;
>>>> +
>>>> +    va_start(args, fmt);
>>>> +    vaf.fmt = fmt;
>>>> +    vaf.va = &args;
>>>> +
>>>> +    log_emit_dmesg(pdev, cper_sev, sigid, component, location, data, len, &vaf);
>>>> +    log_emit_cper(pdev, cper_sev, sigid, component, location, data, len, &vaf);
>>>> +
>>>  From kunit example I got this output for hardware errors.
>>> drm-kunit-mock-device demo_dmesg.drm-kunit-mock-device: [drm] *ERROR*
>>> SIGID=204 (0102030405060708090a0b0c) [Hardware Error]: testing HARDWARE
>>> signature
>>> drm-kunit-mock-device demo_dmesg.drm-kunit-mock-device: [drm] *ERROR*
>>> SIGID=202 CORRECTED (0102030405060708090a0b0c) [Hardware Error]: Tile1:
>>> testing HARDWARE signature
>>>
>>> SIGIDs 202 and 204 correspond to the XE_RAS_COMP_DEVICE_MEMORY and
>>> XE_RAS_COMP_FABRIC components returned by firmware via xe_ras_error_class.
>>> If we want the component name to be included in the error message, what
>>> should be passed to the logging helper? The current KUnit test uses
>>> XE_LOG_COMPONENT_NONE, so no component information is being emitted.
>> Our design goal here was to minimize the SIGIDs and make it per site
>> due to maintenability concerns.
>>
>> There will be a field in the actual CPER with location and that can
>> also be printed in the msg if needed. In a matter of fact, this
>> is what we currently do anyway.
>>
>> Increasing this to a per case id is a big no!
> 
> Agreed.
> 
> Thanks,
> Badal
> 
>>
>> Thanks,
>> Rodrigo.
>>
>>> Thanks,
>>> Badal
>>>
>>>> +    va_end(args);
>>>> +}
>>>> diff --git a/drivers/gpu/drm/xe/xe_log.h b/drivers/gpu/drm/xe/xe_log.h
>>>> new file mode 100644
>>>> index 000000000000..d475e816ee0b
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/xe/xe_log.h
>>>> @@ -0,0 +1,20 @@
>>>> +/* SPDX-License-Identifier: MIT */
>>>> +/*
>>>> + * Copyright © 2026 Intel Corporation
>>>> + */
>>>> +
>>>> +#ifndef _XE_LOG_H_
>>>> +#define _XE_LOG_H_
>>>> +
>>>> +#include <linux/cper.h>
>>>> +
>>>> +#include "abi/xe_sigid_abi.h"
>>>> +
>>>> +struct pci_dev;
>>>> +
>>>> +__printf(8, 9)
>>>> +void xe_log_emit(struct pci_dev *pdev, int cper_sev, enum xe_sigid sigid,
>>>> +         u32 component, u32 location, const void *data, size_t len,
>>>> +         const char *fmt, ...);
>>>> +
>>>> +#endif