On 2026. július 15., szerda 13:08:20 közép-európai nyári idő sashiko-
[email protected] 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 <[email protected]>
>
> 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));
> > +}
