[igt-dev] [PATCH i-g-t 3/4] lib/i915_blt: Extract init functions for blt_copy_object
Karolina Stolarek
karolina.stolarek at intel.com
Thu Dec 22 09:50:45 UTC 2022
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.stolarek at intel.com>
>> ---
>> 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
>>
More information about the igt-dev
mailing list