From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTPS id 23E2510E10B for ; Mon, 27 Mar 2023 14:06:29 +0000 (UTC) Message-ID: Date: Mon, 27 Mar 2023 16:06:13 +0200 Content-Language: en-US To: Vikas Srivastava References: <20230324133346.2284000-1-vikas.srivastava@intel.com> <20230324133346.2284000-5-vikas.srivastava@intel.com> From: Karolina Stolarek In-Reply-To: <20230324133346.2284000-5-vikas.srivastava@intel.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Subject: Re: [igt-dev] [PATCH i-g-t 4/4] tests/i915/gem_linear_blits: Enable XY_FAST_COPY_BLT copy instruction List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: igt-dev@lists.freedesktop.org Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: On 24.03.2023 14:33, Vikas Srivastava wrote: > From: Arjun Melkaveri > > Test case uses legacy command XY_SRC_COPY_BLT_CMD which is > not supported on newer platforms. Modified test to > use XY_FAST_COPY_BLT. > > Signed-off-by: Arjun Melkaveri > Co-developed-by: Vikas Srivastava Nice use of the tag, I should probably start using it in my patches as well... > Signed-off-by: Vikas Srivastava > --- > tests/i915/gem_linear_blits.c | 66 +++++++++++++++++++++++------------ > 1 file changed, 43 insertions(+), 23 deletions(-) > > diff --git a/tests/i915/gem_linear_blits.c b/tests/i915/gem_linear_blits.c > index fac25095f5..6fa6cd4976 100644 > --- a/tests/i915/gem_linear_blits.c > +++ b/tests/i915/gem_linear_blits.c > @@ -48,6 +48,7 @@ > #include "i915/gem_create.h" > #include "igt.h" > #include "igt_types.h" > +#include "i915/i915_blt.h" > > IGT_TEST_DESCRIPTION("Test doing many blits with a working set larger than the" > " aperture size."); > @@ -67,6 +68,7 @@ static void copy(int fd, uint64_t ahnd, 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; > + static uint32_t devid; > int i = 0; > > memset(obj, 0, sizeof(obj)); > @@ -83,29 +85,47 @@ static void copy(int fd, uint64_t ahnd, uint32_t dst, uint32_t src, > obj[2].offset = CANONICAL(obj[2].offset); > obj[2].flags = EXEC_OBJECT_SUPPORTS_48B_ADDRESS; > > - 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++] = obj[0].offset; > - if (intel_gen(intel_get_drm_devid(fd)) >= 8) > - batch[i++] = obj[0].offset >> 32; > - batch[i++] = 0; /* src x1,y1 */ > - batch[i++] = WIDTH*4; > - batch[i++] = obj[1].offset; > - if (intel_gen(intel_get_drm_devid(fd)) >= 8) > - batch[i++] = obj[1].offset >> 32; > - batch[i++] = MI_BATCH_BUFFER_END; > - batch[i++] = MI_NOOP; > + devid = intel_get_drm_devid(fd); > + > + if (blt_has_fast_copy(fd)) { Don't forget to check the order, like I said in 1/4. > + batch[i++] = XY_FAST_COPY_BLT; > + batch[i++] = XY_FAST_COPY_COLOR_DEPTH_32 | WIDTH * 4; > + batch[i++] = 0; /* dst x1,y1 */ > + batch[i++] = (HEIGHT << 16) | WIDTH; /* dst x2,y2 */ > + batch[i++] = obj[0].offset; /* dst address lower bits */ > + batch[i++] = obj[0].offset >> 32; /* dst address upper bits */ > + batch[i++] = 0; /* src x1,y1 */ > + batch[i++] = WIDTH * 4; /* src pitch */ Just wanted to thank you for addressing my comments, even these for such minor changes :) > + batch[i++] = obj[1].offset; /* src address lower bits */ > + batch[i++] = obj[1].offset >> 32; /* src address upper bits */ > + batch[i++] = MI_BATCH_BUFFER_END; > + batch[i++] = MI_NOOP; > + } else if (blt_has_xy_src_copy(fd)) { > + 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++] = obj[0].offset; > + if (intel_gen(devid) >= 8) > + batch[i++] = obj[0].offset >> 32; > + batch[i++] = 0; /* src x1,y1 */ > + batch[i++] = WIDTH * 4; > + batch[i++] = obj[1].offset; > + if (intel_gen(devid) >= 8) > + batch[i++] = obj[1].offset >> 32; > + batch[i++] = MI_BATCH_BUFFER_END; > + batch[i++] = MI_NOOP; > + } else > + igt_assert_f(0, "No supported blit command found\n"); My comment on the brackets applies here as well. So, let's wait for the CI results (or check this and 3/4 locally), and if everything is fine and brackets are balanced, I can r-b these patches. Many thanks, Karolina > > gem_write(fd, obj[2].handle, 0, batch, i * sizeof(batch[0])); >