From: "Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>
To: Riana Tauro <riana.tauro@intel.com>, <intel-xe@lists.freedesktop.org>
Cc: <anshuman.gupta@intel.com>, <rodrigo.vivi@intel.com>,
<aravind.iddamsetty@linux.intel.com>, <badal.nilawar@intel.com>,
<raag.jadav@intel.com>, <ravi.kishore.koppuravuri@intel.com>,
<mallesh.koujalagi@intel.com>, <soham.purkait@intel.com>,
<tejas.upadhyay@intel.com>
Subject: Re: [PATCH v2 3/6] drm/xe: Separate drm-ras netlink data from device and firmware RAS state
Date: Tue, 8 Sep 2026 00:34:40 +0530 [thread overview]
Message-ID: <c6b2e25b-ced3-41a7-b2a0-2029877bf351@intel.com> (raw)
In-Reply-To: <20260907094706.1407436-11-riana.tauro@intel.com>
On 07-09-2026 15:17, Riana Tauro wrote:
> Keep the DRM RAS data used for netlink reporting separate from the
> device and firmware RAS state in xe_device.
>
> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
> ---
> drivers/gpu/drm/xe/xe_device_types.h | 11 +++++++++--
> drivers/gpu/drm/xe/xe_drm_ras.c | 16 ++++++++--------
> drivers/gpu/drm/xe/xe_drm_ras_types.h | 3 ---
> drivers/gpu/drm/xe/xe_hw_error.c | 6 +++---
> drivers/gpu/drm/xe/xe_ras.c | 15 +++++++--------
> drivers/gpu/drm/xe/xe_ras.h | 6 ++++++
> drivers/gpu/drm/xe/xe_ras_types.h | 10 ++++++++++
> drivers/gpu/drm/xe/xe_ttm_vram_mgr.c | 3 ++-
> 8 files changed, 45 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 180d450a6deb..f9dd65d94417 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -21,6 +21,7 @@
> #include "xe_platform_types.h"
> #include "xe_pmu_types.h"
> #include "xe_pt_types.h"
> +#include "xe_ras_types.h"
> #include "xe_sriov_pf_types.h"
> #include "xe_sriov_types.h"
> #include "xe_sriov_vf_types.h"
> @@ -550,8 +551,14 @@ struct xe_device {
> /** @pmu: performance monitoring unit */
> struct xe_pmu pmu;
>
> - /** @ras: RAS structure for device */
> - struct xe_drm_ras ras;
> + /** @ras: RAS (Reliability, Availability, Serviceability) structures */
> + struct {
> + /** @ras.nl_data: drm-ras netlink data */
> + struct xe_drm_ras nl_data;
> +
> + /** @ras.state: RAS device and firmware state */
> + struct xe_ras_state state;
> + } ras;
+1 for this separation. Looks much cleaner
>
> /** @i2c: I2C host controller */
> struct xe_i2c *i2c;
> diff --git a/drivers/gpu/drm/xe/xe_drm_ras.c b/drivers/gpu/drm/xe/xe_drm_ras.c
> index 7f3695707611..38d77561facf 100644
> --- a/drivers/gpu/drm/xe/xe_drm_ras.c
> +++ b/drivers/gpu/drm/xe/xe_drm_ras.c
> @@ -20,7 +20,7 @@ static int query_error_counter(struct xe_device *xe,
> enum drm_xe_ras_error_severity severity,
> u32 error_id, const char **name, u32 *val)
> {
> - struct xe_drm_ras *ras = &xe->ras;
> + struct xe_drm_ras *ras = &xe->ras.nl_data;
> struct xe_drm_ras_counter *info = ras->info[severity];
>
> if (!info || !info[error_id].name)
> @@ -41,7 +41,7 @@ static int clear_error_counter(struct xe_device *xe,
> enum drm_xe_ras_error_severity severity,
> u32 error_id)
> {
> - struct xe_drm_ras *ras = &xe->ras;
> + struct xe_drm_ras *ras = &xe->ras.nl_data;
> struct xe_drm_ras_counter *info = ras->info[severity];
>
> if (!info || !info[error_id].name)
> @@ -90,7 +90,7 @@ static int query_correctable_error_threshold(struct drm_ras_node *ep, u32 error_
> const char **name, u32 *threshold)
> {
> struct xe_device *xe = ep->priv;
> - struct xe_drm_ras *ras = &xe->ras;
> + struct xe_drm_ras *ras = &xe->ras.nl_data;
> struct xe_drm_ras_counter *info = ras->info[DRM_XE_RAS_ERR_SEV_CORRECTABLE];
>
> if (!info || !info[error_id].name)
> @@ -106,7 +106,7 @@ static int query_correctable_error_threshold(struct drm_ras_node *ep, u32 error_
> static int set_correctable_error_threshold(struct drm_ras_node *ep, u32 error_id, u32 threshold)
> {
> struct xe_device *xe = ep->priv;
> - struct xe_drm_ras *ras = &xe->ras;
> + struct xe_drm_ras *ras = &xe->ras.nl_data;
> struct xe_drm_ras_counter *info = ras->info[DRM_XE_RAS_ERR_SEV_CORRECTABLE];
>
> if (!info || !info[error_id].name)
> @@ -142,7 +142,7 @@ 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;
> + struct xe_drm_ras *ras = &xe->ras.nl_data;
> const char *device_name;
>
> device_name = kasprintf(GFP_KERNEL, "%04x:%02x:%02x.%d",
> @@ -190,7 +190,7 @@ static void cleanup_node(struct drm_device *drm, void *node)
>
> static int register_nodes(struct xe_device *xe)
> {
> - struct xe_drm_ras *ras = &xe->ras;
> + struct xe_drm_ras *ras = &xe->ras.nl_data;
> struct drm_ras_node *node;
> int i, ret;
>
> @@ -230,7 +230,7 @@ static int register_nodes(struct xe_device *xe)
> */
> void xe_drm_ras_event(struct xe_device *xe, u32 component, u32 severity, u32 value)
> {
> - struct xe_drm_ras *ras = &xe->ras;
> + struct xe_drm_ras *ras = &xe->ras.nl_data;
> struct xe_drm_ras_counter *info = ras->info[severity];
> struct drm_ras_node *node;
> int ret;
> @@ -260,7 +260,7 @@ void xe_drm_ras_event(struct xe_device *xe, u32 component, u32 severity, u32 val
> */
> int xe_drm_ras_init(struct xe_device *xe)
> {
> - struct xe_drm_ras *ras = &xe->ras;
> + struct xe_drm_ras *ras = &xe->ras.nl_data;
> struct drm_ras_node *node;
> int err;
>
> diff --git a/drivers/gpu/drm/xe/xe_drm_ras_types.h b/drivers/gpu/drm/xe/xe_drm_ras_types.h
> index 0be218ba2db7..8d729ad6a264 100644
> --- a/drivers/gpu/drm/xe/xe_drm_ras_types.h
> +++ b/drivers/gpu/drm/xe/xe_drm_ras_types.h
> @@ -43,9 +43,6 @@ struct xe_drm_ras {
>
> /** @info: info array for all types of errors */
> struct xe_drm_ras_counter *info[DRM_XE_RAS_ERR_SEV_MAX];
> -
> - /** @disable_vram_page_offline: cached configfs policy, immutable after init */
> - bool disable_vram_page_offline;
> };
>
> #endif
> diff --git a/drivers/gpu/drm/xe/xe_hw_error.c b/drivers/gpu/drm/xe/xe_hw_error.c
> index 5f2abc9485ff..f53a6b3055de 100644
> --- a/drivers/gpu/drm/xe/xe_hw_error.c
> +++ b/drivers/gpu/drm/xe/xe_hw_error.c
> @@ -240,7 +240,7 @@ static void log_soc_error(struct xe_tile *tile, const char * const *reg_info,
> {
> const char *severity_str = error_severity[severity];
> struct xe_device *xe = tile_to_xe(tile);
> - struct xe_drm_ras *ras = &xe->ras;
> + struct xe_drm_ras *ras = &xe->ras.nl_data;
> struct xe_drm_ras_counter *info = ras->info[severity];
> const char *name;
>
> @@ -260,7 +260,7 @@ static void gt_hw_error_handler(struct xe_tile *tile, const enum hardware_error
> {
> const enum drm_xe_ras_error_severity severity = hw_err_to_severity(hw_err);
> struct xe_device *xe = tile_to_xe(tile);
> - struct xe_drm_ras *ras = &xe->ras;
> + struct xe_drm_ras *ras = &xe->ras.nl_data;
> struct xe_drm_ras_counter *info = ras->info[severity];
> struct xe_mmio *mmio = &tile->mmio;
> unsigned long err_stat = 0;
> @@ -422,7 +422,7 @@ static void hw_error_source_handler(struct xe_tile *tile, const enum hardware_er
> const enum drm_xe_ras_error_severity severity = hw_err_to_severity(hw_err);
> const char *severity_str = error_severity[severity];
> struct xe_device *xe = tile_to_xe(tile);
> - struct xe_drm_ras *ras = &xe->ras;
> + struct xe_drm_ras *ras = &xe->ras.nl_data;
> struct xe_drm_ras_counter *info = ras->info[severity];
> unsigned long flags, err_src;
> u32 err_bit;
> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> index 648e46df831d..b5b0c1a3a663 100644
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c
> @@ -1126,19 +1126,18 @@ void xe_ras_init(struct xe_device *xe)
> {
> int ret;
>
> - /*
> - * TODO: Replace platform check with xe->info.has_disable_vram_page_offline
> - * once the feature flag is plumbed through device info.
> - */
> - if (xe->info.platform == XE_CRESCENTISLAND)
> - xe->ras.disable_vram_page_offline =
> - xe_configfs_get_disable_vram_page_offline(to_pci_dev(xe->drm.dev));
> -
> xe_drm_ras_init(xe);
>
> if (!xe->info.has_sysctrl)
> return;
>
I believe has_sysctrl is currently enabled only for CRI. Is it possible
that future platforms will support sysctrl but not VRAM page offlining?
If so, it would be better to have a separate device-info flag for that,
or at least keep a TODO here.
> + /*
> + * TODO: Replace platform check with xe->info.has_disable_vram_page_offline
> + * once the feature flag is plumbed through device info.
> + */
> + xe->ras.state.disable_page_offline =
> + xe_configfs_get_disable_vram_page_offline(to_pci_dev(xe->drm.dev));
> +
> if (IS_ENABLED(CONFIG_PCIEAER))
> ras_usp_aer_init(xe);
>
> diff --git a/drivers/gpu/drm/xe/xe_ras.h b/drivers/gpu/drm/xe/xe_ras.h
> index 0b8669f28d56..6e65443cd262 100644
> --- a/drivers/gpu/drm/xe/xe_ras.h
> +++ b/drivers/gpu/drm/xe/xe_ras.h
> @@ -7,6 +7,7 @@
> #define _XE_RAS_H_
>
> #include <linux/types.h>
> +#include "xe_device_types.h"
No, please. Let's not bring this dependency here. Move the function
definition to xe_ras.c.
> #include "xe_ras_types.h"
>
> struct xe_device;
> @@ -21,4 +22,9 @@ int xe_ras_set_threshold(struct xe_device *xe, u8 severity, u8 component, u32 th
> void xe_ras_init(struct xe_device *xe);
> enum xe_ras_recovery_action xe_ras_process_errors(struct xe_device *xe);
>
> +static inline bool xe_ras_get_disable_page_offline(struct xe_device *xe)
> +{
> + return xe->ras.state.disable_page_offline;
> +}
> +
> #endif
> diff --git a/drivers/gpu/drm/xe/xe_ras_types.h b/drivers/gpu/drm/xe/xe_ras_types.h
> index 7bfd86c9d135..ca459e785add 100644
> --- a/drivers/gpu/drm/xe/xe_ras_types.h
> +++ b/drivers/gpu/drm/xe/xe_ras_types.h
> @@ -414,4 +414,14 @@ struct xe_ras_set_health_response {
> /** @reserved1: Reserved for future use */
> u32 reserved1[2];
> } __packed;
> +
> +/* Device structures */
> +
> +/**
> + * struct xe_ras_state - RAS device and firmware state
> + */
> +struct xe_ras_state {
> + /** @disable_page_offline: cached configfs policy, immutable after init */
> + bool disable_page_offline;
> +};
> #endif
> diff --git a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> index 9a514d983e90..2c4722a956a0 100644
> --- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> +++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> @@ -24,6 +24,7 @@
> #include "xe_mmio.h"
> #include "xe_pm.h"
> #include "xe_printk.h"
> +#include "xe_ras.h"
> #include "xe_res_cursor.h"
> #include "xe_ttm_stolen_mgr.h"
> #include "xe_ttm_vram_mgr.h"
> @@ -933,7 +934,7 @@ int xe_ttm_vram_handle_addr_fault(struct xe_device *xe, u64 addr)
> vram_mgr = &vr->ttm;
> mm = &vram_mgr->mm;
>
> - if (xe->ras.disable_vram_page_offline) {
> + if (xe_ras_get_disable_page_offline(xe)) {
> xe_err(xe, "0x%llx is reported as corrupted address by HW\n",
> addr);
> return -EOPNOTSUPP;
next prev parent reply other threads:[~2026-09-07 19:04 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-07 9:47 [PATCH v2 0/6] Add support to handle memory double-bit ecc errors Riana Tauro
2026-09-07 9:47 ` [PATCH v2 1/6] drm/xe/xe_ras: Handle page offline requests for device memory " Riana Tauro
2026-09-07 10:14 ` sashiko-bot
2026-09-07 17:35 ` Ghimiray, Himal Prasad
2026-09-11 6:45 ` Tauro, Riana
2026-09-07 9:47 ` [PATCH v2 2/6] drm/xe/xe_ras: Add support to query page offline queue and list Riana Tauro
2026-09-07 10:00 ` sashiko-bot
2026-09-07 18:25 ` Ghimiray, Himal Prasad
2026-09-11 6:11 ` Tauro, Riana
2026-09-11 6:39 ` Ghimiray, Himal Prasad
2026-09-07 9:47 ` [PATCH v2 3/6] drm/xe: Separate drm-ras netlink data from device and firmware RAS state Riana Tauro
2026-09-07 19:04 ` Ghimiray, Himal Prasad [this message]
2026-09-07 19:21 ` Ghimiray, Himal Prasad
2026-09-09 5:43 ` Tauro, Riana
2026-09-07 9:47 ` [PATCH v2 4/6] drm/xe/xe_ras: Add function to get maximum pages firmware can store Riana Tauro
2026-09-07 19:21 ` Ghimiray, Himal Prasad
2026-09-07 19:24 ` Ghimiray, Himal Prasad
2026-09-09 5:23 ` Tauro, Riana
2026-09-07 9:47 ` [PATCH v2 5/6] drm/xe/xe_ttm_vram: Report max_pages reported by firmware in debugfs Riana Tauro
2026-09-07 19:26 ` Ghimiray, Himal Prasad
2026-09-07 9:47 ` [PATCH v2 6/6] drm/xe/xe_ras: Track offlined pages by firmware to avoid duplicates Riana Tauro
2026-09-07 10:02 ` sashiko-bot
2026-09-07 11:01 ` ✓ CI.KUnit: success for Add support to handle memory double-bit ecc errors (rev2) Patchwork
2026-09-07 11:45 ` ✓ Xe.CI.BAT: " Patchwork
2026-09-07 13:41 ` ✓ Xe.CI.FULL: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c6b2e25b-ced3-41a7-b2a0-2029877bf351@intel.com \
--to=himal.prasad.ghimiray@intel.com \
--cc=anshuman.gupta@intel.com \
--cc=aravind.iddamsetty@linux.intel.com \
--cc=badal.nilawar@intel.com \
--cc=intel-xe@lists.freedesktop.org \
--cc=mallesh.koujalagi@intel.com \
--cc=raag.jadav@intel.com \
--cc=ravi.kishore.koppuravuri@intel.com \
--cc=riana.tauro@intel.com \
--cc=rodrigo.vivi@intel.com \
--cc=soham.purkait@intel.com \
--cc=tejas.upadhyay@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox