Re: [igt-dev] [PATCH i-g-t v3 1/6] i915/lib: Add new library for blitter and tiling formats

[Karolina Stolarek <karolina.stolarek@intel.com>]

Hi Kamil,

On 11.01.2023 17:42, Kamil Konieczny wrote:
> Hi Karolina,

<snip>

>> +#include "igt_core.h"
> 
> imho we should have smaller include here as we need only uint32_t,
> not all igt_core.

I've just realised that I want to use igt_core.h. I'm using quite useful 
wrappers like igt_require_f and igt_info, and I'd like to keep them.

Thanks,
Karolina
> 
>> +
>> +/**
>> + * SECTION:intel_blt_info
>> + * @short_description: blitter library to query for available commands and tiling formats
>> + * @title: Intel blitter info
>> + * @include: intel_blt_info.h
>> + *
>> + * # Introduction
>> + *
>> + * When we do a blitter copy, a number of different tiling formats can be used.
>> + * The list of available formats and commands varies between generations, in
>> + * some cases even within the generation (e.g. block copy tiling formats offered
>> + * by TGL vs DG2). Such information is required by different tests, so it's
>> + * beneficial to store it in one place. `intel_blt_info` is a blitter library
>> + * that describes available commands with a list of supported tiling formats.
>> + * They are encapsulated in static `blt_cmd_info` instances, each of them
>> + * defined per generation or platform.
>> + *
>> + * Tiling formats here are described by blt_tiling_type enum, which represents
>> + * shifts used to create bit flags of supported tiling formats:
>> + * `.supported_tiling = BLT_BIT(T_LINEAR) | BLT_BIT(T_XMAJOR) | BLT_BIT(T_YMAJOR)`
>> + *
>> + * # Usage
>> + *
>> + * - blt_supports_command(info, cmd) - checks if a blt_cmd_type instance has an
>> + *				       entry for the command
>> + * - blt_cmd_supports_tiling(info, cmd, tiling) - checks if a tiling format is
>> + *						  supported by the command. Can
>> + *						  also handle the case when the
>> + *						  command is not available on
>> + *						  the platform.
>> + *
>> + * These general checks can be wrapped in a command or tiling specific check,
>> + * provided by other libraries.
>> + *
>> + */
>> +
>> +enum blt_tiling_type {
>> +	T_LINEAR,
>> +	T_XMAJOR,
>> +	T_YMAJOR,
>> +	T_TILE4,
>> +	T_TILE64,
>> +	T_YFMAJOR,
>> +	__MAX_TILING
> --------- ^
> imho __BLT_MAX_TILING would be better.
> 
>> +};
>> +
>> +enum blt_cmd_type {
>> +	SRC_COPY,
>> +	XY_SRC_COPY,
>> +	XY_FAST_COPY,
>> +	XY_BLOCK_COPY,
>> +	__MAX_CMD
> --------- ^
> Here this is too general, __BLT_MAX_CMD would be better.
> 
> Regards,
> Kamil
> 
>> +};
>> +
>> +struct blt_tiling_info {
>> +	enum blt_cmd_type blt_cmd_type;
>> +	uint32_t supported_tiling;
>> +};
>> +
>> +struct blt_cmd_info {
>> +	struct blt_tiling_info const *supported_tiling[__MAX_CMD];
>> +};
>> +
>> +const struct blt_cmd_info pre_gen8_blt_info;
>> +const struct blt_cmd_info gen8_blt_info;
>> +const struct blt_cmd_info gen11_blt_info;
>> +const struct blt_cmd_info gen12_blt_info;
>> +const struct blt_cmd_info gen12_dg2_blt_info;
>> +const struct blt_cmd_info gen12_atsm_blt_info;
>> +
>> +#define for_each_tiling(__tiling) \
>> +	for (__tiling = T_LINEAR; __tiling < __MAX_TILING; __tiling++)
>> +
>> +bool blt_supports_command(const struct blt_cmd_info *info,
>> +			  enum blt_cmd_type cmd);
>> +bool blt_cmd_supports_tiling(const struct blt_cmd_info *info,
>> +			     enum blt_cmd_type cmd,
>> +			     enum blt_tiling_type tiling);
>> +
>> +void blt_dump_blt_cmd_info(struct blt_cmd_info const *info);
>> +const char *blt_tiling_name(enum blt_tiling_type tiling);
>> +
>> +#endif // BLT_TILING_H
>> diff --git a/lib/meson.build b/lib/meson.build
>> index cc784686..765f5687 100644
>> --- a/lib/meson.build
>> +++ b/lib/meson.build
>> @@ -11,6 +11,7 @@ lib_sources = [
>>   	'i915/gem_ring.c',
>>   	'i915/gem_mman.c',
>>   	'i915/gem_vm.c',
>> +	'i915/intel_blt_info.c',
>>   	'i915/intel_decode.c',
>>   	'i915/intel_memory_region.c',
>>   	'i915/intel_mocs.c',
>> diff --git a/tests/i915/gem_ccs.c b/tests/i915/gem_ccs.c
>> index 751f65e6..9e304774 100644
>> --- a/tests/i915/gem_ccs.c
>> +++ b/tests/i915/gem_ccs.c
>> @@ -46,7 +46,7 @@ struct test_config {
>>   
>>   static void set_object(struct blt_copy_object *obj,
>>   		       uint32_t handle, uint64_t size, uint32_t region,
>> -		       uint8_t mocs, enum blt_tiling tiling,
>> +		       uint8_t mocs, enum blt_tiling_type tiling,
>>   		       enum blt_compression compression,
>>   		       enum blt_compression_type compression_type)
>>   {
>> @@ -108,7 +108,7 @@ static void set_surf_object(struct blt_ctrl_surf_copy_object *obj,
>>   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 blt_tiling tiling,
>> +	      enum blt_tiling_type tiling,
>>   	      enum blt_compression compression,
>>   	      enum blt_compression_type compression_type,
>>   	      bool create_mapping)
>> @@ -374,7 +374,7 @@ static void block_copy(int i915,
>>   		       const intel_ctx_t *ctx,
>>   		       const struct intel_execution_engine2 *e,
>>   		       uint32_t region1, uint32_t region2,
>> -		       enum blt_tiling mid_tiling,
>> +		       enum blt_tiling_type mid_tiling,
>>   		       const struct test_config *config)
>>   {
>>   	struct blt_copy_data blt = {};
>> @@ -492,7 +492,7 @@ static void block_multicopy(int i915,
>>   			    const intel_ctx_t *ctx,
>>   			    const struct intel_execution_engine2 *e,
>>   			    uint32_t region1, uint32_t region2,
>> -			    enum blt_tiling mid_tiling,
>> +			    enum blt_tiling_type mid_tiling,
>>   			    const struct test_config *config)
>>   {
>>   	struct blt_copy3_data blt3 = {};
>> @@ -581,7 +581,7 @@ static const struct {
>>   	const char *suffix;
>>   	void (*copyfn)(int, const intel_ctx_t *,
>>   		       const struct intel_execution_engine2 *,
>> -		       uint32_t, uint32_t, enum blt_tiling,
>> +		       uint32_t, uint32_t, enum blt_tiling_type,
>>   		       const struct test_config *);
>>   } copyfns[] = {
>>   	[BLOCK_COPY] = { "", block_copy },
>> @@ -596,6 +596,7 @@ static void block_copy_test(int i915,
>>   {
>>   	struct igt_collection *regions;
>>   	const struct intel_execution_engine2 *e;
>> +	int tiling;
>>   
>>   	if (config->compression && !blt_supports_compression(i915))
>>   		return;
>> @@ -603,7 +604,7 @@ static void block_copy_test(int i915,
>>   	if (config->inplace && !config->compression)
>>   		return;
>>   
>> -	for (int tiling = T_LINEAR; tiling <= T_TILE64; tiling++) {
>> +	for_each_tiling(tiling) {
>>   		if (!blt_supports_tiling(i915, tiling) ||
>>   		    (param.tiling >= 0 && param.tiling != tiling))
>>   			continue;
>> diff --git a/tests/i915/gem_lmem_swapping.c b/tests/i915/gem_lmem_swapping.c
>> index 75121d41..9388d4de 100644
>> --- a/tests/i915/gem_lmem_swapping.c
>> +++ b/tests/i915/gem_lmem_swapping.c
>> @@ -78,7 +78,7 @@ struct object {
>>   
>>   static void set_object(struct blt_copy_object *obj,
>>   		       uint32_t handle, uint64_t size, uint32_t region,
>> -		       uint8_t mocs, enum blt_tiling tiling,
>> +		       uint8_t mocs, enum blt_tiling_type tiling,
>>   		       enum blt_compression compression,
>>   		       enum blt_compression_type compression_type)
>>   {
>> -- 
>> 2.25.1
>>