Re: [PATCH v4 8/9] drm/xe/xe_hw_error: Handle CSC Firmware reported Hardware errors

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Wed, Jul 09, 2025 at 04:50:20PM +0530, Riana Tauro wrote:
>Add support to handle CSC firmware reported errors. When CSC firmware
>errors are encoutered, a error interrupt is received by the GFX device as
>a MSI interrupt.
>
>Device Source control registers indicates the source of the error as CSC
>The HEC error status register indicates that the error is firmware reported
>Depending on the type of error, the error cause is written to the HEC
>Firmware error register.
>
>On encountering such CSC firmware errors, the graphics device is
>non-recoverable from driver context. The only way to recover from these
>errors is firmware flash. The device is then wedged and userspace is
>notified with a drm uevent
>
>v2: use vendor recovery method with
>    runtime survivability (Christian, Rodrigo, Raag)
>
>v3: move declare wedged to runtime survivability mode (Rodrigo)
>
>Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>---
> drivers/gpu/drm/xe/regs/xe_gsc_regs.h      |  2 +
> drivers/gpu/drm/xe/regs/xe_hw_error_regs.h |  7 ++-
> drivers/gpu/drm/xe/xe_device_types.h       |  3 +
> drivers/gpu/drm/xe/xe_hw_error.c           | 68 +++++++++++++++++++++-
> 4 files changed, 78 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/regs/xe_gsc_regs.h b/drivers/gpu/drm/xe/regs/xe_gsc_regs.h
>index 9b66cc972a63..180be82672ab 100644
>--- a/drivers/gpu/drm/xe/regs/xe_gsc_regs.h
>+++ b/drivers/gpu/drm/xe/regs/xe_gsc_regs.h
>@@ -13,6 +13,8 @@
>
> /* Definitions of GSC H/W registers, bits, etc */
>
>+#define BMG_GSC_HECI1_BASE	0x373000
>+
> #define MTL_GSC_HECI1_BASE	0x00116000
> #define MTL_GSC_HECI2_BASE	0x00117000
>
>diff --git a/drivers/gpu/drm/xe/regs/xe_hw_error_regs.h b/drivers/gpu/drm/xe/regs/xe_hw_error_regs.h
>index ed9b81fb28a0..c146b9ef44eb 100644
>--- a/drivers/gpu/drm/xe/regs/xe_hw_error_regs.h
>+++ b/drivers/gpu/drm/xe/regs/xe_hw_error_regs.h
>@@ -6,10 +6,15 @@
> #ifndef _XE_HW_ERROR_REGS_H_
> #define _XE_HW_ERROR_REGS_H_
>
>+#define HEC_UNCORR_ERR_STATUS(base)                    XE_REG((base) + 0x118)
>+#define    UNCORR_FW_REPORTED_ERR                      BIT(6)
>+
>+#define HEC_UNCORR_FW_ERR_DW0(base)                    XE_REG((base) + 0x124)
>+
> #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))
>-
>+#define   XE_CSC_ERROR				BIT(17)
> #endif
>diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>index ca300338e8c2..283d5c88758e 100644
>--- a/drivers/gpu/drm/xe/xe_device_types.h
>+++ b/drivers/gpu/drm/xe/xe_device_types.h
>@@ -241,6 +241,9 @@ struct xe_tile {
> 	/** @memirq: Memory Based Interrupts. */
> 	struct xe_memirq memirq;
>
>+	/** @csc_hw_error_work: worker to report CSC HW errors */
>+	struct work_struct csc_hw_error_work;
>+
> 	/** @pcode: tile's PCODE */
> 	struct {
> 		/** @pcode.lock: protecting tile's PCODE mailbox data */
>diff --git a/drivers/gpu/drm/xe/xe_hw_error.c b/drivers/gpu/drm/xe/xe_hw_error.c
>index 0f2590839900..7cc9b8a7fa1a 100644
>--- a/drivers/gpu/drm/xe/xe_hw_error.c
>+++ b/drivers/gpu/drm/xe/xe_hw_error.c
>@@ -3,12 +3,16 @@
>  * Copyright © 2025 Intel Corporation
>  */
>
>+#include "regs/xe_gsc_regs.h"
> #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"
>+#include "xe_survivability_mode.h"
>+
>+#define  HEC_UNCORR_FW_ERR_BITS 4
>
> /* Error categories reported by hardware */
> enum hardware_error {
>@@ -18,6 +22,13 @@ enum hardware_error {
> 	HARDWARE_ERROR_MAX,
> };
>
>+static const char * const hec_uncorrected_fw_errors[] = {
>+	"Fatal",
>+	"CSE Disabled",
>+	"FD Corruption",
>+	"Data Corruption"
>+};
>+
> static const char *hw_error_to_str(const enum hardware_error hw_err)
> {
> 	switch (hw_err) {
>@@ -32,6 +43,56 @@ static const char *hw_error_to_str(const enum hardware_error hw_err)
> 	}
> }
>
>+static void csc_hw_error_work(struct work_struct *work)
>+{
>+	struct xe_tile *tile = container_of(work, typeof(*tile), csc_hw_error_work);
>+	struct xe_device *xe = tile_to_xe(tile);
>+	int ret;
>+
>+	ret = xe_survivability_mode_runtime_enable(xe);

xe_survivability_mode_runtime_enable() returns if it's not BMG, not dgfx 
etc., so does it make sense to not even queue the work if those 
conditions are not met?

>+	if (ret)
>+		drm_err(&xe->drm, "Failed to enable runtime survivability mode\n");
>+}
>+
>+static void csc_hw_error_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);
>+	struct xe_mmio *mmio = &tile->mmio;
>+	u32 base, err_bit, err_src;
>+	unsigned long fw_err;
>+
>+	if (xe->info.platform != XE_BATTLEMAGE)
>+		return;
>+
>+	/* Not supported in BMG */
>+	if (hw_err == HARDWARE_ERROR_CORRECTABLE)
>+		return;
>+
>+	base = BMG_GSC_HECI1_BASE;
>+	lockdep_assert_held(&xe->irq.lock);
>+	err_src = xe_mmio_read32(mmio, HEC_UNCORR_ERR_STATUS(base));
>+	if (!err_src) {
>+		drm_err_ratelimited(&xe->drm, HW_ERR "Tile%d reported HEC_ERR_STATUS_%s blank\n",
>+				    tile->id, hw_err_str);
>+		return;
>+	}
>+
>+	if (err_src & UNCORR_FW_REPORTED_ERR) {
>+		fw_err = xe_mmio_read32(mmio, HEC_UNCORR_FW_ERR_DW0(base));
>+		for_each_set_bit(err_bit, &fw_err, HEC_UNCORR_FW_ERR_BITS) {
>+			drm_err_ratelimited(&xe->drm, HW_ERR
>+					    "%s: HEC Uncorrected FW %s error reported, bit[%d] is set\n",
>+					     hw_err_str, hec_uncorrected_fw_errors[err_bit],
>+					     err_bit);
>+
>+			schedule_work(&tile->csc_hw_error_work);
>+		}
>+	}
>+
>+	xe_mmio_write32(mmio, HEC_UNCORR_ERR_STATUS(base), err_src);
>+}
>+
> 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);
>@@ -50,7 +111,8 @@ static void hw_error_source_handler(struct xe_tile *tile, const enum hardware_er
> 		goto unlock;
> 	}
>
>-	/* TODO: Process errrors per source */
>+	if (err_src & XE_CSC_ERROR)
>+		csc_hw_error_handler(tile, hw_err);
>
> 	xe_mmio_write32(&tile->mmio, DEV_ERR_STAT_REG(hw_err), err_src);
>
>@@ -101,8 +163,12 @@ static void process_hw_errors(struct xe_device *xe)
>  */
> void xe_hw_error_init(struct xe_device *xe)
> {
>+	struct xe_tile *tile = xe_device_get_root_tile(xe);
>+
> 	if (!IS_DGFX(xe) || IS_SRIOV_VF(xe))
> 		return;
>
>+	INIT_WORK(&tile->csc_hw_error_work, csc_hw_error_work);

Same here, why have a worker if it's not BMG?

Also, reiterating a previous comment in another patch - if the feature 
can be defined as a has_ struct member in the pci/gt info that could 
streamline the checks.

Thanks,
Umesh

>+
> 	process_hw_errors(xe);
> }
>-- 
>2.47.1
>