Re: [PATCH 3/4] drm/amd/display: Support DRM format modifiers on GFX6-8

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The calculation of `block_size_log2` for GFX6 modifiers computes the log2 of the block's pixel count rather than its byte size, allowing undersized framebuffers to bypass validation.
- [High] The new format modifier validation rigidly rejects `RENDER` micro-tiling mode (value 3), breaking backward compatibility with existing userspace buffers.
- [Medium] Memory leak in KUnit test `dm_test_get_plane_modifiers` due to an overwritten pointer.
--

--- Patch [3]: [PATCH 3/4] drm/amd/display: Support DRM format modifiers on GFX6-8 ---
commit 4836999b9da380df8ff4cbe5d660f33f0b1bc33d
Author: Timur Kristóf <timur.kristof@gmail.com>

drm/amd/display: Support DRM format modifiers on GFX6-8

Expose displayable DRM format modifiers based on which
tiling modes are supported by the GFX block. This patch calculates
and exposes proper macro and micro tiling modes for GFX6-8 hardware.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> index 26f35434a92f0..2706b2ea4eae2 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
[ ... ]
> @@ -468,6 +519,249 @@ static void amdgpu_dm_plane_add_gfx10_1_modifiers(const struct amdgpu_device *ad
[ ... ]
> +/**
> + * amdgpu_dm_plane_gfx6_format_mod_supported() - Check if a modifier is supported on GFX6-8
> + *
> + * @adev: amdgpu_device pointer
> + * @bpp: bits per pixel in the image
> + * @modifier: the modifier whose support we check
> + *
> + * On GFX6-8, not all DRM format modifier can be used with all image formats.
> + * Check whether the specified modifier is supported with the given bits per pixel value.
> + */
> +static bool amdgpu_dm_plane_gfx6_format_mod_supported(const struct amdgpu_device *adev,
> +						      const u32 bpp,
> +						      const u64 modifier)
> +{
> +	const u32 array_mode = AMD_FMT_MOD_GET(TILE, modifier);
> +	const u32 micro_tile_mode = AMD_FMT_MOD_GET(MICROTILE, modifier);
> +
> +	if (!IS_AMD_FMT_MOD(modifier))
> +		return false;
> +
> +	/* GFX9 and newer format modifiers are not supported on GFX6-8 yet. */
> +	if (AMD_FMT_MOD_GET(TILE_VERSION, modifier) != AMD_FMT_MOD_TILE_VER_GFX6)
> +		return false;
> +
> +	/* GFX8 and older format modifiers are not supported on GFX9+ yet. */
> +	if (adev->family >= AMDGPU_FAMILY_AI)
> +		return false;
> +
> +	/* GFX6-7 doesn't have any DCC, GFX8 doesn't have displayable DCC. */
> +	if (AMD_FMT_MOD_GET(DCC, modifier))
> +		return false;
> +
> +	/*
> +	 * For now, only expose 1D and 2D tiles THIN array modes.
> +	 * Linear is already exposed through DRM_FORMAT_MOD_LINEAR.
> +	 */
> +	if (array_mode != AMD_FMT_MOD_TILE_GFX6_1D_TILED_THIN1 &&
> +	    array_mode != AMD_FMT_MOD_TILE_GFX6_2D_TILED_THIN1)
> +		return false;
> +
> +	/*
> +	 * For now, only expose DISPLAY micro tile mode.
> +	 * THIN, DEPTH and THICK modes are not displayable.
> +	 * ROTATED has never been supported on Linux.
> +	 */
> +	if (micro_tile_mode != AMD_FMT_MOD_MICROTILE_DISPLAY)
> +		return false;

[Severity: High]
Does this strict micro tile mode check break backward compatibility with
existing userspace?

Prior to this patch, userspace could allocate a buffer with the RENDER
micro-tiling mode (value 3) and it was explicitly allowed by the legacy check
for compatibility with Mesa's radeonsi driver.

By exposing modifiers on GFX6-8 here, the legacy check is disabled, and the
kernel converts the tiling flags to an implicit modifier. This modifier gets
rejected by the strict AMD_FMT_MOD_MICROTILE_DISPLAY check in
amdgpu_display_verify_sizes_gfx6(), which can cause compositors to fail to
display scanout buffers.

> +
> +	/* Verify that the modifier is the same that we'd expose for this bpp */
> +	return amdgpu_dm_plane_calc_gfx6_mod(adev, bpp, array_mode) == modifier;
> +}
[ ... ]
> @@ -749,12 +1043,6 @@ static void amdgpu_dm_plane_add_gfx12_modifiers(struct amdgpu_device *adev,
>  STATIC_IFN_KUNIT int amdgpu_dm_plane_get_plane_modifiers(struct amdgpu_device *adev, unsigned int plane_type, uint64_t **mods)
>  {
>  	uint64_t size = 0, capacity = 128;
> -	*mods = NULL;
> -
> -	/* We have not hooked up any pre-GFX9 modifiers. */
> -	if (adev->family < AMDGPU_FAMILY_AI)
> -		return 0;
> -

[Severity: Medium]
Can this cause a KUnit test failure and memory leak?

The test dm_test_get_plane_modifiers() in
drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_plane_test.c asserts
that amdgpu_dm_plane_get_plane_modifiers() leaves `mods` as NULL for
AMDGPU_FAMILY_SI. Since this patch now allocates and populates `mods` for
GFX6 hardware, the test expectation fails.

Because the execution continues in the test, it passes the already-allocated
pointer back into the function which overwrites the pointer, leaking the
previous array allocation.

>  	*mods = kmalloc_array(capacity, sizeof(uint64_t), GFP_KERNEL);
>  
>  	if (plane_type == DRM_PLANE_TYPE_CURSOR) {
> @@ -764,6 +1052,13 @@ STATIC_IFN_KUNIT int amdgpu_dm_plane_get_plane_modifiers(struct amdgpu_device *a
>  	}
>  
>  	switch (adev->family) {
> +	case AMDGPU_FAMILY_SI:
> +	case AMDGPU_FAMILY_CI:
> +	case AMDGPU_FAMILY_KV:
> +	case AMDGPU_FAMILY_VI:
> +	case AMDGPU_FAMILY_CZ:
> +		amdgpu_dm_plane_add_gfx6_modifiers(adev, mods, &size, &capacity);
> +		break;

[Severity: High]
Could exposing these modifiers allow undersized framebuffers to bypass validation?

By adding these modifiers, the code now allows userspace to create framebuffers
with GFX6 modifiers via addfb2. In drivers/gpu/drm/amd/amdgpu/amdgpu_display.c,
amdgpu_display_verify_sizes() calculates block_size_log2 for GFX6:

    block_size_log2 = ilog2(block_width * block_height);

This calculates the log2 of the pixel count rather than the byte size, because
it is missing the format_info->cpp[i] multiplier. The missing multiplier
causes the byte size validation to incorrectly pass for undersized buffers.
This can lead to the display hardware reading beyond the allocated GEM object
bounds during scanout.

>  	case AMDGPU_FAMILY_AI:
>  	case AMDGPU_FAMILY_RV:
>  		amdgpu_dm_plane_add_gfx9_modifiers(adev, mods, &size, &capacity);

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