[igt-dev] [PATCH i-g-t v2 2/6] lib: Update platform definitions with blitter information

[Karolina Stolarek]

Hi Kamil,

Thanks for your review.

On 28.12.2022 17:41, Kamil Konieczny wrote:
> Hi Karolina,
> 
> On 2022-12-23 at 12:13:47 +0100, Karolina Stolarek wrote:
>> Update entries in intel_device_info to store information on
>> supported blitter commands and tiling formats. Add predicates
>> that check if block or fast copy are supported. Update block
>> copy tests to use the new checks.
>>
>> Signed-off-by: Karolina Stolarek <karolina.stolarek at intel.com>
> 
> Please put here list of Cc:
> 
> Cc: Zbigniew Kempczynski <zbigniew.kempczynski at intel.com>
> Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>

Will do in the next version

>> ---
>>   lib/i915/i915_blt.c     | 75 +++++++++++++++++++++++++++++++----------
>>   lib/i915/i915_blt.h     |  6 +++-
>>   lib/intel_chipset.c     |  1 -
>>   lib/intel_chipset.h     |  4 +++
>>   lib/intel_device_info.c | 47 ++++++++++++++++++++++++++
>>   tests/i915/gem_ccs.c    |  4 +--
>>   6 files changed, 115 insertions(+), 22 deletions(-)
>>
>> diff --git a/lib/i915/i915_blt.c b/lib/i915/i915_blt.c
>> index 6db135d1..00cf7470 100644
>> --- a/lib/i915/i915_blt.c
>> +++ b/lib/i915/i915_blt.c
>> @@ -208,34 +208,73 @@ bool blt_supports_compression(int i915)
>>   }
>>   
>>   /**
>> - * blt_supports_tiling:
>> + * blt_has_block_copy
>>    * @i915: drm fd
>> - * @tiling: tiling id
>>    *
>> - * Function checks if blitter supports @tiling on @i915 device.
>> + * Check if block copy is supported by @i915 device
>>    *
>>    * Returns:
>>    * true if it does, false otherwise.
>>    */
>> -bool blt_supports_tiling(int i915, enum blt_tiling_type tiling)
>> +bool blt_has_block_copy(int i915)
>>   {
>> -	uint32_t devid = intel_get_drm_devid(i915);
>> +	const struct blt_cmd_info
>> +			*blt_info = intel_get_blt_info(intel_get_drm_devid(i915));
>>   
>> -	if (tiling == T_XMAJOR) {
>> -		if (IS_TIGERLAKE(devid) || IS_DG1(devid))
>> -			return false;
>> -		else
>> -			return true;
>> -	}
>> +	return blt_supports_command(blt_info, XY_BLOCK_COPY);
>> +}
>>   
>> -	if (tiling == T_YMAJOR) {
>> -		if (IS_TIGERLAKE(devid) || IS_DG1(devid))
>> -			return true;
>> -		else
>> -			return false;
>> -	}
>> +/**
>> + * blt_has_fast_copy
>> + * @i915: drm fd
>> + *
>> + * Check if fast copy is supported by @i915 device
>> + *
>> + * Returns:
>> + * true if it does, false otherwise.
>> + */
>> +bool blt_has_fast_copy(int i915)
>> +{
>> +	const struct blt_cmd_info
>> +			*blt_info = intel_get_blt_info(intel_get_drm_devid(i915));
>> +
>> +	return blt_supports_command(blt_info, XY_FAST_COPY);
>> +}
>> +
>> +/**
>> + * blt_fast_copy_supports_tiling
>> + * @i915: drm fd
>> + * @tiling: tiling format
>> + *
>> + * Check if fast copy provided by @i915 device supports @tiling format
>> + *
>> + * Returns:
>> + * true if it does, false otherwise.
>> + */
>> +bool blt_fast_copy_supports_tiling(int i915, enum blt_tiling_type tiling)
>> +{
>> +	const struct blt_cmd_info
>> +			*blt_info = intel_get_blt_info(intel_get_drm_devid(i915));
>> +
>> +	return blt_cmd_supports_tiling(blt_info, XY_FAST_COPY, tiling);
>> +}
>> +
>> +/**
>> + * blt_block_copy_supports_tiling
>> + * @i915: drm fd
>> + * @tiling: tiling format
>> + *
>> + * Check if block copy provided by @i915 device supports @tiling format
>> + *
>> + * Returns:
>> + * true if it does, false otherwise.
>> + */
>> +bool blt_block_copy_supports_tiling(int i915, enum blt_tiling_type tiling)
>> +{
>> +	const struct blt_cmd_info
>> +			*blt_info = intel_get_blt_info(intel_get_drm_devid(i915));
>>   
>> -	return true;
>> +	return blt_cmd_supports_tiling(blt_info, XY_BLOCK_COPY, tiling);
>>   }
>>   
>>   static int __block_tiling(enum blt_tiling_type tiling)
>> diff --git a/lib/i915/i915_blt.h b/lib/i915/i915_blt.h
>> index 8fa480b8..3730c7c0 100644
>> --- a/lib/i915/i915_blt.h
>> +++ b/lib/i915/i915_blt.h
>> @@ -158,7 +158,11 @@ struct blt_ctrl_surf_copy_data {
>>   };
>>   
>>   bool blt_supports_compression(int i915);
>> -bool blt_supports_tiling(int i915, enum blt_tiling_type tiling);
>> +
>> +bool blt_has_block_copy(int i915);
>> +bool blt_has_fast_copy(int i915);
>> +bool blt_fast_copy_supports_tiling(int i915, enum blt_tiling_type tiling);
>> +bool blt_block_copy_supports_tiling(int i915, enum blt_tiling_type tiling);
>>   
>>   uint64_t emit_blt_block_copy(int i915,
>>   			     uint64_t ahnd,
>> diff --git a/lib/intel_chipset.c b/lib/intel_chipset.c
>> index efb6f177..4ac067df 100644
>> --- a/lib/intel_chipset.c
>> +++ b/lib/intel_chipset.c
>> @@ -41,7 +41,6 @@
>>   #include "drmtest.h"
>>   #include "intel_chipset.h"
>>   #include "igt_core.h"
>> -
> - ^
> 
> Please do not make unrelated changes in lib (here line was deleted).

Whoops, didn't mean to add this in, sorry!

>>   /**
>>    * SECTION:intel_chipset
>>    * @short_description: Feature macros and chipset helpers
>> diff --git a/lib/intel_chipset.h b/lib/intel_chipset.h
>> index 9b39472a..a9801b28 100644
>> --- a/lib/intel_chipset.h
>> +++ b/lib/intel_chipset.h
>> @@ -31,6 +31,8 @@
>>   #include <pciaccess.h>
>>   #include <stdbool.h>
>>   
>> +#include "i915/intel_blt_info.h"
>> +
> 
> Imho this is a little too big change.

The only way to make it smaller is to add the field in one commit and 
update intel_device_info definitions later. The latter change is still 
being quite big, so I'm not sure if it's worth it.

> 
>>   #define BIT(x) (1ul <<(x))
>>   
>>   struct pci_device *intel_get_pci_device(void);
>> @@ -86,11 +88,13 @@ struct intel_device_info {
>>   	bool is_alderlake_p : 1;
>>   	bool is_alderlake_n : 1;
>>   	bool is_meteorlake : 1;
>> +	const struct blt_cmd_info *blt_tiling;
> 
> I would expect here to have bits for supported copy commands
> like has_xy_block_copy, has_xy_src_block_copy, has_xy_fast_copy
> and other. 

We could have many, many more of these commands, like 10 or more. I'd 
keep it hidden behind in a lib. Checks if a specific command is 
supported are added in the next patch of the series.

> At the end of lib there are macros like HAS_FLATCCS(devid)
> so there could be added new ones like HAS_XY_FAST_COPY(devid) >
> You added comment at your first patch 1/6 that YFmajor tiling
> is supported only for gen9 and only for specific blt command (fast copy),
> so imho it adds too many complicated info here. I would prefer to
> keep that in blt library, not here.

I'm sorry, but I don't follow -- how does it add information about it 
here? Do you mean importing enums such as that for tiling? If so, I 
agree, it's not ideal. But even now we're polluting this lib with tiling 
info (see has_4tile field) where this is more explicit than what I propose.

All the best,
Karolina

> 
> Regards,
> Kamil
> 
>>   	const char *codename;
>>   };
>>   
>>   const struct intel_device_info *intel_get_device_info(uint16_t devid) __attribute__((pure));
>>   
>> +const struct blt_cmd_info *intel_get_blt_info(uint16_t devid) __attribute__((pure));
>>   unsigned intel_gen(uint16_t devid) __attribute__((pure));
>>   unsigned intel_graphics_ver(uint16_t devid) __attribute__((pure));
>>   unsigned intel_display_ver(uint16_t devid) __attribute__((pure));
>> diff --git a/lib/intel_device_info.c b/lib/intel_device_info.c
>> index 68dd17ee..ad9dffd8 100644
>> --- a/lib/intel_device_info.c
>> +++ b/lib/intel_device_info.c
>> @@ -145,6 +145,7 @@ static const struct intel_device_info intel_sandybridge_info = {
>>   	.graphics_ver = 6,
>>   	.display_ver = 6,
>>   	.is_sandybridge = true,
>> +	.blt_tiling = &pre_gen8_blt_info,
>>   	.codename = "sandybridge"
>>   };
>>   static const struct intel_device_info intel_sandybridge_m_info = {
>> @@ -152,6 +153,7 @@ static const struct intel_device_info intel_sandybridge_m_info = {
>>   	.display_ver = 6,
>>   	.is_mobile = true,
>>   	.is_sandybridge = true,
>> +	.blt_tiling = &pre_gen8_blt_info,
>>   	.codename = "sandybridge"
>>   };
>>   
>> @@ -159,6 +161,7 @@ static const struct intel_device_info intel_ivybridge_info = {
>>   	.graphics_ver = 7,
>>   	.display_ver = 7,
>>   	.is_ivybridge = true,
>> +	.blt_tiling = &pre_gen8_blt_info,
>>   	.codename = "ivybridge"
>>   };
>>   static const struct intel_device_info intel_ivybridge_m_info = {
>> @@ -166,6 +169,7 @@ static const struct intel_device_info intel_ivybridge_m_info = {
>>   	.display_ver = 7,
>>   	.is_mobile = true,
>>   	.is_ivybridge = true,
>> +	.blt_tiling = &pre_gen8_blt_info,
>>   	.codename = "ivybridge"
>>   };
>>   
>> @@ -173,6 +177,7 @@ static const struct intel_device_info intel_valleyview_info = {
>>   	.graphics_ver = 7,
>>   	.display_ver = 7,
>>   	.is_valleyview = true,
>> +	.blt_tiling = &pre_gen8_blt_info,
>>   	.codename = "valleyview"
>>   };
>>   
>> @@ -180,6 +185,7 @@ static const struct intel_device_info intel_valleyview_info = {
>>   	.graphics_ver = 7, \
>>   	.display_ver = 7, \
>>   	.is_haswell = true, \
>> +	.blt_tiling = &pre_gen8_blt_info, \
>>   	.codename = "haswell"
>>   
>>   static const struct intel_device_info intel_haswell_gt1_info = {
>> @@ -201,6 +207,7 @@ static const struct intel_device_info intel_haswell_gt3_info = {
>>   	.graphics_ver = 8, \
>>   	.display_ver = 8, \
>>   	.is_broadwell = true, \
>> +	.blt_tiling = &gen8_blt_info, \
>>   	.codename = "broadwell"
>>   
>>   static const struct intel_device_info intel_broadwell_gt1_info = {
>> @@ -226,12 +233,14 @@ static const struct intel_device_info intel_cherryview_info = {
>>   	.graphics_ver = 8,
>>   	.display_ver = 8,
>>   	.is_cherryview = true,
>> +	.blt_tiling = &gen8_blt_info,
>>   	.codename = "cherryview"
>>   };
>>   
>>   #define SKYLAKE_FIELDS \
>>   	.graphics_ver = 9, \
>>   	.display_ver = 9, \
>> +	.blt_tiling = &gen11_blt_info, \
>>   	.codename = "skylake", \
>>   	.is_skylake = true
>>   
>> @@ -259,6 +268,7 @@ static const struct intel_device_info intel_broxton_info = {
>>   	.graphics_ver = 9,
>>   	.display_ver = 9,
>>   	.is_broxton = true,
>> +	.blt_tiling = &gen11_blt_info,
>>   	.codename = "broxton"
>>   };
>>   
>> @@ -266,6 +276,7 @@ static const struct intel_device_info intel_broxton_info = {
>>   	.graphics_ver = 9, \
>>   	.display_ver = 9, \
>>   	.is_kabylake = true, \
>> +	.blt_tiling = &gen11_blt_info, \
>>   	.codename = "kabylake"
>>   
>>   static const struct intel_device_info intel_kabylake_gt1_info = {
>> @@ -292,6 +303,7 @@ static const struct intel_device_info intel_geminilake_info = {
>>   	.graphics_ver = 9,
>>   	.display_ver = 9,
>>   	.is_geminilake = true,
>> +	.blt_tiling = &gen11_blt_info,
>>   	.codename = "geminilake"
>>   };
>>   
>> @@ -299,6 +311,7 @@ static const struct intel_device_info intel_geminilake_info = {
>>   	.graphics_ver = 9, \
>>   	.display_ver = 9, \
>>   	.is_coffeelake = true, \
>> +	.blt_tiling = &gen11_blt_info, \
>>   	.codename = "coffeelake"
>>   
>>   static const struct intel_device_info intel_coffeelake_gt1_info = {
>> @@ -320,6 +333,7 @@ static const struct intel_device_info intel_coffeelake_gt3_info = {
>>   	.graphics_ver = 9, \
>>   	.display_ver = 9, \
>>   	.is_cometlake = true, \
>> +	.blt_tiling = &gen11_blt_info, \
>>   	.codename = "cometlake"
>>   
>>   static const struct intel_device_info intel_cometlake_gt1_info = {
>> @@ -336,6 +350,7 @@ static const struct intel_device_info intel_cannonlake_info = {
>>   	.graphics_ver = 10,
>>   	.display_ver = 10,
>>   	.is_cannonlake = true,
>> +	.blt_tiling = &gen11_blt_info,
>>   	.codename = "cannonlake"
>>   };
>>   
>> @@ -343,6 +358,7 @@ static const struct intel_device_info intel_icelake_info = {
>>   	.graphics_ver = 11,
>>   	.display_ver = 11,
>>   	.is_icelake = true,
>> +	.blt_tiling = &gen11_blt_info,
>>   	.codename = "icelake"
>>   };
>>   
>> @@ -350,6 +366,7 @@ static const struct intel_device_info intel_elkhartlake_info = {
>>   	.graphics_ver = 11,
>>   	.display_ver = 11,
>>   	.is_elkhartlake = true,
>> +	.blt_tiling = &gen11_blt_info,
>>   	.codename = "elkhartlake"
>>   };
>>   
>> @@ -357,6 +374,7 @@ static const struct intel_device_info intel_jasperlake_info = {
>>   	.graphics_ver = 11,
>>   	.display_ver = 11,
>>   	.is_jasperlake = true,
>> +	.blt_tiling = &gen11_blt_info,
>>   	.codename = "jasperlake"
>>   };
>>   
>> @@ -364,6 +382,7 @@ static const struct intel_device_info intel_tigerlake_gt1_info = {
>>   	.graphics_ver = 12,
>>   	.display_ver = 12,
>>   	.is_tigerlake = true,
>> +	.blt_tiling = &gen12_blt_info,
>>   	.codename = "tigerlake",
>>   	.gt = 1,
>>   };
>> @@ -372,6 +391,7 @@ static const struct intel_device_info intel_tigerlake_gt2_info = {
>>   	.graphics_ver = 12,
>>   	.display_ver = 12,
>>   	.is_tigerlake = true,
>> +	.blt_tiling = &gen12_blt_info,
>>   	.codename = "tigerlake",
>>   	.gt = 2,
>>   };
>> @@ -380,6 +400,7 @@ static const struct intel_device_info intel_rocketlake_info = {
>>   	.graphics_ver = 12,
>>   	.display_ver = 12,
>>   	.is_rocketlake = true,
>> +	.blt_tiling = &gen12_blt_info,
>>   	.codename = "rocketlake"
>>   };
>>   
>> @@ -388,6 +409,7 @@ static const struct intel_device_info intel_dg1_info = {
>>   	.graphics_rel = 10,
>>   	.display_ver = 12,
>>   	.is_dg1 = true,
>> +	.blt_tiling = &gen12_blt_info,
>>   	.codename = "dg1"
>>   };
>>   
>> @@ -398,6 +420,7 @@ static const struct intel_device_info intel_dg2_info = {
>>   	.has_4tile = true,
>>   	.is_dg2 = true,
>>   	.codename = "dg2",
>> +	.blt_tiling = &gen12_dg2_blt_info,
>>   	.has_flatccs = true,
>>   };
>>   
>> @@ -405,6 +428,7 @@ static const struct intel_device_info intel_alderlake_s_info = {
>>   	.graphics_ver = 12,
>>   	.display_ver = 12,
>>   	.is_alderlake_s = true,
>> +	.blt_tiling = &gen12_blt_info,
>>   	.codename = "alderlake_s"
>>   };
>>   
>> @@ -412,6 +436,7 @@ static const struct intel_device_info intel_raptorlake_s_info = {
>>   	.graphics_ver = 12,
>>   	.display_ver = 12,
>>   	.is_raptorlake_s = true,
>> +	.blt_tiling = &gen12_blt_info,
>>   	.codename = "raptorlake_s"
>>   };
>>   
>> @@ -419,6 +444,7 @@ static const struct intel_device_info intel_alderlake_p_info = {
>>   	.graphics_ver = 12,
>>   	.display_ver = 13,
>>   	.is_alderlake_p = true,
>> +	.blt_tiling = &gen12_blt_info,
>>   	.codename = "alderlake_p"
>>   };
>>   
>> @@ -426,6 +452,7 @@ static const struct intel_device_info intel_alderlake_n_info = {
>>   	.graphics_ver = 12,
>>   	.display_ver = 13,
>>   	.is_alderlake_n = true,
>> +	.blt_tiling = &gen12_blt_info,
>>   	.codename = "alderlake_n"
>>   };
>>   
>> @@ -436,6 +463,7 @@ static const struct intel_device_info intel_ats_m_info = {
>>   	.is_dg2 = true,
>>   	.has_4tile = true,
>>   	.codename = "ats_m",
>> +	.blt_tiling = &gen12_atsm_blt_info,
>>   	.has_flatccs = true,
>>   };
>>   
>> @@ -583,6 +611,25 @@ out:
>>   	return cache;
>>   }
>>   
>> +/**
>> + * intel_get_blt_info:
>> + * @devid: pci device id
>> + *
>> + * Looks up the Blitter information about commands and tiling formats supported
>> + * by the device.
>> + *
>> + * Returns:
>> + * The associated blt_cmd_info, NULL if no such information is found
>> + */
>> +const struct blt_cmd_info *intel_get_blt_info(uint16_t devid)
>> +{
>> +	const struct intel_device_info *dev_info;
>> +
>> +	dev_info = intel_get_device_info(devid);
>> +
>> +	return dev_info->blt_tiling;
>> +}
>> +
>>   /**
>>    * intel_gen:
>>    * @devid: pci device id
>> diff --git a/tests/i915/gem_ccs.c b/tests/i915/gem_ccs.c
>> index ed60b81c..10327050 100644
>> --- a/tests/i915/gem_ccs.c
>> +++ b/tests/i915/gem_ccs.c
>> @@ -605,7 +605,7 @@ static void block_copy_test(int i915,
>>   		return;
>>   
>>   	for_each_tiling(tiling) {
>> -		if (!blt_supports_tiling(i915, tiling) ||
>> +		if (!blt_block_copy_supports_tiling(i915, tiling) ||
>>   		    (param.tiling >= 0 && param.tiling != tiling))
>>   			continue;
>>   
>> @@ -703,7 +703,7 @@ igt_main_args("bf:pst:W:H:", NULL, help_str, opt_handler, NULL)
>>   	igt_fixture {
>>   		i915 = drm_open_driver(DRIVER_INTEL);
>>   		igt_require_gem(i915);
>> -		igt_require(AT_LEAST_GEN(intel_get_drm_devid(i915), 12) > 0);
>> +		igt_require(blt_has_block_copy(i915));
>>   
>>   		query_info = gem_get_query_memory_regions(i915);
>>   		igt_require(query_info);
>> -- 
>> 2.25.1
>>