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 Fri, Jul 11, 2025 at 11:16:15AM +0530, Riana Tauro wrote:
>Hi Umesh
>
>On 7/11/2025 6:06 AM, Umesh Nerlige Ramappa wrote:
>>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?
>
>CSC work is only scheduled for BMG in the below handler.
>The bit is not present in prior platforms
>>
>>>+    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.
>
>This is only initialization. The queueing is done in the handler.
>If it is supported from a particular platform then it seems unnecessary.
>Should i add a function instead?

No, this is good enough if the worker is queued for supported platform.

Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

Thanks,
Umesh
>
>Thanks,
>Riana
>
>>
>>Thanks,
>>Umesh
>>
>>>+
>>>    process_hw_errors(xe);
>>>}
>>>-- 
>>>2.47.1
>>>
>
>