Re: [PATCH v4 2/4] drm/xe/xe_drm_ras: Add support for drm ras

[Raag Jadav <raag.jadav@intel.com>]

On Mon, Jan 19, 2026 at 09:30:24AM +0530, Riana Tauro wrote:
> Allocate correctable, uncorrectable nodes for every xe device
> Each node contains error classes, counters and respective
> query counter functions.

...

> +static int hw_query_error_counter(struct xe_drm_ras_counter *info,
> +				  u32 error_id, const char **name, u32 *val)
> +{
> +	if (error_id < DRM_XE_RAS_ERROR_CLASS_GT || error_id >= DRM_XE_RAS_ERROR_CLASS_MAX)

This looks like it can be in_range().

> +		return -EINVAL;
> +
> +	if (!info[error_id].name)
> +		return -ENOENT;
> +
> +	*name = info[error_id].name;
> +	*val = atomic64_read(&info[error_id].counter);
> +
> +	return 0;
> +}
> +
> +static int query_uncorrectable_error_counters(struct drm_ras_node *ep,

This is named as 'counters' but I only see a single call here. What am
I missing?

> +					      u32 error_id, const char **name,
> +					      u32 *val)

Can this be less lines?

> +{
> +	struct xe_device *xe = ep->priv;
> +	struct xe_drm_ras *ras = &xe->ras;
> +	struct xe_drm_ras_counter *info = ras->info[DRM_XE_RAS_ERROR_SEVERITY_UNCORRECTABLE];
> +
> +	return hw_query_error_counter(info, error_id, name, val);
> +}
> +
> +static int query_correctable_error_counters(struct drm_ras_node *ep,

Same as above.

> +					    u32 error_id, const char **name,
> +					    u32 *val)

Same as above.

> +{
> +	struct xe_device *xe = ep->priv;
> +	struct xe_drm_ras *ras = &xe->ras;
> +	struct xe_drm_ras_counter *info = ras->info[DRM_XE_RAS_ERROR_SEVERITY_CORRECTABLE];
> +
> +	return hw_query_error_counter(info, error_id, name, val);
> +}
> +
> +static struct xe_drm_ras_counter *allocate_and_copy_counters(struct xe_device *xe)
> +{
> +	struct xe_drm_ras_counter *counter;
> +	int i;
> +
> +	counter = drmm_kcalloc(&xe->drm, DRM_XE_RAS_ERROR_CLASS_MAX,
> +			       sizeof(struct xe_drm_ras_counter), GFP_KERNEL);

I'd make this robust against type changes, i.e. sizeof(*counter).

> +	if (!counter)
> +		return ERR_PTR(-ENOMEM);
> +
> +	for (i = 0; i < DRM_XE_RAS_ERROR_CLASS_MAX; i++) {
> +		if (!errors[i])
> +			continue;
> +
> +		counter[i].name = errors[i];
> +		atomic64_set(&counter[i].counter, 0);

Doesn't drmm_kcalloc() already take care of this?

> +	}
> +
> +	return counter;
> +}
> +
> +static int assign_node_params(struct xe_device *xe, struct drm_ras_node *node,
> +			      const enum drm_xe_ras_error_severity severity)
> +{
> +	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> +	struct xe_drm_ras *ras = &xe->ras;
> +	const char *device_name;
> +
> +	device_name = kasprintf(GFP_KERNEL, "%04x:%02x:%02x.%d",
> +				pci_domain_nr(pdev->bus), pdev->bus->number,
> +				PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> +
> +	node->device_name = device_name;
> +	node->node_name = error_severity[severity];
> +	node->type = DRM_RAS_NODE_TYPE_ERROR_COUNTER;
> +	node->error_counter_range.first = DRM_XE_RAS_ERROR_CLASS_GT;
> +	node->error_counter_range.last = DRM_XE_RAS_ERROR_CLASS_MAX - 1;
> +	node->priv = xe;
> +
> +	ras->info[severity] = allocate_and_copy_counters(xe);
> +	if (IS_ERR(ras->info[severity]))
> +		return PTR_ERR(ras->info[severity]);
> +
> +	if (severity == DRM_XE_RAS_ERROR_SEVERITY_CORRECTABLE)
> +		node->query_error_counter = query_correctable_error_counters;
> +	else
> +		node->query_error_counter = query_uncorrectable_error_counters;

Shouldn't this have explicit severity check, atleast for future proofing?

> +
> +	return 0;
> +}
> +
> +static int register_nodes(struct xe_device *xe)
> +{
> +	struct xe_drm_ras *ras = &xe->ras;
> +	int i;
> +
> +	for_each_error_severity(i) {
> +		struct drm_ras_node *node = &ras->node[i];
> +		int ret;
> +
> +		ret = assign_node_params(xe, node, i);
> +		if (ret)
> +			return ret;
> +
> +		ret = drm_ras_node_register(node);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void xe_drm_ras_unregister_nodes(void *arg)
> +{
> +	struct xe_device *xe = arg;
> +	struct xe_drm_ras *ras = &xe->ras;
> +	int i;
> +
> +	for_each_error_severity(i) {
> +		struct drm_ras_node *node = &ras->node[i];
> +
> +		drm_ras_node_unregister(node);
> +
> +		if (i == 0)
> +			kfree(node->device_name);

Aren't we allocating this for each node?

> +	}
> +}
> +
> +/**
> + * xe_drm_ras_allocate_nodes - Allocate DRM RAS nodes
> + * @xe: xe device instance
> + *
> + * Allocate and register DRM RAS nodes per device
> + *
> + * Return: 0 on success, error code on failure
> + */
> +int xe_drm_ras_allocate_nodes(struct xe_device *xe)
> +{
> +	struct xe_drm_ras *ras = &xe->ras;
> +	struct drm_ras_node *node;
> +	int err;
> +
> +	node = drmm_kcalloc(&xe->drm, DRM_XE_RAS_ERROR_SEVERITY_MAX, sizeof(struct drm_ras_node),

Ditto for robust against type changes.

> +			    GFP_KERNEL);
> +	if (!node)
> +		return -ENOMEM;
> +
> +	ras->node = node;
> +
> +	err = register_nodes(xe);
> +	if (err) {
> +		drm_err(&xe->drm, "Failed to register drm ras node\n");
> +		return err;
> +	}
> +
> +	err = devm_add_action_or_reset(xe->drm.dev, xe_drm_ras_unregister_nodes, xe);
> +	if (err) {
> +		drm_err(&xe->drm, "Failed to add action for xe drm_ras\n");
> +		return err;
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/xe/xe_drm_ras.h b/drivers/gpu/drm/xe/xe_drm_ras.h
> new file mode 100644
> index 000000000000..2d714342e4e5
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_drm_ras.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2026 Intel Corporation
> + */
> +#ifndef XE_DRM_RAS_H_
> +#define XE_DRM_RAS_H_
> +
> +struct xe_device;
> +
> +#define for_each_error_severity(i)	\
> +	for (i = 0; i < DRM_XE_RAS_ERROR_SEVERITY_MAX; i++)
> +
> +int xe_drm_ras_allocate_nodes(struct xe_device *xe);
> +
> +#endif
> diff --git a/drivers/gpu/drm/xe/xe_drm_ras_types.h b/drivers/gpu/drm/xe/xe_drm_ras_types.h
> new file mode 100644
> index 000000000000..528c708e57da
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_drm_ras_types.h
> @@ -0,0 +1,49 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2026 Intel Corporation
> + */
> +
> +#ifndef _XE_DRM_RAS_TYPES_H_
> +#define _XE_DRM_RAS_TYPES_H_
> +
> +#include <drm/xe_drm.h>
> +#include <linux/atomic.h>
> +
> +struct drm_ras_node;
> +
> +/* Error categories reported by hardware */
> +enum hardware_error {
> +	HARDWARE_ERROR_CORRECTABLE = 0,
> +	HARDWARE_ERROR_NONFATAL = 1,
> +	HARDWARE_ERROR_FATAL = 2,
> +	HARDWARE_ERROR_MAX,
> +};
> +
> +/**
> + * struct xe_drm_ras_counter - XE RAS counter
> + *
> + * This structure contains error class and counter information
> + */
> +struct xe_drm_ras_counter {
> +	/** @name: error class name */
> +	const char *name;
> +
> +	/** @counter: count of error */
> +	atomic64_t counter;
> +};
> +
> +/**
> + * struct xe_drm_ras - XE DRM RAS structure
> + *
> + * This structure has details of error counters
> + */
> +struct xe_drm_ras {
> +	/** @node: DRM RAS node */
> +	struct drm_ras_node *node;
> +
> +	/** @info: info array for all types of errors */
> +	struct xe_drm_ras_counter *info[DRM_XE_RAS_ERROR_SEVERITY_MAX];
> +

Nit: Redundant blank line.

> +};
> +
> +#endif
> diff --git a/drivers/gpu/drm/xe/xe_hw_error.c b/drivers/gpu/drm/xe/xe_hw_error.c
> index 8c65291f36fc..b42495d3015a 100644
> --- a/drivers/gpu/drm/xe/xe_hw_error.c
> +++ b/drivers/gpu/drm/xe/xe_hw_error.c
> @@ -10,20 +10,14 @@
>  #include "regs/xe_irq_regs.h"
>  
>  #include "xe_device.h"
> +#include "xe_drm_ras.h"
>  #include "xe_hw_error.h"
>  #include "xe_mmio.h"
>  #include "xe_survivability_mode.h"
>  
>  #define  HEC_UNCORR_FW_ERR_BITS 4
>  extern struct fault_attr inject_csc_hw_error;
> -
> -/* Error categories reported by hardware */
> -enum hardware_error {
> -	HARDWARE_ERROR_CORRECTABLE = 0,
> -	HARDWARE_ERROR_NONFATAL = 1,
> -	HARDWARE_ERROR_FATAL = 2,
> -	HARDWARE_ERROR_MAX,
> -};
> +static const char * const error_severity[] = DRM_XE_RAS_ERROR_SEVERITY_NAMES;

This is unrelated to uapi changes, shouldn't we split this into a separate
patch?

...

> +/**
> + * enum drm_xe_ras_error_severity - DRM RAS error severity.
> + */
> +enum drm_xe_ras_error_severity {
> +	/** @DRM_XE_RAS_ERROR_SEVERITY_CORRECTABLE: Correctable Error */
> +	DRM_XE_RAS_ERROR_SEVERITY_CORRECTABLE = 0,

DRM_XE_RAS_ERR_SEV_*? (and same for this entire file)

> +	/** @DRM_XE_RAS_ERROR_UNCORRECTABLE: Uncorrectable Error */

Match with actual name.

> +	DRM_XE_RAS_ERROR_SEVERITY_UNCORRECTABLE,
> +	/** @DRM_XE_RAS_ERROR_SEVERITY_MAX: Max severity */
> +	DRM_XE_RAS_ERROR_SEVERITY_MAX /* non-ABI */
> +};
> +
> +/**
> + * enum drm_xe_ras_error_class - DRM RAS error classes.
> + */
> +enum drm_xe_ras_error_class {
> +	/** @DRM_XE_RAS_ERROR_CLASS_GT: GT Error */
> +	DRM_XE_RAS_ERROR_CLASS_GT = 1,
> +	/** @DRM_XE_RAS_ERROR_CLASS_SOC: SoC Error */
> +	DRM_XE_RAS_ERROR_CLASS_SOC,
> +	/** @DRM_XE_RAS_ERROR_CLASS_MAX: Max Error */
> +	DRM_XE_RAS_ERROR_CLASS_MAX	/* non-ABI */

I don't find 'CLASS' to be much translatable since it can inherently mean
anything, but I'm not sure if this to match with spec naming.

PS: I've used 'COMP' for component in my series[1], but upto you.
Also, please help review it in case I've missed anything.

[1] https://lore.kernel.org/intel-xe/20260116093432.914040-1-raag.jadav@intel.com/

Raag

> +};
> +
> +/*
> + * Error severity to name mapping.
> + */
> +#define DRM_XE_RAS_ERROR_SEVERITY_NAMES {					\
> +	[DRM_XE_RAS_ERROR_SEVERITY_CORRECTABLE] = "correctable-errors",		\
> +	[DRM_XE_RAS_ERROR_SEVERITY_UNCORRECTABLE] = "uncorrectable-errors",	\
> +}
> +
> +/*
> + * Error class to name mapping.
> + */
> +#define DRM_XE_RAS_ERROR_CLASS_NAMES {					\
> +	[DRM_XE_RAS_ERROR_CLASS_GT] = "GT",				\
> +	[DRM_XE_RAS_ERROR_CLASS_SOC] = "SoC"				\
> +}
> +
>  #if defined(__cplusplus)
>  }
>  #endif
> -- 
> 2.47.1
>