Re: [PATCH v2] drm/xe/cri: Set downstream access to L3 class 3 and write-back

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/xe: GRAPHICS_VERSION() in mert_was fails with NULL gt in rule_matches()
--

commit 5c800a0713723914264e86ed44f6bc8185c5ecb8
Author: Fei Yang <fei.yang@intel.com>

drm/xe/cri: Set downstream access to L3 class 3 and write-back

This commit adds a dedicated MERT workaround table and an RTP helper for
full-register device MMIO writes, programming DWNSTRM_ACCESS_MODE_GAMREQ_MERTSS
with 0xcc on graphics version 3511 during probe and resume.

> diff --git a/drivers/gpu/drm/xe/xe_rtp.c b/drivers/gpu/drm/xe/xe_rtp.c
> index 6a8d6ea68f25d..539d06eaba6fc 100644
> --- a/drivers/gpu/drm/xe/xe_rtp.c
> +++ b/drivers/gpu/drm/xe/xe_rtp.c

[ ... ]

> @@ -413,6 +414,44 @@ void xe_rtp_process(struct xe_rtp_process_ctx *ctx,
>  }
>  EXPORT_SYMBOL_IF_KUNIT(xe_rtp_process);
>  
> +/**
> + * xe_rtp_apply_device_mmio_writes - Apply full-register device MMIO writes
> + * @xe: Device to apply the actions to
> + * @table: RTP action table containing only full-register writes
> + *
> + * Device registers are accessed via the root tile without GT forcewake or
> + * GT save/restore bookkeeping.
> + *
> + * Return: 0 on success or -EINVAL for an unsupported action.
> + */
> +int xe_rtp_apply_device_mmio_writes(struct xe_device *xe,
> +				    const struct xe_rtp_table_sr *table)
> +{
> +	struct xe_mmio *mmio = xe_root_tile_mmio(xe);
> +
> +	for (size_t i = 0; i < table->n_entries; i++) {
> +		const struct xe_rtp_entry_sr *entry = &table->entries[i];
> +
> +		if (!rule_matches(xe, NULL, NULL, entry->rules, entry->n_rules))
> +			continue;
> +
> +		for (size_t j = 0; j < entry->n_actions; j++) {
> +			const struct xe_rtp_action *action = &entry->actions[j];
> +
> +			if (drm_WARN_ON(&xe->drm, action->clr_bits != ~0u ||
> +					action->reg.mcr || action->reg.masked ||
> +					action->flags || action->use_func))
> +				return -EINVAL;
> +
> +			xe_mmio_write32(mmio, action->reg, action->set_bits);
> +		}
> +
> +		drm_dbg(&xe->drm, "Applied device workaround %s\n", entry->name);
> +	}
> +
> +	return 0;
> +}
> +
>  bool xe_rtp_match_always(const struct xe_device *xe,
>  			 const struct xe_gt *gt,
>  			 const struct xe_hw_engine *hwe)

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_wa.c b/drivers/gpu/drm/xe/xe_wa.c
> index c94fe7753f9a9..18f953ffbc1cf 100644
> --- a/drivers/gpu/drm/xe/xe_wa.c
> +++ b/drivers/gpu/drm/xe/xe_wa.c

[ ... ]

> @@ -823,6 +825,13 @@ VISIBLE_IF_KUNIT __maybe_unused const struct xe_rtp_table device_oob_was = {
>  };
>  EXPORT_SYMBOL_IF_KUNIT(device_oob_was);
>  
> +static const struct xe_rtp_table_sr mert_was = XE_RTP_TABLE_SR(
> +	{ XE_RTP_NAME("14028586678"),
> +	  XE_RTP_RULES(GRAPHICS_VERSION(3511)),
> +	  XE_RTP_ACTIONS(WR(DWNSTRM_ACCESS_MODE_GAMREQ_MERTSS, 0xcc))
> +	},
> +);

[Severity: High]
Does using GRAPHICS_VERSION(3511) in mert_was trigger a drm_WARN_ON and
prevent workaround 14028586678 from ever being applied?

When xe_wa_apply_mert_workarounds() runs on a non-VF device with MERT
during xe_device_probe(), xe_pm_resume(), or xe_pm_runtime_resume(), it
calls xe_rtp_apply_device_mmio_writes(xe, &mert_was), which evaluates the
table rules with gt set to NULL:

drivers/gpu/drm/xe/xe_rtp.c:xe_rtp_apply_device_mmio_writes() {
	...
		if (!rule_matches(xe, NULL, NULL, entry->rules, entry->n_rules))
			continue;
	...
}

However, GRAPHICS_VERSION() expands to XE_RTP_MATCH_GRAPHICS_VERSION, which
requires a non-NULL gt in rule_match_item():

drivers/gpu/drm/xe/xe_rtp.c:rule_match_item() {
	...
	case XE_RTP_MATCH_GRAPHICS_VERSION:
		if (drm_WARN_ON(&xe->drm, !gt))
			return false;

		return xe->info.graphics_verx100 == r->ver_start &&
			(!has_samedia(xe) || !xe_gt_is_media_type(gt));
	...
}

Because gt is NULL, wouldn't rule_match_item() hit drm_WARN_ON(&xe->drm, !gt)
and return false, skipping the write to DWNSTRM_ACCESS_MODE_GAMREQ_MERTSS?

[ ... ]

> +int xe_wa_apply_mert_workarounds(struct xe_device *xe)
> +{
> +	if (!xe_device_has_mert(xe) || IS_SRIOV_VF(xe))
> +		return 0;
> +
> +	return xe_rtp_apply_device_mmio_writes(xe, &mert_was);
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020001513.1528267-1-daniel.charles@intel.com?part=1