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

["Summers, Stuart" <stuart.summers@intel.com>]

On Tue, 2026-08-04 at 21:21 +0000, Summers, Stuart wrote:
> On Thu, 2026-07-30 at 17:20 +0200, 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.
> 
> I'm a little worried we're introducing some ABI with this that isn't
> really maintainable in the long term: we might decide to change the
> flow or change the way an error is reported or the situation that
> triggers this error from firmware or hardware might change for some
> reason. Does this lock us into a solution for all of this? I still
> need
> to go through the full patch series...
> 
> The dmesg entries are generally for human debuggability. I get the
> desire to make these easier to parse for an AI tool or generated
> script, but we also don't want to prevent debug related changes for
> error handling and reporting.

Also, should we be implementing something like this more generically
across DRM or even outside of DRM and then having device-specific error
conditions within a particular range?

Thanks,
Stuart

> 
> Thanks,
> Stuart
> 
> > 
> > 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
> > + * 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
> > + * 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).
> > + *
> > + * 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
> > + * ---------
> > + *
> > + * 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)
> > + *   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
> > +#define INTEL_SIGID_GPU_END                    999
> > +
> > +#define INTEL_SIGID_GPU_XE_START               100
> > +#define INTEL_SIGID_GPU_XE_END                 299
> > +
> > +#define INTEL_SIGID_GPU_XE_SOFTWARE_START      100
> > +#define INTEL_SIGID_GPU_XE_SOFTWARE_END                199
> > +#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]
> > + * @XE_SIGID_GT_TDR: Engine hang / timeout detection and recovery
> > (reset). [RETRY]
> > + * @XE_SIGID_MEM_FAULT: VM bind, page fault or GTT fault.
> > [COLLECT]
> > + * @XE_SIGID_IO_BUS: Runtime PCIe / IOMMU / MMIO access fault.
> > [RECOVER]
> > + *
> > + * The situations the driver detects and reports in software.
> > Values
> > are
> > + * numbered sequentially, are only ever appended, and are never
> > renumbered or
> > + * reused. The tag in brackets is the default resolution bucket
> > (see
> > the `Xe
> > + * Error Signatures (SIGID)`_ section).
> > + *
> > + * Firmware- and hardware-originated signatures are not listed
> > here;
> > they are
> > + * logged as reported by those layers.
> > + */
> > +enum xe_sigid {
> > +       XE_SIGID_SW                     =
> > INTEL_SIGID_GPU_XE_SOFTWARE_START,
> > +       XE_SIGID_PROBE                  =
> > INTEL_SIGID_GPU_XE_SOFTWARE_START + 1,
> > +       XE_SIGID_WEDGED                 =
> > INTEL_SIGID_GPU_XE_SOFTWARE_START + 2,
> > +       XE_SIGID_SURVIVABILITY          =
> > INTEL_SIGID_GPU_XE_SOFTWARE_START + 3,
> > +       XE_SIGID_RUNTIME_FW             =
> > INTEL_SIGID_GPU_XE_SOFTWARE_START + 4,
> > +       XE_SIGID_DEVICE_FW              =
> > INTEL_SIGID_GPU_XE_SOFTWARE_START + 5,
> > +       XE_SIGID_GT_TDR                 =
> > INTEL_SIGID_GPU_XE_SOFTWARE_START + 6,
> > +       XE_SIGID_MEM_FAULT              =
> > INTEL_SIGID_GPU_XE_SOFTWARE_START + 7,
> > +       XE_SIGID_IO_BUS                 =
> > INTEL_SIGID_GPU_XE_SOFTWARE_START + 8,
> > +};
> > +
> > +#endif
> > diff --git a/drivers/gpu/drm/xe/xe_log.c
> > b/drivers/gpu/drm/xe/xe_log.c
> > new file mode 100644
> > index 000000000000..70a41bdf1a01
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xe/xe_log.c
> > @@ -0,0 +1,135 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2026 Intel Corporation
> > + */
> > +
> > +#include "xe_log.h"
> > +#include "xe_printk.h"
> > +
> > +static void log_emit_cper(struct pci_dev *pdev, int cper_sev, enum
> > xe_sigid sigid,
> > +                         u32 component, u32 location, const void
> > *data, size_t len,
> > +                         struct va_format *vaf)
> > +{
> > +       /* TODO */
> > +}
> > +
> > +static bool is_hw_sigid(enum xe_sigid sigid)
> > +{
> > +       return (int)sigid >= INTEL_SIGID_GPU_XE_HARDWARE_START;
> > +}
> > +
> > +static bool is_sev_error(int cper_sev)
> > +{
> > +       return cper_sev != CPER_SEV_INFORMATIONAL;
> > +}
> > +
> > +static const char *log_hwe_prefix(int cper_sev, enum xe_sigid
> > sigid)
> > +{
> > +       return is_sev_error(cper_sev) && is_hw_sigid(sigid) ?
> > HW_ERR
> > : "";
> > +}
> > +
> > +static const char *log_sev_prefix(int cper_sev)
> > +{
> > +       switch (cper_sev) {
> > +       case CPER_SEV_FATAL:
> > +               return "FATAL ";
> > +       case CPER_SEV_RECOVERABLE:
> > +               return "";
> > +       case CPER_SEV_CORRECTED:
> > +               return "CORRECTED ";
> > +       default:
> > +               return "";
> > +       }
> > +}
> > +
> > +#define __LOG_DRM_PRINTK_FMT(fmt, args...)     "[drm] " fmt,
> > ##args
> > +#define __LOG_DRM_PRINTK_ERR_FMT(fmt,
> > args...) __LOG_DRM_PRINTK_FMT("*ERROR* " fmt, args)
> > +
> > +static void log_dmesg_vprintk(struct pci_dev *pdev, int cper_sev,
> > struct va_format *vaf)
> > +{
> > +       if (cper_sev == CPER_SEV_INFORMATIONAL)
> > +               pci_info(pdev, __LOG_DRM_PRINTK_FMT("%pV", vaf));
> > +       else
> > +               pci_err(pdev, __LOG_DRM_PRINTK_ERR_FMT("%pV",
> > vaf));
> > +}
> > +
> > +static void log_dmesg_printf(struct pci_dev *pdev, int cper_sev,
> > const char *fmt, ...)
> > +{
> > +       struct va_format vaf;
> > +       va_list args;
> > +
> > +       va_start(args, fmt);
> > +       vaf.fmt = fmt;
> > +       vaf.va = &args;
> > +
> > +       log_dmesg_vprintk(pdev, cper_sev, &vaf);
> > +
> > +       va_end(args);
> > +}
> > +
> > +static void log_emit_dmesg(struct pci_dev *pdev, int cper_sev,
> > enum
> > xe_sigid sigid,
> > +                          u32 component, u32 location, const void
> > *data, size_t len,
> > +                          struct va_format *vaf)
> > +{
> > +       const char *hwe_prefix = log_hwe_prefix(cper_sev, sigid);
> > +       const char *sev_prefix = log_sev_prefix(cper_sev);
> > +
> > +       /* TODO: add component/location details */
> > +
> > +       if (IS_ERR(data))
> > +               log_dmesg_printf(pdev, cper_sev, "SIGID=%u %s(%pe)
> > %s%pV",
> > +                                sigid, sev_prefix, data,
> > hwe_prefix,
> > vaf);
> > +       else if (data && len)
> > +               log_dmesg_printf(pdev, cper_sev, "SIGID=%u
> > %s(%*phN)
> > %s%pV",
> > +                                sigid, sev_prefix, (int)len, data,
> > hwe_prefix, vaf);
> > +       else
> > +               log_dmesg_printf(pdev, cper_sev, "SIGID=%u
> > %s%s%pV",
> > +                                sigid, sev_prefix, hwe_prefix,
> > vaf);
> > +}
> > +
> > +/**
> > + * xe_log_emit() - Emit a structured SIGID log entry
> > + * @pdev: the &pci_dev device
> > + * @cper_sev: CPER severity (CPER_SEV_FATAL, CPER_SEV_RECOVERABLE,
> > ...)
> > + * @sigid: signature identifier, see &enum xe_sigid
> > + * @component: component identifer
> > + * @location: location details of the @component
> > + * @data: pointer to the additional details, or ERR_PTR, or NULL
> > if
> > not applicable
> > + * @len: length of the @data in bytes, or 0 if not applicable
> > + * @fmt: printf-style format string
> > + * @...: format arguments
> > + *
> > + * Emits a dmesg line that includes a single stable, machine-
> > matchable token
> > + * ``SIGID=<n>`` followed by the optional severity token (like
> > ``FATAL``) and,
> > + * when @data pointer is set, either the error printed with %pe or
> > a
> > packed hex
> > + * dump of the @data binary blob. The dmesg line will also include
> > printf-style
> > + * text message.
> > + *
> > + * Note that the full dmesg line, with the free text message, is
> > only a debugging
> > + * aid, not an interface! Only the ``SIGID=<n>`` token is stable
> > there.
> > + * The durable machine record is the CPER carrying the same SIGID.
> > + *
> > + * Note: generation of the CPER record is a planned follow-up.
> > + *
> > + * 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] *ERROR* 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);
> > +
> > +       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
>