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

[Riana Tauro <riana.tauro@intel.com>]

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?

Thanks,
Riana

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