Re: [PATCH 07/11] drm/xe: Run page table updates on the media GT when there is no primary

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/xe: uninitialized usm.bb_pool and NULL paging_hwe0 on media
  GT
- [Medium] drm/xe: silent no-op blit jobs on video engine in xe_migrate
  when no primary GT
It also noticed 2 related pre-existing issues this patch does not
introduce:
- [Medium] drm/xe: memory leak in xe_migrate_init() [1]
- [High] drm/xe/migrate: missing fence sync and leak in
  xe_migrate_vram_copy_chunk() [2]
--

commit 1ce7e5d250d71a445cdd76886f86f447f08f9a48
Author: Stuart Summers <stuart.summers@intel.com>

drm/xe: Run page table updates on the media GT when there is no primary

This commit adds xe_tile_pt_gt() and xe_tile_pt_class() so page table
updates and migration queue initialization fall back to the media GT and a
video decode engine when a tile has no primary GT.

> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
> index 19c37eb315893..754592c5cf881 100644
> --- a/drivers/gpu/drm/xe/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/xe_migrate.c
[ ... ]
> @@ -478,7 +479,7 @@ static void xe_migrate_prepare_vm(struct xe_tile *tile, struct xe_migrate *m,
>  		if (xe->info.has_usm) {
>  			xe_tile_assert(tile, xe_bo_size(batch) == SZ_1M);
>  
> -			batch = tile->primary_gt->usm.bb_pool->bo;
> +			batch = xe_tile_pt_gt(tile)->usm.bb_pool->bo;
>  			m->usm_batch_base_ofs = m->batch_base_ofs + SZ_1M;
>  			xe_tile_assert(tile, xe_bo_size(batch) == SZ_512K);
>  
> @@ -499,7 +500,7 @@ static void xe_migrate_prepare_vm(struct xe_tile *tile, struct xe_migrate *m,
>  		m->batch_base_ofs = xe_migrate_vram_ofs(xe, batch_addr, false);
>  
>  		if (xe->info.has_usm) {
> -			batch = tile->primary_gt->usm.bb_pool->bo;
> +			batch = xe_tile_pt_gt(tile)->usm.bb_pool->bo;
>  			batch_addr = xe_bo_addr(batch, 0, XE_PAGE_SIZE);
>  			m->usm_batch_base_ofs = xe_migrate_vram_ofs(xe, batch_addr, false);
>  		}

[Severity: High]
Can this dereference a NULL usm.bb_pool when xe_tile_pt_gt(tile) falls back
to the media GT?

On Xe2+ platforms with the primary GT disabled via gt_types_allowed=media in
configfs, xe->info.has_usm is still true from the hardware graphics
descriptor, but gt_init_with_all_forcewake() only initializes
gt->usm.bb_pool when xe_gt_is_main_type(gt) is true:

drivers/gpu/drm/xe/xe_gt.c:gt_init_with_all_forcewake() {
	if (xe_gt_is_main_type(gt)) {
		if (gt_to_xe(gt)->info.has_usm) {
			...
			gt->usm.bb_pool = xe_sa_bo_manager_init(...);
		}
	}

	if (gt == xe_tile_pt_gt(gt_to_tile(gt))) {
		err = xe_migrate_init(gt_to_tile(gt)->migrate);
}

In addition, when xe->info.has_usm is true, xe_migrate_init() and
xe_exec_queue_create_bind() require gt->usm.paging_hwe0 and
gt->usm.paging_logical_mask and return -EINVAL if they are unset, but those
fields are only populated from XE_ENGINE_CLASS_COPY engines and remain zero
on a media GT.

> @@ -787,7 +788,7 @@ static void xe_migrate_ulls_exit(struct work_struct *work)
>  int xe_migrate_init(struct xe_migrate *m)
>  {
>  	struct xe_tile *tile = m->tile;
> -	struct xe_gt *primary_gt = tile->primary_gt;
> +	struct xe_gt *gt = xe_tile_pt_gt(tile);
>  	struct xe_device *xe = tile_to_xe(tile);
>  	struct xe_vm *vm;
>  	int err;
> @@ -803,8 +804,8 @@ int xe_migrate_init(struct xe_migrate *m)
>  		goto err_out;
>  
>  	if (xe->info.has_usm) {
> -		struct xe_hw_engine *hwe0 = primary_gt->usm.paging_hwe0;
> -		u32 logical_mask = primary_gt->usm.paging_logical_mask;
> +		struct xe_hw_engine *hwe0 = gt->usm.paging_hwe0;
> +		u32 logical_mask = gt->usm.paging_logical_mask;
>  
>  		if (!hwe0 || !logical_mask) {
>  			err = -EINVAL;
> @@ -821,8 +822,8 @@ int xe_migrate_init(struct xe_migrate *m)
>  					    EXEC_QUEUE_FLAG_MIGRATE |
>  					    EXEC_QUEUE_FLAG_LOW_LATENCY, 0);
>  	} else {
> -		m->q = xe_exec_queue_create_class(xe, primary_gt, vm,
> -						  XE_ENGINE_CLASS_COPY,
> +		m->q = xe_exec_queue_create_class(xe, gt, vm,
> +						  xe_tile_pt_class(tile),
>  						  EXEC_QUEUE_FLAG_KERNEL |
>  						  EXEC_QUEUE_FLAG_MIGRATE, 0);
>  	}

[Severity: Medium]
This problem wasn't introduced by this patch, but if exec queue creation
fails in xe_migrate_init() [1], does jumping to err_out leak m->pt_bo?

xe_migrate_lock_prepare_vm() allocates m->pt_bo before the queue is created,
while the xe_migrate_fini() devm action that unpins m->pt_bo is only
registered after queue creation succeeds, and err_out only calls
xe_vm_close_and_put(vm).

[ ... ]

> @@ -1714,7 +1715,7 @@ struct dma_fence *xe_migrate_vram_copy_chunk(struct xe_bo *vram_bo, u64 vram_off
>  {
>  	struct xe_device *xe = xe_bo_device(vram_bo);
>  	struct xe_tile *tile = vram_bo->tile;
> -	struct xe_gt *gt = tile->primary_gt;
> +	struct xe_gt *gt = xe_tile_pt_gt(tile);
>  	struct xe_migrate *m = tile->migrate;
>  	struct dma_fence *fence = NULL;
>  	struct ttm_resource *vram = vram_bo->ttm.resource;

[Severity: High]
This problem wasn't introduced by this patch, but if xe_bb_new() or
xe_bb_create_migration_job() fails on a second or later pass of the copy
loop in xe_migrate_vram_copy_chunk() [2], does returning ERR_PTR(err)
directly leak the previous pass's fence and leave its DMA in flight?

Each completed pass stores a reference to the submitted job's fence in
local variable fence, but the error returns inside the while (size) loop do
not wait on or put fence before returning:

drivers/gpu/drm/xe/xe_migrate.c:xe_migrate_vram_copy_chunk() {
	while (size) {
		...
		bb = xe_bb_new(gt, batch_size, usm);
		if (IS_ERR(bb)) {
			err = PTR_ERR(bb);
			return ERR_PTR(err);
		}
		...
		scoped_guard(mutex, &m->job_mutex) {
			dma_fence_put(fence);
			fence = xe_migrate_job_push(m, job);
}

> @@ -1930,7 +1931,7 @@ struct dma_fence *xe_migrate_clear(struct xe_migrate *m,
>  	bool clear_vram = mem_type_is_vram(dst->mem_type);
>  	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;
> +	struct xe_gt *gt = xe_tile_pt_gt(m->tile);
>  	struct xe_device *xe = gt_to_xe(gt);
>  	bool clear_only_system_ccs = false;
>  	struct dma_fence *fence = NULL;
> @@ -2295,7 +2296,7 @@ static struct dma_fence *xe_migrate_vram(struct xe_migrate *m,
>  					 struct dma_fence *deps,
>  					 const enum xe_migrate_copy_dir dir)
>  {
> -	struct xe_gt *gt = m->tile->primary_gt;
> +	struct xe_gt *gt = xe_tile_pt_gt(m->tile);
>  	struct xe_device *xe = gt_to_xe(gt);
>  	bool use_usm_batch = xe->info.has_usm;
>  	struct dma_fence *fence = NULL;

[Severity: Medium]
When m->q is created on XE_ENGINE_CLASS_VIDEO_DECODE, won't migration blit
jobs submitted through xe_migrate_vram(), xe_migrate_clear(), and
__xe_migrate_copy() complete and signal their fence without executing the
copy or clear batch?

xe_bb_create_migration_job() places the PTE updates in
job->ptrs[0].batch_addr and the copy or clear instructions in
job->ptrs[1].batch_addr, but emit_job_gen12_video() only emits
job->ptrs[0].batch_addr for a width-1 queue:

drivers/gpu/drm/xe/xe_ring_ops.c:emit_job_gen12_video() {
	for (i = 0; i < job->q->width; ++i)
		__emit_job_gen12_video(job, job->q->lrc[i],
				       job->ptrs[i].batch_addr,
				       &job->ptrs[i].head,
				       xe_sched_job_lrc_seqno(job));
}

Even after commit "drm/xe: Fall back to CPU copies when the tile cannot
blit" later in this series, callers such as xe_ttm_access_memory() (for
VRAM accesses >= SZ_16K via xe_migrate_access_memory() -> xe_migrate_vram())
and xe_vram_memtest() (via xe_migrate_clear()) do not check
xe_migrate_can_blit(), silently dropping VRAM reads/writes or failing the
boot memory test with -EIO.

[1] https://sashiko.dev/#/bug/linux-21e51c6b-d9bc-4f0f-9122-4598c0472959
[2] https://sashiko.dev/#/bug/linux-2b53019e-7f14-4ba6-80c3-68e86fcb7785

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110231442.218721-13-stuart.summers@intel.com?part=7