Re: [PATCH v2 1/2] drm/radeon: Inline drm_crtc_init()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/radeon: radeon_crtc_init() registers DCE4 plane formats on
  all ASICs
- [Low] drm/radeon: commit message inaccurately claims drm_crtc_init()
  formats are used
--

commit 7f6e83326a814c1dd5b8538076f37ab12152984e
Author: Thomas Zimmermann <tzimmermann@suse.de>

drm/radeon: Inline drm_crtc_init()

This commit inlines drm_crtc_init() into radeon_crtc_init(), allocating
a primary plane with an explicit format list and initializing the CRTC via
drm_crtc_init_with_planes().

> As in drm_crtc_init(), create a primary plane for the CRTC. Use the same
> formats as with drm_crtc_init().

[Severity: Low]
Is the commit message still accurate here after v2?  drm_crtc_init() uses
safe_modeset_formats, which only contains DRM_FORMAT_XRGB8888 and
DRM_FORMAT_ARGB8888, whereas v2 expanded radeon_primary_plane_formats[]
to 16 formats.

> diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
> index aac6733ddd82c..1a03c641d72fe 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,31 @@
>  #include "radeon.h"
>  #include "radeon_kms.h"
>  
> +static const u32 radeon_primary_plane_formats[] = {
> +	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_ARGB8888,
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_XRGB2101010,
> +	DRM_FORMAT_ARGB2101010,
> +	DRM_FORMAT_BGRX1010102,
> +	DRM_FORMAT_BGRA1010102,
> +	DRM_FORMAT_RGB565,
> +	DRM_FORMAT_XRGB1555,
> +	DRM_FORMAT_ARGB1555,
> +	DRM_FORMAT_BGRX5551,
> +	DRM_FORMAT_BGRA5551,
> +	DRM_FORMAT_XRGB4444,
> +	DRM_FORMAT_ARGB4444,
> +	DRM_FORMAT_C8,
> +};

[ ... ]

> @@ -680,20 +706,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 registering radeon_primary_plane_formats unconditionally on all
ASICs cause regressions on AVIVO (DCE1-DCE3) and pre-AVIVO hardware?

radeon_primary_plane_formats[] matches dce4_crtc_do_set_base(), whereas
drm_crtc_init() previously advertised only XRGB8888 and ARGB8888 on the
plane and set primary->format_default = true.

On AVIVO hardware (!ASIC_IS_DCE4(rdev)), avivo_crtc_do_set_base() does
not support DRM_FORMAT_ARGB1555, DRM_FORMAT_BGRX5551,
DRM_FORMAT_BGRA5551, DRM_FORMAT_BGRX1010102, or DRM_FORMAT_BGRA1010102.
When userspace selects one of these newly advertised formats,
avivo_crtc_do_set_base() pins target_fb's BO in VRAM before rejecting the
format and returning -EINVAL without unpinning rbo or old_fb:

drivers/gpu/drm/radeon/atombios_crtc.c:avivo_crtc_do_set_base() {
    ...
	r = radeon_bo_pin(rbo, RADEON_GEM_DOMAIN_VRAM, &fb_location);
	if (unlikely(r != 0)) {
		radeon_bo_unreserve(rbo);
		return -EINVAL;
	}
	radeon_bo_get_tiling_flags(rbo, &tiling_flags, NULL);
	radeon_bo_unreserve(rbo);

	switch (target_fb->format->format) {
    ...
	default:
		DRM_ERROR("Unsupported screen format %p4cc\n",
			  &target_fb->format->format);
		return -EINVAL;
	}
}

This permanently leaks the VRAM pin reference (and leaves scanout
unprogrammed during full modesets because atombios_crtc_mode_set()
ignores the return value of atombios_crtc_set_base()).

On pre-AVIVO hardware (!ASIC_IS_AVIVO(rdev)), radeon_crtc_do_set_base()
only switches on target_fb->format->cpp[0] * 8.  Advertising the 10-bpc,
BGR, 4444, and 5551 formats causes them to be misprogrammed as RGB565 or
XRGB8888, while omitting DRM_FORMAT_RGB888 (24-bpp) causes
drm_mode_setcrtc() to reject a format that pre-AVIVO CRTCs support now
that primary->format_default is false.

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