Re: [PATCH v2 00/22] drm/xe: Add structured SIGID error logging infrastructure

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

On Wed, Jul 29, 2026 at 11:31:46AM +0300, Jani Nikula wrote:
> On Tue, 28 Jul 2026, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote:
> > 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.
> >
> > Design decisions:
> >
> >  - Software-emitted signatures only. This header enumerates just the
> >    situations the driver detects and reports itself. Signatures that
> >    originate in firmware or hardware are identified by those layers (via
> >    their own records/counters) and are logged as received -- minting a
> >    driver-side id for them would duplicate an id the reporting layer
> >    already owns.
> >
> >  - Flat catalogue, chosen per report site. Each site emits the single
> >    most specific situation for that site, so a multi-layer failure
> >    produces a chain of reports rather than one ambiguous classification
> >    (e.g. a failed GT reset reports GT_TDR and then WEDGED). A site that
> >    matches no defined situation keeps using ordinary xe_err() /
> >    xe_gt_err() rather than forcing a wrong id.
> >
> >  - Stable numbering. A single flat list numbered sequentially from 1, in
> >    introduction order. Values are only ever appended, never renumbered
> >    or reused.
> >
> >  - First-order action. Each SIGID carries a coarse, in-tree resolution
> >    bucket (COLLECT / RETRY / UPDATE / RECOVER) so it is actionable
> >    without an external reference, and so every new id must declare what
> >    to do about it.
> >
> >  - Severity is decoupled from the SIGID and chosen at the call site via
> >    xe_ras_log_fatal() / _recoverable() / _info(); the same situation can
> >    be reported at different severities depending on the instance.
> >
> >  - dmesg stays close to a normal xe error line by reusing xe_err() /
> >    xe_gt_err() (and their Tile/GT decoration); the only stable,
> >    machine-matchable token added is SIGID=<n>. dmesg is not an ABI --
> >    the durable machine record is the CPER carrying the same SIGID (a
> >    planned follow-up, left as a TODO).
> >
> > Wire up a representative site for each software signature so the set is
> > exercised rather than merely declared:
> >
> >  - PROBE: xe workqueue allocation failure during early init
> >  - WEDGED: xe_device_declare_wedged() (drop redundant "CRITICAL" + BDF)
> >  - SURVIVABILITY: entering boot survivability mode
> >  - RUNTIME_FW: GuC mmio request failure
> >  - DEVICE_FW: PCODE mailbox failure
> >  - GT_TDR: GT reset failure (which then chains into a WEDGED report)
> >  - MEM_FAULT: page-fault queue overflow
> >  - IO_BUS: PCI re-enable failure after a bus reset
> 
> The idea is not new, see for example [1] and [2]. I'm not sure if that
> was ever merged, though. Would be good to know what came of it, and why.
> 
> There's also the printk index support already in the kernel [3]. Did you
> look into that? If not, please do. If yes, please iterate why that can't
> be used, or extended, and a local mechanism is required.

Well, I do agree with your feeling here. When this request first came to us,
it was framed similarly to the two older proposals you referenced.
I pushed back making it very clear that our kernel log is not an ABI and
it will never be... that our debug messages are for our own developers to
consume and edit as the code evolves. So, I refused and blocked any
attempt to modify all the messages and force them to follow this format.

So, this evolved a bit since. I hope. We have agreed that the ABI itself
is not the dmesg, but the CPER that is emitted on tracefs... that's the
ultimate goal. The message here is just an extra helper and it brings
both our CPER/tracefs and the dmesg helper. So, the printk index cannot
be used here. It has no mechanism to influence or annotate what goes
into CPER records. And even if we modified it, its volatile number
doesn't work with the CPER goals.

The goal is to have this SIGID mostly identifying FW/HW stuff and a few
of the key points that could help providing a guidance/map to a recovery
path. With an intentionally narrow scope.

> 
> FWIW, I think merging driver local mechanisms for what should really be
> generic kernel stuff usually ends up being a maintenance PITA. It's easy
> in the beginning, a burden in the end, and even harder to migrate to
> generic mechanisms. We've seen this over and over in i915, even for
> superficially small things. Part of the reason for xe existing in the
> first place is to use common stuff instead of reinventing the weels.

I agree with you as well on the pain point of maintaining this in the
driver and this is why I liked Michal's macros. At the same time, I do
agree that the macros could be improved for easy cscope usage indeed too.

I also agree with the sentiment that it will be even harder to migrate
to generic mechanisms if needed. But honestly, I doubt that this specific
Intel GPU IDs will go beyond our GPU CPERs. So, let's just contain this
in the mapping macros here for now, use in a very few key places and
move forward.

Thanks,
Rodrigo.

> 
> BR,
> Jani.
> 
> 
> [1] https://lwn.net/Articles/464276/
> [2] https://lwn.net/Articles/492125/
> [3] https://lkml.org/lkml/2021/6/15/1053
> 
> >
> > Signed-off-by: Mallesh Koujalagi <mallesh.koujalagi@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > Cc: Matthew Brost <matthew.brost@intel.com>
> > Cc: Aravind Iddamsetty <aravind.iddamsetty@intel.com>
> > Cc: Riana Tauro <riana.tauro@intel.com>
> > Cc: Raag Jadav <raag.jadav@intel.com>
> > Cc: Badal Nilawar <badal.nilawar@intel.com>
> >
> > v1: https://patchwork.freedesktop.org/series/171022/#rev1
> > v2: component/location dmesg decorations moved away from macros
> >     HW component identifiers related to RAS/FW enum values
> >     CORRECTED severity rendered as an error
> >     more support for const pointers
> >     log 'probe blocked' as INFO
> >     fix kunit device setup
> >     more tests
> >
> > Mallesh Koujalagi (1):
> >   drm/xe/log: Add structured SIGID error logging infrastructure
> >
> > Michal Wajdeczko (21):
> >   drm/xe: Introduce xe_any helpers
> >   drm/xe/log: Introduce structured component/location identifiers
> >   drm/xe/log: Add component/location decorations to dmesg
> >   drm/xe/log: Add SIGID log helpers for severity
> >   drm/xe/log: Add SIGID log helpers for location
> >   drm/xe/log: Add SIGID log helpers for location & severity
> >   drm/xe/log: Add SIGID log helpers for components
> >   drm/xe/log: Add SIGID log helpers for errno-only
> >   drm/xe/log: Add hardware error signatures
> >   drm/xe/log: Extend components list with hardware items
> >   drm/xe/ras: Check RAS and LOG component definitions
> >   drm/xe/kunit: Setup driver data in the test device
> >   drm/xe/tests: Add Kunit tests for xe_log
> >   drm/xe: Report 'probe blocked' error using SIGID
> >   drm/xe: Report 'device wedged' errors using SIGID
> >   drm/xe: Report 'Survivability Mode' errors using SIGID
> >   drm/xe/guc: Report 'GuC mmio' errors using SIGID
> >   drm/xe/pcode: Report 'Mailbox failed' error using SIGID
> >   drm/xe/gt: Report 'reset failed' errors using SIGID
> >   drm/xe/gt: Report 'pagefault' errors using SIGID
> >   drm/xe/pci: Report 'cannot re-enable' error using SIGID
> >
> >  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_log_abi.h         | 197 ++++++++
> >  drivers/gpu/drm/xe/abi/xe_sigid_abi.h       | 195 ++++++++
> >  drivers/gpu/drm/xe/tests/xe_kunit_helpers.c |   4 +
> >  drivers/gpu/drm/xe/tests/xe_log_kunit.c     | 480 ++++++++++++++++++++
> >  drivers/gpu/drm/xe/xe_any.h                 | 112 +++++
> >  drivers/gpu/drm/xe/xe_device.c              |  16 +-
> >  drivers/gpu/drm/xe/xe_gt.c                  |   7 +-
> >  drivers/gpu/drm/xe/xe_guc.c                 |  18 +-
> >  drivers/gpu/drm/xe/xe_log.c                 | 212 +++++++++
> >  drivers/gpu/drm/xe/xe_log.h                 | 158 +++++++
> >  drivers/gpu/drm/xe/xe_pagefault.c           |  10 +-
> >  drivers/gpu/drm/xe/xe_pci.c                 |   5 +-
> >  drivers/gpu/drm/xe/xe_pci_error.c           |   7 +-
> >  drivers/gpu/drm/xe/xe_pcode.c               |   5 +-
> >  drivers/gpu/drm/xe/xe_ras.c                 |  11 +
> >  drivers/gpu/drm/xe/xe_survivability_mode.c  |  24 +-
> >  19 files changed, 1438 insertions(+), 39 deletions(-)
> >  create mode 100644 Documentation/gpu/xe/xe_sigid.rst
> >  create mode 100644 drivers/gpu/drm/xe/abi/xe_log_abi.h
> >  create mode 100644 drivers/gpu/drm/xe/abi/xe_sigid_abi.h
> >  create mode 100644 drivers/gpu/drm/xe/tests/xe_log_kunit.c
> >  create mode 100644 drivers/gpu/drm/xe/xe_any.h
> >  create mode 100644 drivers/gpu/drm/xe/xe_log.c
> >  create mode 100644 drivers/gpu/drm/xe/xe_log.h
> 
> -- 
> Jani Nikula, Intel