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? > + > + 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/[email protected]?part=3
