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 E666E10E148 for ; Mon, 30 Jan 2023 08:21:38 +0000 (UTC) Message-ID: <2ab35ef4-c027-99c5-721f-95b845cd0620@intel.com> Date: Mon, 30 Jan 2023 09:21:09 +0100 Content-Language: en-US To: =?UTF-8?Q?Zbigniew_Kempczy=c5=84ski?= References: <20230126121350.kdyvqhcr477izj5e@zkempczy-mobl2> From: Karolina Stolarek In-Reply-To: <20230126121350.kdyvqhcr477izj5e@zkempczy-mobl2> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit MIME-Version: 1.0 Subject: Re: [igt-dev] [PATCH i-g-t 2/2] lib/i915_blt: Split up blt_copy_object functions 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: Hi, On 26.01.2023 13:13, Zbigniew Kempczyński wrote: > On Mon, Jan 23, 2023 at 11:52:43AM +0100, Karolina Stolarek wrote: >> Add dedicated functions to set and create blt_copy_object depending on >> the copy type. Extract the common path so it can be called both in fast >> and block copy setup. Move mocs field in blt_copy_object so it's next >> to the compression-specific fields. >> >> Signed-off-by: Karolina Stolarek >> --- >> lib/i915/i915_blt.c | 92 ++++++++++++++++++++++++---------- >> lib/i915/i915_blt.h | 29 +++++++---- >> tests/i915/gem_ccs.c | 39 +++++++------- >> tests/i915/gem_exercise_blt.c | 18 +++---- >> tests/i915/gem_lmem_swapping.c | 13 ++--- >> 5 files changed, 117 insertions(+), 74 deletions(-) >> >> diff --git a/lib/i915/i915_blt.c b/lib/i915/i915_blt.c >> index 3e64efeb..979c7161 100644 >> --- a/lib/i915/i915_blt.c >> +++ b/lib/i915/i915_blt.c >> @@ -1214,12 +1214,29 @@ void blt_set_batch(struct blt_copy_batch *batch, >> batch->region = region; >> } >> >> -struct blt_copy_object * >> -blt_create_object(int i915, uint32_t region, >> - uint32_t width, uint32_t height, uint32_t bpp, uint8_t mocs, >> - enum blt_tiling_type tiling, >> - enum blt_compression compression, >> - enum blt_compression_type compression_type) >> +static void blt_set_object_common(struct blt_copy_object *obj, >> + uint32_t handle, uint64_t size, >> + uint32_t region, enum blt_tiling_type tiling) >> +{ >> + obj->handle = handle; >> + obj->size = size; >> + obj->region = region; >> + obj->tiling = tiling; >> +} >> + >> +static void blt_set_object_compression(struct blt_copy_object *obj, uint8_t mocs, >> + enum blt_compression compression, >> + enum blt_compression_type compression_type) >> +{ >> + obj->mocs = mocs; >> + obj->compression = compression; >> + obj->compression_type = compression_type; >> +} >> + >> +static struct blt_copy_object * >> +blt_create_object_common(int i915, uint32_t region, uint32_t width, >> + uint32_t height, uint32_t bpp, >> + enum blt_tiling_type tiling) > > This should be public. We assume above fields are common to all > supported commands we have (block-copy, fast-copy, etc), so user > can call this generic function and it should work for all > blit instructions. That means that you should fill mocs here even > if user didn't explicitly passed this value. > > Rename this to blt_create_object(), I don't think _common suffix > is really necessary here. But this means we'd have blt_create_block_copy and blt_create_object, as it wouldn't make sense to also have blt_create_fast_object. Having a special treatment of block-copy would make things confusing. The best option would be to have blt_create_fast_object and call it inside blt_create_block_object. It'd would be fine by the user, maybe slightly confusing for people who maintain the lib, but it should be alright. I think we should separate the commands, even if they use the same struct underneath. Thanks, Karolina > -- > Zbigniew > >> { >> struct blt_copy_object *obj; >> uint64_t size = width * height * bpp / 8; >> @@ -1228,20 +1245,58 @@ blt_create_object(int i915, uint32_t region, >> >> obj = calloc(1, sizeof(*obj)); >> >> - obj->size = size; >> igt_assert(__gem_create_in_memory_regions(i915, &handle, >> &size, region) == 0); >> + obj->ptr = gem_mmap__device_coherent(i915, handle, 0, size, >> + PROT_READ | PROT_WRITE); >> + obj->size = size; >> >> - blt_set_object(obj, handle, size, region, mocs, tiling, >> - compression, compression_type); >> + blt_set_object_common(obj, handle, size, region, tiling); >> blt_set_geom(obj, stride, 0, 0, width, height, 0, 0); >> >> - obj->ptr = gem_mmap__device_coherent(i915, handle, 0, size, >> - PROT_READ | PROT_WRITE); >> + return obj; >> +} >> + >> +struct blt_copy_object * >> +blt_create_block_object(int i915, uint32_t region, >> + uint32_t width, uint32_t height, uint32_t bpp, >> + uint8_t mocs, enum blt_tiling_type tiling, >> + enum blt_compression compression, >> + enum blt_compression_type compression_type) > a> +{ >> + struct blt_copy_object *obj; >> + >> + obj = blt_create_object_common(i915, region, width, height, bpp, tiling); >> + blt_set_object_compression(obj, mocs, compression, compression_type); >> >> return obj; >> } >> >> +struct blt_copy_object * >> +blt_create_fast_object(int i915, uint32_t region, uint32_t width, >> + uint32_t height, uint32_t bpp, >> + enum blt_tiling_type tiling) >> +{ >> + return blt_create_object_common(i915, region, width, height, bpp, tiling); >> +} >> + >> +void blt_set_block_object(struct blt_copy_object *obj, >> + uint32_t handle, uint64_t size, uint32_t region, >> + uint8_t mocs, enum blt_tiling_type tiling, >> + enum blt_compression compression, >> + enum blt_compression_type compression_type) >> +{ >> + blt_set_object_common(obj, handle, size, region, tiling); >> + blt_set_object_compression(obj, mocs, compression, compression_type); >> +} >> + >> +void blt_set_fast_object(struct blt_copy_object *obj, >> + uint32_t handle, uint64_t size, >> + uint32_t region, enum blt_tiling_type tiling) >> +{ >> + blt_set_object_common(obj, handle, size, region, tiling); >> +} >> + >> void blt_destroy_object(int i915, struct blt_copy_object *obj) >> { >> if (obj->ptr) >> @@ -1251,21 +1306,6 @@ void blt_destroy_object(int i915, struct blt_copy_object *obj) >> free(obj); >> } >> >> -void blt_set_object(struct blt_copy_object *obj, >> - uint32_t handle, uint64_t size, uint32_t region, >> - uint8_t mocs, enum blt_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 blt_set_object_ext(struct blt_block_copy_object_ext *obj, >> uint8_t compression_format, >> uint16_t surface_width, uint16_t surface_height, >> diff --git a/lib/i915/i915_blt.h b/lib/i915/i915_blt.h >> index eaf4cc1f..1bd29c8c 100644 >> --- a/lib/i915/i915_blt.h >> +++ b/lib/i915/i915_blt.h >> @@ -75,8 +75,8 @@ struct blt_copy_object { >> uint32_t handle; >> uint32_t region; >> uint64_t size; >> - uint8_t mocs; >> enum blt_tiling_type tiling; >> + uint8_t mocs; /* BC only */ >> enum blt_compression compression; /* BC only */ >> enum blt_compression_type compression_type; /* BC only */ >> uint32_t pitch; >> @@ -214,17 +214,24 @@ void blt_set_batch(struct blt_copy_batch *batch, >> uint32_t handle, uint64_t size, uint32_t region); >> >> struct blt_copy_object * >> -blt_create_object(int i915, uint32_t region, >> - uint32_t width, uint32_t height, uint32_t bpp, uint8_t mocs, >> - enum blt_tiling_type tiling, >> - enum blt_compression compression, >> - enum blt_compression_type compression_type); >> +blt_create_block_object(int i915, uint32_t region, >> + uint32_t width, uint32_t height, uint32_t bpp, >> + uint8_t mocs, enum blt_tiling_type tiling, >> + enum blt_compression compression, >> + enum blt_compression_type compression_type); >> +struct blt_copy_object * >> +blt_create_fast_object(int i915, uint32_t region, >> + uint32_t width, uint32_t height, uint32_t bpp, >> + enum blt_tiling_type tiling); >> void blt_destroy_object(int i915, struct blt_copy_object *obj); >> -void blt_set_object(struct blt_copy_object *obj, >> - uint32_t handle, uint64_t size, uint32_t region, >> - uint8_t mocs, enum blt_tiling_type tiling, >> - enum blt_compression compression, >> - enum blt_compression_type compression_type); >> +void blt_set_block_object(struct blt_copy_object *obj, >> + uint32_t handle, uint64_t size, uint32_t region, >> + uint8_t mocs, enum blt_tiling_type tiling, >> + enum blt_compression compression, >> + enum blt_compression_type compression_type); >> +void blt_set_fast_object(struct blt_copy_object *obj, >> + uint32_t handle, uint64_t size, uint32_t region, >> + enum blt_tiling_type tiling); >> void blt_set_object_ext(struct blt_block_copy_object_ext *obj, >> uint8_t compression_format, >> uint16_t surface_width, uint16_t surface_height, >> diff --git a/tests/i915/gem_ccs.c b/tests/i915/gem_ccs.c >> index b7a32673..7e16c792 100644 >> --- a/tests/i915/gem_ccs.c >> +++ b/tests/i915/gem_ccs.c >> @@ -360,12 +360,12 @@ static void block_copy(int i915, >> if (!blt_supports_compression(i915) && !IS_METEORLAKE(devid)) >> pext = NULL; >> >> - src = blt_create_object(i915, region1, width, height, bpp, uc_mocs, >> - T_LINEAR, COMPRESSION_DISABLED, comp_type); >> - mid = blt_create_object(i915, mid_region, width, height, bpp, uc_mocs, >> - mid_tiling, mid_compression, comp_type); >> - dst = blt_create_object(i915, region1, width, height, bpp, uc_mocs, >> - T_LINEAR, COMPRESSION_DISABLED, comp_type); >> + src = blt_create_block_object(i915, region1, width, height, bpp, uc_mocs, >> + T_LINEAR, COMPRESSION_DISABLED, comp_type); >> + mid = blt_create_block_object(i915, mid_region, width, height, bpp, uc_mocs, >> + mid_tiling, mid_compression, comp_type); >> + dst = blt_create_block_object(i915, region1, width, height, bpp, uc_mocs, >> + T_LINEAR, COMPRESSION_DISABLED, comp_type); >> igt_assert(src->size == dst->size); >> PRINT_SURFACE_INFO("src", src); >> PRINT_SURFACE_INFO("mid", mid); >> @@ -428,8 +428,9 @@ static void block_copy(int i915, >> blt_set_object_ext(&ext.src, mid_compression_format, width, height, SURFACE_TYPE_2D); >> blt_set_object_ext(&ext.dst, 0, width, height, SURFACE_TYPE_2D); >> if (config->inplace) { >> - blt_set_object(&blt.dst, mid->handle, dst->size, mid->region, 0, >> - T_LINEAR, COMPRESSION_DISABLED, comp_type); >> + blt_set_block_object(&blt.dst, mid->handle, dst->size, >> + mid->region, 0, T_LINEAR, >> + COMPRESSION_DISABLED, comp_type); >> blt.dst.ptr = mid->ptr; >> } >> >> @@ -476,14 +477,14 @@ static void block_multicopy(int i915, >> if (!blt_supports_compression(i915)) >> pext3 = NULL; >> >> - src = blt_create_object(i915, region1, width, height, bpp, uc_mocs, >> - T_LINEAR, COMPRESSION_DISABLED, comp_type); >> - mid = blt_create_object(i915, mid_region, width, height, bpp, uc_mocs, >> - mid_tiling, mid_compression, comp_type); >> - dst = blt_create_object(i915, region1, width, height, bpp, uc_mocs, >> - mid_tiling, COMPRESSION_DISABLED, comp_type); >> - final = blt_create_object(i915, region1, width, height, bpp, uc_mocs, >> - T_LINEAR, COMPRESSION_DISABLED, comp_type); >> + src = blt_create_block_object(i915, region1, width, height, bpp, uc_mocs, >> + T_LINEAR, COMPRESSION_DISABLED, comp_type); >> + mid = blt_create_block_object(i915, mid_region, width, height, bpp, >> + uc_mocs, mid_tiling, mid_compression, comp_type); >> + dst = blt_create_block_object(i915, region1, width, height, bpp, uc_mocs, >> + mid_tiling, COMPRESSION_DISABLED, comp_type); >> + final = blt_create_block_object(i915, region1, width, height, bpp, uc_mocs, >> + T_LINEAR, COMPRESSION_DISABLED, comp_type); >> igt_assert(src->size == dst->size); >> PRINT_SURFACE_INFO("src", src); >> PRINT_SURFACE_INFO("mid", mid); >> @@ -501,9 +502,9 @@ static void block_multicopy(int i915, >> blt_set_copy_object(&blt3.final, final); >> >> if (config->inplace) { >> - blt_set_object(&blt3.dst, mid->handle, dst->size, mid->region, >> - mid->mocs, mid_tiling, COMPRESSION_DISABLED, >> - comp_type); >> + blt_set_block_object(&blt3.dst, mid->handle, dst->size, >> + mid->region, mid->mocs, mid_tiling, >> + COMPRESSION_DISABLED, comp_type); >> blt3.dst.ptr = mid->ptr; >> } >> >> diff --git a/tests/i915/gem_exercise_blt.c b/tests/i915/gem_exercise_blt.c >> index b1123356..5b7178df 100644 >> --- a/tests/i915/gem_exercise_blt.c >> +++ b/tests/i915/gem_exercise_blt.c >> @@ -135,12 +135,9 @@ static void fast_copy_emit(int i915, const intel_ctx_t *ctx, >> >> igt_assert(__gem_create_in_memory_regions(i915, &bb, &bb_size, region1) == 0); >> >> - src = blt_create_object(i915, region1, width, height, bpp, 0, >> - T_LINEAR, COMPRESSION_DISABLED, 0); >> - mid = blt_create_object(i915, region2, width, height, bpp, 0, >> - mid_tiling, COMPRESSION_DISABLED, 0); >> - dst = blt_create_object(i915, region1, width, height, bpp, 0, >> - T_LINEAR, COMPRESSION_DISABLED, 0); >> + src = blt_create_fast_object(i915, region1, width, height, bpp, T_LINEAR); >> + mid = blt_create_fast_object(i915, region2, width, height, bpp, mid_tiling); >> + dst = blt_create_fast_object(i915, region1, width, height, bpp, T_LINEAR); >> igt_assert(src->size == dst->size); >> >> PRINT_SURFACE_INFO("src", src); >> @@ -195,12 +192,9 @@ static void fast_copy(int i915, const intel_ctx_t *ctx, >> >> igt_assert(__gem_create_in_memory_regions(i915, &bb, &bb_size, region1) == 0); >> >> - src = blt_create_object(i915, region1, width, height, bpp, 0, >> - T_LINEAR, COMPRESSION_DISABLED, 0); >> - mid = blt_create_object(i915, region2, width, height, bpp, 0, >> - mid_tiling, COMPRESSION_DISABLED, 0); >> - dst = blt_create_object(i915, region1, width, height, bpp, 0, >> - T_LINEAR, COMPRESSION_DISABLED, 0); >> + src = blt_create_fast_object(i915, region1, width, height, bpp, T_LINEAR); >> + mid = blt_create_fast_object(i915, region2, width, height, bpp, mid_tiling); >> + dst = blt_create_fast_object(i915, region1, width, height, bpp, T_LINEAR); >> igt_assert(src->size == dst->size); >> >> blt_surface_fill_rect(i915, src, width, height); >> diff --git a/tests/i915/gem_lmem_swapping.c b/tests/i915/gem_lmem_swapping.c >> index 55b044ec..3a6308ed 100644 >> --- a/tests/i915/gem_lmem_swapping.c >> +++ b/tests/i915/gem_lmem_swapping.c >> @@ -317,9 +317,9 @@ static void __do_evict(int i915, >> >> tmp->handle = gem_create_in_memory_regions(i915, params->size.max, >> INTEL_MEMORY_REGION_ID(I915_SYSTEM_MEMORY, 0)); >> - blt_set_object(tmp, tmp->handle, params->size.max, >> - INTEL_MEMORY_REGION_ID(I915_SYSTEM_MEMORY, 0), >> - intel_get_uc_mocs(i915), T_LINEAR, >> + blt_set_block_object(tmp, tmp->handle, params->size.max, >> + INTEL_MEMORY_REGION_ID(I915_SYSTEM_MEMORY, 0), >> + intel_get_uc_mocs(i915), T_LINEAR, >> COMPRESSION_DISABLED, COMPRESSION_TYPE_3D); >> blt_set_geom(tmp, stride, 0, 0, width, height, 0, 0); >> } >> @@ -348,9 +348,10 @@ static void __do_evict(int i915, >> >> obj->blt_obj = calloc(1, sizeof(*obj->blt_obj)); >> igt_assert(obj->blt_obj); >> - blt_set_object(obj->blt_obj, obj->handle, obj->size, region_id, >> - intel_get_uc_mocs(i915), T_LINEAR, >> - COMPRESSION_ENABLED, COMPRESSION_TYPE_3D); >> + blt_set_block_object(obj->blt_obj, obj->handle, obj->size, >> + region_id, intel_get_uc_mocs(i915), >> + T_LINEAR, COMPRESSION_ENABLED, >> + COMPRESSION_TYPE_3D); >> blt_set_geom(obj->blt_obj, stride, 0, 0, width, height, 0, 0); >> init_object_ccs(i915, obj, tmp, rand(), blt_ctx, >> region_id, ahnd); >> -- >> 2.25.1 >>