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

["Timur Kristóf" <timur.kristof@gmail.com>]

On 2026. július 15., szerda 13:08:20 közép-európai nyári idő sashiko-
bot@kernel.org wrote:
> 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?

This can't happen in practice. amdgpu_dm_plane_get_plane_modifiers() allocates 
enough space for 128 modifiers but amdgpu_dm_plane_add_gfx6_modifiers() only 
adds up to 4 modifiers to the array.

That being said, let me know what you recommend to do with this.

> 
> > +
> > +	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.

This may be a real issue. I will look into this.

> 
> > +	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.

The macro tile mode array has macro tile modes for 8 bpp, but DC doesn't 
expose any formats with 8 bpp. So we don't need to expose modifiers for 8 bpp 
either.

> 
> > +	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));
> > +}