[PATCH] drm/amd/display: ignore modifiers when checking for format support

[PATCH] drm/amd/display: ignore modifiers when checking for format support

[Aurabindo Pillai]

[Why&How]
There are cases where swizzle modes are set but modifiers arent. For
such a userspace, we need not check modifiers while checking
compatibilty in the drm hook for checking plane format.

Ignore checking modifiers but check the DCN generation for the
supported swizzle mode.

Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 51 +++++++++++++++++--
 1 file changed, 46 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 2023baf41b7e..1322df491736 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4938,6 +4938,7 @@ static bool dm_plane_format_mod_supported(struct drm_plane *plane,
 {
 	struct amdgpu_device *adev = drm_to_adev(plane->dev);
 	const struct drm_format_info *info = drm_format_info(format);
+	struct hw_asic_id asic_id = adev->dm.dc->ctx->asic_id;
 	int i;
 
 	enum dm_micro_swizzle microtile = modifier_gfx9_swizzle_mode(modifier) & 3;
@@ -4955,13 +4956,53 @@ static bool dm_plane_format_mod_supported(struct drm_plane *plane,
 		return true;
 	}
 
-	/* Check that the modifier is on the list of the plane's supported modifiers. */
-	for (i = 0; i < plane->modifier_count; i++) {
-		if (modifier == plane->modifiers[i])
+	/* check if swizzle mode is supported by this version of DCN */
+	switch (asic_id.chip_family) {
+		case FAMILY_SI:
+		case FAMILY_CI:
+		case FAMILY_KV:
+		case FAMILY_CZ:
+		case FAMILY_VI:
+			/* AI and earlier asics does not have modifier support */
+			return false;
+			break;
+		case FAMILY_AI:
+		case FAMILY_RV:
+		case FAMILY_NV:
+		case FAMILY_VGH:
+		case FAMILY_YELLOW_CARP:
+		case AMDGPU_FAMILY_GC_10_3_6:
+		case AMDGPU_FAMILY_GC_10_3_7:
+			switch (AMD_FMT_MOD_GET(TILE, modifier)) {
+				case AMD_FMT_MOD_TILE_GFX9_64K_R_X:
+				case AMD_FMT_MOD_TILE_GFX9_64K_D_X:
+				case AMD_FMT_MOD_TILE_GFX9_64K_S_X:
+				case AMD_FMT_MOD_TILE_GFX9_64K_D:
+					return true;
+					break;
+				default:
+					return false;
+					break;
+			}
+			break;
+		case AMDGPU_FAMILY_GC_11_0_0:
+			switch (AMD_FMT_MOD_GET(TILE, modifier)) {
+				case AMD_FMT_MOD_TILE_GFX11_256K_R_X:
+				case AMD_FMT_MOD_TILE_GFX9_64K_R_X:
+				case AMD_FMT_MOD_TILE_GFX9_64K_D_X:
+				case AMD_FMT_MOD_TILE_GFX9_64K_S_X:
+				case AMD_FMT_MOD_TILE_GFX9_64K_D:
+					return true;
+					break;
+				default:
+					return false;
+					break;
+			}
+			break;
+		default:
+			ASSERT(0); /* Unknown asic */
 			break;
 	}
-	if (i == plane->modifier_count)
-		return false;
 
 	/*
 	 * For D swizzle the canonical modifier depends on the bpp, so check
-- 
2.36.1

Re: [PATCH] drm/amd/display: ignore modifiers when checking for format support

[Marek Olšák]

[-- Attachment #1: Type: text/plain, Size: 4240 bytes --]

Reviewed-by: Marek Olšák <marek.olsak@amd.com>

Marek

On Thu, Jun 9, 2022 at 10:27 AM Aurabindo Pillai <aurabindo.pillai@amd.com>
wrote:

> [Why&How]
> There are cases where swizzle modes are set but modifiers arent. For
> such a userspace, we need not check modifiers while checking
> compatibilty in the drm hook for checking plane format.
>
> Ignore checking modifiers but check the DCN generation for the
> supported swizzle mode.
>
> Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 51 +++++++++++++++++--
>  1 file changed, 46 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 2023baf41b7e..1322df491736 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4938,6 +4938,7 @@ static bool dm_plane_format_mod_supported(struct
> drm_plane *plane,
>  {
>         struct amdgpu_device *adev = drm_to_adev(plane->dev);
>         const struct drm_format_info *info = drm_format_info(format);
> +       struct hw_asic_id asic_id = adev->dm.dc->ctx->asic_id;
>         int i;
>
>         enum dm_micro_swizzle microtile =
> modifier_gfx9_swizzle_mode(modifier) & 3;
> @@ -4955,13 +4956,53 @@ static bool dm_plane_format_mod_supported(struct
> drm_plane *plane,
>                 return true;
>         }
>
> -       /* Check that the modifier is on the list of the plane's supported
> modifiers. */
> -       for (i = 0; i < plane->modifier_count; i++) {
> -               if (modifier == plane->modifiers[i])
> +       /* check if swizzle mode is supported by this version of DCN */
> +       switch (asic_id.chip_family) {
> +               case FAMILY_SI:
> +               case FAMILY_CI:
> +               case FAMILY_KV:
> +               case FAMILY_CZ:
> +               case FAMILY_VI:
> +                       /* AI and earlier asics does not have modifier
> support */
> +                       return false;
> +                       break;
> +               case FAMILY_AI:
> +               case FAMILY_RV:
> +               case FAMILY_NV:
> +               case FAMILY_VGH:
> +               case FAMILY_YELLOW_CARP:
> +               case AMDGPU_FAMILY_GC_10_3_6:
> +               case AMDGPU_FAMILY_GC_10_3_7:
> +                       switch (AMD_FMT_MOD_GET(TILE, modifier)) {
> +                               case AMD_FMT_MOD_TILE_GFX9_64K_R_X:
> +                               case AMD_FMT_MOD_TILE_GFX9_64K_D_X:
> +                               case AMD_FMT_MOD_TILE_GFX9_64K_S_X:
> +                               case AMD_FMT_MOD_TILE_GFX9_64K_D:
> +                                       return true;
> +                                       break;
> +                               default:
> +                                       return false;
> +                                       break;
> +                       }
> +                       break;
> +               case AMDGPU_FAMILY_GC_11_0_0:
> +                       switch (AMD_FMT_MOD_GET(TILE, modifier)) {
> +                               case AMD_FMT_MOD_TILE_GFX11_256K_R_X:
> +                               case AMD_FMT_MOD_TILE_GFX9_64K_R_X:
> +                               case AMD_FMT_MOD_TILE_GFX9_64K_D_X:
> +                               case AMD_FMT_MOD_TILE_GFX9_64K_S_X:
> +                               case AMD_FMT_MOD_TILE_GFX9_64K_D:
> +                                       return true;
> +                                       break;
> +                               default:
> +                                       return false;
> +                                       break;
> +                       }
> +                       break;
> +               default:
> +                       ASSERT(0); /* Unknown asic */
>                         break;
>         }
> -       if (i == plane->modifier_count)
> -               return false;
>
>         /*
>          * For D swizzle the canonical modifier depends on the bpp, so
> check
> --
> 2.36.1
>
>

[-- Attachment #2: Type: text/html, Size: 5529 bytes --]

RE: [PATCH] drm/amd/display: ignore modifiers when checking for format support

[Chen, Guchun]

+					return true;
+					break;

Possibly a coding style problem, 'break' after 'return' looks redundant.

Regards,
Guchun

-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Aurabindo Pillai
Sent: Thursday, June 9, 2022 10:27 PM
To: Olsak, Marek <Marek.Olsak@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Li, Sun peng (Leo) <Sunpeng.Li@amd.com>; Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>; Li, Roman <Roman.Li@amd.com>; Qiao, Ken <Ken.Qiao@amd.com>; Pillai, Aurabindo <Aurabindo.Pillai@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>
Subject: [PATCH] drm/amd/display: ignore modifiers when checking for format support

[Why&How]
There are cases where swizzle modes are set but modifiers arent. For such a userspace, we need not check modifiers while checking compatibilty in the drm hook for checking plane format.

Ignore checking modifiers but check the DCN generation for the supported swizzle mode.

Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 51 +++++++++++++++++--
 1 file changed, 46 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 2023baf41b7e..1322df491736 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4938,6 +4938,7 @@ static bool dm_plane_format_mod_supported(struct drm_plane *plane,  {
 	struct amdgpu_device *adev = drm_to_adev(plane->dev);
 	const struct drm_format_info *info = drm_format_info(format);
+	struct hw_asic_id asic_id = adev->dm.dc->ctx->asic_id;
 	int i;
 
 	enum dm_micro_swizzle microtile = modifier_gfx9_swizzle_mode(modifier) & 3; @@ -4955,13 +4956,53 @@ static bool dm_plane_format_mod_supported(struct drm_plane *plane,
 		return true;
 	}
 
-	/* Check that the modifier is on the list of the plane's supported modifiers. */
-	for (i = 0; i < plane->modifier_count; i++) {
-		if (modifier == plane->modifiers[i])
+	/* check if swizzle mode is supported by this version of DCN */
+	switch (asic_id.chip_family) {
+		case FAMILY_SI:
+		case FAMILY_CI:
+		case FAMILY_KV:
+		case FAMILY_CZ:
+		case FAMILY_VI:
+			/* AI and earlier asics does not have modifier support */
+			return false;
+			break;
+		case FAMILY_AI:
+		case FAMILY_RV:
+		case FAMILY_NV:
+		case FAMILY_VGH:
+		case FAMILY_YELLOW_CARP:
+		case AMDGPU_FAMILY_GC_10_3_6:
+		case AMDGPU_FAMILY_GC_10_3_7:
+			switch (AMD_FMT_MOD_GET(TILE, modifier)) {
+				case AMD_FMT_MOD_TILE_GFX9_64K_R_X:
+				case AMD_FMT_MOD_TILE_GFX9_64K_D_X:
+				case AMD_FMT_MOD_TILE_GFX9_64K_S_X:
+				case AMD_FMT_MOD_TILE_GFX9_64K_D:
+					return true;
+					break;
+				default:
+					return false;
+					break;
+			}
+			break;
+		case AMDGPU_FAMILY_GC_11_0_0:
+			switch (AMD_FMT_MOD_GET(TILE, modifier)) {
+				case AMD_FMT_MOD_TILE_GFX11_256K_R_X:
+				case AMD_FMT_MOD_TILE_GFX9_64K_R_X:
+				case AMD_FMT_MOD_TILE_GFX9_64K_D_X:
+				case AMD_FMT_MOD_TILE_GFX9_64K_S_X:
+				case AMD_FMT_MOD_TILE_GFX9_64K_D:
+					return true;
+					break;
+				default:
+					return false;
+					break;
+			}
+			break;
+		default:
+			ASSERT(0); /* Unknown asic */
 			break;
 	}
-	if (i == plane->modifier_count)
-		return false;
 
 	/*
 	 * For D swizzle the canonical modifier depends on the bpp, so check
--
2.36.1

Re: [PATCH] drm/amd/display: ignore modifiers when checking for format support

[Pillai, Aurabindo]

[-- Attachment #1: Type: text/plain, Size: 5487 bytes --]

[AMD Official Use Only - General]

Thanks for noticing, will fix it in a separate patch since I already merged this.

--

Regards,
Jay
________________________________
From: Chen, Guchun <Guchun.Chen@amd.com>
Sent: Thursday, June 9, 2022 9:28 PM
To: Pillai, Aurabindo <Aurabindo.Pillai@amd.com>; Olsak, Marek <Marek.Olsak@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Cc: Li, Sun peng (Leo) <Sunpeng.Li@amd.com>; Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>; Li, Roman <Roman.Li@amd.com>; Qiao, Ken <Ken.Qiao@amd.com>; Pillai, Aurabindo <Aurabindo.Pillai@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>
Subject: RE: [PATCH] drm/amd/display: ignore modifiers when checking for format support

+                                       return true;
+                                       break;

Possibly a coding style problem, 'break' after 'return' looks redundant.

Regards,
Guchun

-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Aurabindo Pillai
Sent: Thursday, June 9, 2022 10:27 PM
To: Olsak, Marek <Marek.Olsak@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Li, Sun peng (Leo) <Sunpeng.Li@amd.com>; Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>; Li, Roman <Roman.Li@amd.com>; Qiao, Ken <Ken.Qiao@amd.com>; Pillai, Aurabindo <Aurabindo.Pillai@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>
Subject: [PATCH] drm/amd/display: ignore modifiers when checking for format support

[Why&How]
There are cases where swizzle modes are set but modifiers arent. For such a userspace, we need not check modifiers while checking compatibilty in the drm hook for checking plane format.

Ignore checking modifiers but check the DCN generation for the supported swizzle mode.

Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 51 +++++++++++++++++--
 1 file changed, 46 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 2023baf41b7e..1322df491736 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4938,6 +4938,7 @@ static bool dm_plane_format_mod_supported(struct drm_plane *plane,  {
         struct amdgpu_device *adev = drm_to_adev(plane->dev);
         const struct drm_format_info *info = drm_format_info(format);
+       struct hw_asic_id asic_id = adev->dm.dc->ctx->asic_id;
         int i;

         enum dm_micro_swizzle microtile = modifier_gfx9_swizzle_mode(modifier) & 3; @@ -4955,13 +4956,53 @@ static bool dm_plane_format_mod_supported(struct drm_plane *plane,
                 return true;
         }

-       /* Check that the modifier is on the list of the plane's supported modifiers. */
-       for (i = 0; i < plane->modifier_count; i++) {
-               if (modifier == plane->modifiers[i])
+       /* check if swizzle mode is supported by this version of DCN */
+       switch (asic_id.chip_family) {
+               case FAMILY_SI:
+               case FAMILY_CI:
+               case FAMILY_KV:
+               case FAMILY_CZ:
+               case FAMILY_VI:
+                       /* AI and earlier asics does not have modifier support */
+                       return false;
+                       break;
+               case FAMILY_AI:
+               case FAMILY_RV:
+               case FAMILY_NV:
+               case FAMILY_VGH:
+               case FAMILY_YELLOW_CARP:
+               case AMDGPU_FAMILY_GC_10_3_6:
+               case AMDGPU_FAMILY_GC_10_3_7:
+                       switch (AMD_FMT_MOD_GET(TILE, modifier)) {
+                               case AMD_FMT_MOD_TILE_GFX9_64K_R_X:
+                               case AMD_FMT_MOD_TILE_GFX9_64K_D_X:
+                               case AMD_FMT_MOD_TILE_GFX9_64K_S_X:
+                               case AMD_FMT_MOD_TILE_GFX9_64K_D:
+                                       return true;
+                                       break;
+                               default:
+                                       return false;
+                                       break;
+                       }
+                       break;
+               case AMDGPU_FAMILY_GC_11_0_0:
+                       switch (AMD_FMT_MOD_GET(TILE, modifier)) {
+                               case AMD_FMT_MOD_TILE_GFX11_256K_R_X:
+                               case AMD_FMT_MOD_TILE_GFX9_64K_R_X:
+                               case AMD_FMT_MOD_TILE_GFX9_64K_D_X:
+                               case AMD_FMT_MOD_TILE_GFX9_64K_S_X:
+                               case AMD_FMT_MOD_TILE_GFX9_64K_D:
+                                       return true;
+                                       break;
+                               default:
+                                       return false;
+                                       break;
+                       }
+                       break;
+               default:
+                       ASSERT(0); /* Unknown asic */
                         break;
         }
-       if (i == plane->modifier_count)
-               return false;

         /*
          * For D swizzle the canonical modifier depends on the bpp, so check
--
2.36.1


[-- Attachment #2: Type: text/html, Size: 14054 bytes --]

Re: [PATCH] drm/amd/display: ignore modifiers when checking for format support

[Bas Nieuwenhuizen]

On Thu, Jun 9, 2022 at 4:27 PM Aurabindo Pillai
<aurabindo.pillai@amd.com> wrote:
>
> [Why&How]
> There are cases where swizzle modes are set but modifiers arent. For
> such a userspace, we need not check modifiers while checking
> compatibilty in the drm hook for checking plane format.
>
> Ignore checking modifiers but check the DCN generation for the
> supported swizzle mode.
>
> Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 51 +++++++++++++++++--
>  1 file changed, 46 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 2023baf41b7e..1322df491736 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4938,6 +4938,7 @@ static bool dm_plane_format_mod_supported(struct drm_plane *plane,
>  {
>         struct amdgpu_device *adev = drm_to_adev(plane->dev);
>         const struct drm_format_info *info = drm_format_info(format);
> +       struct hw_asic_id asic_id = adev->dm.dc->ctx->asic_id;
>         int i;
>
>         enum dm_micro_swizzle microtile = modifier_gfx9_swizzle_mode(modifier) & 3;
> @@ -4955,13 +4956,53 @@ static bool dm_plane_format_mod_supported(struct drm_plane *plane,
>                 return true;
>         }
>
> -       /* Check that the modifier is on the list of the plane's supported modifiers. */
> -       for (i = 0; i < plane->modifier_count; i++) {
> -               if (modifier == plane->modifiers[i])
> +       /* check if swizzle mode is supported by this version of DCN */
> +       switch (asic_id.chip_family) {
> +               case FAMILY_SI:
> +               case FAMILY_CI:
> +               case FAMILY_KV:
> +               case FAMILY_CZ:
> +               case FAMILY_VI:
> +                       /* AI and earlier asics does not have modifier support */
> +                       return false;
> +                       break;
> +               case FAMILY_AI:
> +               case FAMILY_RV:
> +               case FAMILY_NV:
> +               case FAMILY_VGH:
> +               case FAMILY_YELLOW_CARP:
> +               case AMDGPU_FAMILY_GC_10_3_6:
> +               case AMDGPU_FAMILY_GC_10_3_7:
> +                       switch (AMD_FMT_MOD_GET(TILE, modifier)) {
> +                               case AMD_FMT_MOD_TILE_GFX9_64K_R_X:
> +                               case AMD_FMT_MOD_TILE_GFX9_64K_D_X:
> +                               case AMD_FMT_MOD_TILE_GFX9_64K_S_X:
> +                               case AMD_FMT_MOD_TILE_GFX9_64K_D:
> +                                       return true;
> +                                       break;
> +                               default:
> +                                       return false;
> +                                       break;
> +                       }
> +                       break;
> +               case AMDGPU_FAMILY_GC_11_0_0:
> +                       switch (AMD_FMT_MOD_GET(TILE, modifier)) {
> +                               case AMD_FMT_MOD_TILE_GFX11_256K_R_X:
> +                               case AMD_FMT_MOD_TILE_GFX9_64K_R_X:
> +                               case AMD_FMT_MOD_TILE_GFX9_64K_D_X:
> +                               case AMD_FMT_MOD_TILE_GFX9_64K_S_X:
> +                               case AMD_FMT_MOD_TILE_GFX9_64K_D:
> +                                       return true;
> +                                       break;
> +                               default:
> +                                       return false;
> +                                       break;
> +                       }
> +                       break;
> +               default:
> +                       ASSERT(0); /* Unknown asic */
>                         break;
>         }

This seems broken to me. AFAICT we always return in the switch so the
code after this that checks for valid DCC usage isn't executed.
Checking the list of modifiers is also essential to make sure other
stuff in the modifier is set properly.

If you have userspace that is not using modifiers that is giving you
issues, a better place to look might be
convert_tiling_flags_to_modifier in amdgpu_display.c

> -       if (i == plane->modifier_count)
> -               return false;
>
>         /*
>          * For D swizzle the canonical modifier depends on the bpp, so check
> --
> 2.36.1
>

Re: [PATCH] drm/amd/display: ignore modifiers when checking for format support

[Marek Olšák]

[-- Attachment #1: Type: text/plain, Size: 4962 bytes --]

Bas, the code was literally rejecting swizzle modes that were not in the
modifier list, which was incorrect. That's because the modifier list is a
subset of all supported swizzle modes.

Marek

On Sun, Jun 12, 2022 at 7:54 PM Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
wrote:

> On Thu, Jun 9, 2022 at 4:27 PM Aurabindo Pillai
> <aurabindo.pillai@amd.com> wrote:
> >
> > [Why&How]
> > There are cases where swizzle modes are set but modifiers arent. For
> > such a userspace, we need not check modifiers while checking
> > compatibilty in the drm hook for checking plane format.
> >
> > Ignore checking modifiers but check the DCN generation for the
> > supported swizzle mode.
> >
> > Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> > ---
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 51 +++++++++++++++++--
> >  1 file changed, 46 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index 2023baf41b7e..1322df491736 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -4938,6 +4938,7 @@ static bool dm_plane_format_mod_supported(struct
> drm_plane *plane,
> >  {
> >         struct amdgpu_device *adev = drm_to_adev(plane->dev);
> >         const struct drm_format_info *info = drm_format_info(format);
> > +       struct hw_asic_id asic_id = adev->dm.dc->ctx->asic_id;
> >         int i;
> >
> >         enum dm_micro_swizzle microtile =
> modifier_gfx9_swizzle_mode(modifier) & 3;
> > @@ -4955,13 +4956,53 @@ static bool dm_plane_format_mod_supported(struct
> drm_plane *plane,
> >                 return true;
> >         }
> >
> > -       /* Check that the modifier is on the list of the plane's
> supported modifiers. */
> > -       for (i = 0; i < plane->modifier_count; i++) {
> > -               if (modifier == plane->modifiers[i])
> > +       /* check if swizzle mode is supported by this version of DCN */
> > +       switch (asic_id.chip_family) {
> > +               case FAMILY_SI:
> > +               case FAMILY_CI:
> > +               case FAMILY_KV:
> > +               case FAMILY_CZ:
> > +               case FAMILY_VI:
> > +                       /* AI and earlier asics does not have modifier
> support */
> > +                       return false;
> > +                       break;
> > +               case FAMILY_AI:
> > +               case FAMILY_RV:
> > +               case FAMILY_NV:
> > +               case FAMILY_VGH:
> > +               case FAMILY_YELLOW_CARP:
> > +               case AMDGPU_FAMILY_GC_10_3_6:
> > +               case AMDGPU_FAMILY_GC_10_3_7:
> > +                       switch (AMD_FMT_MOD_GET(TILE, modifier)) {
> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_R_X:
> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_D_X:
> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_S_X:
> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_D:
> > +                                       return true;
> > +                                       break;
> > +                               default:
> > +                                       return false;
> > +                                       break;
> > +                       }
> > +                       break;
> > +               case AMDGPU_FAMILY_GC_11_0_0:
> > +                       switch (AMD_FMT_MOD_GET(TILE, modifier)) {
> > +                               case AMD_FMT_MOD_TILE_GFX11_256K_R_X:
> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_R_X:
> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_D_X:
> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_S_X:
> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_D:
> > +                                       return true;
> > +                                       break;
> > +                               default:
> > +                                       return false;
> > +                                       break;
> > +                       }
> > +                       break;
> > +               default:
> > +                       ASSERT(0); /* Unknown asic */
> >                         break;
> >         }
>
> This seems broken to me. AFAICT we always return in the switch so the
> code after this that checks for valid DCC usage isn't executed.
> Checking the list of modifiers is also essential to make sure other
> stuff in the modifier is set properly.
>
> If you have userspace that is not using modifiers that is giving you
> issues, a better place to look might be
> convert_tiling_flags_to_modifier in amdgpu_display.c
>
> > -       if (i == plane->modifier_count)
> > -               return false;
> >
> >         /*
> >          * For D swizzle the canonical modifier depends on the bpp, so
> check
> > --
> > 2.36.1
> >
>

[-- Attachment #2: Type: text/html, Size: 6692 bytes --]

Re: [PATCH] drm/amd/display: ignore modifiers when checking for format support

[Bas Nieuwenhuizen]

On Mon, Jun 13, 2022 at 1:47 PM Marek Olšák <maraeo@gmail.com> wrote:
>
> Bas, the code was literally rejecting swizzle modes that were not in the modifier list, which was incorrect. That's because the modifier list is a subset of all supported swizzle modes.

That was WAI. The kernel is now in charge of rejecting stuff that is
not capable of being displayed.

Allowing all in format_mod_supported has several implications on
exposed & accepted modifiers as well, that should be avoided even if
we should do a behavior change for non-modifiers: We now expose (i.e.
list) modifiers for formats which they don't support and we removed
the check that the modifier is in the list for commits with modifiers
too. Hence this logic would need a serious rework instead of the patch
that was sent.

What combinations were failing, and can't we just add modifiers for them?




>
> Marek
>
> On Sun, Jun 12, 2022 at 7:54 PM Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> wrote:
>>
>> On Thu, Jun 9, 2022 at 4:27 PM Aurabindo Pillai
>> <aurabindo.pillai@amd.com> wrote:
>> >
>> > [Why&How]
>> > There are cases where swizzle modes are set but modifiers arent. For
>> > such a userspace, we need not check modifiers while checking
>> > compatibilty in the drm hook for checking plane format.
>> >
>> > Ignore checking modifiers but check the DCN generation for the
>> > supported swizzle mode.
>> >
>> > Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
>> > ---
>> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 51 +++++++++++++++++--
>> >  1 file changed, 46 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> > index 2023baf41b7e..1322df491736 100644
>> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> > @@ -4938,6 +4938,7 @@ static bool dm_plane_format_mod_supported(struct drm_plane *plane,
>> >  {
>> >         struct amdgpu_device *adev = drm_to_adev(plane->dev);
>> >         const struct drm_format_info *info = drm_format_info(format);
>> > +       struct hw_asic_id asic_id = adev->dm.dc->ctx->asic_id;
>> >         int i;
>> >
>> >         enum dm_micro_swizzle microtile = modifier_gfx9_swizzle_mode(modifier) & 3;
>> > @@ -4955,13 +4956,53 @@ static bool dm_plane_format_mod_supported(struct drm_plane *plane,
>> >                 return true;
>> >         }
>> >
>> > -       /* Check that the modifier is on the list of the plane's supported modifiers. */
>> > -       for (i = 0; i < plane->modifier_count; i++) {
>> > -               if (modifier == plane->modifiers[i])
>> > +       /* check if swizzle mode is supported by this version of DCN */
>> > +       switch (asic_id.chip_family) {
>> > +               case FAMILY_SI:
>> > +               case FAMILY_CI:
>> > +               case FAMILY_KV:
>> > +               case FAMILY_CZ:
>> > +               case FAMILY_VI:
>> > +                       /* AI and earlier asics does not have modifier support */
>> > +                       return false;
>> > +                       break;
>> > +               case FAMILY_AI:
>> > +               case FAMILY_RV:
>> > +               case FAMILY_NV:
>> > +               case FAMILY_VGH:
>> > +               case FAMILY_YELLOW_CARP:
>> > +               case AMDGPU_FAMILY_GC_10_3_6:
>> > +               case AMDGPU_FAMILY_GC_10_3_7:
>> > +                       switch (AMD_FMT_MOD_GET(TILE, modifier)) {
>> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_R_X:
>> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_D_X:
>> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_S_X:
>> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_D:
>> > +                                       return true;
>> > +                                       break;
>> > +                               default:
>> > +                                       return false;
>> > +                                       break;
>> > +                       }
>> > +                       break;
>> > +               case AMDGPU_FAMILY_GC_11_0_0:
>> > +                       switch (AMD_FMT_MOD_GET(TILE, modifier)) {
>> > +                               case AMD_FMT_MOD_TILE_GFX11_256K_R_X:
>> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_R_X:
>> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_D_X:
>> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_S_X:
>> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_D:
>> > +                                       return true;
>> > +                                       break;
>> > +                               default:
>> > +                                       return false;
>> > +                                       break;
>> > +                       }
>> > +                       break;
>> > +               default:
>> > +                       ASSERT(0); /* Unknown asic */
>> >                         break;
>> >         }
>>
>> This seems broken to me. AFAICT we always return in the switch so the
>> code after this that checks for valid DCC usage isn't executed.
>> Checking the list of modifiers is also essential to make sure other
>> stuff in the modifier is set properly.
>>
>> If you have userspace that is not using modifiers that is giving you
>> issues, a better place to look might be
>> convert_tiling_flags_to_modifier in amdgpu_display.c
>>
>> > -       if (i == plane->modifier_count)
>> > -               return false;
>> >
>> >         /*
>> >          * For D swizzle the canonical modifier depends on the bpp, so check
>> > --
>> > 2.36.1
>> >

Re: [PATCH] drm/amd/display: ignore modifiers when checking for format support

[Marek Olšák]

[-- Attachment #1: Type: text/plain, Size: 6598 bytes --]

We can reject invalid modifiers elsewhere, but it can't be here because
it's also the non-modifier path.

We expose 256KB_R_X or 64KB_R_X modifiers depending on chip-specific
settings, but not both. Only the optimal option is exposed. This is OK for
modifiers, but not OK with AMD-specific BO metadata where the UMD
determines the swizzle mode.

Marek

On Tue, Jun 14, 2022 at 8:38 PM Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
wrote:

> On Mon, Jun 13, 2022 at 1:47 PM Marek Olšák <maraeo@gmail.com> wrote:
> >
> > Bas, the code was literally rejecting swizzle modes that were not in the
> modifier list, which was incorrect. That's because the modifier list is a
> subset of all supported swizzle modes.
>
> That was WAI. The kernel is now in charge of rejecting stuff that is
> not capable of being displayed.
>
> Allowing all in format_mod_supported has several implications on
> exposed & accepted modifiers as well, that should be avoided even if
> we should do a behavior change for non-modifiers: We now expose (i.e.
> list) modifiers for formats which they don't support and we removed
> the check that the modifier is in the list for commits with modifiers
> too. Hence this logic would need a serious rework instead of the patch
> that was sent.
>
> What combinations were failing, and can't we just add modifiers for them?
>
>
>
>
> >
> > Marek
> >
> > On Sun, Jun 12, 2022 at 7:54 PM Bas Nieuwenhuizen <
> bas@basnieuwenhuizen.nl> wrote:
> >>
> >> On Thu, Jun 9, 2022 at 4:27 PM Aurabindo Pillai
> >> <aurabindo.pillai@amd.com> wrote:
> >> >
> >> > [Why&How]
> >> > There are cases where swizzle modes are set but modifiers arent. For
> >> > such a userspace, we need not check modifiers while checking
> >> > compatibilty in the drm hook for checking plane format.
> >> >
> >> > Ignore checking modifiers but check the DCN generation for the
> >> > supported swizzle mode.
> >> >
> >> > Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> >> > ---
> >> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 51
> +++++++++++++++++--
> >> >  1 file changed, 46 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> > index 2023baf41b7e..1322df491736 100644
> >> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> > @@ -4938,6 +4938,7 @@ static bool
> dm_plane_format_mod_supported(struct drm_plane *plane,
> >> >  {
> >> >         struct amdgpu_device *adev = drm_to_adev(plane->dev);
> >> >         const struct drm_format_info *info = drm_format_info(format);
> >> > +       struct hw_asic_id asic_id = adev->dm.dc->ctx->asic_id;
> >> >         int i;
> >> >
> >> >         enum dm_micro_swizzle microtile =
> modifier_gfx9_swizzle_mode(modifier) & 3;
> >> > @@ -4955,13 +4956,53 @@ static bool
> dm_plane_format_mod_supported(struct drm_plane *plane,
> >> >                 return true;
> >> >         }
> >> >
> >> > -       /* Check that the modifier is on the list of the plane's
> supported modifiers. */
> >> > -       for (i = 0; i < plane->modifier_count; i++) {
> >> > -               if (modifier == plane->modifiers[i])
> >> > +       /* check if swizzle mode is supported by this version of DCN
> */
> >> > +       switch (asic_id.chip_family) {
> >> > +               case FAMILY_SI:
> >> > +               case FAMILY_CI:
> >> > +               case FAMILY_KV:
> >> > +               case FAMILY_CZ:
> >> > +               case FAMILY_VI:
> >> > +                       /* AI and earlier asics does not have
> modifier support */
> >> > +                       return false;
> >> > +                       break;
> >> > +               case FAMILY_AI:
> >> > +               case FAMILY_RV:
> >> > +               case FAMILY_NV:
> >> > +               case FAMILY_VGH:
> >> > +               case FAMILY_YELLOW_CARP:
> >> > +               case AMDGPU_FAMILY_GC_10_3_6:
> >> > +               case AMDGPU_FAMILY_GC_10_3_7:
> >> > +                       switch (AMD_FMT_MOD_GET(TILE, modifier)) {
> >> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_R_X:
> >> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_D_X:
> >> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_S_X:
> >> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_D:
> >> > +                                       return true;
> >> > +                                       break;
> >> > +                               default:
> >> > +                                       return false;
> >> > +                                       break;
> >> > +                       }
> >> > +                       break;
> >> > +               case AMDGPU_FAMILY_GC_11_0_0:
> >> > +                       switch (AMD_FMT_MOD_GET(TILE, modifier)) {
> >> > +                               case AMD_FMT_MOD_TILE_GFX11_256K_R_X:
> >> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_R_X:
> >> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_D_X:
> >> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_S_X:
> >> > +                               case AMD_FMT_MOD_TILE_GFX9_64K_D:
> >> > +                                       return true;
> >> > +                                       break;
> >> > +                               default:
> >> > +                                       return false;
> >> > +                                       break;
> >> > +                       }
> >> > +                       break;
> >> > +               default:
> >> > +                       ASSERT(0); /* Unknown asic */
> >> >                         break;
> >> >         }
> >>
> >> This seems broken to me. AFAICT we always return in the switch so the
> >> code after this that checks for valid DCC usage isn't executed.
> >> Checking the list of modifiers is also essential to make sure other
> >> stuff in the modifier is set properly.
> >>
> >> If you have userspace that is not using modifiers that is giving you
> >> issues, a better place to look might be
> >> convert_tiling_flags_to_modifier in amdgpu_display.c
> >>
> >> > -       if (i == plane->modifier_count)
> >> > -               return false;
> >> >
> >> >         /*
> >> >          * For D swizzle the canonical modifier depends on the bpp,
> so check
> >> > --
> >> > 2.36.1
> >> >
>

[-- Attachment #2: Type: text/html, Size: 9038 bytes --]