From: Kamil Konieczny <kamil.konieczny@linux.intel.com>
To: igt-dev@lists.freedesktop.org
Subject: Re: [igt-dev] [PATCH i-g-t v5] tests/i915/gem_userptr_blits: Added XY_FAST_COPY_BLT Command for MTL
Date: Mon, 5 Dec 2022 17:02:23 +0100 [thread overview]
Message-ID: <Y44WD8lSfJJywWmh@kamilkon-desk1> (raw)
In-Reply-To: <20221123142548.128038-1-vikas.srivastava@intel.com>
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 <arjun.melkaveri@intel.com>
>
> Test case uses legacy command which is not supported on MTL.
> Modified test to use XY_FAST_COPY_BLT.
>
> Signed-off-by: Arjun Melkaveri <arjun.melkaveri@intel.com>
> Acked-by: Priyanka Dandamudi <priyanka.dandamudi@intel.com>
> ---
> 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
>
prev parent reply other threads:[~2022-12-05 16:02 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-23 14:25 [igt-dev] [PATCH i-g-t v5] tests/i915/gem_userptr_blits: Added XY_FAST_COPY_BLT Command for MTL Vikas Srivastava
2022-11-23 15:09 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/i915/gem_userptr_blits: Added XY_FAST_COPY_BLT Command for MTL (rev5) Patchwork
2022-12-05 16:02 ` Kamil Konieczny [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y44WD8lSfJJywWmh@kamilkon-desk1 \
--to=kamil.konieczny@linux.intel.com \
--cc=igt-dev@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox