Re: [PATCH 2/2] drm/xe/tests: Cover a large write to an unaligned bo offset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/xe/tests: 64B bounce loop in access_memory_test_unaligned_large_write()
--

commit 983819064579a8f3c33e9b1bc03bdf5b989cdcfe
Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>

drm/xe/tests: Cover a large write to an unaligned bo offset

This commit adds a KUnit test case to drivers/gpu/drm/xe/tests/xe_migrate.c
that writes and reads back a 16 MiB buffer object from an unaligned offset of
4 bytes using xe_migrate_access_memory() to exercise the
MAX_PREEMPTDISABLE_TRANSFER chunking path.

> diff --git a/drivers/gpu/drm/xe/tests/xe_migrate.c b/drivers/gpu/drm/xe/tests/xe_migrate.c
> index a9ef8f2dcebbf..715de366d33c0 100644
> --- a/drivers/gpu/drm/xe/tests/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/tests/xe_migrate.c
> @@ -1041,6 +1041,53 @@ static void access_memory_test_run_tile(struct xe_device *xe,
>  	kvfree(buf);
>  }
>  
> +static void access_memory_test_unaligned_large_write(struct kunit *test,
> +						     struct xe_tile *tile)
> +{
> +	const u32 size = SZ_16M, offset = 4;

[ ... ]

> +	xe_bo_lock(bo, false);
> +	ret = xe_migrate_access_memory(tile->migrate, bo, offset, buf,
> +				       size - offset, true);
> +	if (!ret) {
> +		memset(buf, 0, size);
> +		ret = xe_migrate_access_memory(tile->migrate, bo, offset, buf,
> +					       size - offset, false);
> +	}

[Severity: Medium]
On discrete GPUs without MEM_COPY instruction support (where
xe->info.has_mem_copy_instr is false, such as DG1, DG2, ATS-M, and PVC), will
calling xe_migrate_access_memory() with offset = 4 for a 16 MiB transfer
fall into the 64-byte bounce-buffer loop for the entire buffer instead of
exercising the MAX_PREEMPTDISABLE_TRANSFER chunking path?

In drivers/gpu/drm/xe/tests/xe_migrate.c, access_memory_test_run_device()
calls access_memory_test_unaligned_large_write() on all discrete devices.
In drivers/gpu/drm/xe/xe_migrate.c, xe_migrate_access_memory() checks:

	/* Use bounce buffer for small access and unaligned access */
	if (!xe->info.has_mem_copy_instr &&
	    (!IS_ALIGNED(len, 4) ||
	     !IS_ALIGNED(page_offset, XE_CACHELINE_BYTES) ||
	     !IS_ALIGNED(offset, XE_CACHELINE_BYTES))) {
		...
		do {
			int copy_bytes = min_t(int, bytes_left,
					       XE_CACHELINE_BYTES -
					       (offset & XE_CACHELINE_MASK));
			...
			err = xe_migrate_access_memory(m, bo,
						       offset &
						       ~XE_CACHELINE_MASK,
						       bounce,
						       XE_CACHELINE_BYTES, 0);
			...
			if (write) {
				...
				err = xe_migrate_access_memory(m, bo,
							       offset & ~XE_CACHELINE_MASK,
							       bounce,
							       XE_CACHELINE_BYTES, write);
			...
		} while (bytes_left);

		kfree(bounce);
		return err;
	}

Because offset = 4 is not aligned to XE_CACHELINE_BYTES (64),
!IS_ALIGNED(offset, XE_CACHELINE_BYTES) is true when
!xe->info.has_mem_copy_instr. Rather than bouncing only the unaligned head,
that loop processes all bytes_left (SZ_16M - 4) in 64-byte chunks.

For a 16 MiB write followed by a 16 MiB read, this performs 262,144 write
iterations (each doing two synchronous 64-byte xe_migrate_access_memory()
calls) plus 262,144 synchronous 64-byte read calls, totaling 786,432
sequential synchronous GPU jobs per tile and bypassing the main
MAX_PREEMPTDISABLE_TRANSFER loop on those platforms.

Should offset be aligned to XE_CACHELINE_BYTES (for example, offset = 64) on
!xe->info.has_mem_copy_instr platforms so that it remains unaligned to the
page and 256-byte pitch while still reaching the main transfer loop without
timing out?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027145237.1862132-1-mika.kuoppala@linux.intel.com?part=2