Re: [CI v3] drm/xe/vram: Add early VRAM health check

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Forcing xe_device_l2_flush() causes unsupported MMIO writes to Xe2-specific registers on pre-Xe2 platforms.
- [High] xe_migrate_debug_ccs_overlap() ignores migration job creation failures, causing xe_vram_memtest() to silently corrupt CCS metadata with uninitialized memory.
- [Medium] The memtest_bo reserved for each tile is leaked if xe_device_probe() fails before reaching xe_vram_memtest().
- [Medium] xe_device_l2_flush() incorrectly flushes only the root GT's L2 cache on multi-tile systems, rendering the flush ineffective for non-root tiles.
- [Medium] Missing write memory barrier (xe_device_wmb) before xe_device_l2_flush creates a race condition.
--

commit 1d02ef806f72d6fb5a8aebd6553ae34e7b2019e8
Author: Matthew Auld <matthew.auld@intel.com>

drm/xe/vram: Add early VRAM health check
    
Use the last page as a canary for BAR sizing, CCS sizing, identity map
setup. If something is wrong the last page is where we will likely find
it. Hit it with everything we have.

> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 396d02eb2af83..601c372775eff 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -1051,6 +1051,8 @@ int xe_device_probe(struct xe_device *xe)
>  	if (err)
>  		return err;
>  
> +	xe_vram_reserve_memtest_bo(xe);
> +
>  	for_each_tile(tile, xe, id) {
>  		err = xe_tile_init(tile);
>  		if (err)
>  			return err;

[Severity: Medium]
Will this leak the allocated BOs if one of these subsequent initialization
steps fails? xe_vram_reserve_memtest_bo() pins the BO into
tile->mem.memtest_bo but doesn't seem to register a devm action or provide
automatic cleanup for the probe failure path before xe_vram_memtest()
completes.

[ ... ]
> @@ -1270,7 +1276,7 @@ bool xe_device_is_l2_flush_optimized(struct xe_device *xe)
>  	return false;
>  }
>  
> -void xe_device_l2_flush(struct xe_device *xe)
> +void xe_device_l2_flush(struct xe_device *xe, bool force)
>  {
>  	struct xe_gt *gt;
>  
>  	gt = xe_root_mmio_gt(xe);

[Severity: Medium]
Does this function hardcode the flush to only the root GT? When called inside
the per-tile loop in xe_vram_memtest(), it looks like this will always only
flush Tile 0's L2 cache, skipping the other tiles.

>  	if (!gt)
>  		return;
>  
> -	if (!XE_GT_WA(gt, 16023588340))
> +	if (!force && !XE_GT_WA(gt, 16023588340))
>  		return;
>  
>  	CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FW_GT);

[Severity: High]
Does forcing this bypass the workaround check on pre-Xe2 platforms?
XE2_GLOBAL_INVAL is an Xe2-specific register, but xe_vram_memtest() sets
force to true on any device with flat CCS (e.g., DG2). Could this cause
unsupported MMIO writes and hardware errors on those older platforms?

[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
> index 75b83687f1b5d..271bcfb0859ce 100644
> --- a/drivers/gpu/drm/xe/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/xe_migrate.c
[ ... ]
> @@ -2633,3 +2633,64 @@ void xe_migrate_job_lock_assert(struct xe_exec_queue *q)
> +#if IS_ENABLED(CONFIG_DRM_XE_DEBUG_MEM)
> +void xe_migrate_debug_ccs_overlap(struct xe_migrate *m,
> +				  struct xe_bo *scratch_bo,
> +				  bool write_to_ccs)
> +{
[ ... ]
> +	job = xe_bb_create_migration_job(m->q, bb,
> +					 xe_migrate_batch_base(m, xe->info.has_usm),
> +					 0);
> +	if (!IS_ERR(job)) {
> +		xe_sched_job_add_migrate_flush(job, MI_FLUSH_DW_CCS);
> +
> +		mutex_lock(&m->job_mutex);
> +		xe_sched_job_arm(job);
> +
> +		fence = dma_fence_get(&job->drm.s_fence->finished);
> +		xe_sched_job_push(job);
> +		mutex_unlock(&m->job_mutex);
> +
> +		dma_fence_wait(fence, false);
> +		dma_fence_put(fence);
> +	} else {
> +		drm_warn(&xe->drm, "Failed to create job for VRAM overlap check\n");
> +	}
> +
> +	xe_bb_free(bb, NULL);
> +}
> +#endif

[Severity: High]
What happens if the migration job fails to create here? This function returns
void, swallowing the error. If the first call in xe_vram_memtest() fails to
save the original CCS metadata, it leaves scratch_bo_before uninitialized,
and the final call will later restore this uninitialized garbage into the CCS
metadata. Could this silently corrupt hardware state?

> diff --git a/drivers/gpu/drm/xe/xe_vram.c b/drivers/gpu/drm/xe/xe_vram.c
> index 7b4478fb1697c..393ab8761686c 100644
> --- a/drivers/gpu/drm/xe/xe_vram.c
> +++ b/drivers/gpu/drm/xe/xe_vram.c
[ ... ]
> +int xe_vram_memtest(struct xe_device *xe)
> +{
[ ... ]
> +		if (xe_device_has_flat_ccs(xe)) {
> +			struct xe_bo *scratch_bo_before;
> +			struct xe_bo *scratch_bo_after;
[ ... ]
> +			/* Save original CCS metadata for PA 0 */
> +			xe_migrate_debug_ccs_overlap(tile->migrate, scratch_bo_before, false);
> +
> +			/* Fill last page with CPU */
> +			xe_map_memset(xe, &last_page_bo->vmap, 0, 0x5A, SZ_64K);
> +			xe_device_l2_flush(xe, true);

[Severity: Medium]
Should there be an xe_device_wmb() here before flushing the L2 cache?
xe_map_memset() uses Write-Combining (WC) semantics, so the GPU might read
stale data if the CPU's WC buffers aren't fully flushed to memory before the
MMIO flush command is received.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827144244.477862-2-matthew.auld@intel.com?part=1