Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Backwards Compatibility / Functional Regression: Restricting supported formats on primary plane - [Medium] Memory leak of `radeon_crtc` on the error path when `drm_crtc_init_with_planes()` fails. --
commit a5951b1d6bc3cbef461bfcabdab5aeff88a28da4 Author: Shixiong Ou <[email protected]> drm/radeon: Create the primary plane in the driver This commit replaces the fixed format primary plane created by drm_crtc_init() with a driver-owned primary plane in the radeon driver. This allows the driver to set the blend mode property to PREMULTI, resolving warnings during drm_mode_config_validate(). > diff --git a/drivers/gpu/drm/radeon/radeon_display.c > b/drivers/gpu/drm/radeon/radeon_display.c > index aac6733ddd82c..058ec866f02b8 100644 > --- a/drivers/gpu/drm/radeon/radeon_display.c > +++ b/drivers/gpu/drm/radeon/radeon_display.c [ ... ] > @@ -664,6 +667,19 @@ radeon_crtc_set_config(struct drm_mode_set *set, > return ret; > } > > +static const uint32_t radeon_primary_formats[] = { > + /* > + * The display engine programs an ARGB8888 surface format for both > + * XRGB8888 and ARGB8888 framebuffers. > + */ > + DRM_FORMAT_XRGB8888, > + DRM_FORMAT_ARGB8888, > +}; > + > +static const struct drm_plane_funcs radeon_primary_plane_funcs = { > + DRM_PLANE_NON_ATOMIC_FUNCS, > +}; > + > static const struct drm_crtc_funcs radeon_crtc_funcs = { > .cursor_set2 = radeon_crtc_cursor_set2, > .cursor_move = radeon_crtc_cursor_move, [ ... ] > @@ -693,7 +711,29 @@ static void radeon_crtc_init(struct drm_device *dev, int > index) > return; > } > > - drm_crtc_init(dev, &radeon_crtc->base, &radeon_crtc_funcs); > + primary = __drm_universal_plane_alloc(dev, sizeof(*primary), 0, 0, > + &radeon_primary_plane_funcs, > + radeon_primary_formats, > + > ARRAY_SIZE(radeon_primary_formats), > + NULL, DRM_PLANE_TYPE_PRIMARY, > NULL); [Severity: High] Will this break backwards compatibility for userspace programs configuring legacy pixel formats? The array radeon_primary_formats explicitly lists only DRM_FORMAT_XRGB8888 and DRM_FORMAT_ARGB8888. When __drm_universal_plane_alloc() is called here, it leaves primary->format_default as false (since it is zero-initialized). Because of this, the core DRM modeset logic in drm_mode_setcrtc() will now strictly check pixel formats against this restricted list and return -EINVAL for other formats. The hardware supports and previously accepted legacy formats (like RGB565, 8-bit, 15-bit, and 24-bit color) which are handled in radeon_crtc_do_set_base(). > + if (IS_ERR(primary)) { > + destroy_workqueue(radeon_crtc->flip_queue); > + kfree(radeon_crtc); > + return; > + } > + > + ret = drm_crtc_init_with_planes(dev, &radeon_crtc->base, primary, NULL, > + &radeon_crtc_funcs, NULL); > + if (ret) { > + dev_err(dev->dev, "Failed to init CRTC %d: %d\n", index, ret); > + drm_plane_cleanup(primary); > + kfree(primary); > + destroy_workqueue(radeon_crtc->flip_queue); > + return; > + } [Severity: Medium] Does this error path leak the dynamically allocated radeon_crtc object? If drm_crtc_init_with_planes() fails during device initialization, the code cleans up the primary plane and destroys the flip queue, but it appears to return without calling kfree(radeon_crtc). -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
