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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Aug 05, 2026 at 07:23:10PM +0200, Michal Wajdeczko wrote:
> 
> 
> On 8/4/2026 8:52 PM, Rodrigo Vivi wrote:
> > On Tue, Aug 04, 2026 at 08:30:47PM +0530, Tauro, Riana wrote:
> >> Hi Mallesh/Michal
> >>
> >> On 30-07-2026 20:50, Michal Wajdeczko wrote:
> >>> From: Mallesh Koujalagi <mallesh.koujalagi@intel.com>
> >>>
> >>> Today the driver reports faults with ad-hoc drm_err()/xe_gt_err()
> >>> strings that have no stable shape. That is readable for a human, but it
> >>> gives fleet tooling nothing durable to match on: the wording changes
> >>> between releases, lines can be rate-limited or dropped under an error
> >>> storm, and there is no consistent way to ask "which recognised fault
> >>> just happened?".
> >>>
> >>> Introduce a signature identifier (SIGID): a small, stable integer that
> >>> names one recognised Xe fault situation and serves as the primary handle
> >>> for triage. A SIGID maps, through published end-user documentation, to a
> >>> description and a recommended action; the driver only has to emit the
> >>> right SIGID next to the usual human-readable text.
> >>>
> >>> Signed-off-by: Mallesh Koujalagi <mallesh.koujalagi@intel.com>
> >>> Assisted-by: Copilot:Opus-4.8
> >>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >>> Co-developed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >>> ---
> >>> Cc: Yoni Levitt <yoni.levitt@intel.com>
> >>> Cc: Aravind Iddamsetty <aravind.iddamsetty@intel.com>
> >>> Cc: Raag Jadav <raag.jadav@intel.com>
> >>> Cc: Riana Tauro <riana.tauro@intel.com>
> >>> ---
> >>> v2: CORRECTED is still an error (Michal)
> >>>      prepare to decorate dmesg with comp/loc (Michal)
> >>> ---
> >>>   Documentation/gpu/xe/index.rst        |   1 +
> >>>   Documentation/gpu/xe/xe_sigid.rst     |  14 ++
> >>>   drivers/gpu/drm/xe/Makefile           |   1 +
> >>>   drivers/gpu/drm/xe/abi/xe_sigid_abi.h | 183 ++++++++++++++++++++++++++
> >>>   drivers/gpu/drm/xe/xe_log.c           | 135 +++++++++++++++++++
> >>>   drivers/gpu/drm/xe/xe_log.h           |  20 +++
> >>>   6 files changed, 354 insertions(+)
> >>>   create mode 100644 Documentation/gpu/xe/xe_sigid.rst
> >>>   create mode 100644 drivers/gpu/drm/xe/abi/xe_sigid_abi.h
> >>>   create mode 100644 drivers/gpu/drm/xe/xe_log.c
> >>>   create mode 100644 drivers/gpu/drm/xe/xe_log.h
> >>>
> >>> diff --git a/Documentation/gpu/xe/index.rst b/Documentation/gpu/xe/index.rst
> >>> index 665c0e93601c..0247a255f7e6 100644
> >>> --- a/Documentation/gpu/xe/index.rst
> >>> +++ b/Documentation/gpu/xe/index.rst
> >>> @@ -35,3 +35,4 @@ The display, or :ref:`drm-kms`, support for drm/xe is provided by
> >>>      xe-drm-usage-stats.rst
> >>>      xe_configfs
> >>>      xe_gt_stats
> >>> +   xe_sigid
> >>> diff --git a/Documentation/gpu/xe/xe_sigid.rst b/Documentation/gpu/xe/xe_sigid.rst
> >>> new file mode 100644
> >>> index 000000000000..45d84a62f185
> >>> --- /dev/null
> >>> +++ b/Documentation/gpu/xe/xe_sigid.rst
> >>> @@ -0,0 +1,14 @@
> >>> +.. SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> >>> +
> >>> +========
> >>> +Xe SIGID
> >>> +========
> >>> +
> >>> +.. kernel-doc:: drivers/gpu/drm/xe/abi/xe_sigid_abi.h
> >>> +   :doc: Xe Error Signatures (SIGID)
> >>> +
> >>> +Signature Identifiers
> >>> +=====================
> >>> +
> >>> +.. kernel-doc:: drivers/gpu/drm/xe/abi/xe_sigid_abi.h
> >>> +   :internal:
> >>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> >>> index 67ada1d6c2fb..7ac3954737f9 100644
> >>> --- a/drivers/gpu/drm/xe/Makefile
> >>> +++ b/drivers/gpu/drm/xe/Makefile
> >>> @@ -87,6 +87,7 @@ xe-y += xe_bb.o \
> >>>   	xe_hw_fence.o \
> >>>   	xe_irq.o \
> >>>   	xe_late_bind_fw.o \
> >>> +	xe_log.o \
> >>>   	xe_lrc.o \
> >>>   	xe_mem_pool.o \
> >>>   	xe_migrate.o \
> >>> diff --git a/drivers/gpu/drm/xe/abi/xe_sigid_abi.h b/drivers/gpu/drm/xe/abi/xe_sigid_abi.h
> >>> new file mode 100644
> >>> index 000000000000..99717fdf74a6
> >>> --- /dev/null
> >>> +++ b/drivers/gpu/drm/xe/abi/xe_sigid_abi.h
> >>> @@ -0,0 +1,183 @@
> >>> +/* SPDX-License-Identifier: MIT */
> >>> +/*
> >>> + * Copyright © 2026 Intel Corporation
> >>> + */
> >>> +
> >>> +#ifndef _ABI_XE_SIGID_ABI_H_
> >>> +#define _ABI_XE_SIGID_ABI_H_
> >>> +
> >>> +/**
> >>> + * DOC: Xe Error Signatures (SIGID)
> >>> + *
> >>> + * What SIGID stands for
> >>> + * ---------------------
> >>> + *
> >>> + * SIGID is short for *Signature Identifier*. A SIGID is a small, stable integer
> >>> + * that names one *recognised Xe fault situation* -- nothing more. It is the
> >>> + * primary handle used for triage: a SIGID maps to a human description and a
> >> The SIG ID maps to a report site as mentioned in "How to pick a sigid" not a
> >> human description.
> > 
> > Indeed, perhaps with simple:
> > s/maps to a human description/maps to a report site/
> > 
> > we get some consistency?! 
> >>
> >>> + * recommended first action. A coarse first-order action is documented in-tree
> >>> + * per SIGID (see "First-order action" below) so the id is actionable on its
> >>> + * own; published end-user documentation refines it with finer, cross-product
> >>> + * detail. The driver's only job is to emit the right SIGID next to the usual
> >>> + * human-readable text.
> >>> + *
> >>> + * Why this exists
> >>> + * ---------------
> >>> + *
> >>> + * Today the driver reports faults with ad-hoc ``drm_err()`` / ``xe_gt_err()``
> >>> + * strings that have no stable shape. That is fine for a human reading dmesg,
> >>> + * but it gives fleet tooling nothing durable to match on: the wording changes
> >>> + * between releases, lines can be rate-limited or dropped under an error storm,
> >>> + * and there is no consistent way to ask "which recognised fault just happened?"
> >>> + * A SIGID answers exactly that one question, identically across driver and
> >>> + * firmware versions, and (eventually) across other Intel devices in a node.
> >>> + *
> >>> + * What a SIGID is (and is not)
> >>> + * ----------------------------
> >>> + *
> >>> + * A SIGID names *which situation* is being reported. It deliberately does not
> >> This should also be consistent with "report site" instead of situation.
> > 
> > Agree.
> > s/situation/report site/
> > 
> >>> + * encode the detailed reason or the outcome. Those are carried alongside it::
> >>> + *
> >>> + *   SIGID    -> which recognised situation is being reported
> >>> + *   severity -> how serious this instance is (see below -- not fixed per SIGID)
> >>> + *   errno    -> the failing operation's error, shown with %pe
> >>> + *   message  -> free-form human-readable context
> >>> + *
> >>> + * Severity is independent of the SIGID. The same situation can be reported at
> >>> + * different severities depending on the instance and the recovery taken, so a
> >>> + * SIGID is never tied to one severity; the reporting site chooses it by calling
> >>> + * the matching xe_log_*() helper (see xe_log.h).
> >>> + *
> >>
> >> It'd be more intuitive for readers if section "When to use SIGID logging" is
> >> moved before how to pick one.
> > 
> > It makes sense to me.
> > 
> >>> + * How to pick a SIGID (the uniqueness rule)
> >>> + * -----------------------------------------
> >>> + *
> >>> + * Pick per *report site*, not per incident. Each site emits the single most
> >>> + * specific recognised situation *for that site* -- so the question is never
> >>> + * "classify this whole failure", it is "what does this site detect?", which has
> >>> + * one answer. A single underlying failure therefore legitimately produces a
> >>> + * *chain* of reports from different layers, each with its own SIGID -- e.g. a
> >>> + * GuC communication failure is reported as %XE_SIGID_RUNTIME_FW by the firmware
> >>> + * path, the failed recovery as %XE_SIGID_GT_TDR by the reset path, and an
> >>> + * aborted bind as %XE_SIGID_PROBE by the probe path. That chain lets triage
> >>> + * follow a fault from origin to final effect; it is not a duplicate.
> >>> + *
> >>> + * If a site does not match any defined situation, keep using the ordinary
> >>> + * ``xe_err()`` / ``xe_gt_err()`` logging rather than forcing a SIGID: a wrong
> >>> + * or over-broad classification is harder to retire than a missing one. When a
> >>> + * new situation is genuinely worth triaging, add it to the list below.
> >>> + *
> >>> + * Scope: software-emitted signatures only
> >>> + * ---------------------------------------
> >>> + *
> >>> + * This header enumerates only the situations that the *driver itself* detects
> >>> + * and reports from software: probe abort, wedged, survivability, driver-
> >>> + * detected firmware failures, engine TDR, memory faults and IO/bus faults.
> >>> + * These are the only values the driver assigns.
> >>> + *
> >>> + * Signatures that *originate* in firmware or hardware are a different thing:
> >>> + * they are produced and identified by the firmware or the hardware itself
> >>> + * (e.g. via their own records or error counters), and the driver merely logs
> >>> + * them as they are given to us. They are deliberately *not* enumerated here --
> >>> + * minting a driver-side id for a firmware/hardware-reported error would only
> >>> + * duplicate an identifier the reporting layer already owns. The two
> >>> + * driver-detected firmware situations below (%XE_SIGID_RUNTIME_FW,
> >>> + * %XE_SIGID_DEVICE_FW) are software signatures: they mark that *the driver*
> >>> + * observed a firmware problem, not a signature reported by the firmware.
> >>> + *
> >>> + * Numbering
> >>> + * ---------
> >>
> >> This section also needs to be on the top. It can be missed if it is at the
> >> bottom of the document.
> > 
> > Also agree.
> > 
> >>
> >>> + *
> >>> + * SIGIDs are a single flat list numbered sequentially within the assigned range,
> >>> + * in the order the situations were introduced. Values are stable: once assigned
> >>> + * they are only ever appended, never renumbered or reused.
> >>> + *
> >>> + * A retired situation is deprecated in place, never re-purposed.
> >>> + *
> >>> + * First-order action (resolution buckets)
> >>> + * ---------------------------------------
> >>> + *
> >>> + * So that a SIGID is actionable on its own, each one is tagged with a coarse
> >>> + * *resolution bucket*: the first thing an operator should do on seeing it. The
> >>> + * bucket is a stable, driver-owned hint; external documentation may refine it,
> >>> + * but the in-tree value always stands on its own. Every new SIGID must pick a
> >>> + * bucket, which forces the question "what should someone do about this?" to be
> >>> + * answered up front. The buckets are::
> >>> + *
> >>> + *   COLLECT  -- capture logs and open a bug report
> >>> + *   RETRY    -- transient or already recovered; watch for recurrence
> >>> + *   UPDATE   -- a firmware update / flash is required
> >>> + *   RECOVER  -- an explicit recovery step is needed (rebind, bus reset)
> >>
> >>
> >> Do we actually need resolution buckets defined here? RECOVER or UPDATE seem
> >> a bit vague since states like
> >>  WEDGED/SURVIVABILITY have different ways to recover depending on context.
> >> Wouldn't detailed resolution steps in another
> >> document be better than in logs?
> > 
> > Fair enough. I would prefer we have some recommendation for a consistent
> > end to end story without depending on external docs and all.
> > However I do agree that the vagueness in some cases here can defeat the
> > purpose and mostly the conflict with the wedge.
> > 
> > Aravind was already complaining about these buckets. So, perhaps let's just
> > remove. But also for consistency we need to change the rest of the text above
> > and below:
> > 
> > - drop "maps to … a recommended first action … 
> > - Delete the whole First-order action (resolution buckets) sectio
> > - Strip the [TAG] from all nine enum entries.
> > - Drop the dmesg note "the bucket … is not printed on the dmesg line.
> > 
> > Michal, what are your thoughts?
> 
> here is updated DOC section, please check if I get it right
> 
> /**
>  * DOC: Xe Error Signatures (SIGID)
>  *
>  * What SIGID stands for
>  * ---------------------
>  *
>  * SIGID is short for *Signature Identifier*. It is a small, stable integer
>  * that names one of *recognised fault site* -- nothing more. It is the
>  * primary handle used for triage and maps directly to specific report site.
>  *
>  * Numbering
>  * ---------
>  *
>  * SIGIDs are a single flat list numbered sequentially within the assigned range,
>  * in the order the fault sites were introduced. Values are stable: once assigned
>  * they are only ever appended, never renumbered or reused. A retired fault site
>  * SIGID value is deprecated in place, never re-purposed.
>  *
>  * Why this exists
>  * ---------------
>  *
>  * Today the driver reports faults with ad-hoc ``xe_err()`` / ``xe_gt_err()``
>  * strings that have no stable shape. That is fine for a human reading dmesg,
>  * but it gives fleet tooling nothing durable to match on: the wording changes
>  * between releases, lines can be rate-limited or dropped under an error storm,
>  * and there is no consistent way to ask "which recognised fault just happened?"
>  *
>  * A SIGID answers exactly that one question, identically across driver and
>  * firmware versions, and (eventually) across other Intel devices in a node.
>  *
>  * What a SIGID is not
>  * -------------------
>  *
>  * SIGID deliberately does not encode the detailed reason or the outcome. Those
>  * are carried alongside it::
>  *
>  *   SIGID    -> which recognised fault site is being reported
>  *   severity -> how serious this instance is
>  *   errno    -> the failing operation's error, if available, shown with %pe
>  *   message  -> free-form human-readable context
>  *
>  * Severity is independent of the SIGID. The same SIGID can be reported at
>  * different severities depending on the instance and the recovery taken.
>  *
>  * When to use SIGID logging
>  * -------------------------
>  *
>  * The xe_log_*() helpers are for these recognised fault sites only --
>  * important, operator-relevant faults and events. The driver's only job is to
>  * emit the right SIGID next to the usual human-readable text.
> 
>  * They are not a replacement for ``xe_info()`` / ``xe_dbg()`` / tracing, nor
>  * for one-off diagnostics; using them for ordinary logging would dilute the
>  * fault stream. Not every ``xe_err()`` needs to become a SIGID report -- only
>  * those that correspond to a published fault sites.
>  *
>  * SIGID log output (dmesg vs. the machine record)
>  * -----------------------------------------------
>  *
>  * The dmesg line stays close to a normal xe error message so it remains
>  * readable for admins; the only stable, machine-matchable token on it is
>  * ``SIGID=<n>`` (``dmesg | grep SIGID=``).
>  *
>  * The full dmesg line is not an ABI: the surrounding text may change freely,
>  * and lines may be dropped. The durable record for tooling is the CPER record
>  * carrying the same SIGID (generation is a planned follow-up).
>  *
>  * How to pick a SIGID (the uniqueness rule)
>  * -----------------------------------------
>  *
>  * Pick per *report site*, not per incident. Each site emits the single most
>  * specific recognised SIGID *for that site* -- so the question is never
>  * "classify this whole failure", it is "what does this site detect?", which has
>  * one answer. A single underlying failure therefore legitimately produces a
>  * *chain* of reports from different layers, each with its own SIGID -- e.g. a
>  * GuC communication failure is reported as %XE_SIGID_RUNTIME_FW by the firmware
>  * path, the failed recovery as %XE_SIGID_GT_TDR by the reset path, and an
>  * aborted bind as %XE_SIGID_PROBE by the probe path. That chain lets triage
>  * follow a fault from origin to final effect; it is not a duplicate.
>  *
>  * If a site does not match any defined SIGID, keep using the ordinary
>  * ``xe_err()`` / ``xe_gt_err()`` logging rather than forcing a SIGID: a wrong
>  * or over-broad classification is harder to retire than a missing one. When a
>  * new report site is genuinely worth triaging, add it to the list below.
>  *
>  * Scope: software vs hardware emitted signatures
>  * ----------------------------------------------
>  *
>  * Some SIGID represents fault sites that the *driver itself* detects and
>  * reports from the software POV: probe abort, wedged, survivability, driver-
>  * detected firmware failures, engine TDR, memory faults and IO/bus faults.
>  * These are the only values the driver assigns on its own.
>  *
>  * Signatures that *originate* in firmware or hardware are a different thing:
>  * they are produced and identified by the firmware or the hardware itself
>  * (e.g. via their own records or error counters), and the driver merely logs
>  * them as they are given to us. They are deliberately enumerated separately.
>  *
>  * The two driver-detected firmware situations below (%XE_SIGID_RUNTIME_FW,
>  * %XE_SIGID_DEVICE_FW) are software signatures: they mark that *the driver*
>  * observed a firmware problem, not a signature reported by the firmware.
>  */

looks good to me. Indeed cleaner...

> 
> 
> > 
> > Thanks,
> > Rodrigo.
> > 
> >>
> >>> + *   IGNORE   -- ignore if the SIGID severity is INFORMATIONAL
> >>> + *
> >>> + * The bucket is documentation only -- it is recorded per SIGID in the enum
> >>> + * kernel-doc below and is not printed on the (deliberately lean) dmesg line.
> >>> + *
> >>> + * When to use SIGID logging
> >>> + * -------------------------
> >>> + *
> >>> + * The xe_log_*() helpers are for these recognised fault situations only --
> >>> + * important, operator-relevant faults and events. They are not a replacement
> >>> + * for ``xe_info()`` / ``xe_dbg()`` / tracing, nor for one-off diagnostics;
> >>> + * using them for ordinary logging would dilute the fault stream. Not every
> >>> + * ``xe_err()`` needs to become a SIGID report -- only those that correspond to
> >>> + * a published situation.
> >>> + *
> >>> + * dmesg vs. the machine record
> >>> + * ----------------------------
> >>> + *
> >>> + * The dmesg line stays close to a normal xe error message so it remains
> >>> + * readable for admins; the only stable, machine-matchable token on it is
> >>> + * ``SIGID=<n>`` (``dmesg | grep SIGID=``). dmesg is not an ABI: the surrounding
> >>> + * text may change freely, and lines may be dropped. The durable record for
> >>> + * tooling is the CPER record carrying the same SIGID (generation is a planned
> >>> + * follow-up).
> >>> + */
> >>> +
> >>> +/*
> >>> + * Top level Intel Error Signature Identifiers.
> >>> + */
> >>> +#define INTEL_SIGID_INVALID			0
> >>> +#define INTEL_SIGID_GPU_START			100
> >>
> >> why does the sigid start from 100?
> 
> there was an offline agreement with Yoni to start GPU SIGIDs from 100
> with the limit up to 999 for any future GPU SIGIDs we may want to have
> 
> >>
> >>> +#define INTEL_SIGID_GPU_END			999
> >>> +
> >>> +#define INTEL_SIGID_GPU_XE_START		100
> >>> +#define INTEL_SIGID_GPU_XE_END			299
> 
> and for the XE we should use range 100..299
> >>> +
> >>> +#define INTEL_SIGID_GPU_XE_SOFTWARE_START	100
> >>> +#define INTEL_SIGID_GPU_XE_SOFTWARE_END		199
> 
> with the explicit split for SW/HW originated 'fault sites'
> 
> >>> +#define INTEL_SIGID_GPU_XE_HARDWARE_START	200
> >>> +#define INTEL_SIGID_GPU_XE_HARDWARE_END		299
> >>> +
> >>> +/**
> >>> + * enum xe_sigid - Stable Xe Error Signature Identifiers (SIGID).
> >>> + * @XE_SIGID_SW: Software component failure. [COLLECT]
> >>> + * @XE_SIGID_PROBE: Device probe/bind was aborted. [COLLECT]
> >>> + * @XE_SIGID_WEDGED: Device was declared wedged and is no longer usable. [RECOVER]
> >>> + * @XE_SIGID_SURVIVABILITY: Device entered survivability mode. [UPDATE]
> >>> + * @XE_SIGID_RUNTIME_FW: Driver-detected runtime firmware failure, GuC/HuC/GSC. [RETRY]
> >>> + * @XE_SIGID_DEVICE_FW: Driver-detected device firmware failure, PCODE/sysctrl. [RETRY]
> >>
> >> Pcode or sysctrl errors cannot be retried. Pcode init failures cause
> >> survivability mode.
> >> RAS sysctrl errors require a secondary bus reset. We could have other
> >> firmwares in future with different
> >> recovery.
> >> That is why it would be better to drop resolution buckets in logs.
> >>
> >> @aravind thoughts?
> >>
> >> Thanks
> >> Riana
> >>