HI,
Comments inline.
-----Original Message-----
From: Stolarek, Karolina <karolina.stolarek at intel.com>
Sent: Monday, November 13, 2023 12:42 AM
To: Jahagirdar, Akshata <akshata.jahagirdar at intel.com>
Cc: igt-dev at lists.freedesktop.org; Siddiqui, Ayaz A <ayaz.siddiqui at intel.com>; Kempczynski, Zbigniew <zbigniew.kempczynski at intel.com>; Auld, Matthew <matthew.auld at intel.com>
Subject: Re: [PATCH i-g-t 1/5] lib: add blt command properties for lunarlake
On 10.11.2023 22:24, Akshata Jahagirdar wrote:
> Add blt_cmd_info structs to describe properties of XY_BLOCK_COPY and XY_FAST_COPY blitter commands for XE2 platform. Updated the definitions for Lunarlake.
Nit: break the commit message at 80 column
AJ: Will fix this.
>
> Signed-off-by: Akshata Jahagirdar <akshata.jahagirdar at intel.com>
> ---
> lib/intel_cmds_info.c | 24 ++++++++++++++++++++++++
> lib/intel_cmds_info.h | 1 +
> lib/intel_device_info.c | 2 +-
> 3 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/lib/intel_cmds_info.c b/lib/intel_cmds_info.c index
> 2e51ec081..fc3554271 100644
> --- a/lib/intel_cmds_info.c
> +++ b/lib/intel_cmds_info.c
> @@ -67,6 +67,22 @@ static const struct blt_cmd_info
> BLT_CMD_EXTENDED |
> BLT_CMD_SUPPORTS_COMPRESSION);
>
> +static const struct blt_cmd_info
> + xe2_xy_block_copy = BLT_INFO_EXT(XY_BLOCK_COPY,
> + BIT(T_LINEAR) |
> + BIT(T_XMAJOR) |
> + BIT(T_TILE4) |
> + BIT(T_TILE64),
> + BLT_CMD_EXTENDED |
> + BLT_CMD_SUPPORTS_COMPRESSION);
I know this might be too much work, but could we confirm that XMAJOR is actually supported with block copy on this platform? This could be as easy as running xe_ccs test with -p parameter, and then checking the mid image and the tiling pattern. But I won't argue it's essential for r-b, it's more of a sanity check.
AJ: I have verified this through bspec, as well as tests are passing on the platform.
> +
> +static const struct blt_cmd_info
> + xe2_xy_fast_copy = BLT_INFO(XY_FAST_COPY,
> + BIT(T_LINEAR) |
> + BIT(T_XMAJOR) |
> + BIT(T_TILE4) |
> + BIT(T_TILE64));
This looks a lot like dg2_xy_fast_copy definition. Why not reuse it when defining xe2_cmds_info?
AJ: Yes, I thought of keeping it separate to address any other differences.
With these comments addressed:
Reviewed-by: Karolina Stolarek <karolina.stolarek at intel.com>
> +
> static const struct blt_cmd_info
> mtl_xy_block_copy = BLT_INFO_EXT(XY_BLOCK_COPY,
> BIT(T_LINEAR) |
> @@ -169,6 +185,14 @@ const struct intel_cmds_info gen12_pvc_cmds_info = {
> }
> };
>
> +
> +const struct intel_cmds_info xe2_cmds_info = {
> + .blt_cmds = {
> + [XY_FAST_COPY] = &xe2_xy_fast_copy,
> + [XY_BLOCK_COPY] = &xe2_xy_block_copy,
> + }
> +};
> +
> const struct blt_cmd_info *blt_get_cmd_info(const struct intel_cmds_info *cmds_info,
> enum blt_cmd_type cmd)
> {
> diff --git a/lib/intel_cmds_info.h b/lib/intel_cmds_info.h index
> f9e3932d1..0a83b6a44 100644
> --- a/lib/intel_cmds_info.h
> +++ b/lib/intel_cmds_info.h
> @@ -55,6 +55,7 @@ extern const struct intel_cmds_info gen12_cmds_info;
> extern const struct intel_cmds_info gen12_dg2_cmds_info;
> extern const struct intel_cmds_info gen12_mtl_cmds_info;
> extern const struct intel_cmds_info gen12_pvc_cmds_info;
> +extern const struct intel_cmds_info xe2_cmds_info;
>
> #define for_each_tiling(__tiling) \
> for (__tiling = T_LINEAR; __tiling < __BLT_MAX_TILING; __tiling++)
> diff --git a/lib/intel_device_info.c b/lib/intel_device_info.c index
> 34817f7b6..a669797c3 100644
> --- a/lib/intel_device_info.c
> +++ b/lib/intel_device_info.c
> @@ -511,7 +511,7 @@ static const struct intel_device_info intel_lunarlake_info = {
> .has_4tile = true,
> .is_lunarlake = true,
> .codename = "lunarlake",
> - .cmds_info = &gen12_pvc_cmds_info,
> + .cmds_info = &xe2_cmds_info,
> };
>
> static const struct pci_id_match intel_device_match[] = {