Re: [PATCH v5 6/7] drm/xe: capture L3 node status registers in devcoredump

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/xe: unconditional MCR read of L3_NODE_IDLE in devcoredump warns on Xe_LP
--

commit a9405a6ae4ac2ac19e20c8fe6b73ddfd266de12d
Author: Nareshkumar Gollakoti <naresh.kumar.g@intel.com>

drm/xe: capture L3 node status registers in devcoredump

This commit adds the L3_NODE_IDLE and L3STAT_LBCF_GFX register definitions
and includes them as a node status section in the devcoredump snapshot. It
also extends struct xe_reg_desc to support MCR registers so they can be read
via xe_gt_mcr_unicast_read_any() during snapshot capture.

> diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> index 2e4cb4957479f..55f0eee4e20e8 100644
> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> @@ -454,6 +454,9 @@
>  #define   L3_ESC_MASK				REG_BIT(0)
>  #define   L3_ESC(value)				REG_FIELD_PREP(L3_ESC_MASK, value)
>  
> +#define L3_NODE_IDLE				XE_REG_MCR(0xb0b0)
> +#define L3STAT_LBCF_GFX				XE_REG_MCR(0xb128)
> +
>  #define XEHP_L3NODEARBCFG			XE_REG_MCR(0xb0b4)
>  #define   XEHP_LNESPARE				REG_BIT(19)
>  
> diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c
> index 8d59f80e2929f..de9ef3e0cb5dd 100644
> --- a/drivers/gpu/drm/xe/xe_devcoredump.c
> +++ b/drivers/gpu/drm/xe/xe_devcoredump.c

[ ... ]

> @@ -119,6 +121,11 @@ static const struct xe_reg_desc xe_dbg_gam_pf_reglist[] = {
>  	XE_REG_DESC(PAGE_FLTRPTQ_TAIL_GAMCTRL, "PAGE_FLTRPTQ_TAIL_GAMCTRL"),
>  };
>  
> +static const struct xe_reg_desc xe_dbg_l3_node_reglist[] = {
> +	XE_REG_DESC_MCR(L3_NODE_IDLE, "L3_NODE_IDLE"),
> +	XE_REG_DESC_MCR(L3STAT_LBCF_GFX, "L3STAT_LBCF_GFX"),
> +};

[ ... ]

> @@ -262,8 +275,13 @@ xe_dbg_reg_snapshot_capture_target(struct xe_gt *gt,
>  
>  	snap->desc	= reglist;
>  	snap->num_regs = reglist->num_regs;
> -	for (i = 0; i < reglist->num_regs; i++)
> -		snap->values[i] = xe_mmio_read32(&gt->mmio, reglist->regs[i].reg);
> +	for (i = 0; i < reglist->num_regs; i++) {
> +		if (reglist->regs[i].reg.mcr)
> +			snap->values[i] = xe_gt_mcr_unicast_read_any(gt,
> +								     reglist->regs[i].mcr_reg);
> +		else
> +			snap->values[i] = xe_mmio_read32(&gt->mmio, reglist->regs[i].reg);
> +	}
>  
>  	return snap;
>  }

[ ... ]

> @@ -602,6 +625,10 @@ static void xe_devcoredump_capture_gt_regs(struct xe_devcoredump_snapshot *ss,
>  						       other_gt,
>  						       XE_DBG_REGSET_GAM_PF,
>  						       XE_GT_TYPE_MAIN);
> +	ss->ns = xe_dbg_gt_snapshot_capture(gt,
> +					    other_gt,
> +					    XE_DBG_REGSET_L3_NODE,
> +					    XE_GT_TYPE_MAIN);
>  }

[Severity: Medium]
Will capturing XE_DBG_REGSET_L3_NODE unconditionally on all platforms trigger
a kernel warning in xe_gt_mcr_get_nonterminated_steering() when a devcoredump
is taken on Xe_LP platforms (such as TGL, RKL, DG1, or ADL)?

L3_NODE_IDLE is defined at offset 0xb0b0 with XE_REG_MCR(), which statically
sets reg.mcr = 1 in xe_dbg_l3_node_reglist[]. During a GPU hang or job
timeout, the capture path runs:

  devcoredump_snapshot()
    -> xe_devcoredump_capture_gt_regs()
      -> xe_dbg_gt_snapshot_capture()
        -> xe_dbg_reg_snapshot_capture_target()
          -> xe_gt_mcr_unicast_read_any(gt, L3_NODE_IDLE)
            -> xe_gt_mcr_get_nonterminated_steering()

However, on pre-Xe_HP platforms (GRAPHICS_VERx100(xe) < 1250), the
0x00b000-0x00b0ff LNCF/NODE range is not an MCR range. In
drivers/gpu/drm/xe/xe_gt_mcr.c:xe_gt_mcr_init_early(), only the L3BANK
(0x00b100-0x00b3ff) and DSS steering tables are registered:

	} else {
		gt->steering[L3BANK].ranges = xelp_l3bank_steering_table;
		gt->steering[DSS].ranges = xelp_dss_steering_table;
	}

When xe_gt_mcr_get_nonterminated_steering() looks up 0xb0b0 on those
platforms, it does not find it in any steering table, fires xe_gt_WARN(), and
steers MCR_SELECTOR to (0, 0) for a non-MCR register:

	xe_gt_WARN(gt, true,
		   "Did not find MCR register %#x in any MCR steering table\n",
		   reg.addr);
	*group = 0;
	*instance = 0;

Should xe_dbg_reg_snapshot_capture_target() check xe_gt_mcr_check_reg() (or
should XE_DBG_REGSET_L3_NODE capture be gated on platform version) before
calling xe_gt_mcr_unicast_read_any()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023060554.2643515-1-naresh.kumar.g@intel.com?part=6