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

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

On Thu, Aug 06, 2026 at 03:10:04PM -0400, Summers, Stuart wrote:
> On Thu, 2026-08-06 at 13:31 +0200, Michal Wajdeczko wrote:
> > 
> > 
> > On 8/6/2026 12:24 AM, Summers, Stuart wrote:
> > > On Tue, 2026-08-04 at 21:36 -0400, Rodrigo Vivi wrote:
> > > > On Tue, Aug 04, 2026 at 05:21:05PM -0400, 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
> > > > 
> > > > I understand your feeling. We've been all through that:
> > > > 
> > > > https://lore.kernel.org/intel-xe/amqhoFzQaf1HsuFq@intel.com/
> > > > 
> > > > > we're introducing some ABI with this that isn't
> > > > > really maintainable in the long term:
> > > > 
> > > > I understand the fear and indeed the first proposals I got was
> > > > unmaintainable. My first record of pushing back on having
> > > > something
> > > > like this was November last year.
> > > > 
> > > > But I respectfully disagree here. This latest version is imho
> > > > organized and concise.
> > > > 
> > > > > 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.
> > > > 
> > > > You are right, dmesg is not ABI and it will never be. these logs
> > > > are aimed for developers and developers are free to change them
> > > > as
> > > > needed. This was a big counter-requirement I gave to the original
> > > > idea.
> > > > 
> > > > We are not moving all the logs to this format we are not
> > > > promising
> > > > dmesg stability.
> > > > 
> > > > The numbering stability however needs to be somewhat stable for
> > > > the CPER log in tracefs, that's the ABI. But then that meaning
> > > > shouldn't change if the code has to change. A new number should
> > > > be needed if the component/location/severity or recommended
> > > > recovery needs to be different.
> > > > 
> > > > But like I told Raag as well, no developer needs to invent any
> > > > number, if they don't know just use regular log messages.
> > > > We are not going to move all the logs towards this thing.
> > > > Also, the location of the issue is what triggers the ID...
> > > > it is very simple by nature. And we need to keep it simple.
> > > > 
> > > > > Does this lock us into a solution for all of this? I still need
> > > > > to go through the full patch series...
> > > > 
> > > > Yes, please take a look to the series. All reviews are welcomed.
> > > > 
> > > > > 
> > > > > 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.
> > > > 
> > > > We are not promising this. The stable ABI is only the CPER on
> > > > tracefs.
> > > > 
> > > > We need to always keep this in mind as stated in
> > > > Documentation/core-api/printk-index.rst:
> > > > 
> > > > """
> > > > The kernel messages are evolving together with the code. As a
> > > > result,
> > > > particular kernel messages are not KABI and never will be!
> > > > """
> > > > 
> > > > Thanks,
> > > > Rodrigo.
> > > > 
> > > > > 
> > > > > 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).
> > > 
> > > Ok I realize I'm coming late to the party here - I just haven't had
> > > the
> > 
> > (no worries, I was late too)
> > 
> > > time to review this in detail. I really don't like having this ABI-
> > > adjacent implementation. It feels like we will be on the hook for
> > > maintaining things in the future that will limit our ability to
> > > implement changes and debug. I get the notes that Rodrigo has
> > > above,
> > > but this just feels like the wrong approach to me.
> > > 
> > > That said, I don't want to block the work here. I know we have some
> > > users looking for this for their own debug.
> > > 
> > > You have "dmesg is not ABI" here which is a start. What happens if
> > > we
> > > decide to drop one of these messages? 
> > 
> > we don't require any use of xe_log() to be permanent (see below)
> > it can changed or dropped or replaced back with xe_err() any time.
> > 
> > > Is it only the ID itself that we
> > > want to be stable and monotonically incrementing? 
> > 
> > correct, just use of the same SIGID in the log will always mean the
> > same
> > 
> > > Or is the message
> > > itself supposed to be stable? 
> > 
> > message can be changed any time, like in regular xe_err()
> > 
> > it is just an additional hint for debug (together with SEVERITY
> > and reported errno)
> > 
> > > If we drop all references to a particular
> > > ID is that ok? 
> > 
> > yes, if the ID is no longer applicable
> > and this was already stated in DOC
> > 
> > > What if we have 10s of IDs or more that have no use in
> > > the future and we move on to the next section? We don't care about
> > > cleanup of this kind of thing? 
> > 
> > legacy SIGIDs stays forever, there will be no ID value reuse
> > (also see DOC) but due to the way they are defined, it is unlikely
> > that we will drop them
> > 
> > > Or the line above about "dmesg is not
> > > ABI" means we can really do whatever we want with it?
> > 
> > I assume the only new requirement for us would be that we
> > should not drop all xe_logs for any SIGID which is still
> > applicable (and was not replaced with other SIGID)

Well, if the code changes in a way that the function doesn't
exist anymore, what should we do?
We cannot guarantee that.

> 
> It would be nice to make that explicit in the documentation.

Right, we probably need some explicit mention about this case.
But we shouldn't commit to not remove a log line.

> 
> I'm still pretty worried about the contractual aspect of this with
> users who start relying on this. But if we take that out, from a purely
> kernel debug usage aspect, I do like having this to help categorize
> issues and flows. It seems like we should be able to do something with
> this in printk directly (or the drm_* variants) rather than having
> something specific to the GPU here. But as long as we don't make it too
> strict, hopefully we can expand if there's interest there.
> 
> Let me go through in detail later today and get back.

Yes please! :)

> 
> Thanks,
> Stuart
> 
> > 
> > > 
> > > Thanks,
> > > Stuart
> > > 
>