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

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

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?

Thanks
Riana

> 
> Raag