On Tue, 2025-07-15 at 16:17 +0530, Riana Tauro wrote:
> Gfx device reports two classes of errors: uncorrectable and
> correctable. Depending on the severity uncorrectable errors are
> further
> classified Non-Fatal and Fatal
>
> Correctable and Non-Fatal errors: These errors are reported as MSI.
> Bits in
> the Master Interrupt Register indicate the class of the error.
> The source of the error is then read from the Device Error Source
> Register.
>
> Fatal errors: These are reported as PCIe errors
> When a PCIe error is asserted, the OS will perform a SBR (Secondary
> Bus reset) which causes the driver to reload.
> The error registers are sticky and the values are maintained through
> SBR
>
> Add basic support to handle these errors
>
> Bspec: 50875, 53073, 53074, 53075, 53076
>
> v2: Remove platform checks and print raw error value (Stuart)
> Format commit message (Umesh)
>
> Cc: Stuart Summers <stuart.summ...@intel.com>
> Co-developed-by: Himal Prasad Ghimiray
> <himal.prasad.ghimi...@intel.com>
> Signed-off-by: Himal Prasad Ghimiray
> <himal.prasad.ghimi...@intel.com>
> Signed-off-by: Riana Tauro <riana.ta...@intel.com>
> Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.rama...@intel.com>
> ---
> drivers/gpu/drm/xe/Makefile | 1 +
> drivers/gpu/drm/xe/regs/xe_hw_error_regs.h | 15 +++
> drivers/gpu/drm/xe/regs/xe_irq_regs.h | 1 +
> drivers/gpu/drm/xe/xe_hw_error.c | 106
> +++++++++++++++++++++
> drivers/gpu/drm/xe/xe_hw_error.h | 15 +++
> drivers/gpu/drm/xe/xe_irq.c | 4 +
> 6 files changed, 142 insertions(+)
> create mode 100644 drivers/gpu/drm/xe/regs/xe_hw_error_regs.h
> create mode 100644 drivers/gpu/drm/xe/xe_hw_error.c
> create mode 100644 drivers/gpu/drm/xe/xe_hw_error.h
>
> diff --git a/drivers/gpu/drm/xe/Makefile
> b/drivers/gpu/drm/xe/Makefile
> index 07c71a29963d..02f73dffec55 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -80,6 +80,7 @@ xe-y += xe_bb.o \
> xe_hw_engine.o \
> xe_hw_engine_class_sysfs.o \
> xe_hw_engine_group.o \
> + xe_hw_error.o \
> xe_hw_fence.o \
> xe_irq.o \
> xe_lrc.o \
> diff --git a/drivers/gpu/drm/xe/regs/xe_hw_error_regs.h
> b/drivers/gpu/drm/xe/regs/xe_hw_error_regs.h
> new file mode 100644
> index 000000000000..ed9b81fb28a0
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/regs/xe_hw_error_regs.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2025 Intel Corporation
> + */
> +
> +#ifndef _XE_HW_ERROR_REGS_H_
> +#define _XE_HW_ERROR_REGS_H_
> +
> +#define DEV_ERR_STAT_NONFATAL 0x100178
> +#define DEV_ERR_STAT_CORRECTABLE 0x10017c
> +#define
> DEV_ERR_STAT_REG(x) XE_REG(_PICK_EVEN((x), \
> +
> DEV_ERR_STAT_CORRECTABLE, \
> +
> DEV_ERR_STAT_NONFATAL))
> +
> +#endif
> diff --git a/drivers/gpu/drm/xe/regs/xe_irq_regs.h
> b/drivers/gpu/drm/xe/regs/xe_irq_regs.h
> index 13635e4331d4..7c2a3a140142 100644
> --- a/drivers/gpu/drm/xe/regs/xe_irq_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_irq_regs.h
> @@ -18,6 +18,7 @@
> #define GFX_MSTR_IRQ XE_REG(0x190010,
> XE_REG_OPTION_VF)
> #define MASTER_IRQ REG_BIT(31)
> #define GU_MISC_IRQ REG_BIT(29)
> +#define ERROR_IRQ(x) REG_BIT(26 + (x))
> #define DISPLAY_IRQ REG_BIT(16)
> #define I2C_IRQ REG_BIT(12)
> #define GT_DW_IRQ(x) REG_BIT(x)
> diff --git a/drivers/gpu/drm/xe/xe_hw_error.c
> b/drivers/gpu/drm/xe/xe_hw_error.c
> new file mode 100644
> index 000000000000..0b09f9950cb9
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_hw_error.c
> @@ -0,0 +1,106 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2025 Intel Corporation
> + */
> +
> +#include "regs/xe_hw_error_regs.h"
> +#include "regs/xe_irq_regs.h"
> +
> +#include "xe_device.h"
> +#include "xe_hw_error.h"
> +#include "xe_mmio.h"
> +
> +/* Error categories reported by hardware */
> +enum hardware_error {
> + HARDWARE_ERROR_CORRECTABLE = 0,
> + HARDWARE_ERROR_NONFATAL = 1,
> + HARDWARE_ERROR_FATAL = 2,
> + HARDWARE_ERROR_MAX,
> +};
> +
> +static const char *hw_error_to_str(const enum hardware_error hw_err)
> +{
> + switch (hw_err) {
> + case HARDWARE_ERROR_CORRECTABLE:
> + return "CORRECTABLE";
> + case HARDWARE_ERROR_NONFATAL:
> + return "NONFATAL";
> + case HARDWARE_ERROR_FATAL:
> + return "FATAL";
> + default:
> + return "UNKNOWN";
> + }
> +}
> +
> +static void hw_error_source_handler(struct xe_tile *tile, const enum
> hardware_error hw_err)
> +{
> + const char *hw_err_str = hw_error_to_str(hw_err);
> + struct xe_device *xe = tile_to_xe(tile);
> + unsigned long flags;
> + u32 err_src;
> +
> + spin_lock_irqsave(&xe->irq.lock, flags);
> + err_src = xe_mmio_read32(&tile->mmio,
> DEV_ERR_STAT_REG(hw_err));
> + if (!err_src) {
> + drm_err_ratelimited(&xe->drm, HW_ERR "Tile%d reported
> DEV_ERR_STAT_%s blank!\n",
> + tile->id, hw_err_str);
This message is essentially the same as the one below, just with
"blank" instead of "0x0". And writing back a 0 IMO is inconsequential
since this already is some kind of hardware bug (bad interrupt, MMIO
write error, etc) if we get to this point.
Can we save a few lines and get rid of this if check? So the function
just becomes:
spin_lock()
read_mmio()
print_results()
write_mmio()
spin_unlock()
Without any goto or extra check.
> + goto unlock;
> + }
> +
> + drm_err_ratelimited(&xe->drm, HW_ERR "Tile%d reported %s
> error 0x%x\n",
> + tile->id, hw_err_str, err_src);
> +
> + xe_mmio_write32(&tile->mmio, DEV_ERR_STAT_REG(hw_err),
> err_src);
> +
> +unlock:
> + spin_unlock_irqrestore(&xe->irq.lock, flags);
> +}
> +
> +/**
> + * xe_hw_error_irq_handler - irq handling for hw errors
> + * @tile: tile instance
> + * @master_ctl: value read from master interrupt register
> + *
> + * Xe platforms add three error bits to the master interrupt
> register to support error handling.
> + * These three bits are used to convey the class of error FATAL,
> NONFATAL, or CORRECTABLE.
> + * To process the interrupt, determine the source of error by
> reading the Device Error Source
> + * Register that corresponds to the class of error being serviced.
> + */
> +void xe_hw_error_irq_handler(struct xe_tile *tile, const u32
> master_ctl)
> +{
> + enum hardware_error hw_err;
> +
> + for (hw_err = 0; hw_err < HARDWARE_ERROR_MAX; hw_err++)
> + if (master_ctl & ERROR_IRQ(hw_err))
> + hw_error_source_handler(tile, hw_err);
> +}
> +
> +/*
> + * Process hardware errors during boot
> + */
> +static void process_hw_errors(struct xe_device *xe)
> +{
> + struct xe_tile *tile;
> + u32 master_ctl;
> + u8 id;
> +
> + for_each_tile(tile, xe, id) {
> + master_ctl = xe_mmio_read32(&tile->mmio,
> GFX_MSTR_IRQ);
> + xe_hw_error_irq_handler(tile, master_ctl);
> + xe_mmio_write32(&tile->mmio, GFX_MSTR_IRQ,
> master_ctl);
> + }
> +}
> +
> +/**
> + * xe_hw_error_init - Initialize hw errors
> + * @xe: xe device instance
> + *
> + * Initialize and process hw errors
> + */
> +void xe_hw_error_init(struct xe_device *xe)
> +{
> + if (IS_SRIOV_VF(xe))
> + return;
> +
Maybe a comment here as to why we are doing this on init? Something
like:
/* Check for errors that occurred during boot prior to driver load */
Thanks,
Stuart
> + process_hw_errors(xe);
> +}
> diff --git a/drivers/gpu/drm/xe/xe_hw_error.h
> b/drivers/gpu/drm/xe/xe_hw_error.h
> new file mode 100644
> index 000000000000..d86e28c5180c
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_hw_error.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2025 Intel Corporation
> + */
> +#ifndef XE_HW_ERROR_H_
> +#define XE_HW_ERROR_H_
> +
> +#include <linux/types.h>
> +
> +struct xe_tile;
> +struct xe_device;
> +
> +void xe_hw_error_irq_handler(struct xe_tile *tile, const u32
> master_ctl);
> +void xe_hw_error_init(struct xe_device *xe);
> +#endif
> diff --git a/drivers/gpu/drm/xe/xe_irq.c
> b/drivers/gpu/drm/xe/xe_irq.c
> index 5df5b8c2a3e4..870edaf69388 100644
> --- a/drivers/gpu/drm/xe/xe_irq.c
> +++ b/drivers/gpu/drm/xe/xe_irq.c
> @@ -18,6 +18,7 @@
> #include "xe_gt.h"
> #include "xe_guc.h"
> #include "xe_hw_engine.h"
> +#include "xe_hw_error.h"
> #include "xe_i2c.h"
> #include "xe_memirq.h"
> #include "xe_mmio.h"
> @@ -468,6 +469,7 @@ static irqreturn_t dg1_irq_handler(int irq, void
> *arg)
> xe_mmio_write32(mmio, GFX_MSTR_IRQ, master_ctl);
>
> gt_irq_handler(tile, master_ctl, intr_dw, identity);
> + xe_hw_error_irq_handler(tile, master_ctl);
>
> /*
> * Display interrupts (including display backlight
> operations
> @@ -756,6 +758,8 @@ int xe_irq_install(struct xe_device *xe)
> int nvec = 1;
> int err;
>
> + xe_hw_error_init(xe);
> +
> xe_irq_reset(xe);
>
> if (xe_device_has_msix(xe)) {
