[igt-dev] [PATCH i-g-t 2/2] tests/xe/xe_copy_basic: Add copy basic test to exercise blt commands
Ch, Sai Gowtham
sai.gowtham.ch at intel.com
Mon Aug 21 13:29:11 UTC 2023
>-----Original Message-----
>From: Stolarek, Karolina <karolina.stolarek at intel.com>
>Sent: Friday, August 18, 2023 7:10 PM
>To: Ch, Sai Gowtham <sai.gowtham.ch at intel.com>
>Cc: igt-dev at lists.freedesktop.org
>Subject: Re: [igt-dev] [PATCH i-g-t 2/2] tests/xe/xe_copy_basic: Add copy basic
>test to exercise blt commands
>
>Hi Sai,
>
>On 18.08.2023 07:19, sai.gowtham.ch at intel.com wrote:
>> From: Sai Gowtham Ch <sai.gowtham.ch at intel.com>
>>
>> Add copy basic test to exercise copy commands like mem-copy and mem-
>set.
>>
>> Signed-off-by: Sai Gowtham Ch <sai.gowtham.ch at intel.com>
>> ---
>> tests/meson.build | 1 +
>> tests/xe/xe_copy_basic.c | 262
>+++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 263 insertions(+)
>> create mode 100644 tests/xe/xe_copy_basic.c
>>
>> diff --git a/tests/meson.build b/tests/meson.build index
>> 58061dbc2..323c2108a 100644
>> --- a/tests/meson.build
>> +++ b/tests/meson.build
>> @@ -266,6 +266,7 @@ xe_progs = [
>> 'xe_ccs',
>> 'xe_create',
>> 'xe_compute',
>> + 'xe_copy_basic',
>> 'xe_dma_buf_sync',
>> 'xe_debugfs',
>> 'xe_evict',
>> diff --git a/tests/xe/xe_copy_basic.c b/tests/xe/xe_copy_basic.c new
>> file mode 100644 index 000000000..ce1223eb1
>> --- /dev/null
>> +++ b/tests/xe/xe_copy_basic.c
>> @@ -0,0 +1,262 @@
>> +/* SPDX-License-Identifier: MIT */
>
>Nit: this line uses formatting dedicated for header files, use // instead
>
>> +/* > +* Copyright © 2023 Intel Corporation
>
>Nit: Add spaces to align *s to /*
>
>> +*
>> +* Authors:
>> +* Sai Gowtham Ch <sai.gowtham.ch at intel.com>
>> +*/
>> +
>> +#include "igt.h"
>> +#include "xe_drm.h"
>> +#include "xe/xe_ioctl.h"
>> +#include "xe/xe_query.h"
>> +#include "lib/intel_mocs.h"
>> +#include "lib/intel_reg.h"
>> +#include "lib/igt_syncobj.h"
>> +#include "lib/xe/xe_util.h"
>> +
>> +/**
>> + * TEST: Test to valiudate copy commands on xe
>
> From what I understand, the TEST field is just the name of the specific test,
>the rest should go to Description. Also, s/valiudate/validate/
I think we are following the same for tests we have.
My bad Will change the Validate spell in next version.
>
>> + * Category: Software building block
>> + * Sub-category: Copy
>> + * Functionality: blitter
>> + * Test category: functionality test
>> + */
>> +
>> +enum command {
>> + MEM_COPY,
>> + MEM_SET,
>> +};
>> +
>> +static int objcmp(int fd, uint32_t src, uint32_t dst,
>> + uint32_t src_size, uint32_t dst_size)
>
> ^-- misaligned line
>
>> +{
>> + void *buf_src, *buf_dst;
>> + int ret = 0;
>> +
>> + buf_src = xe_bo_map(fd, src, src_size);
>> + buf_dst = xe_bo_map(fd, dst, dst_size);
>> +
>> + ret = memcmp(buf_src, buf_dst , src_size);
>
>Whoops, an extra space before ","
>
>> +
>> + munmap(buf_src, src_size);
>> + munmap(buf_dst, dst_size);
>> +
>> + return ret;
>> +}
>> +
>> +/**
>> + * SUBTEST: mem-copy
>> + * Description: Test validates MEM_COPY command, it takes various
>> + * parameters needed for the filling batch buffer for MEM_COPY
>command.
>> + * Run type: FULL
>> + */
>> +
>
>I _think_ that an empty line before this and the other doc is not needed, but
>I'd need to double-check in other tests.
>
>> +static void
>> +igt_mem_copy(int fd, uint32_t src, uint32_t dst,
>> + uint32_t size, uint32_t col_size, uint32_t src_pitch,
>
>Nit: misaligned line that uses spaces instead of tabs, could you correct it in
>the next version?
>
>Also, I have a question about src_pitch and dst_pitch arguments -- is there a
>test case where they are different from 0? If not, could we drop them from
>the arguments list?
We can change it to 0, however intention of having this is to understand bb better.
Will add comments and make them 0.
>
>> + uint32_t dst_pitch, uint32_t vm, uint32_t engine) {
>> + struct drm_xe_sync sync = {
>> + .flags = DRM_XE_SYNC_SYNCOBJ | DRM_XE_SYNC_SIGNAL,
>> + };
>> + struct drm_xe_exec exec = {
>> + .num_batch_buffer = 1,
>> + .num_syncs = 1,
>> + .syncs = to_user_pointer(&sync),
>> + };
>> +
>> + uint32_t bb_handle, syncobj;
>> + struct {
>> + uint32_t batch[12];
>> + uint32_t data;
>> + } *data;
>> +
>> + uint64_t bb_offset, src_offset, dst_offset;
>> + uint64_t alignment;
>> + uint8_t src_mocs = intel_get_uc_mocs(fd);
>> + uint64_t bb_size = xe_get_default_alignment(fd);
>> + uint8_t dst_mocs = src_mocs;
>> + uint64_t ahnd;
>> + int i;
>> +
>> + alignment = xe_get_default_alignment(fd);
>> +
>> + bb_handle = xe_bo_create_flags(fd, 0, bb_size,
>visible_vram_if_possible(fd, 0));
>> + data = xe_bo_map(fd, bb_handle, bb_size);
>> +
>> + ahnd = intel_allocator_open_full(fd, vm, 0, 0,
>INTEL_ALLOCATOR_SIMPLE,
>> + ALLOC_STRATEGY_LOW_TO_HIGH, 0);
>
>Slightly misaligned, from what I can see. You can check these with
>scripts/checkpatch.pl from the kernel repo.
>
>> + src_offset = get_offset(ahnd, src, size, alignment);
>> + dst_offset = get_offset(ahnd, dst, size, alignment);
>> + bb_offset = get_offset(ahnd, bb_handle, bb_size, alignment);
>> +
>> + i = 0;
>> + data->batch[i++] = MEM_COPY_CMD;
>> + data->batch[i++] = size - 1;
>> + data->batch[i++] = col_size - 1;
>> + data->batch[i++] = src_pitch;
>> + data->batch[i++] = dst_pitch;
>> + data->batch[i++] = src_offset;
>> + data->batch[i++] = src_offset << 32;
>> + data->batch[i++] = dst_offset;
>> + data->batch[i++] = dst_offset << 32;
>> + data->batch[i++] = src_mocs << MEM_COPY_MOCS_SHIFT | dst_mocs;
>> + data->batch[i++] = MI_BATCH_BUFFER_END;
>> + data->batch[i++] = MI_NOOP;
>
>I see that in both batches we use MI_NOOP -- is there a reason for doing that?
To make sure bb is aligned, and pad out to qword boundary.
>
>> +
>> + syncobj = syncobj_create(fd, 0);
>> + sync.handle = syncobj;
>> +
>> + xe_vm_bind_sync(fd, vm, bb_handle, 0, bb_offset, bb_size);
>> +
>> + exec.exec_queue_id = engine;
>> + exec.address = bb_offset;
>> + sync.handle = syncobj;
>> + igt_assert_eq(igt_ioctl(fd, DRM_IOCTL_XE_EXEC, &exec), 0);
>> +
>> + gem_close(fd, bb_handle);
>> + put_ahnd(ahnd);
>> + munmap(data, bb_size);
>> +
>^--- extra empty line, it would be good to delete it
>
>Also, that's the comment that applies both to mem_copy and mem_set --
>shouldn't we call syncobj_destroy() at the end of the test?
Missed it will fix this.
>
>> +}
>> +
>> +/**
>> + * SUBTEST: mem-set
>> + * Description: Test validates MEM_SET command.
>> + * RUN type: FULL
>> + */
>> +
>> +static void igt_mem_set(int fd, uint32_t dst, size_t size, uint32_t height,
>> + uint32_t fill_data, uint32_t vm, uint32_t engine) {
>> + struct drm_xe_sync sync = {
>> + .flags = DRM_XE_SYNC_SYNCOBJ | DRM_XE_SYNC_SIGNAL,
>> + };
>> + struct drm_xe_exec exec = {
>> + .num_batch_buffer = 1,
>> + .num_syncs = 1,
>> + .syncs = to_user_pointer(&sync),
>> + };
>> + struct {
>> + uint32_t batch[12];
>> + uint32_t data;
>> + } *data;
>> +
>> + uint32_t syncobj;
>> + uint64_t dst_offset, ahnd;
>> + uint8_t dst_mocs = intel_get_uc_mocs(fd);
>> + int b;
>> +
>> + data = xe_bo_map(fd, dst, size);
>> + ahnd = intel_allocator_open(fd, 0, INTEL_ALLOCATOR_RELOC);
>
>Before we had a simple allocator, why do we pick a different one in this one?
Intension was to create separately in mem_copy and mem_set, However makes sense to me
Will create ahnd for src and dst in copy_test, use them accordingly.
>
>> + dst_offset = intel_allocator_alloc_with_strategy(ahnd, dst, size, 0,
>> +
> ALLOC_STRATEGY_LOW_TO_HIGH);
>
>Alignment is slightly off here
>
>> +
>> + b = 0;
>> + data->batch[b++] = MEM_SET_CMD;
>> + data->batch[b++] = size - 1;
>> + data->batch[b++] = height;
>> + data->batch[b++] = 2 * size - 1;
>
> From what I understand, this filed is ignored for linear fill and we have no
>matrix case. Do you have a plans of adding one? If not, I think it should be
>fine to set it to 0, but you'd need to test my assumption.
>
>> + data->batch[b++] = dst_offset;
>> + data->batch[b++] = dst_offset << 32;
>> + data->batch[b++] = (fill_data << 24) | dst_mocs;
>> + data->batch[b++] = MI_BATCH_BUFFER_END;
>> + data->batch[b++] = MI_NOOP;
>> + igt_assert(b <= ARRAY_SIZE(data->batch));
>> +
>> + syncobj = syncobj_create(fd, 0);
>> + sync.handle = syncobj;
>> +
>> + xe_vm_bind_sync(fd, vm, dst, 0, dst_offset, size);
>> +
>> + exec.exec_queue_id = engine;
>> + exec.address = dst_offset;
>> + sync.handle = syncobj;
>> + igt_assert_eq(igt_ioctl(fd, DRM_IOCTL_XE_EXEC, &exec), 0);
>> +
>> + munmap(data, size);
>> + put_ahnd(ahnd);
>
>Is gem_close() call missing from here?
Nope it's closed in copy_test.
>
>> +}
>> +
>> +static void copy_test(int fd, enum command cmd,
>> + struct drm_xe_engine_class_instance *hwe) {
>> + uint32_t src_size, dst_size;
>> + uint32_t src, dst, vm, engine;
>> + char c = 'a';
>> + size_t bo_size = xe_get_default_alignment(fd);
>> + uint32_t temp_buffer[bo_size];
>> +
>> + src = xe_bo_create_flags(fd, 0, bo_size, visible_vram_memory(fd, hwe-
>>gt_id));
>> + dst = xe_bo_create_flags(fd, 0, bo_size, visible_vram_memory(fd,
>> +hwe->gt_id));
>
>This test only tests lmem<->lmem scenario, what about smem<->smem and
>lmem<->smem?
>
>> + vm = xe_vm_create(fd, 0, 0);
>> + engine = xe_exec_queue_create(fd, vm, hwe, 0);
>
>Given the recent rename from xe_engine to xe_exec_queue, would it be
>possible to rename this variable and param names to match it? Leaving
>engine here is slightly confusing.
I feel this should be easy to understand, instead of name them to queues,
However I can change them if we are following the same for rest of the tests in IGT.
>
>> +
>> + /* Fill a pattern in the buffer */
>> + for (int i = 0; i < bo_size; i++) {
>> + temp_buffer[i] = c++ % 16;
>> + temp_buffer[i] |= (c++ % 16) << 8;
>> + temp_buffer[i] |= (c++ % 16) << 16;
>> + temp_buffer[i] |= (c++ % 16) << 24;
>
>For each byte, we're using a different value. It's more of a nit, but I wonder if
>we could just increment c for each 4 byte chunk, not every byte.
Sure Will have a look.
>
>> + }
>> +
>> + src_size = bo_size;
>> + dst_size = bo_size;
>> +
>> + switch (cmd) {
>> + case MEM_COPY: /* MEM_COPY_CMD */
>> + igt_mem_copy(fd,
>> + src,
>> + dst,
>> + bo_size,
>> + 1,
>> + 0,
>> + 0,
>> + vm,
>> + engine);
>> + break;
>> +
>> + case MEM_SET:
>> + igt_mem_set(fd,
>> + dst,
>> + bo_size,
>> + 1,
>> + temp_buffer[0] & 0xff,
>> + vm,
>> + engine);
>> + src_size = 1;
>> + break;
>> + }
>
>It's more of a suggestion -- could we add comments to each argument what it
>stands for? That would ease the reading process, it took some time for me to
>map each value with the specific parameter.
Sure will do that.
>
>> +
>> + igt_assert_eq(objcmp(fd, src, src_size, dst, dst_size), 0);
>> + gem_close(fd, src);
>> + gem_close(fd, dst);
>> +
>> + xe_exec_queue_destroy(fd, engine);
>> + xe_vm_destroy(fd, vm);
>> +}
>> +
>> +igt_main
>> +{
>> + struct drm_xe_engine_class_instance *hwe;
>> + int fd;
>> +
>> + igt_fixture {
>> + fd = drm_open_driver(DRIVER_XE);
>> + xe_device_get(fd);
>
>How is this test going to be run? As a part of CI or manually? If the former, I
>think we should add a check to see if a specific platform supports MEM_COPY
>or MEM_SET and skip if it doesn't. We could do it by adding information on
>supported commands to intel_cmd_info.c and implementing a helper to
>check for it, something similar to
>blt_fast_copy_supports_tiling() function.
As per my understanding these command should work on all platforms.
>
>All the best,
>Karolina
>
>> + }
>> +
>> + igt_subtest("mem-set")
>> + xe_for_each_hw_engine(fd, hwe)
>> + copy_test(fd, MEM_SET, hwe);
>> +
>> + igt_subtest("mem-copy")
>> + xe_for_each_hw_engine(fd, hwe)
>> + copy_test(fd, MEM_COPY, hwe);
>> +
>> + igt_fixture {
>> + drm_close_driver(fd);
>> + }
>> +}
>> +
More information about the igt-dev
mailing list