[PATCH v2] drm/radeon: Create the primary plane in the driver

[PATCH v2] drm/radeon: Create the primary plane in the driver

[oushixiong1025]

From: Shixiong Ou <oushixiong@kylinos.cn>

drm_crtc_init() creates the primary plane from a fixed format list
without a "pixel blend mode" property, so drm_mode_config_validate()
warns although the display engine does support per-pixel alpha on the
primary surface.

Replace it with a driver-owned primary plane advertising XRGB8888 and
ARGB8888, allocated by drmm_universal_plane_alloc(), with the blend
mode property set to the PREMULTI mode userspace has always assumed
when the property was absent.

Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
---
v1->v2:
  - Open-code DRM_PLANE_NON_ATOMIC_FUNCS without .destroy, which
    drmm_universal_plane_alloc() rejects
  - Use drmm_universal_plane_alloc() for automatic cleanup
  - Move the formats comment above the array

 drivers/gpu/drm/radeon/radeon_display.c | 38 ++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index aac6733ddd82..6427feb5a5c5 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -30,6 +30,7 @@
 
 #include <asm/div64.h>
 
+#include <drm/drm_blend.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_device.h>
 #include <drm/drm_drv.h>
@@ -38,6 +39,8 @@
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_modeset_helper.h>
+#include <drm/drm_plane.h>
+#include <drm/drm_plane_helper.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_vblank.h>
 #include <drm/radeon_drm.h>
@@ -664,6 +667,20 @@ radeon_crtc_set_config(struct drm_mode_set *set,
 	return ret;
 }
 
+/*
+ * The display engine programs an ARGB8888 surface format for both
+ * XRGB8888 and ARGB8888 framebuffers.
+ */
+static const uint32_t radeon_primary_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,
+};
+
 static const struct drm_crtc_funcs radeon_crtc_funcs = {
 	.cursor_set2 = radeon_crtc_cursor_set2,
 	.cursor_move = radeon_crtc_cursor_move,
@@ -681,6 +698,7 @@ static void radeon_crtc_init(struct drm_device *dev, int index)
 {
 	struct radeon_device *rdev = dev->dev_private;
 	struct radeon_crtc *radeon_crtc;
+	struct drm_plane *primary;
 
 	radeon_crtc = kzalloc_obj(*radeon_crtc);
 	if (radeon_crtc == NULL)
@@ -693,7 +711,20 @@ static void radeon_crtc_init(struct drm_device *dev, int index)
 		return;
 	}
 
-	drm_crtc_init(dev, &radeon_crtc->base, &radeon_crtc_funcs);
+	primary = drmm_universal_plane_alloc(dev, struct drm_plane, dev, 0,
+					     &radeon_primary_plane_funcs,
+					     radeon_primary_formats,
+					     ARRAY_SIZE(radeon_primary_formats),
+					     NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (IS_ERR(primary))
+		goto err_free_crtc;
+
+	if (drm_crtc_init_with_planes(dev, &radeon_crtc->base, primary, NULL,
+				      &radeon_crtc_funcs, NULL))
+		goto err_free_crtc;
+
+	drm_plane_create_blend_mode_property(primary,
+					     BIT(DRM_MODE_BLEND_PREMULTI));
 
 	drm_mode_crtc_set_gamma_size(&radeon_crtc->base, 256);
 	radeon_crtc->crtc_id = index;
@@ -713,6 +744,11 @@ static void radeon_crtc_init(struct drm_device *dev, int index)
 		radeon_atombios_init_crtc(dev, radeon_crtc);
 	else
 		radeon_legacy_init_crtc(dev, radeon_crtc);
+	return;
+
+err_free_crtc:
+	destroy_workqueue(radeon_crtc->flip_queue);
+	kfree(radeon_crtc);
 }
 
 static const char *encoder_names[38] = {
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

Re: [PATCH v2] drm/radeon: Create the primary plane in the driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Critical] Missing `.destroy` callback in `radeon_primary_plane_funcs` causes a NULL pointer dereference in `drm_mode_config_cleanup()` during driver unload or probe failure.
- [High] Freeing `radeon_crtc` on `drm_crtc_init_with_planes()` failure introduces a Use-After-Free due to a dangling list pointer in DRM core.

Pre-existing issues:
- [High] Unhandled `-ENOMEM` from `drm_mode_crtc_set_gamma_size` leads to guaranteed NULL pointer dereference in LUT loading functions.
- [High] Silent initialization failures in `radeon_crtc_init` leave unpopulated CRTC slots, leading to NULL pointer dereferences later.
--

commit e5375b01119a0846e47327447f621f702f5ebac8
Author: Shixiong Ou <oushixiong@kylinos.cn>

drm/radeon: Create the primary plane in the driver

This patch creates a driver-owned primary plane advertising XRGB8888 and
ARGB8888, allocated via drmm_universal_plane_alloc(). It sets a blend mode
property to prevent validation warnings that occur when falling back to
the legacy fixed format list in the DRM core.

> diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
> index aac6733ddd82c..6427feb5a5c57 100644
> --- a/drivers/gpu/drm/radeon/radeon_display.c
> +++ b/drivers/gpu/drm/radeon/radeon_display.c
[ ... ]
> @@ -664,6 +667,20 @@ radeon_crtc_set_config(struct drm_mode_set *set,
>  	return ret;
>  }
>  
> +/*
> + * The display engine programs an ARGB8888 surface format for both
> + * XRGB8888 and ARGB8888 framebuffers.
> + */
> +static const uint32_t radeon_primary_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,
> +};
> +

[Severity: Critical]
Does omitting the .destroy callback here lead to a NULL pointer dereference
on module unload or initialization failure?

While drmm_universal_plane_alloc() requires the destroy callback to be
omitted for managed cleanup, the legacy radeon driver explicitly calls
drm_mode_config_cleanup() in radeon_modeset_fini().

Because drm_mode_config_cleanup() iterates over dev->mode_config.plane_list
and unconditionally executes plane->funcs->destroy(plane), won't it attempt
to dereference a NULL function pointer when it encounters this newly added
primary plane?

>  static const struct drm_crtc_funcs radeon_crtc_funcs = {
>  	.cursor_set2 = radeon_crtc_cursor_set2,
>  	.cursor_move = radeon_crtc_cursor_move,
[ ... ]
> @@ -693,7 +711,20 @@ static void radeon_crtc_init(struct drm_device *dev, int index)
>  		return;
>  	}
>  
> -	drm_crtc_init(dev, &radeon_crtc->base, &radeon_crtc_funcs);
> +	primary = drmm_universal_plane_alloc(dev, struct drm_plane, dev, 0,
> +					     &radeon_primary_plane_funcs,
> +					     radeon_primary_formats,
> +					     ARRAY_SIZE(radeon_primary_formats),
> +					     NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> +	if (IS_ERR(primary))
> +		goto err_free_crtc;
> +

[Severity: High]
This is a pre-existing issue, but this patch introduces new failure paths
like this one that expose it.

If we jump to err_free_crtc and return early from radeon_crtc_init(), the
array slot rdev->mode_info.crtcs[index] is left unpopulated (NULL). The
caller, radeon_modeset_init(), does not check for failures and proceeds.

Will this lead to a NULL pointer dereference later when other paths (like
watermark or bandwidth updates) unconditionally access
rdev->mode_info.crtcs[i]->base.enabled?

> +	if (drm_crtc_init_with_planes(dev, &radeon_crtc->base, primary, NULL,
> +				      &radeon_crtc_funcs, NULL))
> +		goto err_free_crtc;
> +
> +	drm_plane_create_blend_mode_property(primary,
> +					     BIT(DRM_MODE_BLEND_PREMULTI));
>  
>  	drm_mode_crtc_set_gamma_size(&radeon_crtc->base, 256);

[Severity: High]
This isn't a bug introduced by this patch, but the return value of
drm_mode_crtc_set_gamma_size() is ignored here.

If drm_mode_crtc_set_gamma_size() returns -ENOMEM, it leaves
crtc->gamma_store as NULL. Later, in hardware programming paths like
dce4_crtc_load_lut(), the driver assigns:

    r = crtc->gamma_store;

and unconditionally iterates up to 256, dereferencing the NULL pointer.
Should the error return from drm_mode_crtc_set_gamma_size() be handled
to prevent this?

>  	radeon_crtc->crtc_id = index;
[ ... ]
> @@ -713,6 +744,11 @@ static void radeon_crtc_init(struct drm_device *dev, int index)
>  		radeon_atombios_init_crtc(dev, radeon_crtc);
>  	else
>  		radeon_legacy_init_crtc(dev, radeon_crtc);
> +	return;
> +
> +err_free_crtc:
> +	destroy_workqueue(radeon_crtc->flip_queue);
> +	kfree(radeon_crtc);
>  }

[Severity: High]
Can freeing radeon_crtc here cause a use-after-free regression by leaving a
dangling pointer in the DRM core?

Inside __drm_crtc_init_with_planes(), the CRTC is added to the DRM core's
dev->mode_config.crtc_list early via list_add_tail().

If a late initialization step within that function fails (such as
drm_crtc_crc_init()), the DRM core returns an error but fails to unlink
the CRTC from the list. If we immediately kfree(radeon_crtc) here, any
subsequent iteration over crtc_list (like during drm_mode_config_cleanup())
will access freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903071409.427010-1-oushixiong1025@163.com?part=1