[igt-dev] [PATCH i-g-t 2/2] tests/xe/xe_copy_basic: Add copy basic test to exercise blt commands
Karolina Stolarek
karolina.stolarek at intel.com
Tue Aug 22 08:14:22 UTC 2023
Hi Sai,
On 21.08.2023 15:29, Ch, Sai Gowtham wrote:
>
>
>> -----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.
Right, I'm also not too passionate about it, so you can leave the doc as
it is
>>
>>> + * 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.
Right. And thanks, comments will help here, I think
>>
>>> + 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.
Ah, my bad!
>>
>>> +}
>>> +
>>> +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.
I also reviewed a patch that does a similar rename, so I thought that it
would make sense to keep it consistent accross the codebase.
>>
>>> +
>>> + /* 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.
Hmm, I just checked the documentation and it looks like only selected
platforms can handle these commands, so we'd need to skip the test for
these that don't via igt_require (similarly to what is done in
(xe|gem)_exercise_blt). But, like I said, that would add a couple of
patches if you want to use intel_cmds_info lib.
Many thanks,
Karolina
>>
>> 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