Re: [PATCH v3 6/7] drm/xe/xe_hw_error: Handle CSC Firmware reported Hardware errors

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

Hi Rodrigo

On 7/3/2025 3:05 AM, Rodrigo Vivi wrote:
> On Wed, Jul 02, 2025 at 07:41:16PM +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)
>>
>> 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.c             | 11 +++-
>>   drivers/gpu/drm/xe/xe_device_types.h       |  3 +
>>   drivers/gpu/drm/xe/xe_hw_error.c           | 70 +++++++++++++++++++++-
>>   5 files changed, 88 insertions(+), 5 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.c b/drivers/gpu/drm/xe/xe_device.c
>> index d6b680abc3ae..fbc50cebfc11 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
>> @@ -1154,6 +1154,7 @@ static void xe_device_wedged_fini(struct drm_device *drm, void *arg)
>>    */
>>   void xe_device_declare_wedged(struct xe_device *xe)
>>   {
>> +	unsigned long recovery_method;
>>   	struct xe_gt *gt;
>>   	u8 id;
>>   
>> @@ -1169,6 +1170,12 @@ void xe_device_declare_wedged(struct xe_device *xe)
>>   		return;
>>   	}
>>   
>> +	/* Default recovery method */
>> +	recovery_method = DRM_WEDGE_RECOVERY_REBIND | DRM_WEDGE_RECOVERY_BUS_RESET;
>> +
>> +	if (xe_survivability_mode_is_runtime(xe))
>> +		recovery_method = DRM_WEDGE_RECOVERY_VENDOR;
> 
> what about the DRM_WEDGE_RECOVERY_VENDOR as an option to this function?
> > Then, from the survivability mode you call:
> xe_device_declare_wedged(xe, DRM_WEDGE_RECOVERY_VENDOR)

The default method is used in most of the cases, that is the reason i 
didn't use parameter.

How about retaining this patch if the method is different from default.
https://patchwork.freedesktop.org/patch/660131/?series=149756&rev=2 ?

> 
>> +
>>   	for_each_gt(gt, xe, id)
>>   		xe_gt_declare_wedged(gt);
>>   
>> @@ -1181,8 +1188,6 @@ void xe_device_declare_wedged(struct xe_device *xe)
>>   			dev_name(xe->drm.dev));
>>   
>>   		/* Notify userspace of wedged device */
>> -		drm_dev_wedged_event(&xe->drm,
>> -				     DRM_WEDGE_RECOVERY_REBIND | DRM_WEDGE_RECOVERY_BUS_RESET,
>> -				     NULL);
>> +		drm_dev_wedged_event(&xe->drm, recovery_method, NULL);
>>   	}
>>   }
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>> index 7e4f6d846af6..5daf5ba6bf51 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..73c788fd0dee 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,58 @@ 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_enable(xe, XE_SURVIVABILITY_TYPE_RUNTIME);
>> +	if (ret)
>> +		drm_err(&xe->drm, "Failed to enable runtime survivability mode\n");
> 
> This could simply call a function xe_survivability_mode_runtime(xe), which
> declares the device wedged with vendor specific reason.

Will do this based on the decision in
[3/7]Add support for Runtime survivability mode

Thanks
Riana>
>> +
>> +	xe_device_declare_wedged(xe);
>> +}
>> +
>> +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 +113,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 +165,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);
>> +
>>   	process_hw_errors(xe);
>>   }
>> -- 
>> 2.47.1
>>