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] Missing bit-to-byte conversion in `tile_bytes` calculation.
- [High] NULL pointer dereference in `amdgpu_dm_plane_add_modifier_dedup`.
- [Medium] Missing 8-bpp modifier exposure in `amdgpu_dm_plane_add_gfx6_modifiers`.
--

--- Patch [3]: [PATCH 3/4] drm/amd/display: Support DRM format modifiers on GFX6-8 ---
commit 3fb28c857b70a3a3413b8818708816df4bf9fc7e
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.

> 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 b2507c5de7c86..9d56ce635cbda 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

[ ... ]

> @@ -180,6 +180,19 @@ STATIC_IFN_KUNIT void amdgpu_dm_plane_add_modifier(uint64_t **mods, uint64_t *si
>  }
>  EXPORT_IF_KUNIT(amdgpu_dm_plane_add_modifier);
>  
> +STATIC_IFN_KUNIT void amdgpu_dm_plane_add_modifier_dedup(uint64_t **mods, uint64_t *size,
> +							 uint64_t *cap, uint64_t mod)
> +{
> +	uint64_t i;
> +
> +	for (i = 0; i < *size; ++i)
> +		if ((*mods)[i] == mod)
> +			return;

[Severity: High]
If a previous call to amdgpu_dm_plane_add_modifier() fails to reallocate the
array, it sets *mods to NULL but leaves *size at its previous non-zero value:

amdgpu_dm_plane_add_modifier() {
    ...
		if (!new_mods) {
			kfree(*mods);
			*mods = NULL;
			return;
		}
    ...
}

Will this loop cause a NULL pointer dereference on the next call to 
amdgpu_dm_plane_add_modifier_dedup() since it unconditionally evaluates 
(*mods)[i] without checking if *mods is NULL?

> +
> +	amdgpu_dm_plane_add_modifier(mods, size, cap, mod);
> +}
> +EXPORT_IF_KUNIT(amdgpu_dm_plane_add_modifier_dedup);

[ ... ]

> @@ -468,6 +516,246 @@ static void amdgpu_dm_plane_add_gfx10_1_modifiers(const struct amdgpu_device *ad

[ ... ]

> +/**
> + * amdgpu_dm_plane_get_gfx7_macro_tile_idx() - Get macro tile mode index on GFX7-8
> + *
> + * @bpp: bits per pixel in the image
> + * @tile_split_bytes: actual tile split bytes, see amdgpu_dm_plane_calc_gfx7_tile_split()
> + *
> + * Select which macro tiling mode from the table is suitable for the given bits per pixel,
> + * assuming 2D_TILED_THIN1 array mode and DISPLAY micro tile mode (and no multisampling).
> + * Note that the tiling table is uAPI and userspace relies on specific modes being
> + * present at specific indices.
> + * See CiLib::HwlComputeMacroModeIndex() in addrlib.
> + */
> +static u32 amdgpu_dm_plane_get_gfx7_macro_tile_idx(const u32 bpp, const u32 tile_split_bytes)
> +{
> +	const u32 tile_bytes = clamp(8 * 8 * bpp, 64, tile_split_bytes);

[Severity: High]
Does this calculation compute the microtile size in bits instead of bytes? 
Because it lacks a division by 8 (e.g., (8 * 8 * bpp) / 8), the computed 
tile_bytes is artificially inflated. For a 16bpp format, this would
calculate 1024 instead of 128, leading to an incorrect macro tile index 
being selected which may cause tiled buffers to be rejected or misrendered 
on GFX7-8.

> +	const u32 macro_tile_idx = ilog2(tile_bytes / 64);
> +
> +	WARN_ON(macro_tile_idx >= 16);
> +
> +	return macro_tile_idx;
> +}

[ ... ]

> +/**
> + * amdgpu_dm_plane_add_gfx6_modifiers() - Expose modifiers for GFX6-8
> + *
> + * @adev: amdgpu_device pointer
> + * @mods: Pointer to array of format modifiers
> + * @size: Pointer to size of the array
> + * @capacity: Pointer to capacity of the array
> + *
> + * Calculate a DRM format modifier for macro tiled modes for each supported
> + * bits per pixel value. Use de-duplication because on some GPUs it may happen
> + * that different bpp results in the exact same macro tiling mode, depending
> + * on the tiling table.
> + *
> + * Also expose a micro tiled only mode. This is less optimal, but supported.
> + */
> +static void amdgpu_dm_plane_add_gfx6_modifiers(const struct amdgpu_device *adev,
> +					       u64 **mods,
> +					       u64 *size,
> +					       u64 *capacity)
> +{
> +	/* 2D tiled displayable */
> +	amdgpu_dm_plane_add_modifier(mods, size, capacity,
> +		amdgpu_dm_plane_calc_gfx6_mod(adev, 16, DC_ARRAY_2D_TILED_THIN1));

[Severity: Medium]
Are we missing the 8-bpp modifier exposure here?

The helper function amdgpu_dm_plane_get_gfx6_tile_idx() explicitly supports 
bpp == 8, but we only seem to expose 16, 32, and 64 bpp formats here. 
Without this, userspace might not be able to negotiate 2D tiling for 8-bpp
formats.

> +	amdgpu_dm_plane_add_modifier_dedup(mods, size, capacity,
> +		amdgpu_dm_plane_calc_gfx6_mod(adev, 32, DC_ARRAY_2D_TILED_THIN1));
> +	amdgpu_dm_plane_add_modifier_dedup(mods, size, capacity,
> +		amdgpu_dm_plane_calc_gfx6_mod(adev, 64, DC_ARRAY_2D_TILED_THIN1));
> +
> +	/* 1D tiled displayable */
> +	amdgpu_dm_plane_add_modifier(mods, size, capacity,
> +		amdgpu_dm_plane_calc_gfx6_mod(adev, 0, DC_ARRAY_1D_TILED_THIN1));
> +}

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