From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by gabe.freedesktop.org (Postfix) with ESMTPS id 50CF710E3B1 for ; Tue, 3 Jan 2023 09:18:01 +0000 (UTC) Message-ID: <03805746-6a86-f0aa-7282-cca47fdabfdf@intel.com> Date: Tue, 3 Jan 2023 10:17:39 +0100 Content-Language: en-US To: Kamil Konieczny References: From: Karolina Stolarek In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Subject: Re: [igt-dev] [PATCH i-g-t v2 2/6] lib: Update platform definitions with blitter information 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 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 > > Please put here list of Cc: > > Cc: Zbigniew Kempczynski > Cc: Kamil Konieczny 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 >> #include >> >> +#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 >>