Re: [PATCH v3 07/12] drm/xe/cper: Log CPER records for aggregate counter retrival

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

On Thu, Sep 10, 2026 at 08:27:44AM +0200, Raag Jadav wrote:
> On Sun, Sep 06, 2026 at 10:56:12PM +0530, Badal Nilawar wrote:
> > Log CPER records for aggregate counter retrieval from userspace
> > when cper_on_query sysfs is enabled.
> > 
> > Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_drm_ras_types.h |  3 +
> >  drivers/gpu/drm/xe/xe_ras.c           | 91 ++++++++++++++++++++++++++-
> >  2 files changed, 93 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_drm_ras_types.h b/drivers/gpu/drm/xe/xe_drm_ras_types.h
> > index 0be218ba2db7..8fb5d6457c53 100644
> > --- a/drivers/gpu/drm/xe/xe_drm_ras_types.h
> > +++ b/drivers/gpu/drm/xe/xe_drm_ras_types.h
> > @@ -46,6 +46,9 @@ struct xe_drm_ras {
> >  
> >  	/** @disable_vram_page_offline: cached configfs policy, immutable after init */
> >  	bool disable_vram_page_offline;
> > +
> > +	/** @cper_on_query: emit a CPER record on each counter query */
> > +	bool cper_on_query;
> 
> Nack, cper is unrelated to drm_ras and should not be mixed here.
> This belongs to xe_device with its own state that is maintained
> as something like struct xe_cper.

agree

> 
> Same goes for disable_vram_page_offline, but that I think is upto
> the maintainers.

agree as well, at least until we don't get that mem page offline that Aravind is doing.

> 
> >  };
> >  
> >  #endif
> > diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> > index 7e3e62750448..288dbc0942f5 100644
> > --- a/drivers/gpu/drm/xe/xe_ras.c
> > +++ b/drivers/gpu/drm/xe/xe_ras.c
> > @@ -4,6 +4,7 @@
> >   */
> >  
> >  #include "xe_configfs.h"
> > +#include "xe_cper.h"
> >  #include "xe_debugfs.h"
> >  #include "xe_device.h"
> >  #include "xe_drm_ras.h"
> > @@ -199,6 +200,34 @@ static inline const char *comp_to_str(u8 component)
> >  	return xe_ras_components[component];
> >  }
> >  
> > +static u32 ras_comp_to_hw_sigid(u8 component)
> 
> All the switcheroos are above sev_to_str(), it'd be quite sad for these
> to be left alone here.
> 
> > +{
> > +	switch (component) {
> > +	case XE_RAS_COMP_DEVICE_MEMORY:
> > +		return XE_SIGID_DEVICE_MEMORY;
> > +	case XE_RAS_COMP_CORE_COMPUTE:
> > +		return XE_SIGID_CORE_COMPUTE;
> > +	case XE_RAS_COMP_PCIE:
> > +		return XE_SIGID_PCIE;
> > +	case XE_RAS_COMP_FABRIC:
> > +		return XE_SIGID_FABRIC;
> > +	case XE_RAS_COMP_SOC_INTERNAL:
> > +		return XE_SIGID_SOC_INTERNAL;
> > +	default:
> > +		return U32_MAX;
> > +	}
> > +}
> > +
> > +static u8 ras_sev_to_cper_sev(u8 ras_sev)
> 
> Ditto.
> 
> > +{
> > +	switch (ras_sev) {
> > +	case XE_RAS_SEV_CORRECTABLE:   return CPER_SEV_CORRECTED;
> > +	case XE_RAS_SEV_UNCORRECTABLE: return CPER_SEV_RECOVERABLE;
> > +	case XE_RAS_SEV_INFORMATIONAL: return CPER_SEV_INFORMATIONAL;
> > +	default:                       return CPER_SEV_RECOVERABLE;
> 
> I like this formatting but these should be consistent with similar existing
> switcheroos. So whatever your preference, please make all of them consistent.
> 
> > +	}
> > +}
> > +
> >  static struct pci_dev *find_usp_dev(struct pci_dev *pdev)
> >  {
> >  	struct pci_dev *vsp;
> > @@ -612,6 +641,7 @@ enum xe_ras_recovery_action xe_ras_process_errors(struct xe_device *xe)
> >   */
> >  int xe_ras_get_counter(struct xe_device *xe, u8 severity, u8 component, u32 *value)
> >  {
> > +	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> >  	struct xe_ras_error_class counter = {0};
> >  	struct xe_ras_get_counter_response response = {0};
> >  	int ret;
> > @@ -623,8 +653,13 @@ int xe_ras_get_counter(struct xe_device *xe, u8 severity, u8 component, u32 *val
> >  	ret = xe_ras_get_counter_response(xe, &counter, &response);
> >  	if (ret)
> >  		return ret;
> > -
> 
> Why?
> 
> >  	*value = response.value;
> > +
> > +	if (xe->ras.cper_on_query)
> > +		xe_emit_hardware_error_cper(pdev, ras_sev_to_cper_sev(counter.common.severity),
> > +					    ras_comp_to_hw_sigid(counter.common.component),
> > +					    (struct xe_ras_error_class *)&counter,
> > +					    (struct xe_ras_get_counter_response *)&response);
> 
> Why the casting? What changed?
> 
> >  	return 0;
> >  }
> >  
> > @@ -1069,6 +1104,56 @@ static const struct attribute_group gpu_health_group = {
> >  	.attrs = gpu_health_attrs,
> >  };
> >  
> > +static ssize_t cper_on_query_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +	struct xe_device *xe = kdev_to_xe_device(dev);
> > +
> > +	return sysfs_emit(buf, "%u\n", xe->ras.cper_on_query);
> > +}
> > +
> > +static ssize_t cper_on_query_store(struct device *dev, struct device_attribute *attr,
> > +				   const char *buf, size_t count)
> > +{
> > +	struct xe_device *xe = kdev_to_xe_device(dev);
> > +	bool enable;
> > +	int ret;
> > +
> > +	ret = kstrtobool(buf, &enable);
> > +	if (ret)
> > +		return ret;
> > +
> > +	xe->ras.cper_on_query = enable;
> > +
> > +	return count;
> > +}
> > +static DEVICE_ATTR_ADMIN_RW(cper_on_query);
> > +
> > +static struct attribute *cper_on_query_attrs[] = {
> > +	&dev_attr_cper_on_query.attr,
> > +	NULL
> > +};
> > +
> > +/**
> > + * DOC: CPER on query
> 
> Is this actually hooked to the docs?
> 
> > + *
> > + * On Intel Xe platforms that support the RAS error reporting interface,
> > + * the driver can emit a CPER (Common Platform Error Record) each time an
> > + * error counter is queried. This behaviour is controlled through the
> > + * following sysfs attribute::
> > + *
> > + *     /sys/bus/pci/devices/<device>/cper_on_query
> > + *
> > + * The attribute is a boolean (``0`` or ``1``). When set to ``1``, every
> > + * counter query emits a CPER record built from the associated info queue
> > + * data; when set to ``0`` (default) no record is emitted on query.
> > + *
> > + * Reading the attribute is available to all users and returns the current
> > + * setting, whereas writing is restricted to administrative users.
> > + */
> > +static const struct attribute_group cper_on_query_group = {
> > +	.attrs = cper_on_query_attrs,
> > +};
> > +
> >  /**
> >   * xe_ras_init - Initialize Xe RAS
> >   * @xe: xe device instance
> > @@ -1098,4 +1183,8 @@ void xe_ras_init(struct xe_device *xe)
> >  	ret = devm_device_add_group(xe->drm.dev, &gpu_health_group);
> >  	if (ret)
> >  		xe_err(xe, "Failed to create GPU health sysfs, err=%d\n", ret);
> > +
> > +	ret = devm_device_add_group(xe->drm.dev, &cper_on_query_group);
> > +	if (ret)
> > +		xe_err(xe, "Failed to create cper_on_query sysfs, err=%d\n", ret);
> 
> I really dislike that we're ignoring error here. Same was done with
> gpu_health_group. I know they're non-fatal but it just makes them
> harder to root cause when something else breaks as a side-effect of
> this. But again, not my call.
> 
> Raag
> 
> >  }
> > -- 
> > 2.54.0
> >