Re: [PATCH v5 3/4] drm/xe/xe_migrate: Optimize unaligned access_memory copies

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/xe: Greedy alignment strategy forces pitch 1 fallback for small chunks
--

commit 1f0d5ea5449a5c28cc6017cccf01d783b8426763
Author: Jan Maslak <jan.maslak@intel.com>
drm/xe/xe_migrate: Optimize unaligned access_memory copies

This commit introduces a multi-phase copy strategy in xe_migrate_access_memory()
to reduce the number of copy jobs. It attempts a short linear copy to reach a
favorable destination alignment, uses a matrix copy for the bulk of the data,
and falls back to a linear copy for remaining tail bytes.

> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
> --- a/drivers/gpu/drm/xe/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/xe_migrate.c

[ ... ]

> @@ -2532,7 +2612,6 @@ int xe_migrate_access_memory(struct xe_migrate *m, struct xe_bo *bo,
>  		u64 vram_addr = vram_region_gpu_offset(bo->ttm.resource) +
>  			cursor.start;
>  		int current_bytes;
> -		u32 pitch;
>  
>  		if (cursor.size > MAX_PREEMPTDISABLE_TRANSFER)
>  			current_bytes = min_t(int, bytes_left,
> @@ -2540,15 +2619,62 @@ int xe_migrate_access_memory(struct xe_migrate *m, struct xe_bo *bo,
>  		else
>  			current_bytes = min_t(int, bytes_left, cursor.size);
>  
> -		pitch = xe_migrate_copy_pitch(xe, current_bytes,
> -					      write ? vram_addr :
> -					      (unsigned long)buf & ~PAGE_MASK);
> -		if (xe->info.has_mem_copy_instr)
> -			current_bytes = min_t(int, current_bytes, U16_MAX * pitch);
> -		else
> -			current_bytes = min_t(int, current_bytes,
> -					      round_down(S16_MAX * pitch,
> -							 XE_CACHELINE_BYTES));
> +		{
> +			/*
> +			 * Optimized copy strategy to minimize number of jobs:
> +			 * 1. Linear copy to align dst to best reachable alignment
> +			 * 2. Matrix copy for bulk data copy
> +			 * 3. Single linear copy for remaining bytes
> +			 *
> +			 * The phase logic controls transfer chunking; emit_copy() selects
> +			 * MEM_COPY_CMD or XY_FAST_COPY_BLT based on hardware support.
> +			 */
> +			u64 dst_addr = write ? vram_addr :
> +					       (unsigned long)buf & ~PAGE_MASK;
> +			u32 max_linear = xe->info.has_mem_copy_instr ? U16_MAX :
> +					 round_down(S16_MAX, XE_CACHELINE_BYTES);

[Severity: Medium]
Does using max_linear instead of current_bytes here cause small unaligned
chunks to fall back to a less efficient pitch 1 copy?

If current_bytes is small (e.g., a fragmented 1000-byte chunk),
xe_migrate_best_alignment() might return PAGE_SIZE because the distance to the
next page boundary is less than max_linear. 

However, since that distance is greater than current_bytes, the Phase 1 
condition below (bytes_to_align <= current_bytes) will fail. The code then
skips Phase 2 and falls back to a linear copy for the entire 1000 bytes.

Passing current_bytes instead of max_linear would scale the target alignment,
allowing it to reach intermediate boundaries (like SZ_256 or 4) and use a
matrix copy for the bulk of the chunk.

> +			u32 best_align = xe_migrate_best_alignment(dst_addr, max_linear);
> +			u32 bytes_to_align = best_align ?
> +					     xe_migrate_bytes_to_align(dst_addr, best_align) : 0;
> +
> +			if (bytes_to_align > 0 && bytes_to_align <= (u32)current_bytes) {
> +				/*
> +				 * Phase 1: Linear copy to reach best alignment.
> +				 * Copy just enough bytes to align dst_addr.
> +				 */
> +				current_bytes = min_t(int, bytes_to_align, max_linear);
> +			} else if (IS_ALIGNED(dst_addr, 4)) {

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014142706.598490-1-jan.maslak@intel.com?part=3