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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Jan 09, 2026 at 01:38:44PM +0530, Riana Tauro wrote:
> Hi Raag
> 
> Thank you for the review
> 
> On 12/9/2025 1:52 PM, Raag Jadav wrote:
> > On Fri, Dec 05, 2025 at 02:09:34PM +0530, Riana Tauro wrote:
> > > Allocate correctable, nonfatal and fatal nodes per xe device.
> > > Each node contains error classes, counters and respective
> > > query counter functions.
> > > 
> > > Add basic functionality to create and register drm nodes.
> > > Below operations can be performed using Generic netlink DRM RAS interface
> > > 
> > > List Nodes:
> > > 
> > > $ sudo ynl --family drm_ras  --dump list-nodes
> > > [{'device-name': '0000:03:00.0',
> > >    'node-id': 0,
> > >    'node-name': 'correctable-errors',
> > >    'node-type': 'error-counter'},
> > >   {'device-name': '0000:03:00.0',
> > >    'node-id': 1,
> > >    'node-name': 'nonfatal-errors',
> > >    'node-type': 'error-counter'},
> > >   {'device-name': '0000:03:00.0',
> > >    'node-id': 2,
> > >    'node-name': 'fatal-errors',
> > >    'node-type': 'error-counter'}]
> > > 
> > > Get Error counters:
> > > 
> > > $ sudo ynl --family drm_ras  --dump get-error-counters --json '{"node-id":1}'
> > > [{'error-id': 1, 'error-name': 'Core Compute Error', 'error-value': 0},
> > >   {'error-id': 2, 'error-name': 'SOC Internal Error', 'error-value': 0}]
> > 
> > Minor bikeshedding: Is there anything like 'SOC External'? If not, perhaps
> > simply 'SOC' would be sufficient.
> 
> Agree. SoC should be sufficient
> 
> > 
> > > Query Error counter:
> > > 
> > > $ sudo ynl --family drm_ras --do query-error-counter  --json '{"node-id":1, "error-id":1}'
> > > {'error-id': 1, 'error-name': 'Core Compute Error', 'error-value': 0}
> > 
> > One more (sorry): So this means graphics will be a different id? Or do they
> > overlap? How does it work?
> > 
> 
> Did not get this question.
> 
> > 
> > Also,
> > 
> > [*] I'm not much informed about the history here but the 'error' term
> > seems slapped onto almost everything. We already know it's RAS so perhaps
> > we add it only where make sense and try to simplify some of the naming?
> 
> Let's keep the errors in the node-names. Removing it from error-name should
> be okay. Wil fix ths
> 
> 
> > 
> > ...
> > 
> > > diff --git a/drivers/gpu/drm/xe/xe_drm_ras.c b/drivers/gpu/drm/xe/xe_drm_ras.c
> > > new file mode 100644
> > > index 000000000000..764b14b1edf8
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/xe/xe_drm_ras.c
> > > @@ -0,0 +1,199 @@
> > > +// SPDX-License-Identifier: MIT
> > > +/*
> > > + * Copyright © 2025 Intel Corporation
> > > + */
> > > +
> > > +#include <drm/drm_managed.h>
> > > +#include <drm/drm_ras.h>
> > > +#include <linux/bitmap.h>
> > > +
> > > +#include "xe_device.h"
> > 
> > This inherits some of the debt that should not be there, so let's try to
> > get away with xe_device_types.h where possible. But please double check.
> > 
> > > +#include "xe_drm_ras.h"
> > 
> > ...
> > 
> > > +static struct xe_drm_ras_counter *allocate_and_copy_counters(struct xe_device *xe,
> > > +							     int count)
> > > +{
> > > +	struct xe_drm_ras_counter *counter;
> > > +	int i;
> > > +
> > > +	counter = drmm_kzalloc(&xe->drm, count * sizeof(struct xe_drm_ras_counter), GFP_KERNEL);
> > 
> > Why not drmm_kcalloc()? We get a bonus overflow protection.
> 
> Will check
> 
> > 
> > > +	if (!counter)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	for (i = 0; i < count; i++) {
> > > +		if (!errors[i])
> > > +			continue;
> > > +
> > > +		counter[i].name = errors[i];
> > > +		atomic64_set(&counter[i].counter, 0);
> > > +	}
> > > +
> > > +	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 xe_drm_ras *ras = &xe->ras;
> > > +	int count = 0, ret = 0;
> > 
> > Redundant initializations, also why do we need them?
> 
> redundant code from previous rev. Will remove
> 
> > 
> > > +	count = DRM_XE_RAS_ERROR_CLASS_MAX;
> > > +	node->error_counter_range.first = DRM_XE_RAS_ERROR_CORE_COMPUTE;
> > > +	node->error_counter_range.last = DRM_XE_RAS_ERROR_CLASS_MAX - 1;
> > > +
> > > +	ras->info[severity] = allocate_and_copy_counters(xe, count);
> > 
> > This looks like count should be derived from first and last, or did I
> > miss something?
> 
> assigned it directly. Can be done
> 
> > 
> > > +	if (IS_ERR(ras->info[severity]))
> > > +		return PTR_ERR(ras->info[severity]);
> > > +
> > > +	switch (severity) {
> > > +	case DRM_XE_RAS_ERROR_CORRECTABLE:
> > > +		node->query_error_counter = query_correctable_error_counters;
> > > +		break;
> > > +	case DRM_XE_RAS_ERROR_NONFATAL:
> > > +		node->query_error_counter = query_non_fatal_error_counters;
> > > +		break;
> > > +	case DRM_XE_RAS_ERROR_FATAL:
> > > +		node->query_error_counter = query_fatal_error_counters;
> > > +		break;
> > > +	default:
> > 
> > Do we need this?
> 
> yes.
> 
> > 
> > > +		break;
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int register_nodes(struct xe_device *xe)
> > > +{
> > > +	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> > > +	struct xe_drm_ras *ras = &xe->ras;
> > > +	const char *device_name;
> > > +	int i = 0, ret;
> > 
> > Redundant initialization. Also, ret belongs to the loop below.
> > 
> > > +	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));
> > > +
> > > +	for (i = 0; i < DRM_XE_RAS_ERROR_SEVERITY_MAX; i++) {
> > 
> > We could potentially have a nice for_each_error_severity() now ;)
> 
> Sure. Will check. If its used in multiple places then its worth having a
> helper
> 
> > 
> > > +		struct drm_ras_node *node = &ras->node[i];
> > > +
> > > +		node->device_name = device_name;
> > > +		node->node_name = error_severity[i];
> > > +		node->type = DRM_RAS_NODE_TYPE_ERROR_COUNTER;
> > > +		node->priv = xe;
> > > +
> > > +		ret = assign_node_params(xe, node, i);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		ret = drm_ras_node_register(node);
> > > +		if (ret) {
> > > +			drm_err(&xe->drm, "Failed to register drm ras tile node\n");
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > 
> > ...
> > 
> > > +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_kzalloc(&xe->drm, DRM_XE_RAS_ERROR_SEVERITY_MAX * sizeof(struct drm_ras_node),
> > > +			    GFP_KERNEL);
> > 
> > Ditto for drmm_kcalloc().
> > 
> > > +	if (!node)
> > > +		return -ENOMEM;
> > > +
> > > +	ras->node = node;
> > > +
> > > +	err = register_nodes(xe);
> > > +	if (err) {
> > > +		drm_err(&xe->drm, "Failed to register drm ras node\n");
> > 
> > You wanted to call drm_err_probe(), didn't you ...?
> > 
> > Ah, not upstream yet :(
> > But perhaps an opportunity worth considering.
> > 
> > > +		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");
> > 
> > Ditto.
> > 
> > > +		return err;
> > > +	}
> > > +
> > > +	return 0;
> > 
> > ...
> > 
> > > +/**
> > > + * 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];
> > > +
> > > +};
> > 
> > Either separate the members with blank lines or don't, but let's be
> > consistent.
> 
> Will fix
> 
> > 
> > ...
> > 
> > >   void xe_hw_error_irq_handler(struct xe_tile *tile, const u32 master_ctl)
> > >   {
> > > -	enum hardware_error hw_err;
> > > +	u32 hw_err;
> > >   	if (fault_inject_csc_hw_error())
> > >   		schedule_work(&tile->csc_hw_error_work);
> > > -	for (hw_err = 0; hw_err < HARDWARE_ERROR_MAX; hw_err++)
> > > +	for (hw_err = 0; hw_err < DRM_XE_RAS_ERROR_SEVERITY_MAX; hw_err++)
> > 
> > for_each_error_severity()
> > 
> > >   		if (master_ctl & ERROR_IRQ(hw_err))
> > >   			hw_error_source_handler(tile, hw_err);
> > >   }
> > 
> > ...
> > 
> > > +/**
> > > + * enum drm_xe_ras_error_severity - Supported drm ras error severity.
> > > + */
> > > +enum drm_xe_ras_error_severity {
> > > +	/** @DRM_XE_RAS_ERROR_CORRECTABLE: Correctable Error */
> > > +	DRM_XE_RAS_ERROR_CORRECTABLE = 0,
> > > +	/** @DRM_XE_RAS_ERROR_NONFATAL: Non fatal Error */
> > > +	DRM_XE_RAS_ERROR_NONFATAL,
> > > +	/** @DRM_XE_RAS_ERROR_FATAL: Fatal error */
> > > +	DRM_XE_RAS_ERROR_FATAL,
> > > +	/** @DRM_XE_RAS_ERROR_SEVERITY_MAX: Max severity */
> > > +	DRM_XE_RAS_ERROR_SEVERITY_MAX, /* non-ABI */
> > 
> > This is guaranteed last member, so redundant comma.
> 
> ok
> 
> > 
> > > +};
> > > +
> > > +/**
> > > + * enum drm_xe_ras_error_class - Supported drm ras error classes.
> > > + */
> > > +enum drm_xe_ras_error_class {
> > > +	/** @DRM_XE_RAS_ERROR_CORE_COMPUTE: GT and Media Error */
> > > +	DRM_XE_RAS_ERROR_CORE_COMPUTE = 1,
> > > +	/** @DRM_XE_RAS_ERROR_SOC_INTERNAL: SOC Error */
> > > +	DRM_XE_RAS_ERROR_SOC_INTERNAL,
> > > +	/** @DRM_XE_RAS_ERROR_CLASS_MAX: Max Error */
> > > +	DRM_XE_RAS_ERROR_CLASS_MAX,	/* non-ABI */
> > 
> > Ditto.
> > 
> > > +};
> > 
> > Also, all of the enums share the same DRM_XE_RAS_ERROR_* prefix, so let's try
> > to have distinguishable naming. Perhaps [*] would be useful here as well ;)
> 
> DRM_XE_RAS_ERROR_SEVERITY_* will cause longer names. Any suggestions?

Try this full version first and see how the outcome looks like...
if we are still respecting the line limits without ugly cuts, then let's go with it.
otherwise try something shorter ERR_SEV_ ... or something like that...

> 
> Thanks
> Riana
> 
> > 
> > Raag
>