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