Re: [PATCH 4/5] drm/radeon: Inline drm_crtc_init()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/radeon: radeon_crtc_init() breaks non-XRGB8888/ARGB8888
  modesets
--

commit 084c2fdd462a7f679496ecdf20e1829b6180f22d
Author: Thomas Zimmermann <tzimmermann@suse.de>

drm/radeon: Inline drm_crtc_init()

This commit inlines drm_crtc_init() into radeon_crtc_init() by allocating
a primary plane with __drm_universal_plane_alloc() and initializing the
CRTC via drm_crtc_init_with_planes().

> diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
> index aac6733ddd82c..4b784df4679f9 100644
> --- a/drivers/gpu/drm/radeon/radeon_display.c
> +++ b/drivers/gpu/drm/radeon/radeon_display.c
> @@ -38,6 +38,7 @@
>  #include <drm/drm_framebuffer.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_modeset_helper.h>
> +#include <drm/drm_plane_helper.h>
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_vblank.h>
>  #include <drm/radeon_drm.h>
> @@ -47,6 +48,17 @@
>  #include "radeon.h"
>  #include "radeon_kms.h"
>  
> +static const u32 radeon_primary_plane_formats[] = {
> +	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_ARGB8888,
> +};
> +
> +static const struct drm_plane_funcs radeon_primary_plane_funcs = {
> +	.update_plane = drm_plane_helper_update_primary,
> +	.disable_plane = drm_plane_helper_disable_primary,
> +	.destroy = drm_plane_helper_destroy,
> +};
> +
>  static void avivo_crtc_load_lut(struct drm_crtc *crtc)
>  {
>  	struct radeon_crtc *radeon_crtc = to_radeon_crtc(crtc);
> @@ -680,20 +692,31 @@ static const struct drm_crtc_funcs radeon_crtc_funcs = {
>  static void radeon_crtc_init(struct drm_device *dev, int index)
>  {
>  	struct radeon_device *rdev = dev->dev_private;
> +	struct drm_plane *primary;
>  	struct radeon_crtc *radeon_crtc;
> +	int ret;
>  
> -	radeon_crtc = kzalloc_obj(*radeon_crtc);
> -	if (radeon_crtc == NULL)
> +	primary = __drm_universal_plane_alloc(dev, sizeof(*primary), 0, 0,
> +					      &radeon_primary_plane_funcs,
> +					      radeon_primary_plane_formats,
> +					      ARRAY_SIZE(radeon_primary_plane_formats),
> +					      NULL, DRM_PLANE_TYPE_PRIMARY, NULL);

[Severity: High]
Does restricting radeon_primary_plane_formats[] to only two 32-bit formats
cause drm_mode_setcrtc() to reject other pixel formats supported by radeon?

Previously, drm_crtc_init() set primary->format_default = true, which made
drm_mode_setcrtc() skip the drm_plane_has_format() check and leave format
validation to the CRTC callbacks:

drivers/gpu/drm/drm_crtc.c:drm_mode_setcrtc() {
    ...
		if (!plane->format_default) {
			if (!drm_plane_has_format(plane, fb->format->format, fb->modifier)) {
				drm_dbg_kms(dev, "Invalid pixel format %p4cc, modifier 0x%llx\n",
					    &fb->format->format, fb->modifier);
				ret = -EINVAL;
				goto out;
			}
		}
    ...
}

Allocating the primary plane via __drm_universal_plane_alloc() leaves
primary->format_default as false (and follow-up commit "drm: Remove
drm_crtc_init()" removes format_default altogether). As a result,
drm_mode_setcrtc() now fails with -EINVAL for 8bpp, 15bpp, 16bpp, and 30bpp
framebuffers (such as C8, RGB565, XRGB1555, and XRGB2101010) that
dce4_crtc_do_set_base(), avivo_crtc_do_set_base(), and
radeon_crtc_do_set_base() handle.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110120326.538252-1-tzimmermann@suse.de?part=4