Re: [PATCH v5 2/5] drm/xe/xe_drm_ras: Add support for XE DRM RAS

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

On 2/3/2026 11:28 PM, Raag Jadav wrote:
> On Mon, Feb 02, 2026 at 12:13:58PM +0530, Riana Tauro wrote:
>> Allocate correctable, uncorrectable nodes for every xe device
> 
> Punctuations.
> 
>> Each node contains error component, counters and respective
>> query counter functions.
> 
> Try to utilize the full 75 characters space where possible.
> 
>> Add basic functionality to create and register drm nodes.
>> Below operations can be performed using Generic netlink DRM RAS interface
> 
> Punctuations.

Will fix above

> 
> ...
> 
>> +++ b/drivers/gpu/drm/xe/xe_drm_ras.c
>> @@ -0,0 +1,184 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2026 Intel Corporation
>> + */
>> +
>> +#include <drm/drm_managed.h>
>> +#include <drm/drm_print.h>
>> +#include <drm/drm_ras.h>
>> +#include <linux/bitmap.h>
> 
> Linux includes usually go first.
> 
>> +#include "xe_device_types.h"
>> +#include "xe_drm_ras.h"
>> +
>> +static const char * const errors[] = DRM_XE_RAS_ERROR_COMPONENT_NAMES;
> 
> 'error_component'?

will rename.

> 
>> +static const char * const error_severity[] = DRM_XE_RAS_ERROR_SEVERITY_NAMES;
> 
> ...
> 
>> +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_ERR_COMP_MAX,
>> +			       sizeof(*counter), GFP_KERNEL);
> 
> Can be one line.

yeah will make it a single line

> 
>> +	if (!counter)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	for (i = DRM_XE_RAS_ERR_COMP_CORE_COMPUTE; i < DRM_XE_RAS_ERR_COMP_MAX; i++) {
>> +		if (!errors[i])
>> +			continue;
>> +
>> +		counter[i].name = errors[i];
>> +		atomic_set(&counter[i].counter, 0);
> 
> Do you need this?

It's clear to anyone seeing the code that we need to
initialize to 0.

> 
>> +	}
>> +
>> +	return counter;
>> +}
> 
> ...
> 
>> +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_ERR_SEV_MAX, sizeof(*node),
>> +			    GFP_KERNEL);
> 
> Can be one line.
> 
>> +	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;
>> +}
> 
> ...
> 
>> +++ b/drivers/gpu/drm/xe/xe_drm_ras_types.h
>> @@ -0,0 +1,40 @@
>> +/* 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>
> 
> Ditto for linux includes.

Had thought this needs to be alphabetical. Got a similar
comment in a different patch.
Will fix throughout the series


> 
>> +struct drm_ras_node;
> 

Thank you for the review

Thanks
Riana

> Reviewed-by: Raag Jadav <raag.jadav@intel.com>