Re: [PATCH 13/13] drm/amdgpu: rewrite convert_tiling_flags_to_modifier_gfx12

[Aurabindo Pillai]

Reviewed-by: Aurabindo Pillai <aurabindo.pil...@amd.com>

On 6/26/24 2:31 PM, Marek Olšák wrote:

There were multiple bugs, like checking SWIZZLE_MODE before checking
GFX12_SWIZZLE_MODE, which has undefined behavior.

The function had no effect before (it always returned -EINVAL).

Signed-off-by: Marek Olšák <marek.ol...@amd.com>
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 45 +++++++++------------
  1 file changed, 18 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index ec0299c5918e..092ec11258cd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -724,32 +724,25 @@ extract_render_dcc_offset(struct amdgpu_device *adev,

static int convert_tiling_flags_to_modifier_gfx12(struct amdgpu_framebuffer *afb)

  {
-       struct amdgpu_device *adev = drm_to_adev(afb->base.dev);
-       const struct drm_format_info *format_info;
        u64 modifier = 0;
-       int tile = 0;
-       int swizzle = 0;
+       int swizzle_mode = AMDGPU_TILING_GET(afb->tiling_flags, 
GFX12_SWIZZLE_MODE);

- if (amdgpu_ip_version(adev, GC_HWIP, 0) >= IP_VERSION(12, 0, 0)) {

-               tile = AMD_FMT_MOD_TILE_VER_GFX12;
-               swizzle =  AMDGPU_TILING_GET(afb->tiling_flags, 
GFX12_SWIZZLE_MODE);
+       if (!swizzle_mode) {
+               modifier = DRM_FORMAT_MOD_LINEAR;
+       } else {
+               int max_comp_block =
+                       AMDGPU_TILING_GET(afb->tiling_flags, 
GFX12_DCC_MAX_COMPRESSED_BLOCK);
+
+               modifier =
+                       AMD_FMT_MOD |
+                       AMD_FMT_MOD_SET(TILE_VERSION, 
AMD_FMT_MOD_TILE_VER_GFX12) |
+                       AMD_FMT_MOD_SET(TILE, swizzle_mode) |
+                       AMD_FMT_MOD_SET(DCC, afb->gfx12_dcc) |
+                       AMD_FMT_MOD_SET(DCC_MAX_COMPRESSED_BLOCK, 
max_comp_block);
        }

- modifier =

-               AMD_FMT_MOD |
-               AMD_FMT_MOD_SET(TILE, swizzle) |
-               AMD_FMT_MOD_SET(TILE_VERSION,  tile) |
-               AMD_FMT_MOD_SET(DCC, 0) |
-               AMD_FMT_MOD_SET(DCC_MAX_COMPRESSED_BLOCK, 0);
-
-       format_info = amdgpu_lookup_format_info(afb->base.format->format,
-                                               modifier);
-       if (!format_info)
-               return -EINVAL;
-
        afb->base.modifier = modifier;
        afb->base.flags |= DRM_MODE_FB_MODIFIERS;
-
        return 0;
  }

@@ -777,12 +770,6 @@ static int convert_tiling_flags_to_modifier(struct amdgpu_framebuffer *afb)

                int pipes = ilog2(num_pipes);
                uint32_t dcc_offset = AMDGPU_TILING_GET(afb->tiling_flags, 
DCC_OFFSET_256B);

-

-               if (amdgpu_ip_version(adev, GC_HWIP, 0) >= IP_VERSION(12, 0, 
0)) {
-                       convert_tiling_flags_to_modifier_gfx12(afb);
-                       return 0;
-               }
-
                switch (swizzle >> 2) {
                case 0: /* 256B */
                        block_size_bits = 8;
@@ -1281,7 +1268,11 @@ static int amdgpu_display_framebuffer_init(struct 
drm_device *dev,

if (!dev->mode_config.fb_modifiers_not_supported &&

            !(rfb->base.flags & DRM_MODE_FB_MODIFIERS)) {
-               ret = convert_tiling_flags_to_modifier(rfb);
+               if (amdgpu_ip_version(adev, GC_HWIP, 0) >= IP_VERSION(12, 0, 0))
+                       ret = convert_tiling_flags_to_modifier_gfx12(rfb);
+               else
+                       ret = convert_tiling_flags_to_modifier(rfb);
+
                if (ret) {
                        drm_dbg_kms(dev, "Failed to convert tiling flags 0x%llX to a 
modifier",
                                    rfb->tiling_flags);

--
--

Thanks & Regards,
Aurabindo Pillai