Hi,
On Fri, 12 Sept 2025 at 19:20, Daniel Almeida
<[email protected]> wrote:
> +igt_main {
> + igt_describe("Create a buffer object");
> + igt_subtest("bo_create") {
> + struct panthor_bo bo;
> +
> + igt_panthor_bo_create(fd, &bo, 4096, 0, 0);
> + igt_assert(bo.handle != 0);
Please - here and everywhere else - use the specific igt_assert_*()
macros. Those print out the actual values, so are much more useful
when debugging.
> + igt_describe_f("Create a buffer object whose size is not
> page-aligned, and check "
> + "that the allocated size is rounded up to the next page size
> %lu.",
> + 8192UL);
> + igt_subtest("bo_create_round_size") {
> + struct panthor_bo bo;
> + uint64_t expected_size = 8192;
Shouldn't this be based on the actual page size in use?
> +static void
> +issue_store_multiple(u8 *command_stream, uint64_t kernel_va, uint32_t
> constant)
> +{
> + uint64_t opcode, reg_num, mov48, store_multiple, flush;
> + uint64_t sr, src0, register_bitmap, offset;
> +
> + // MOV48: Load the source register ([r68; r69]) with the
> kernel address
> + opcode = 0x1;
> + reg_num = 68;
> + mov48 = (opcode << 56) | (reg_num << 48) | kernel_va;
> + mov48 = htole64(mov48);
> + memcpy(&command_stream[0], &mov48, sizeof(mov48));
> +
> + // MOV48: Load a known constant into r70
> + opcode = 0x1;
> + reg_num = 70;
> + mov48 = (opcode << 56) | (reg_num << 48) | constant;
> + mov48 = htole64(mov48);
> + memcpy(&command_stream[8], &mov48, sizeof(mov48));
> +
> + // STORE_MULTIPLE: Store the first register to the address
> pointed to by [r68; r69]
> + opcode = 0x15; // STORE_MULTIPLE
> + sr = 70; // Starting from register r70
> + src0 = 68; // Address pointed to by [r68; r69]
> + register_bitmap = 1; // Store the first register
> + offset = 0; // Offset
> + store_multiple = (opcode << 56) | (sr << 48) | (src0 << 40) |
> +
> (register_bitmap << 16) | offset;
> + store_multiple = htole64(store_multiple);
> + memcpy(&command_stream[16], &store_multiple,
> sizeof(store_multiple));
// MOV48 r68, 0 on the below?
> + opcode = 0x1;
> + reg_num = 68;
> + mov48 = (opcode << 56) | (reg_num << 48) | 0;
> + mov48 = htole64(mov48);
> + memcpy(&command_stream[24], &mov48, sizeof(mov48));
// FLUSH_PAGES?
> + opcode = 36;
> + flush = opcode << 56 | 0ull << 48 | reg_num << 40 | 0ull <<
> 16 | 0x233;
> + flush = htole64(flush);
> + memcpy(&command_stream[32], &flush, sizeof(flush));
> +}
> + group_create.queues = queues;
> + group_create.max_compute_cores = 1;
> + group_create.max_fragment_cores = 1;
> + group_create.max_tiler_cores = 1;
> + group_create.priority = PANTHOR_GROUP_PRIORITY_MEDIUM;
> + group_create.compute_core_mask = gpu_info.shader_present &
> 0x1; // Use first core
> + group_create.fragment_core_mask = gpu_info.shader_present &
> 0x1; // Use first core
> + group_create.tiler_core_mask = gpu_info.tiler_present & 0x1;
> // Use first tiler
This will either be 0 (rejected) or 1 (possibly valid but possibly
not). Did you mean (1 << ffs(...))? Same issue below, so maybe this
wants a little helper.
> + igt_assert_eq(igt_ioctl(fd, DRM_IOCTL_PANTHOR_GROUP_CREATE,
> &group_create), 0);
> + igt_assert(group_create.group_handle != 0);
> +
> + // Cleanup: Destroy the group and VM
igt does use /* */ rather than // everywhere, if you're respinning anyway.
> + group_destroy = (struct drm_panthor_group_destroy){
> + .group_handle = group_create.group_handle
> + };
> + igt_assert_eq(igt_ioctl(fd, DRM_IOCTL_PANTHOR_GROUP_DESTROY,
> &group_destroy), 0);
do_ioctl() - here and elsewhere.
> + igt_subtest("group_submit") {
> + struct drm_panthor_gpu_info gpu_info = {};
> + struct drm_panthor_vm_create vm_create = {};
> + struct drm_panthor_group_create group_create = {};
> + struct drm_panthor_queue_create queue = {};
> + struct drm_panthor_obj_array queues = {};
> + struct drm_panthor_group_submit group_submit = {};
> + struct drm_panthor_queue_submit queue_submit = {};
> + struct drm_panthor_group_destroy group_destroy = {};
> + struct drm_panthor_obj_array queue_submits = {};
> + struct drm_panthor_vm_destroy vm_destroy = {};
> + struct drm_panthor_bo_create bo_create = {};
> + struct drm_panthor_vm_bind vm_bind = {};
> + struct drm_panthor_vm_bind_op vm_bind_op = {};
> + struct drm_syncobj_wait wait = {};
> + struct drm_syncobj_create syncobj_create = {};
> + struct drm_panthor_sync_op sync_op = {};
> + struct drm_gem_close gem_close = {};
> + struct drm_syncobj_destroy syncobj_destroy = {};
> + uint64_t command_stream_gpu_addr;
> + uint32_t command_stream_size;
> + uint64_t result_gpu_addr;
> + uint32_t cmd_buf_bo_handle;
> + uint32_t result_bo_handle;
> + uint32_t syncobj_handle;
> + uint8_t command_stream[64] = {0};
> + uint8_t *bo_cpu_addr;
> + uint8_t *result_cpu_addr;
> + const int INITIAL_VA = 0x1000000;
> + uint64_t bo_mmap_offset;
> +
> + igt_panthor_query(fd, DRM_PANTHOR_DEV_QUERY_GPU_INFO,
> + &gpu_info, sizeof(gpu_info), 0);
> + igt_assert(gpu_info.gpu_id != 0);
> +
> + vm_create.flags = 0;
> + igt_assert_eq(igt_ioctl(fd, DRM_IOCTL_PANTHOR_VM_CREATE,
> &vm_create), 0);
> + igt_assert(vm_create.id != 0);
> +
> + bo_create.size = 4096;
> + bo_create.flags = 0;
> + bo_create.exclusive_vm_id = vm_create.id;
> + igt_assert_eq(igt_ioctl(fd, DRM_IOCTL_PANTHOR_BO_CREATE,
> &bo_create), 0);
> + igt_assert(bo_create.handle != 0);
> + cmd_buf_bo_handle = bo_create.handle;
Why not use the helpers here?
> + wait = (struct drm_syncobj_wait) {
> + .handles = (uint64_t)&syncobj_handle,
> + .count_handles = 1,
> + .timeout_nsec = INT64_MAX,
> + .flags = 0,
> + };
> +
> + igt_assert_eq(igt_ioctl(fd, DRM_IOCTL_SYNCOBJ_WAIT, &wait),
> 0);
Similarly, syncobj_create() and syncobj_wait().
Thanks a lot for writing this up. Having more negative cases would be
really nice, but this is a really good start, so I'm happy for it to
land once it's been cleaned up.
Cheers,
Daniel
