From: Karolina Stolarek <karolina.stolarek@intel.com>
To: "Zbigniew Kempczyński" <zbigniew.kempczynski@intel.com>
Cc: igt-dev@lists.freedesktop.org
Subject: Re: [igt-dev] [PATCH i-g-t 2/2] lib/i915_blt: Split up blt_copy_object functions
Date: Mon, 30 Jan 2023 09:21:09 +0100 [thread overview]
Message-ID: <2ab35ef4-c027-99c5-721f-95b845cd0620@intel.com> (raw)
In-Reply-To: <20230126121350.kdyvqhcr477izj5e@zkempczy-mobl2>
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 <karolina.stolarek@intel.com>
>> ---
>> 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
>>
next prev parent reply other threads:[~2023-01-30 8:21 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-23 10:52 [igt-dev] [PATCH i-g-t 0/2] lib/i915_blt: Minor blt_create_object refactoring Karolina Stolarek
2023-01-23 10:52 ` [igt-dev] [PATCH i-g-t 1/2] lib/i915_blt: Remove create_mapping flag from blt_create_object Karolina Stolarek
2023-01-26 12:00 ` Zbigniew Kempczyński
2023-01-30 8:10 ` Karolina Stolarek
2023-01-30 19:30 ` Zbigniew Kempczyński
2023-01-23 10:52 ` [igt-dev] [PATCH i-g-t 2/2] lib/i915_blt: Split up blt_copy_object functions Karolina Stolarek
2023-01-26 12:13 ` Zbigniew Kempczyński
2023-01-30 8:21 ` Karolina Stolarek [this message]
2023-01-30 19:26 ` Zbigniew Kempczyński
2023-01-23 12:52 ` [igt-dev] ✓ Fi.CI.BAT: success for lib/i915_blt: Minor blt_create_object refactoring Patchwork
2023-01-24 1:55 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
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=2ab35ef4-c027-99c5-721f-95b845cd0620@intel.com \
--to=karolina.stolarek@intel.com \
--cc=igt-dev@lists.freedesktop.org \
--cc=zbigniew.kempczynski@intel.com \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.