From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by gabe.freedesktop.org (Postfix) with ESMTPS id F0BE96ED9B for ; Tue, 11 Feb 2020 00:38:12 +0000 (UTC) Date: Tue, 11 Feb 2020 02:37:48 +0200 From: Imre Deak Message-ID: <20200211003748.GA10361@ideak-desk.fi.intel.com> References: <20200207191524.19362-1-imre.deak@intel.com> <20200210221331.GN232048@mdroper-desk1.amr.corp.intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200210221331.GN232048@mdroper-desk1.amr.corp.intel.com> Subject: Re: [igt-dev] [PATCH i-g-t 1/8] lib/igt_draw: Refactor get_tiling calls List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: imre.deak@intel.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: Matt Roper Cc: igt-dev@lists.freedesktop.org, Vanshidhar Konda List-ID: On Mon, Feb 10, 2020 at 02:13:31PM -0800, Matt Roper wrote: > On Fri, Feb 07, 2020 at 09:15:17PM +0200, Imre Deak wrote: > > From: Vanshidhar Konda > > > > Simplify the number of places from which gem_get_tiling method is called > > and call it only if the device has support in hardware for tiling. > > > > v2: > > - Use gem_available_fences() to check for HW detiling. > > > > Signed-off-by: Vanshidhar Konda > > Signed-off-by: Imre Deak > > --- > > lib/igt_draw.c | 56 +++++++++++++++++--------------- > > lib/igt_draw.h | 5 +-- > > tests/kms_frontbuffer_tracking.c | 8 +++-- > > 3 files changed, 37 insertions(+), 32 deletions(-) > > > > diff --git a/lib/igt_draw.c b/lib/igt_draw.c > > index 6950bc49..d227a53a 100644 > > --- a/lib/igt_draw.c > > +++ b/lib/igt_draw.c > > @@ -333,20 +333,19 @@ static void draw_rect_ptr_tiled(void *ptr, uint32_t stride, uint32_t tiling, > > } > > > > static void draw_rect_mmap_cpu(int fd, struct buf_data *buf, struct rect *rect, > > - uint32_t color) > > + uint32_t tiling, uint32_t swizzle, uint32_t color) > > { > > uint32_t *ptr; > > - uint32_t tiling, swizzle; > > > > gem_set_domain(fd, buf->handle, I915_GEM_DOMAIN_CPU, > > I915_GEM_DOMAIN_CPU); > > - igt_require(gem_get_tiling(fd, buf->handle, &tiling, &swizzle)); > > > > /* We didn't implement suport for the older tiling methods yet. */ > > if (tiling != I915_TILING_NONE) > > igt_require(intel_gen(intel_get_drm_devid(fd)) >= 5); > > > > - ptr = gem_mmap__cpu(fd, buf->handle, 0, PAGE_ALIGN(buf->size), 0); > > + ptr = gem_mmap__cpu(fd, buf->handle, 0, PAGE_ALIGN(buf->size), > > + PROT_READ | PROT_WRITE); > > This change doesn't seem to be related to the main purpose of this > patch? It's a consequence of starting to use __gem_mmap_offset() on DG1 after this patch, which uses the prot argument, unlike __gem_mmap() used so far. This should've been part of the commit log. > > > > > switch (tiling) { > > case I915_TILING_NONE: > > @@ -384,14 +383,12 @@ static void draw_rect_mmap_gtt(int fd, struct buf_data *buf, struct rect *rect, > > } > > > > static void draw_rect_mmap_wc(int fd, struct buf_data *buf, struct rect *rect, > > - uint32_t color) > > + uint32_t tiling, uint32_t swizzle, uint32_t color) > > { > > uint32_t *ptr; > > - uint32_t tiling, swizzle; > > > > gem_set_domain(fd, buf->handle, I915_GEM_DOMAIN_GTT, > > I915_GEM_DOMAIN_GTT); > > - igt_require(gem_get_tiling(fd, buf->handle, &tiling, &swizzle)); > > > > /* We didn't implement suport for the older tiling methods yet. */ > > if (tiling != I915_TILING_NONE) > > @@ -495,12 +492,9 @@ static void draw_rect_pwrite_tiled(int fd, struct buf_data *buf, > > } > > > > static void draw_rect_pwrite(int fd, struct buf_data *buf, > > - struct rect *rect, uint32_t color) > > + struct rect *rect, uint32_t tiling, > > + uint32_t swizzle, uint32_t color) > > { > > - uint32_t tiling, swizzle; > > - > > - igt_require(gem_get_tiling(fd, buf->handle, &tiling, &swizzle)); > > - > > switch (tiling) { > > case I915_TILING_NONE: > > draw_rect_pwrite_untiled(fd, buf, rect, color); > > @@ -517,6 +511,7 @@ static void draw_rect_pwrite(int fd, struct buf_data *buf, > > > > static void draw_rect_blt(int fd, struct cmd_data *cmd_data, > > struct buf_data *buf, struct rect *rect, > > + uint32_t tiling, uint32_t swizzle, > > uint32_t color) > > { > > drm_intel_bo *dst; > > @@ -524,11 +519,8 @@ static void draw_rect_blt(int fd, struct cmd_data *cmd_data, > > int blt_cmd_len, blt_cmd_tiling, blt_cmd_depth; > > uint32_t devid = intel_get_drm_devid(fd); > > int gen = intel_gen(devid); > > - uint32_t tiling, swizzle; > > int pitch; > > > > - igt_require(gem_get_tiling(fd, buf->handle, &tiling, &swizzle)); > > - > > dst = gem_handle_to_libdrm_bo(cmd_data->bufmgr, fd, "", buf->handle); > > igt_assert(dst); > > > > @@ -574,6 +566,7 @@ static void draw_rect_blt(int fd, struct cmd_data *cmd_data, > > > > static void draw_rect_render(int fd, struct cmd_data *cmd_data, > > struct buf_data *buf, struct rect *rect, > > + uint32_t tiling, uint32_t swizzle, > > uint32_t color) > > { > > drm_intel_bo *src, *dst; > > @@ -581,21 +574,18 @@ static void draw_rect_render(int fd, struct cmd_data *cmd_data, > > igt_render_copyfunc_t rendercopy = igt_get_render_copyfunc(devid); > > struct igt_buf src_buf = {}, dst_buf = {}; > > struct intel_batchbuffer *batch; > > - uint32_t tiling, swizzle; > > struct buf_data tmp; > > int pixel_size = buf->bpp / 8; > > > > igt_skip_on(!rendercopy); > > > > - igt_require(gem_get_tiling(fd, buf->handle, &tiling, &swizzle)); > > - > > /* We create a temporary buffer and copy from it using rendercopy. */ > > tmp.size = rect->w * rect->h * pixel_size; > > tmp.handle = gem_create(fd, tmp.size); > > tmp.stride = rect->w * pixel_size; > > tmp.bpp = buf->bpp; > > draw_rect_mmap_cpu(fd, &tmp, &(struct rect){0, 0, rect->w, rect->h}, > > - color); > > + I915_TILING_NONE, I915_BIT_6_SWIZZLE_NONE, color); > > > > src = gem_handle_to_libdrm_bo(cmd_data->bufmgr, fd, "", tmp.handle); > > igt_assert(src); > > @@ -647,9 +637,12 @@ static void draw_rect_render(int fd, struct cmd_data *cmd_data, > > */ > > void igt_draw_rect(int fd, drm_intel_bufmgr *bufmgr, drm_intel_context *context, > > uint32_t buf_handle, uint32_t buf_size, uint32_t buf_stride, > > - enum igt_draw_method method, int rect_x, int rect_y, > > - int rect_w, int rect_h, uint32_t color, int bpp) > > + uint32_t tiling, enum igt_draw_method method, > > The new tiling parameter should probably be captured in the kerneldoc > right above this function. Yes, that was missed. > > > + int rect_x, int rect_y, int rect_w, int rect_h, > > + uint32_t color, int bpp) > > { > > + uint32_t buf_tiling, swizzle; > > + > > struct cmd_data cmd_data = { > > .bufmgr = bufmgr, > > .context = context, > > @@ -667,24 +660,32 @@ void igt_draw_rect(int fd, drm_intel_bufmgr *bufmgr, drm_intel_context *context, > > .h = rect_h, > > }; > > > > + swizzle = I915_BIT_6_SWIZZLE_NONE; > > + if (tiling != I915_TILING_NONE && gem_available_fences(fd)) { > > + gem_get_tiling(fd, buf_handle, &buf_tiling, &swizzle); > > + igt_assert(tiling == buf_tiling); > > + } > > + > > switch (method) { > > case IGT_DRAW_MMAP_CPU: > > - draw_rect_mmap_cpu(fd, &buf, &rect, color); > > + draw_rect_mmap_cpu(fd, &buf, &rect, tiling, swizzle, color); > > break; > > case IGT_DRAW_MMAP_GTT: > > draw_rect_mmap_gtt(fd, &buf, &rect, color); > > break; > > case IGT_DRAW_MMAP_WC: > > - draw_rect_mmap_wc(fd, &buf, &rect, color); > > + draw_rect_mmap_wc(fd, &buf, &rect, tiling, swizzle, color); > > break; > > case IGT_DRAW_PWRITE: > > - draw_rect_pwrite(fd, &buf, &rect, color); > > + draw_rect_pwrite(fd, &buf, &rect, tiling, swizzle, color); > > break; > > case IGT_DRAW_BLT: > > - draw_rect_blt(fd, &cmd_data, &buf, &rect, color); > > + draw_rect_blt(fd, &cmd_data, &buf, &rect, tiling, swizzle, > > + color); > > break; > > case IGT_DRAW_RENDER: > > - draw_rect_render(fd, &cmd_data, &buf, &rect, color); > > + draw_rect_render(fd, &cmd_data, &buf, &rect, tiling, swizzle, > > + color); > > break; > > Blt and render draws don't use the swizzle parameter. If the goal is to > keep a consistent parameter list for all these functions, we should > probably add tiling & swizzle to the gtt map function, even though they > won't be used; otherwise we can drop the swizzle parameter from these > two. Ok, will remove swizzle from the blt/render funcs. > > > Matt > > > default: > > igt_assert(false); > > @@ -715,7 +716,8 @@ void igt_draw_rect_fb(int fd, drm_intel_bufmgr *bufmgr, > > int rect_w, int rect_h, uint32_t color) > > { > > igt_draw_rect(fd, bufmgr, context, fb->gem_handle, fb->size, fb->strides[0], > > - method, rect_x, rect_y, rect_w, rect_h, color, > > + igt_fb_mod_to_tiling(fb->modifier), method, > > + rect_x, rect_y, rect_w, rect_h, color, > > igt_drm_format_to_bpp(fb->drm_format)); > > } > > > > diff --git a/lib/igt_draw.h b/lib/igt_draw.h > > index b030131e..ec146754 100644 > > --- a/lib/igt_draw.h > > +++ b/lib/igt_draw.h > > @@ -52,8 +52,9 @@ const char *igt_draw_get_method_name(enum igt_draw_method method); > > > > void igt_draw_rect(int fd, drm_intel_bufmgr *bufmgr, drm_intel_context *context, > > uint32_t buf_handle, uint32_t buf_size, uint32_t buf_stride, > > - enum igt_draw_method method, int rect_x, int rect_y, > > - int rect_w, int rect_h, uint32_t color, int bpp); > > + uint32_t tiling, enum igt_draw_method method, > > + int rect_x, int rect_y, int rect_w, int rect_h, > > + uint32_t color, int bpp); > > > > void igt_draw_rect_fb(int fd, drm_intel_bufmgr *bufmgr, > > drm_intel_context *context, struct igt_fb *fb, > > diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c > > index 2c765c34..9c2c3a5d 100644 > > --- a/tests/kms_frontbuffer_tracking.c > > +++ b/tests/kms_frontbuffer_tracking.c > > @@ -291,6 +291,7 @@ struct { > > int height; > > uint32_t color; > > int bpp; > > + uint32_t tiling; > > } busy_thread = { > > .stop = true, > > }; > > @@ -1126,9 +1127,9 @@ static void *busy_thread_func(void *data) > > while (!busy_thread.stop) > > igt_draw_rect(drm.fd, drm.bufmgr, NULL, busy_thread.handle, > > busy_thread.size, busy_thread.stride, > > - IGT_DRAW_BLT, 0, 0, busy_thread.width, > > - busy_thread.height, busy_thread.color, > > - busy_thread.bpp); > > + busy_thread.tiling, IGT_DRAW_BLT, 0, 0, > > + busy_thread.width, busy_thread.height, > > + busy_thread.color, busy_thread.bpp); > > > > pthread_exit(0); > > } > > @@ -1146,6 +1147,7 @@ static void start_busy_thread(struct igt_fb *fb) > > busy_thread.height = fb->height; > > busy_thread.color = pick_color(fb, COLOR_PRIM_BG); > > busy_thread.bpp = igt_drm_format_to_bpp(fb->drm_format); > > + busy_thread.tiling = igt_fb_mod_to_tiling(fb->modifier); > > > > rc = pthread_create(&busy_thread.thread, NULL, busy_thread_func, NULL); > > igt_assert_eq(rc, 0); > > -- > > 2.23.1 > > > > _______________________________________________ > > igt-dev mailing list > > igt-dev@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/igt-dev > > -- > Matt Roper > Graphics Software Engineer > VTT-OSGC Platform Enablement > Intel Corporation > (916) 356-2795 _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev