From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by gabe.freedesktop.org (Postfix) with ESMTPS id 8897D10E249 for ; Mon, 5 Dec 2022 16:02:31 +0000 (UTC) Date: Mon, 5 Dec 2022 17:02:23 +0100 From: Kamil Konieczny To: igt-dev@lists.freedesktop.org Message-ID: References: <20221123142548.128038-1-vikas.srivastava@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20221123142548.128038-1-vikas.srivastava@intel.com> Subject: Re: [igt-dev] [PATCH i-g-t v5] tests/i915/gem_userptr_blits: Added XY_FAST_COPY_BLT Command for MTL List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: Hi Vikas, I have small nit for subject, avoid using upper case letters in description words, use them only for proper names (like PVC, MTL or other HW/SW names), so tests/i915/gem_userptr_blits: Added XY_FAST_COPY_BLT Command for MTL ---------------------------------------------------- ^ should be: tests/i915/gem_userptr_blits: Added XY_FAST_COPY_BLT command for MTL You can keep it now but please correct it in future patches. On 2022-11-23 at 19:55:48 +0530, Vikas Srivastava wrote: > From: Arjun Melkaveri > > Test case uses legacy command which is not supported on MTL. > Modified test to use XY_FAST_COPY_BLT. > > Signed-off-by: Arjun Melkaveri > Acked-by: Priyanka Dandamudi > --- > tests/i915/gem_userptr_blits.c | 139 ++++++++++++++++++++++----------- > 1 file changed, 92 insertions(+), 47 deletions(-) > > diff --git a/tests/i915/gem_userptr_blits.c b/tests/i915/gem_userptr_blits.c > index 698508669..7ed7ce046 100644 > --- a/tests/i915/gem_userptr_blits.c > +++ b/tests/i915/gem_userptr_blits.c > @@ -99,8 +99,9 @@ static int copy(int fd, uint32_t dst, uint32_t src) > struct drm_i915_gem_relocation_entry reloc[2]; > struct drm_i915_gem_exec_object2 obj[3]; > struct drm_i915_gem_execbuffer2 exec; > + uint32_t devid; > uint32_t handle; > - int ret, i=0; > + int ret, i = 0; This can stay but try to avoid trivial fixes. > uint64_t dst_offset, src_offset, bb_offset; > bool has_relocs = gem_has_relocations(fd); > > @@ -108,29 +109,48 @@ static int copy(int fd, uint32_t dst, uint32_t src) > dst_offset = bb_offset + 4096; > src_offset = dst_offset + WIDTH * HEIGHT * sizeof(uint32_t) * (src != dst); > > - batch[i++] = XY_SRC_COPY_BLT_CMD | > - XY_SRC_COPY_BLT_WRITE_ALPHA | > - XY_SRC_COPY_BLT_WRITE_RGB; > - if (intel_gen(intel_get_drm_devid(fd)) >= 8) > - batch[i - 1] |= 8; > - else > - batch[i - 1] |= 6; > - > - batch[i++] = (3 << 24) | /* 32 bits */ > - (0xcc << 16) | /* copy ROP */ > - WIDTH*4; > - batch[i++] = 0; /* dst x1,y1 */ > - batch[i++] = (HEIGHT << 16) | WIDTH; /* dst x2,y2 */ > - batch[i++] = dst_offset; /* dst reloc */ > - if (intel_gen(intel_get_drm_devid(fd)) >= 8) > - batch[i++] = dst_offset >> 32; > - batch[i++] = 0; /* src x1,y1 */ > - batch[i++] = WIDTH*4; > - batch[i++] = src_offset; /* src reloc */ > - if (intel_gen(intel_get_drm_devid(fd)) >= 8) > - batch[i++] = src_offset >> 32; > - batch[i++] = MI_BATCH_BUFFER_END; > - batch[i++] = MI_NOOP; > + devid = intel_get_drm_devid(fd); > + > + if (intel_graphics_ver(devid) >= IP_VER(12, 60)) { > + batch[i++] = XY_FAST_COPY_BLT; > + batch[i++] = XY_FAST_COPY_COLOR_DEPTH_32 | WIDTH*4; > + batch[i++] = 0;/* dst x1,y1 */ ------------------------------ ^ Put space before comment. > + batch[i++] = (HEIGHT << 16) | WIDTH;/* dst x2,y2 */ --------------------------------------------------- ^ Put space before comment. > + batch[i++] = lower_32_bits(CANONICAL(dst_offset));/* dst address */ ------------------------------------------ ^ -------------------- ^ No need for CANONICAL here, it will only affect high bits. Put also space before comment: batch[i++] = lower_32_bits(dst_offset); /* dst address */ > + batch[i++] = upper_32_bits(CANONICAL(dst_offset)); > + batch[i++] = 0;/* src x1,y1 */ > + batch[i++] = WIDTH*4;/* src pitch */ > + batch[i++] = lower_32_bits(CANONICAL(src_offset));/* src address */ ------------------------------------------ ^ -------------------- ^ Same here. > + batch[i++] = upper_32_bits(CANONICAL(src_offset)); > + batch[i++] = MI_BATCH_BUFFER_END; > + batch[i++] = MI_NOOP; > + } else { > + batch[i++] = XY_SRC_COPY_BLT_CMD | > + XY_SRC_COPY_BLT_WRITE_ALPHA | > + XY_SRC_COPY_BLT_WRITE_RGB; > + if (intel_gen(intel_get_drm_devid(fd)) >= 8) > + batch[i - 1] |= 8; > + else > + batch[i - 1] |= 6; > + > + batch[i++] = (3 << 24) | /* 32 bits */ > + (0xcc << 16) | /* copy ROP */ > + WIDTH*4; > + batch[i++] = 0; /* dst x1,y1 */ > + batch[i++] = (HEIGHT << 16) | WIDTH; /* dst x2,y2 */ > + batch[i++] = lower_32_bits(CANONICAL(dst_offset));/*dst reloc*/ ------------------------------------------ ^ -------------------- ^ Same here. > + batch[i++] = upper_32_bits(CANONICAL(dst_offset)); > + if (intel_gen(intel_get_drm_devid(fd)) >= 8) ----------------------------- ^ Reuse devid here, so if (intel_gen(devid)) >= 8) > + batch[i++] = dst_offset >> 32; > + batch[i++] = 0; /* src x1,y1 */ > + batch[i++] = WIDTH*4; > + batch[i++] = lower_32_bits(CANONICAL(src_offset)); /* src reloc */ ------------------------------------------ ^ No need for CANONICAL here. > + batch[i++] = upper_32_bits(CANONICAL(src_offset)); > + if (intel_gen(intel_get_drm_devid(fd)) >= 8) > + batch[i++] = src_offset >> 32; > + batch[i++] = MI_BATCH_BUFFER_END; > + batch[i++] = MI_NOOP; > + } > > handle = gem_create(fd, 4096); > gem_write(fd, handle, 0, batch, sizeof(batch)); > @@ -204,31 +224,56 @@ blit(int fd, uint32_t dst, uint32_t src, uint32_t *all_bo, int n_bo) > struct drm_i915_gem_relocation_entry reloc[2]; > struct drm_i915_gem_exec_object2 *obj; > struct drm_i915_gem_execbuffer2 exec; > + uint32_t devid; > uint32_t handle; > - int n, ret, i=0; > + int n, ret, i = 0; > + uint64_t dst_offset, src_offset, bb_offset; > > - batch[i++] = XY_SRC_COPY_BLT_CMD | > - XY_SRC_COPY_BLT_WRITE_ALPHA | > - XY_SRC_COPY_BLT_WRITE_RGB; > - if (intel_gen(intel_get_drm_devid(fd)) >= 8) > - batch[i - 1] |= 8; > - else > - batch[i - 1] |= 6; > - batch[i++] = (3 << 24) | /* 32 bits */ > - (0xcc << 16) | /* copy ROP */ > - WIDTH*4; > - batch[i++] = 0; /* dst x1,y1 */ > - batch[i++] = (HEIGHT << 16) | WIDTH; /* dst x2,y2 */ > - batch[i++] = 0; /* dst reloc */ > - if (intel_gen(intel_get_drm_devid(fd)) >= 8) > - batch[i++] = 0; > - batch[i++] = 0; /* src x1,y1 */ > - batch[i++] = WIDTH*4; > - batch[i++] = 0; /* src reloc */ > - if (intel_gen(intel_get_drm_devid(fd)) >= 8) > - batch[i++] = 0; > - batch[i++] = MI_BATCH_BUFFER_END; > - batch[i++] = MI_NOOP; > + bb_offset = 16 << 20; > + dst_offset = bb_offset + 4096; > + src_offset = dst_offset + WIDTH * HEIGHT * sizeof(uint32_t) * (src != dst); > + > + devid = intel_get_drm_devid(fd); > + > + if (intel_graphics_ver(devid) >= IP_VER(12, 60)) { > + batch[i++] = XY_FAST_COPY_BLT; > + batch[i++] = XY_FAST_COPY_COLOR_DEPTH_32 | WIDTH*4; > + batch[i++] = 0;/* dst x1,y1 */ ------------------------------ ^ Same here. > + batch[i++] = (HEIGHT << 16) | WIDTH;/* dst x2,y2 */ --------------------------------------------------- ^ > + batch[i++] = lower_32_bits(CANONICAL(dst_offset));/* dst address */ ------------------------------------------ ^ -------------------- ^ Same here. > + batch[i++] = upper_32_bits(CANONICAL(dst_offset)); > + batch[i++] = 0;/* src x1,y1 */ ------------------------------ ^ Put space before comment. > + batch[i++] = WIDTH*4;/* src pitch */ ------------------------------------ ^ Put space before comment. > + batch[i++] = lower_32_bits(CANONICAL(src_offset));//src address ------------------------------------------ ^ -------------------- ^ Same here, also please be consistent and use the same style for comments, so write /* src address */ > + batch[i++] = upper_32_bits(CANONICAL(src_offset)); > + batch[i++] = MI_BATCH_BUFFER_END; > + batch[i++] = MI_NOOP; > + } else { > + batch[i++] = XY_SRC_COPY_BLT_CMD | > + XY_SRC_COPY_BLT_WRITE_ALPHA | > + XY_SRC_COPY_BLT_WRITE_RGB; > + if (intel_gen(intel_get_drm_devid(fd)) >= 8) > + batch[i - 1] |= 8; > + else > + batch[i - 1] |= 6; > + batch[i++] = (3 << 24) | /* 32 bits */ > + (0xcc << 16) | /* copy ROP */ > + WIDTH*4; > + batch[i++] = 0; /* dst x1,y1 */ > + batch[i++] = (HEIGHT << 16) | WIDTH; /* dst x2,y2 */ > + batch[i++] = lower_32_bits(CANONICAL(dst_offset));/* dst reloc */ ------------------------------------------ ^ -------------------- ^ Same here. > + batch[i++] = upper_32_bits(CANONICAL(dst_offset)); > + if (intel_gen(intel_get_drm_devid(fd)) >= 8) ----------------------------- ^ Reuse devid you got above. > + batch[i++] = 0; > + batch[i++] = 0; /* src x1,y1 */ > + batch[i++] = WIDTH*4; > + batch[i++] = lower_32_bits(CANONICAL(src_offset)); /* src reloc */ ------------------------------------------ ^ Same here. > + batch[i++] = upper_32_bits(CANONICAL(src_offset)); > + if (intel_gen(intel_get_drm_devid(fd)) >= 8) ----------------------------- ^ Reuse devid you got above. Regards, Kamil > + batch[i++] = 0; > + batch[i++] = MI_BATCH_BUFFER_END; > + batch[i++] = MI_NOOP; > + } > > handle = gem_create(fd, 4096); > gem_write(fd, handle, 0, batch, sizeof(batch)); > -- > 2.25.1 >