Re: [PATCH 2/4] drm/amdgpu: Convert tiling flags to modifiers on GFX6-8

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing cpp factor in GFX6 block size calculation enables out-of-bounds VRAM read vulnerability.
- [High] Strict rejection of RENDER microtiling introduces a functional regression for legacy GFX6 compositors.
- [Low] Usage of undefined/DC-specific ASSERT macro in generic AMDGPU code causes build failures or macro misuse.
--

--- Patch [2]: [PATCH 2/4] drm/amdgpu: Convert tiling flags to modifiers on GFX6-8 ---
commit 44a2b5305f951bc68d3e2665bc5db9defc35dd24
Author: Timur Kristóf <timur.kristof@gmail.com>
Subject: drm/amdgpu: Convert tiling flags to modifiers on GFX6-8

This commit converts tiling flags to DRM format modifiers for AMD GFX8 and
older GPUs, ensuring consistency across generations and allowing the display
driver to rely on them. It includes logic to map array modes to modifiers and
verify block sizes for GFX6 specifically.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c

[ ... ]

> +	if (array_mode != AMD_FMT_MOD_TILE_GFX6_1D_TILED_THIN1 &&
> +	    array_mode != AMD_FMT_MOD_TILE_GFX6_2D_TILED_THIN1) {
> +		drm_dbg_kms(dev, "Array mode %u not supported by display driver\n", array_mode);
> +		return -EINVAL;
> +	}
> +	if (micro_tile_mode != AMD_FMT_MOD_MICROTILE_DISPLAY) {
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this break legacy userspace compositors? 

Looking at check_tiling_flags_gfx6(), it explicitly supports RENDER 
microtiling (mode 3):

	micro_tile_mode = AMDGPU_TILING_GET(afb->tiling_flags, MICRO_TILE_MODE);
	switch (micro_tile_mode) {
	case 0: /* DISPLAY */
	case 3: /* RENDER */
		return 0;

Could returning -EINVAL here for RENDER mode cause a regression for legacy 
clients allocating display buffers for 3D performance?

> +		drm_dbg_kms(dev, "Microtile mode %u not displayable\n", micro_tile_mode);
> +		return -EINVAL;
> +	}

[ ... ]

> @@ -1120,6 +1247,15 @@ static int amdgpu_display_verify_sizes(struct amdgpu_framebuffer *rfb)
>  
>  			get_block_dimensions(block_size_log2, format_info->cpp[i],
>  					     &block_width, &block_height);
> +		} else if (AMD_FMT_MOD_GET(TILE_VERSION, modifier) == AMD_FMT_MOD_TILE_VER_GFX6) {
> +			ret = amdgpu_display_verify_sizes_gfx6(rfb->base.dev, modifier,
> +							       &block_width, &block_height);
> +			if (ret)
> +				return ret;
> +
> +			ASSERT(is_power_of_2(block_width));
                        ^^^^^^

[Severity: Low]
Are these ASSERT() macros defined in this context? 

ASSERT is typically a Display Core (DC) specific macro (from dc/os_types.h). 
Since amdgpu_display.c is part of the core driver, would WARN_ON() be more 
appropriate here to avoid potential build failures?

> +			ASSERT(is_power_of_2(block_height));
> +			block_size_log2 = ilog2(block_width * block_height);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Should this calculation include the bytes-per-pixel factor? 

By omitting the format_info->cpp[i] multiplier, block_size_log2 appears to 
underestimate the block size. 

When this is later passed into amdgpu_display_verify_plane(), it calculates:

	unsigned int block_size = 1 << block_size_log2;
        ...
	size = rfb->base.offsets[plane] +
		(uint64_t)rfb->base.pitches[plane] / block_pitch *
		block_size * DIV_ROUND_UP(height, block_height);

	if (rfb->base.obj[0]->size < size) {

An underestimated block_size results in a vastly smaller size requirement, 
which might allow an undersized Buffer Object to pass validation, 
potentially leading to out-of-bounds VRAM reads.

>  		}
>  
>  		ret = amdgpu_display_verify_plane(rfb, i, format_info,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720142440.22076-1-timur.kristof@gmail.com?part=2