Re: [PATCH v3 3/4] drm/xe/xe_hw_error: Add support for GT hardware errors

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

Hi Raag

On 12/10/2025 11:48 PM, Raag Jadav wrote:
> On Fri, Dec 05, 2025 at 02:09:35PM +0530, Riana Tauro wrote:
>> PVC supports GT error reporting via vector registers along with
>> error status register. Add support to report these errors and
>> update respective counters. Incase of Subslice error reported
>> by vector register, process the error status register
>> for applicable bits.
>>
>> Incorporate the counter inside the driver itself and start
>> using the drm_ras generic netlink to report them.
> 
> ...
> 
>> +#define ERR_STAT_GT_COR				0x100160
>> +#define ERR_STAT_GT_NONFATAL			0x100164
>> +#define ERR_STAT_GT_FATAL			0x100168
>> +#define ERR_STAT_GT_REG(x)			XE_REG(_PICK_EVEN((x), \
>> +								 ERR_STAT_GT_COR, \
> 
> Align with braces.

This is aligned with the braces.

> 
>> +								 ERR_STAT_GT_NONFATAL))
>> +
>> +#define  GT_HW_ERROR_MAX_ERR_BITS		16
>> +#define  EU_GRF_ERR				(15)
>> +#define  EU_IC_ERR				(14)
>> +#define  SLM_ERR				(13)
> 
> Nit: I know this consolidates the duplication but separate bits followed
> by their mask would be relatively easier to read IMHO.
> 

Hmm.. okay. Was trying to avoid multiple declarations for the same bit


>> +#define  GUC_COR_ERR				(1)
>> +
>> +#define  GUC_FAT_ERR				(6)
>> +#define  FPU_FAT_ERR				(3)
>> +
>> +#define PVC_COR_ERR_MASK			(BIT(GUC_COR_ERR) | BIT(SLM_ERR) | \
>> +						 BIT(EU_IC_ERR) | BIT(EU_GRF_ERR))
>> +
>> +#define PVC_FAT_ERR_MASK			(BIT(FPU_FAT_ERR) | BIT(GUC_FAT_ERR) | \
>> +						 BIT(EU_GRF_ERR) | BIT(SLM_ERR))
> 
> I had an impression that we used REG_BIT(), or did things change?

I took the previous patch directly. Thanks for catching this. Will fix

> 
>>   #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)
>> +#define   XE_GT_ERROR				BIT(0)
>> +
>> +#define  ERR_STAT_GT_FATAL_VECTOR_0		0x100260
>> +#define  ERR_STAT_GT_FATAL_VECTOR_1		0x100264
>> +
>> +#define  ERR_STAT_GT_FATAL_VECTOR_REG(x)	XE_REG(_PICK_EVEN((x), \
>> +								  ERR_STAT_GT_FATAL_VECTOR_0, \
>> +								  ERR_STAT_GT_FATAL_VECTOR_1))
>> +
>> +#define  ERR_STAT_GT_COR_VECTOR_LEN		(4)
>> +#define  ERR_STAT_GT_COR_VECTOR_0		0x1002a0
>> +#define  ERR_STAT_GT_COR_VECTOR_1		0x1002a4
>> +
>> +#define  ERR_STAT_GT_COR_VECTOR_REG(x)		XE_REG(_PICK_EVEN((x), \
>> +								 ERR_STAT_GT_COR_VECTOR_0,\
> 
> Ditto for alignment. Also, the convention here seems like an empty space
> before the backslash so let's be consistent.

Alignment is proper here too.  It's patchwork :(

> 
>> +								 ERR_STAT_GT_COR_VECTOR_1))
>> +
>> +#define ERR_STAT_GT_VECTOR_REG(hw_err, x)	(hw_err == DRM_XE_RAS_ERROR_CORRECTABLE ? \
>> +						 ERR_STAT_GT_COR_VECTOR_REG(x) : \
>> +						 ERR_STAT_GT_FATAL_VECTOR_REG(x))
>> +
>>   #endif
>> diff --git a/drivers/gpu/drm/xe/xe_hw_error.c b/drivers/gpu/drm/xe/xe_hw_error.c
>> index d63078d00b56..77c90f1b06fd 100644
>> --- a/drivers/gpu/drm/xe/xe_hw_error.c
>> +++ b/drivers/gpu/drm/xe/xe_hw_error.c
>> @@ -3,6 +3,7 @@
>>    * Copyright © 2025 Intel Corporation
>>    */
>>   
>> +#include <linux/bitmap.h>
>>   #include <linux/fault-inject.h>
>>   
>>   #include "regs/xe_gsc_regs.h"
>> @@ -16,6 +17,8 @@
>>   #include "xe_survivability_mode.h"
>>   
>>   #define  HEC_UNCORR_FW_ERR_BITS 4
>> +#define XE_RAS_REG_SIZE 32
> 
> Alignment please! (including the values)
> 
>>   extern struct fault_attr inject_csc_hw_error;
>>   static const char * const error_severity[] = DRM_XE_RAS_ERROR_SEVERITY_NAMES;
>>   
>> @@ -26,6 +29,25 @@ static const char * const hec_uncorrected_fw_errors[] = {
>>   	"Data Corruption"
>>   };
>>   
>> +#define ERR_INDEX(_bit, index) \
>> +	[__ffs(_bit)] = index
> 
> Does this guarantee compile time evaluation for all archs? We might risk
> breaking other builds where the compiler can't figure it out.

Thanks for pointing this out. Will fix this

> 
>> +static const unsigned long xe_hw_error_map[] = {
>> +	ERR_INDEX(XE_GT_ERROR, DRM_XE_RAS_ERROR_CORE_COMPUTE),
>> +};
>> +
>> +enum gt_vector_regs {
>> +	ERR_STAT_GT_VECTOR0 = 0,
>> +	ERR_STAT_GT_VECTOR1,
>> +	ERR_STAT_GT_VECTOR2,
>> +	ERR_STAT_GT_VECTOR3,
>> +	ERR_STAT_GT_VECTOR4,
>> +	ERR_STAT_GT_VECTOR5,
>> +	ERR_STAT_GT_VECTOR6,
>> +	ERR_STAT_GT_VECTOR7,
>> +	ERR_STAT_GT_VECTOR_MAX,
>> +};
>> +
>>   static bool fault_inject_csc_hw_error(void)
>>   {
>>   	return IS_ENABLED(CONFIG_DEBUG_FS) && should_fail(&inject_csc_hw_error, 1);
>> @@ -78,14 +100,136 @@ static void csc_hw_error_handler(struct xe_tile *tile,
>>   	xe_mmio_write32(mmio, HEC_UNCORR_ERR_STATUS(base), err_src);
>>   }
>>   
>> +static void log_hw_error(struct xe_tile *tile, const char *name,
>> +			 const enum drm_xe_ras_error_severity severity)
>> +{
>> +	const char *severity_str = error_severity[severity];
>> +	struct xe_device *xe = tile_to_xe(tile);
>> +
>> +	if (severity == DRM_XE_RAS_ERROR_FATAL)
>> +		drm_err_ratelimited(&xe->drm, "%s %s error detected\n", name, severity_str);
> 
> Adding to my earlier bikeshed[*] about 'error', it's either as part of
> severity_str or message but let's not spam it.
> 
> [*] https://lore.kernel.org/intel-xe/aTfcV5nb_vBOOBvP@black.igk.intel.com/

fixing this

> 
>> +	else
>> +		drm_warn(&xe->drm, "%s %s error detected\n", name, severity_str);
> 
> Ditto.
> 
>> +}
>> +
>> +static void
>> +log_gt_err(struct xe_tile *tile, const char *name, int i, u32 err,
> 
> Could be a single line. Also, 'i' could be unsigned.

okay

> 
>> +	   const enum drm_xe_ras_error_severity severity)
>> +{
>> +	const char *severity_str = error_severity[severity];
>> +	struct xe_device *xe = tile_to_xe(tile);
>> +
>> +	if (severity == DRM_XE_RAS_ERROR_FATAL)
>> +		drm_err_ratelimited(&xe->drm, "%s %s error detected, ERROR_STAT_GT_VECTOR%d:0x%08x\n",
> 
> Same as above.

This is actually the type of GT error so it will not print extra error.
Used here

log_gt_err(tile, "TLB", i, vector, severity);
log_gt_err(tile, "Subslice", i, vector, severity);


> 
>> +				    name, severity_str, i, err);
>> +	else
>> +		drm_warn(&xe->drm, "%s %s error detected, ERROR_STAT_GT_VECTOR%d:0x%08x\n",
> 
> Ditto.
> 
>> +			 name, severity_str, i, err);
>> +}
>> +
>> +static void gt_handle_errors(struct xe_tile *tile,
>> +			     const enum drm_xe_ras_error_severity severity, u32 error_id)
>> +{
>> +	struct xe_device *xe = tile_to_xe(tile);
>> +	struct xe_drm_ras *ras = &xe->ras;
>> +	struct xe_drm_ras_counter *info = ras->info[severity];
>> +	struct xe_mmio *mmio = &tile->mmio;
>> +	unsigned long err_stat = 0;
>> +	int i;
>> +
>> +	if (xe->info.platform != XE_PVC)
>> +		return;
>> +
>> +	for (i = 0; i < ERR_STAT_GT_VECTOR_MAX; i++) {
>> +		u32 vector, val;
>> +
>> +		if (severity == DRM_XE_RAS_ERROR_CORRECTABLE && i >= ERR_STAT_GT_COR_VECTOR_LEN)
> 
> This can be a temp variable which you can use as a for loop condition,
> but please use explicit length per severity so we don't come back
> questioning things.
> 

okay


>> +			break;
>> +
>> +		vector = xe_mmio_read32(mmio, ERR_STAT_GT_VECTOR_REG(severity, i));
>> +		if (!vector)
>> +			continue;
>> +
>> +		switch (i) {
>> +		case ERR_STAT_GT_VECTOR0:
>> +		case ERR_STAT_GT_VECTOR1:
>> +			u32 errbit;
>> +
>> +			val = hweight32(vector);
>> +			atomic64_add(val, &info[error_id].counter);
>> +			log_gt_err(tile, "Subslice", i, vector, severity);
>> +
>> +			if (err_stat)
>> +				break;
> 
> So we won't ever be getting past this point, is that right?

err stat will be read only once. The first time we will not hit this.


> 
>> +			err_stat = xe_mmio_read32(mmio, ERR_STAT_GT_REG(severity));
>> +			for_each_set_bit(errbit, &err_stat, GT_HW_ERROR_MAX_ERR_BITS) {
>> +				if (severity == DRM_XE_RAS_ERROR_CORRECTABLE &&
>> +				    (BIT(errbit) & PVC_COR_ERR_MASK))
>> +					atomic64_inc(&info[error_id].counter);
>> +				if (severity == DRM_XE_RAS_ERROR_FATAL &&
>> +				    (BIT(errbit) & PVC_FAT_ERR_MASK))
>> +					atomic64_inc(&info[error_id].counter);
>> +			}
>> +			if (err_stat)
>> +				xe_mmio_write32(mmio, ERR_STAT_GT_REG(severity), err_stat);
>> +			break;
>> +		case ERR_STAT_GT_VECTOR2:
>> +		case ERR_STAT_GT_VECTOR3:
>> +			val = hweight32(vector);
>> +			atomic64_add(val, &info[error_id].counter);
>> +			log_gt_err(tile, "L3 BANK", i, vector, severity);
>> +			break;
>> +		case ERR_STAT_GT_VECTOR6:
>> +			val = hweight32(vector);
>> +			atomic64_add(val, &info[error_id].counter);
>> +			log_gt_err(tile, "TLB", i, vector, severity);
>> +			break;
>> +		case ERR_STAT_GT_VECTOR7:
>> +			val = hweight32(vector);
>> +			atomic64_add(val, &info[error_id].counter);
>> +			break;
>> +		default:
>> +			log_gt_err(tile, "Undefined", i, vector, severity);
>> +		}
>> +
>> +		xe_mmio_write32(mmio, ERR_STAT_GT_VECTOR_REG(severity, i), vector);
>> +	}
>> +}
>> +
>> +static void gt_hw_error_handler(struct xe_tile *tile,
>> +				const enum drm_xe_ras_error_severity severity, u32 error_id)
> 
> This entire function can be just an explicit NONFATAL case at the top of
> gt_handle_errors().

okay

> 
>> +{
>> +	struct xe_device *xe = tile_to_xe(tile);
>> +	struct xe_drm_ras *ras = &xe->ras;
>> +	struct xe_drm_ras_counter *info = ras->info[severity];
>> +
>> +	switch (severity) {
>> +	case DRM_XE_RAS_ERROR_CORRECTABLE:
>> +		gt_handle_errors(tile, severity, error_id);
>> +		break;
>> +	case DRM_XE_RAS_ERROR_NONFATAL:
>> +		atomic64_inc(&info[error_id].counter);
>> +		log_hw_error(tile, "GT", severity);
>> +		break;
>> +	case DRM_XE_RAS_ERROR_FATAL:
>> +		gt_handle_errors(tile, severity, error_id);
>> +		break;
>> +	default:
>> +		drm_warn(&xe->drm, "Undefined error detected\n");
>> +	}
>> +}
>> +
>>   static void hw_error_source_handler(struct xe_tile *tile, enum drm_xe_ras_error_severity severity)
> 
> Perhaps severity can be const now?

yeah it can be const here. Will add

> 
>>   {
>>   	const char *severity_str = error_severity[severity];
>>   	struct xe_device *xe = tile_to_xe(tile);
>> -	unsigned long flags;
>> -	u32 err_src;
>> +	struct xe_drm_ras *ras = &xe->ras;
>> +	struct xe_drm_ras_counter *info = ras->info[severity];
>> +	unsigned long flags, err_src;
>> +	u32 err_bit;
>>   
>> -	if (xe->info.platform != XE_BATTLEMAGE)
>> +	if (!IS_DGFX(xe))
>>   		return;
>>   
>>   	spin_lock_irqsave(&xe->irq.lock, flags);
> 
> Isn't this IRQ context only? What am I missing?

This is right

> 
>> @@ -96,11 +240,39 @@ static void hw_error_source_handler(struct xe_tile *tile, enum drm_xe_ras_error_
>>   		goto unlock;
>>   	}
>>   
>> -	if (err_src & XE_CSC_ERROR)
>> +	if (err_src & XE_CSC_ERROR) {
> 
> Shouldn't this be inside the loop below?

We do not have a separate type for CSC. And once we get a CSC error, the 
driver will be wedged and only way to recover is firmware flash.

So there is no point of keeping count or checking other bits.


> 
>>   		csc_hw_error_handler(tile, severity);
>> +		goto clear_reg;
> 
> If no, should we not check for other bits before bailing?
> 
>> +	}
>>   
>> -	xe_mmio_write32(&tile->mmio, DEV_ERR_STAT_REG(severity), err_src);
>> +	if (!info) {
>> +		drm_err_ratelimited(&xe->drm, HW_ERR "Errors undefined\n");
>> +		goto clear_reg;
>> +	}
>> +
>> +	for_each_set_bit(err_bit, &err_src, XE_RAS_REG_SIZE) {
>> +		u32 error_id = xe_hw_error_map[err_bit];
>> +		const char *name;
>> +
>> +		name = info[error_id].name;
>> +		if (!name)
>> +			goto clear_reg;
>>   
>> +		if (severity == DRM_XE_RAS_ERROR_FATAL) {
>> +			drm_err_ratelimited(&xe->drm, HW_ERR
>> +					    "TILE%d reported %s %s error, bit[%d] is set\n",
>> +					    tile->id, name, severity_str, err_bit);
>> +		} else {
>> +			drm_warn(&xe->drm, HW_ERR
>> +				 "TILE%d reported %s %s error, bit[%d] is set\n",
>> +				 tile->id, name, severity_str, err_bit);
>> +		}
>> +		if (BIT(err_bit) & XE_GT_ERROR)
> 
> Shouldn't this be compare?

This also won't give an error. But yeah removing ffs so will change this

Thanks
Riana

> 
> Raag
> 
>> +			gt_hw_error_handler(tile, severity, error_id);
>> +	}
>> +
>> +clear_reg:
>> +	xe_mmio_write32(&tile->mmio, DEV_ERR_STAT_REG(severity), err_src);
>>   unlock:
>>   	spin_unlock_irqrestore(&xe->irq.lock, flags);
>>   }
>> -- 
>> 2.47.1
>>