Re: [PATCH v4 2/5] drm/xe/migrate: Support copying between sysmem and stolen

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/xe/migrate: unmapped identity map used for stolen memory on dgfx
- [High] drm/xe/migrate: invalid devmem PTE flag for stolen on pre-1270 igfx
- [Medium] drm/xe/migrate: missing PTE_UPDATE_FLAG_ALLOW_IDENTITY in blt_copy()
- [Low] drm/xe/migrate: consolidate identity map check in xe_migrate_allow_identity()
--

commit dee6bf0b0a807a0e7400bee24300c61c27406789
Author: Maarten Lankhorst <dev@lankhorst.se>

drm/xe/migrate: Support copying between sysmem and stolen

This commit updates xe_migrate_copy() and xe_migrate_clear() to treat
stolen memory as device memory while restricting the VRAM identity map
to discrete graphics so integrated stolen BOs can be copied to and from
sysmem.

> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
> index 19c37eb315893..027a2c9a0a286 100644
> --- a/drivers/gpu/drm/xe/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/xe_migrate.c
> @@ -907,6 +907,7 @@ static bool xe_migrate_allow_identity(u64 size, const struct xe_res_cursor *cur)
>  
>  #define PTE_UPDATE_FLAG_IS_VRAM		BIT(0)
>  #define PTE_UPDATE_FLAG_IS_COMP_PTE	BIT(1)
> +#define PTE_UPDATE_FLAG_ALLOW_IDENTITY	BIT(2)
>  
>  static u32 pte_update_size(struct xe_migrate *m,
>  			   u32 flags,
> @@ -918,9 +919,10 @@ static u32 pte_update_size(struct xe_migrate *m,
>  	u32 cmds = 0;
>  	bool is_vram = PTE_UPDATE_FLAG_IS_VRAM & flags;
>  	bool is_comp_pte = PTE_UPDATE_FLAG_IS_COMP_PTE & flags;
> +	bool allow_identity = PTE_UPDATE_FLAG_ALLOW_IDENTITY & flags;
>  
>  	*L0_pt = pt_ofs;
> -	if (is_vram && xe_migrate_allow_identity(*L0, cur)) {
> +	if (is_vram && allow_identity && xe_migrate_allow_identity(*L0, cur)) {

[Severity: Medium]
Does requiring PTE_UPDATE_FLAG_ALLOW_IDENTITY in pte_update_size() break
blt_copy() in drivers/gpu/drm/xe/tests/xe_migrate.c?

In blt_copy(), pte_flags is set without PTE_UPDATE_FLAG_ALLOW_IDENTITY,
while emit_pte() is still skipped whenever src_is_vram or dst_is_vram is
true:

drivers/gpu/drm/xe/tests/xe_migrate.c:blt_copy() {
    ...
		pte_flags = src_is_vram ? (PTE_UPDATE_FLAG_IS_VRAM |
					   PTE_UPDATE_FLAG_IS_COMP_PTE) : 0;
		batch_size += pte_update_size(m, pte_flags, src, &src_it, &src_L0,
					      &src_L0_ofs, &src_L0_pt, 0, 0,
					      avail_pts);
    ...
		if (src_is_vram)
			xe_res_next(&src_it, src_L0);
		else
			emit_pte(m, bb, src_L0_pt, src_is_vram, false,
				 &src_it, src_L0, src);
    ...
}

Because pte_update_size() no longer takes the identity map branch for
VRAM in blt_copy(), src_L0_ofs and dst_L0_ofs are set to dynamic page
table addresses via xe_migrate_vm_addr(pt_ofs, 0) that are never
populated by emit_pte(), causing xe_validate_ccs_kunit to read and write
through unpopulated PTEs.

>  		/* Offset into identity map. */
>  		*L0_ofs = xe_migrate_vram_ofs(tile_to_xe(m->tile),
>  					      cur->start + vram_region_gpu_offset(res),
> @@ -1206,6 +1208,14 @@ static u32 xe_migrate_ccs_copy(struct xe_migrate *m,
>  	return flush_flags;
>  }
>  
> +static bool is_devmem(struct xe_bo *bo, struct ttm_resource *res)
> +{
> +	if (mem_type_is_vram(res->mem_type) || res->mem_type == XE_PL_STOLEN)
> +		return true;
> +
> +	return false;
> +}
> +
>  static struct dma_fence *__xe_migrate_copy(struct xe_migrate *m,
>  					   struct xe_bo *src_bo,
>  					   struct xe_bo *dst_bo,
> @@ -1226,8 +1236,8 @@ static struct dma_fence *__xe_migrate_copy(struct xe_migrate *m,
>  	int err;
>  	bool src_is_pltt = src->mem_type == XE_PL_TT;
>  	bool dst_is_pltt = dst->mem_type == XE_PL_TT;
> -	bool src_is_vram = mem_type_is_vram(src->mem_type);
> -	bool dst_is_vram = mem_type_is_vram(dst->mem_type);
> +	bool src_is_vram = is_devmem(src_bo, src);
> +	bool dst_is_vram = is_devmem(dst_bo, dst);

[Severity: High]
On integrated platforms with GRAPHICS_VERx100(xe) < 1270 (such as TGL,
RKL, and ADL), will returning true from is_devmem() for XE_PL_STOLEN
cause emit_pte() and emit_clear_main_copy() to program invalid device
memory flags?

In drivers/gpu/drm/xe/xe_bo.c, xe_bo_is_stolen_devmem() only treats
stolen memory as devmem on graphics version 12.70 and newer:

bool xe_bo_is_stolen_devmem(struct xe_bo *bo)
{
	return xe_bo_is_stolen(bo) &&
		GRAPHICS_VERx100(xe_bo_device(bo)) >= 1270;
}

However, is_devmem() ignores its bo argument and returns true for all
XE_PL_STOLEN resources. When copying or clearing a stolen BO in
__xe_migrate_copy() or xe_migrate_clear() on pre-12.70 integrated
platforms, passing is_vram = true into emit_pte() sets devmem = true and
encodes XE_PPGTT_PTE_DM into the PTEs:

drivers/gpu/drm/xe/xe_migrate.c:emit_pte() {
    ...
			if (is_vram) {
				...
				addr += vram_region_gpu_offset(res);
				devmem = true;
			}

			addr = vm->pt_ops->pte_encode_addr(m->tile->xe,
							   addr, pat_index,
							   0, devmem, flags);
    ...
}

Similarly, xe_migrate_clear() passes clear_vram = true to
emit_clear() -> emit_clear_main_copy(), which selects local memory (0x0)
instead of system memory (0x1) in XY_FAST_COLOR_BLT_MEM_TYPE_SHIFT.
Should is_devmem() check xe_bo_is_stolen_devmem(bo) instead of
res->mem_type == XE_PL_STOLEN, or should the cursor initialization for
XE_PL_STOLEN be separated from the devmem PTE and blitter flags on
pre-12.70 platforms?

>  	bool type_device = src_bo->ttm.type == ttm_bo_type_device;
>  	bool needs_ccs_emit = type_device && xe_migrate_needs_ccs_emit(xe);
>  	bool copy_ccs = xe_device_has_flat_ccs(xe) &&
> @@ -1287,6 +1297,7 @@ static struct dma_fence *__xe_migrate_copy(struct xe_migrate *m,
>  
>  		pte_flags = src_is_vram ? PTE_UPDATE_FLAG_IS_VRAM : 0;
>  		pte_flags |= use_comp_pat ? PTE_UPDATE_FLAG_IS_COMP_PTE : 0;
> +		pte_flags |= IS_DGFX(xe) ? PTE_UPDATE_FLAG_ALLOW_IDENTITY : 0;
>  		batch_size += pte_update_size(m, pte_flags, src, &src_it, &src_L0,
>  					      &src_L0_ofs, &src_L0_pt, 0, 0,
>  					      avail_pts);
> @@ -1294,6 +1305,7 @@ static struct dma_fence *__xe_migrate_copy(struct xe_migrate *m,
>  			dst_L0_ofs = src_L0_ofs;
>  		} else {
>  			pte_flags = dst_is_vram ? PTE_UPDATE_FLAG_IS_VRAM : 0;
> +			pte_flags |= IS_DGFX(xe) ? PTE_UPDATE_FLAG_ALLOW_IDENTITY : 0;
>  			batch_size += pte_update_size(m, pte_flags, dst,
>  						      &dst_it, &src_L0,
>  						      &dst_L0_ofs, &dst_L0_pt,
> @@ -1320,13 +1332,13 @@ static struct dma_fence *__xe_migrate_copy(struct xe_migrate *m,
>  			goto err_sync;
>  		}
>  
> -		if (src_is_vram && xe_migrate_allow_identity(src_L0, &src_it))
> +		if (src_is_vram && IS_DGFX(xe) && xe_migrate_allow_identity(src_L0, &src_it))
>  			xe_res_next(&src_it, src_L0);
>  		else
>  			emit_pte(m, bb, src_L0_pt, src_is_vram, copy_system_ccs || use_comp_pat,
>  				 &src_it, src_L0, src);
>  
> -		if (dst_is_vram && xe_migrate_allow_identity(src_L0, &dst_it))
> +		if (dst_is_vram && IS_DGFX(xe) && xe_migrate_allow_identity(src_L0, &dst_it))
>  			xe_res_next(&dst_it, src_L0);
>  		else if (!copy_only_ccs)
>  			emit_pte(m, bb, dst_L0_pt, dst_is_vram, copy_system_ccs,

[Severity: High]
What happens when __xe_migrate_copy() or xe_migrate_clear() operates on
an XE_PL_STOLEN BO on discrete graphics (such as during pinned BO
eviction/restore via xe_bo_evict_pinned_copy() and
xe_bo_restore_pinned())?

Because is_devmem() returns true for XE_PL_STOLEN and xe_res_first()
sets cur->size = size for XE_PL_STOLEN, both pte_update_size() and the
emit_pte() checks in __xe_migrate_copy() and xe_migrate_clear() will
take the identity map path when IS_DGFX(xe) is true.

However, xe_migrate_program_identity() only maps usable VRAM up to
xe_vram_region_usable_size(vram) + dpa_base, which excludes stolen
memory at the top of VRAM:

drivers/gpu/drm/xe/xe_migrate.c:xe_migrate_program_identity() {
    ...
	u64 vram_limit = xe_vram_region_usable_size(vram) + dpa_base;
    ...
}

Won't skipping emit_pte() and using xe_migrate_vram_ofs() for
XE_PL_STOLEN on DGFX access unmapped virtual addresses in the migration
VM?

[ ... ]

> @@ -1927,7 +1939,7 @@ struct dma_fence *xe_migrate_clear(struct xe_migrate *m,
>  				   struct ttm_resource *dst,
>  				   u32 clear_flags)
>  {
> -	bool clear_vram = mem_type_is_vram(dst->mem_type);
> +	bool clear_vram = is_devmem(bo, dst);
>  	bool clear_bo_data = XE_MIGRATE_CLEAR_FLAG_BO_DATA & clear_flags;
>  	bool clear_ccs = XE_MIGRATE_CLEAR_FLAG_CCS_DATA & clear_flags;
>  	struct xe_gt *gt = m->tile->primary_gt;
> @@ -1967,6 +1979,7 @@ struct dma_fence *xe_migrate_clear(struct xe_migrate *m,
>  
>  		/* Calculate final sizes and batch size.. */
>  		pte_flags = clear_vram ? PTE_UPDATE_FLAG_IS_VRAM : 0;
> +		pte_flags |= IS_DGFX(xe) ? PTE_UPDATE_FLAG_ALLOW_IDENTITY : 0;
>  		batch_size = 1 +
>  			pte_update_size(m, pte_flags, src, &src_it,
>  					&clear_L0, &clear_L0_ofs, &clear_L0_pt,
> @@ -1989,7 +2002,7 @@ struct dma_fence *xe_migrate_clear(struct xe_migrate *m,
>  
>  		size -= clear_L0;
>  		/* Preemption is enabled again by the ring ops. */
> -		if (clear_vram && xe_migrate_allow_identity(clear_L0, &src_it)) {
> +		if (clear_vram && IS_DGFX(xe) && xe_migrate_allow_identity(clear_L0, &src_it)) {

[Severity: Low]
As reported by Matthew Brost on v1 [1], this isn't a bug, but could the
identity map check be consolidated inside xe_migrate_allow_identity()
(for instance by checking cur->mem_type != XE_PL_STOLEN) rather than
adding PTE_UPDATE_FLAG_ALLOW_IDENTITY and repeating IS_DGFX(xe) across
__xe_migrate_copy() and xe_migrate_clear()?

In the v1 discussion, it was noted that disallowing XE_PL_STOLEN in
xe_migrate_allow_identity() would avoid scattering these checks (and
would also avoid the DGFX stolen identity map issue above and the
blt_copy() mismatch). Also, if is_devmem() does not use its struct xe_bo
*bo parameter, should that parameter be dropped or used to check
xe_bo_is_stolen_devmem(bo)?

[1] https://lore.kernel.org/r/al5yelaY34ls2%2Fvw@gsse-cloud1.jf.intel.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105161444.593259-7-dev@lankhorst.se?part=2