Re: [PATCH v5 17/25] drm/xe: Add device flag to enable PT mirroring across tiles

[Matthew Brost <matthew.brost@intel.com>]

On Fri, Sep 04, 2026 at 02:43:47AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Unused page tables and scratch buffers are incorrectly allocated for secondary tiles when PT mirroring is disabled, wasting VRAM, and migration VMs are forced to use remote page tables.
> - [Medium] Memory leak of page reclaim entries (`prl->entries`) on error paths or when TLB invalidation is bypassed for tiles with disabled page table mirroring.
> --
> 
> commit 38fe6ff118093de1244d8bbcc84d59616c09c17c
> Author: Matthew Brost <matthew.brost@intel.com>
> 
> drm/xe: Add device flag to enable PT mirroring across tiles
> 
> This commit introduces a device flag to enable or disable page-table
> mirroring across tiles for memory-bandwidth reasons. The flag defaults
> to the existing behavior on PVC but allows flexibility for other devices.
> 
> Link: https://patch.msgid.link/20260228013501.106680-18-matthew.brost@intel.com
> 
> > diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> > index 05bbac6c2632e..bf2cdef44179d 100644
> > --- a/drivers/gpu/drm/xe/xe_pt.c
> > +++ b/drivers/gpu/drm/xe/xe_pt.c
> [ ... ]
> > @@ -2541,9 +2552,21 @@ int xe_pt_update_ops_prepare(struct xe_device *xe, struct xe_vma_ops *vops)
> >  	int id, err;
> >  
> >  	for_each_tile(tile, xe, id) {
> > +		struct xe_vm_pgtable_update_ops *pt_update_ops =
> > +			&vops->pt_update_ops[id];
> > +
> >  		if (!vops->pt_update_ops[id].num_ops)
> >  			continue;
> >  
> > +		if (pt_mirroring_disabled_for_tile(vops->vm, id)) {
> > +			struct xe_page_reclaim_list *prl = &pt_update_ops->prl;
> > +
> > +			/* Transfer root PT update ops PRL to current */
> > +			*prl = vops->pt_update_ops[0].prl;
> > +			xe_page_reclaim_entries_get(prl->entries);
> > +			continue;
> > +		}
> > +
> >  		err = __xe_pt_update_ops_prepare(tile, vops);
> >  		if (err)
> >  			return err;
> [ ... ]
> > @@ -2924,6 +2953,9 @@ void xe_pt_update_ops_fini(struct xe_device *xe, struct xe_vma_ops *vops)
> >  		if (!pt_update_ops->num_ops)
> >  			continue;
> >  
> > +		if (pt_mirroring_disabled_for_tile(vops->vm, id))
> > +			continue;
> > +
> >  		xe_page_reclaim_entries_put(pt_update_ops->prl.entries);
> 
> [Severity: Medium]
> Does this skip releasing the page reclaim entries for secondary tiles?
> 
> When PT mirroring is disabled, xe_pt_update_ops_prepare() takes an extra
> reference to prl->entries for secondary tiles. If the operation succeeds but
> TLB invalidation is bypassed (or if called on cleanup),
> xe_pt_update_ops_fini() is executed. By checking
> pt_mirroring_disabled_for_tile() and continuing, it skips the needed
> xe_page_reclaim_entries_put() call, which can lead to a memory leak.
> 
> >  	}
> >  }
> > @@ -2948,6 +2980,9 @@ void xe_pt_update_ops_abort(struct xe_device *xe, struct xe_vma_ops *vops)
> >  			&vops->pt_update_ops[id];
> >  		int i;
> >  
> > +		if (pt_mirroring_disabled_for_tile(vops->vm, id))
> > +			continue;
> > +
> >  		for (i = pt_update_ops->num_ops - 1; i >= 0; --i) {
> 
> [Severity: Medium]
> Similar to the issue above in xe_pt_update_ops_fini(), does this leak the
> page reclaim entries when an operation is aborted?
> 
> By skipping secondary tiles when mirroring is disabled,
> xe_pt_update_ops_abort() also bypasses xe_pt_update_ops_fini() for those
> tiles, meaning the reference acquired in xe_pt_update_ops_prepare() is
> never put on error paths.
> 

Both of these are correct, pt_mirroring_disabled_for_tile should be
deleted from xe_pt_update_ops_fini.

Matt

> >  			struct xe_vm_pgtable_update_op *pt_op =
> >  				to_pt_op(pt_update_ops, i);
> [ ... ]
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > index fbfc42afef8b1..795d0ebb10047 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> [ ... ]
> > @@ -1738,7 +1748,8 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags, struct xe_file *xef)
> >  
> >  		for_each_tile(tile, xe, id) {
> >  			if (flags & XE_VM_FLAG_MIGRATION &&
> > -			    tile->id != XE_VM_FLAG_TILE_ID(flags))
> > +			    tile->id != XE_VM_FLAG_TILE_ID(flags) &&
> > +			    (vm->xe->info.has_pt_mirror || id))
> >  				continue;
> >  
> >  			vm->pt_root[id] = xe_pt_create(vm, tile, xe->info.vm_max_level,
> 
> [Severity: High]
> Does this allocate unused page tables and scratch buffers for secondary tiles
> when PT mirroring is disabled?
> 
> When has_pt_mirror is false, the loop does not skip secondary tiles for
> normal VMs, causing it to unconditionally allocate pt_root structures and
> scratch pages for all of them. 
> 
> Additionally, for migration VMs on secondary tiles, this allocates both
> pt_root[0] and pt_root[tile_id]. Since xe_vm_pt_root() unconditionally
> returns pt_root[0] when mirroring is disabled, this abandons the memory
> allocated for secondary tiles and forces the migration engine to use
> pt_root[0] over the cross-tile interconnect, defeating the locality benefits.
> 
> [ ... ]
> > @@ -5072,3 +5083,21 @@ void xe_vm_remove_exec_queue(struct xe_vm *vm, struct xe_exec_queue *q)
> >  	}
> >  	up_write(&vm->exec_queues.lock);
> >  }
> > +
> > +/**
> > + * xe_vm_pt_root() - Retrieve VM page-table root
> > + * @vm: The VM.
> > + * @tile_id: Tile ID
> > + *
> > + * Retrieve VM page-table root for a tile ID, used to abstract if PT mirroring is
> > + * enabled across tiles.
> > + *
> > + * Return: VM page-table root for a tile ID
> > + */
> > +struct xe_pt *xe_vm_pt_root(struct xe_vm *vm, u8 tile_id)
> > +{
> > +	if (vm->xe->info.has_pt_mirror)
> > +		return vm->pt_root[tile_id];
> > +
> > +	return vm->pt_root[0];
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260904022207.3490018-1-matthew.brost@intel.com?part=17