From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTPS id 9720610E533 for ; Thu, 22 Dec 2022 09:51:00 +0000 (UTC) Message-ID: Date: Thu, 22 Dec 2022 10:50:45 +0100 Content-Language: en-US To: Kamil Konieczny References: From: Karolina Stolarek In-Reply-To: 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 3/4] lib/i915_blt: Extract init functions for blt_copy_object 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 21.12.2022 14:59, Kamil Konieczny wrote: > Hi Karolina, > > please improve commit description, > imho s/Extract init/Add common/ But we're reusing the code, not adding new one. > On 2022-12-19 at 12:49:10 +0100, Karolina Stolarek wrote: >> gem_ccs and gem_lmem_swapping share a couple of functions. Extract them >> to i915_blt so they are accessible for both tests. Delete local > ---------------------------------------------------- ^ > That sentence is not needed here or if you want you can write > about refactoring (you replaced those functions with lib calls). > >> definitions. > >> >> Signed-off-by: Karolina Stolarek >> --- >> Karolina's comment: I'm not convinced it's the best place for them. >> These functions are more specific to the tests > - ^^^ > Please avoid huge empty spaces here. Heh, I should've eyed it in vim before sending. Anyway, everything below -- is ignored when the patch is applied. > >> than lib functions. So, I'd appreciate >> suggestions on where I can move them to. >> >> lib/i915/i915_blt.c | 81 ++++++++++++++++++++++++++++++++++ >> lib/i915/i915_blt.h | 22 +++++++++ >> tests/i915/gem_ccs.c | 81 ---------------------------------- >> tests/i915/gem_lmem_swapping.c | 36 --------------- >> 4 files changed, 103 insertions(+), 117 deletions(-) >> >> diff --git a/lib/i915/i915_blt.c b/lib/i915/i915_blt.c >> index 2513dfc0..0f9f7265 100644 >> --- a/lib/i915/i915_blt.c >> +++ b/lib/i915/i915_blt.c >> @@ -1053,6 +1053,87 @@ int blt_fast_copy(int i915, >> return ret; >> } >> > > You need to add descriptions to all lib functions you added. I wrote in my cover letter that it's WIP and will add it in v2. >> +void set_geom(struct blt_copy_object *obj, uint32_t pitch, > ------- ^ > The name was ok inside test, but in lib imho it's better add > some common prefix, maybe blt_ ? For example: > > void blt_set_geom(...params here... > Suggested by Zbigniewa and already applies, thanks All the best, Karolina >> + int16_t x1, int16_t y1, int16_t x2, int16_t y2, >> + uint16_t x_offset, uint16_t y_offset) >> +{ >> + obj->pitch = pitch; >> + obj->x1 = x1; >> + obj->y1 = y1; >> + obj->x2 = x2; >> + obj->y2 = y2; >> + obj->x_offset = x_offset; >> + obj->y_offset = y_offset; >> +} >> + >> +void set_batch(struct blt_copy_batch *batch, > ------- ^ > >> + uint32_t handle, uint64_t size, uint32_t region) >> +{ >> + batch->handle = handle; >> + batch->size = size; >> + batch->region = region; >> +} >> + >> +struct blt_copy_object * >> +create_object(int i915, uint32_t region, > ------- ^ > >> + uint32_t width, uint32_t height, uint32_t bpp, uint8_t mocs, >> + enum tiling_type tiling, >> + enum blt_compression compression, >> + enum blt_compression_type compression_type, >> + bool create_mapping) >> +{ >> + struct blt_copy_object *obj; >> + uint64_t size = width * height * bpp / 8; >> + uint32_t stride = tiling == T_LINEAR ? width * 4 : width; >> + uint32_t handle; >> + >> + obj = calloc(1, sizeof(*obj)); >> + >> + obj->size = size; >> + igt_assert(__gem_create_in_memory_regions(i915, &handle, >> + &size, region) == 0); >> + >> + set_object(obj, handle, size, region, mocs, tiling, >> + compression, compression_type); >> + set_geom(obj, stride, 0, 0, width, height, 0, 0); >> + >> + if (create_mapping) >> + obj->ptr = gem_mmap__device_coherent(i915, handle, 0, size, >> + PROT_READ | PROT_WRITE); >> + >> + return obj; >> +} >> + >> +void destroy_object(int i915, struct blt_copy_object *obj) > ------- ^ > >> +{ >> + if (obj->ptr) >> + munmap(obj->ptr, obj->size); >> + >> + gem_close(i915, obj->handle); >> + free(obj); >> +} >> + >> +void set_object(struct blt_copy_object *obj, > ------- ^ > blt_set_object > >> + uint32_t handle, uint64_t size, uint32_t region, >> + uint8_t mocs, enum tiling_type tiling, >> + enum blt_compression compression, >> + enum blt_compression_type compression_type) >> +{ >> + obj->handle = handle; >> + obj->size = size; >> + obj->region = region; >> + obj->mocs = mocs; >> + obj->tiling = tiling; >> + obj->compression = compression; >> + obj->compression_type = compression_type; >> +} >> + >> +void set_blt_object(struct blt_copy_object *obj, > ------- ^ > This will clash with above name blt_set_object ? > Maybe blt_set_copy_object ? > > Regards, > Kamil > >> + const struct blt_copy_object *orig) >> +{ >> + memcpy(obj, orig, sizeof(*obj)); >> +} >> + >> /** >> * blt_surface_fill_rect: >> * @i915: drm fd >> diff --git a/lib/i915/i915_blt.h b/lib/i915/i915_blt.h >> index f1cf5408..1ef459c8 100644 >> --- a/lib/i915/i915_blt.h >> +++ b/lib/i915/i915_blt.h >> @@ -198,6 +198,28 @@ int blt_fast_copy(int i915, >> uint64_t ahnd, >> const struct blt_copy_data *blt); >> >> +void set_geom(struct blt_copy_object *obj, uint32_t pitch, >> + int16_t x1, int16_t y1, int16_t x2, int16_t y2, >> + uint16_t x_offset, uint16_t y_offset); >> +void set_batch(struct blt_copy_batch *batch, >> + uint32_t handle, uint64_t size, uint32_t region); >> + >> +struct blt_copy_object * >> +create_object(int i915, uint32_t region, >> + uint32_t width, uint32_t height, uint32_t bpp, uint8_t mocs, >> + enum tiling_type tiling, >> + enum blt_compression compression, >> + enum blt_compression_type compression_type, >> + bool create_mapping); >> +void destroy_object(int i915, struct blt_copy_object *obj); >> +void set_object(struct blt_copy_object *obj, >> + uint32_t handle, uint64_t size, uint32_t region, >> + uint8_t mocs, enum tiling_type tiling, >> + enum blt_compression compression, >> + enum blt_compression_type compression_type); >> +void set_blt_object(struct blt_copy_object *obj, >> + const struct blt_copy_object *orig); >> + >> void blt_surface_info(const char *info, >> const struct blt_copy_object *obj); >> void blt_surface_fill_rect(int i915, const struct blt_copy_object *obj, >> diff --git a/tests/i915/gem_ccs.c b/tests/i915/gem_ccs.c >> index 4c137c94..30ee60fd 100644 >> --- a/tests/i915/gem_ccs.c >> +++ b/tests/i915/gem_ccs.c >> @@ -44,42 +44,6 @@ struct test_config { >> bool suspend_resume; >> }; >> >> -static void set_object(struct blt_copy_object *obj, >> - uint32_t handle, uint64_t size, uint32_t region, >> - uint8_t mocs, enum tiling_type tiling, >> - enum blt_compression compression, >> - enum blt_compression_type compression_type) >> -{ >> - obj->handle = handle; >> - obj->size = size; >> - obj->region = region; >> - obj->mocs = mocs; >> - obj->tiling = tiling; >> - obj->compression = compression; >> - obj->compression_type = compression_type; >> -} >> - >> -static void set_geom(struct blt_copy_object *obj, uint32_t pitch, >> - int16_t x1, int16_t y1, int16_t x2, int16_t y2, >> - uint16_t x_offset, uint16_t y_offset) >> -{ >> - obj->pitch = pitch; >> - obj->x1 = x1; >> - obj->y1 = y1; >> - obj->x2 = x2; >> - obj->y2 = y2; >> - obj->x_offset = x_offset; >> - obj->y_offset = y_offset; >> -} >> - >> -static void set_batch(struct blt_copy_batch *batch, >> - uint32_t handle, uint64_t size, uint32_t region) >> -{ >> - batch->handle = handle; >> - batch->size = size; >> - batch->region = region; >> -} >> - >> static void set_object_ext(struct blt_block_copy_object_ext *obj, >> uint8_t compression_format, >> uint16_t surface_width, uint16_t surface_height, >> @@ -105,51 +69,6 @@ static void set_surf_object(struct blt_ctrl_surf_copy_object *obj, >> obj->access_type = access_type; >> } >> >> -static struct blt_copy_object * >> -create_object(int i915, uint32_t region, >> - uint32_t width, uint32_t height, uint32_t bpp, uint8_t mocs, >> - enum tiling_type tiling, >> - enum blt_compression compression, >> - enum blt_compression_type compression_type, >> - bool create_mapping) >> -{ >> - struct blt_copy_object *obj; >> - uint64_t size = width * height * bpp / 8; >> - uint32_t stride = tiling == T_LINEAR ? width * 4 : width; >> - uint32_t handle; >> - >> - obj = calloc(1, sizeof(*obj)); >> - >> - obj->size = size; >> - igt_assert(__gem_create_in_memory_regions(i915, &handle, >> - &size, region) == 0); >> - >> - set_object(obj, handle, size, region, mocs, tiling, >> - compression, compression_type); >> - set_geom(obj, stride, 0, 0, width, height, 0, 0); >> - >> - if (create_mapping) >> - obj->ptr = gem_mmap__device_coherent(i915, handle, 0, size, >> - PROT_READ | PROT_WRITE); >> - >> - return obj; >> -} >> - >> -static void destroy_object(int i915, struct blt_copy_object *obj) >> -{ >> - if (obj->ptr) >> - munmap(obj->ptr, obj->size); >> - >> - gem_close(i915, obj->handle); >> - free(obj); >> -} >> - >> -static void set_blt_object(struct blt_copy_object *obj, >> - const struct blt_copy_object *orig) >> -{ >> - memcpy(obj, orig, sizeof(*obj)); >> -} >> - >> #define PRINT_SURFACE_INFO(name, obj) do { \ >> if (param.print_surface_info) \ >> blt_surface_info((name), (obj)); } while (0) >> diff --git a/tests/i915/gem_lmem_swapping.c b/tests/i915/gem_lmem_swapping.c >> index 8cff35d5..746fbf80 100644 >> --- a/tests/i915/gem_lmem_swapping.c >> +++ b/tests/i915/gem_lmem_swapping.c >> @@ -76,42 +76,6 @@ struct object { >> struct blt_copy_object *blt_obj; >> }; >> >> -static void set_object(struct blt_copy_object *obj, >> - uint32_t handle, uint64_t size, uint32_t region, >> - uint8_t mocs, enum tiling_type tiling, >> - enum blt_compression compression, >> - enum blt_compression_type compression_type) >> -{ >> - obj->handle = handle; >> - obj->size = size; >> - obj->region = region; >> - obj->mocs = mocs; >> - obj->tiling = tiling; >> - obj->compression = compression; >> - obj->compression_type = compression_type; >> -} >> - >> -static void set_geom(struct blt_copy_object *obj, uint32_t pitch, >> - int16_t x1, int16_t y1, int16_t x2, int16_t y2, >> - uint16_t x_offset, uint16_t y_offset) >> -{ >> - obj->pitch = pitch; >> - obj->x1 = x1; >> - obj->y1 = y1; >> - obj->x2 = x2; >> - obj->y2 = y2; >> - obj->x_offset = x_offset; >> - obj->y_offset = y_offset; >> -} >> - >> -static void set_batch(struct blt_copy_batch *batch, >> - uint32_t handle, uint64_t size, uint32_t region) >> -{ >> - batch->handle = handle; >> - batch->size = size; >> - batch->region = region; >> -} >> - >> static void set_object_ext(struct blt_block_copy_object_ext *obj, >> uint8_t compression_format, >> uint16_t surface_width, uint16_t surface_height, >> -- >> 2.25.1 >>