[PATCH 0/5] drm/amdgpu,radeon: Inline drm_crtc_init()

[PATCH 0/5] drm/amdgpu,radeon: Inline drm_crtc_init()

[Thomas Zimmermann]

The helper drm_crtc_init() sets up a CRTC with a primary plane for
non-atomic mode setting. Only drivers for some AMD hardware still
use it.

Inline drm_crtc_init() into amdgpu and radeon and remove it from
DRM. All new code implements atomic mode setting, which does not
use drm_crtc_init().

DRM support for kernel panic is only implemented in amdgpu. Take
the panic helpers from drm_crtc_init() in amdgpu, but clean it up
in radeon.

With this series in place, DRM's helpers for drawing panic screens
can be further integrated with other drawing helpers, as outlined
in [1].

Tested on radeon with a Radeon HD 6570.

[1] https://lore.kernel.org/dri-devel/20261019154612.777074-1-tzimmermann@suse.de/

Thomas Zimmermann (5):
  drm/amdgpu: dce_v6_0: Inline drm_crtc_init()
  drm/amdgpu: dce_v8_0: Inline drm_crtc_init()
  drm/amdgpu: dce_v10_0: Inline drm_crtc_init()
  drm/radeon: Inline drm_crtc_init()
  drm: Remove drm_crtc_init()

 drivers/gpu/drm/amd/amdgpu/dce_v10_0.c  | 44 ++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/dce_v6_0.c   | 44 ++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/dce_v8_0.c   | 44 ++++++++++++++-
 drivers/gpu/drm/drm_crtc.c              | 26 +++------
 drivers/gpu/drm/drm_modeset_helper.c    | 74 -------------------------
 drivers/gpu/drm/radeon/radeon_display.c | 47 +++++++++++++---
 include/drm/drm_modeset_helper.h        |  3 -
 include/drm/drm_plane.h                 |  5 --
 8 files changed, 170 insertions(+), 117 deletions(-)

-- 
2.56.0

[PATCH 1/5] drm/amdgpu: dce_v6_0: Inline drm_crtc_init()

[Thomas Zimmermann]

Inline drm_crtc_init() into DCE v6 CRTC initialization. The helper is
only used by drivers for AMD hardware to implement legacy mode setting.

As in drm_crtc_init(), create a primary plane for the CRTC. Use the same
formats as with drm_crtc_init(). Format modifiers are still marked as
unsupported and handled internally by the plane. For the CRTC, call
drm_crtc_init_with_planes() for initialization. Clean up on errors.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

dce 6
---
 drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 44 +++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
index f0722bdaeab6..18c5d21be45e 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
@@ -27,6 +27,8 @@
 #include <drm/drm_fourcc.h>
 #include <drm/drm_modeset_helper.h>
 #include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_panic_helper.h>
+#include <drm/drm_plane_helper.h>
 #include <drm/drm_vblank.h>
 
 #include "amdgpu.h"
@@ -2607,6 +2609,18 @@ static const struct drm_crtc_helper_funcs dce_v6_0_crtc_helper_funcs = {
 	.get_scanout_position = amdgpu_crtc_get_scanout_position,
 };
 
+static const u32 dce_v6_0_drm_primary_plane_formats[] = {
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_ARGB8888,
+};
+
+static const struct drm_plane_funcs dce_v6_0_drm_primary_plane_funcs = {
+	.update_plane = drm_plane_helper_update_primary,
+	.disable_plane = drm_plane_helper_disable_primary,
+	.destroy = drm_plane_helper_destroy,
+	DRM_PANIC_PLANE_FUNCS,
+};
+
 static void dce_v6_0_panic_flush(struct drm_plane *plane)
 {
 	struct drm_framebuffer *fb;
@@ -2635,14 +2649,31 @@ static const struct drm_plane_helper_funcs dce_v6_0_drm_primary_plane_helper_fun
 
 static int dce_v6_0_crtc_init(struct amdgpu_device *adev, int index)
 {
+	struct drm_device *dev = adev_to_drm(adev);
+	struct drm_plane *primary;
 	struct amdgpu_crtc *amdgpu_crtc;
+	int ret;
+
+	/* possible_crtc's will be filled in later by crtc_init */
+	primary = __drm_universal_plane_alloc(dev, sizeof(*primary), 0, 0,
+					      &dce_v6_0_drm_primary_plane_funcs,
+					      dce_v6_0_drm_primary_plane_formats,
+					      ARRAY_SIZE(dce_v6_0_drm_primary_plane_formats),
+					      NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (IS_ERR(primary))
+		return PTR_ERR(primary);
 
 	amdgpu_crtc = kzalloc(sizeof(struct amdgpu_crtc) +
 			      (AMDGPUFB_CONN_LIMIT * sizeof(struct drm_connector *)), GFP_KERNEL);
-	if (amdgpu_crtc == NULL)
-		return -ENOMEM;
+	if (!amdgpu_crtc) {
+		ret = -ENOMEM;
+		goto err_drm_plane_cleanup;
+	}
 
-	drm_crtc_init(adev_to_drm(adev), &amdgpu_crtc->base, &dce_v6_0_crtc_funcs);
+	ret = drm_crtc_init_with_planes(dev, &amdgpu_crtc->base, primary, NULL,
+					&dce_v6_0_crtc_funcs, NULL);
+	if (ret)
+		goto err_kfree;
 
 	drm_mode_crtc_set_gamma_size(&amdgpu_crtc->base, 256);
 	amdgpu_crtc->crtc_id = index;
@@ -2663,6 +2694,13 @@ static int dce_v6_0_crtc_init(struct amdgpu_device *adev, int index)
 	drm_plane_helper_add(amdgpu_crtc->base.primary, &dce_v6_0_drm_primary_plane_helper_funcs);
 
 	return 0;
+
+err_kfree:
+	kfree(amdgpu_crtc);
+err_drm_plane_cleanup:
+	drm_plane_cleanup(primary);
+	kfree(primary);
+	return ret;
 }
 
 static int dce_v6_0_early_init(struct amdgpu_ip_block *ip_block)
-- 
2.56.0

[PATCH 2/5] drm/amdgpu: dce_v8_0: Inline drm_crtc_init()

[Thomas Zimmermann]

Inline drm_crtc_init() into DCE v8 CRTC initialization. The helper is
only used by drivers for AMD hardware to implement legacy mode setting.

As in drm_crtc_init(), create a primary plane for the CRTC. Use the same
formats as with drm_crtc_init(). Format modifiers are still marked as
unsupported and handled internally by the plane. For the CRTC, call
drm_crtc_init_with_planes() for initialization. Clean up on errors.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 44 +++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
index 3f5247314ede..ef96df27bc53 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
@@ -25,6 +25,8 @@
 #include <drm/drm_fourcc.h>
 #include <drm/drm_modeset_helper.h>
 #include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_panic_helper.h>
+#include <drm/drm_plane_helper.h>
 #include <drm/drm_vblank.h>
 
 #include "amdgpu.h"
@@ -2566,6 +2568,18 @@ static const struct drm_crtc_helper_funcs dce_v8_0_crtc_helper_funcs = {
 	.get_scanout_position = amdgpu_crtc_get_scanout_position,
 };
 
+static const u32 dce_v8_0_drm_primary_plane_formats[] = {
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_ARGB8888,
+};
+
+static const struct drm_plane_funcs dce_v8_0_drm_primary_plane_funcs = {
+	.update_plane = drm_plane_helper_update_primary,
+	.disable_plane = drm_plane_helper_disable_primary,
+	.destroy = drm_plane_helper_destroy,
+	DRM_PANIC_PLANE_FUNCS,
+};
+
 static void dce_v8_0_panic_flush(struct drm_plane *plane)
 {
 	struct drm_framebuffer *fb;
@@ -2593,14 +2607,31 @@ static const struct drm_plane_helper_funcs dce_v8_0_drm_primary_plane_helper_fun
 
 static int dce_v8_0_crtc_init(struct amdgpu_device *adev, int index)
 {
+	struct drm_device *dev = adev_to_drm(adev);
+	struct drm_plane *primary;
 	struct amdgpu_crtc *amdgpu_crtc;
+	int ret;
+
+	/* possible_crtc's will be filled in later by crtc_init */
+	primary = __drm_universal_plane_alloc(dev, sizeof(*primary), 0, 0,
+					      &dce_v8_0_drm_primary_plane_funcs,
+					      dce_v8_0_drm_primary_plane_formats,
+					      ARRAY_SIZE(dce_v8_0_drm_primary_plane_formats),
+					      NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (IS_ERR(primary))
+		return PTR_ERR(primary);
 
 	amdgpu_crtc = kzalloc(sizeof(struct amdgpu_crtc) +
 			      (AMDGPUFB_CONN_LIMIT * sizeof(struct drm_connector *)), GFP_KERNEL);
-	if (amdgpu_crtc == NULL)
-		return -ENOMEM;
+	if (!amdgpu_crtc) {
+		ret = -ENOMEM;
+		goto err_drm_plane_cleanup;
+	}
 
-	drm_crtc_init(adev_to_drm(adev), &amdgpu_crtc->base, &dce_v8_0_crtc_funcs);
+	ret = drm_crtc_init_with_planes(dev, &amdgpu_crtc->base, primary, NULL,
+					&dce_v8_0_crtc_funcs, NULL);
+	if (ret)
+		goto err_kfree;
 
 	drm_mode_crtc_set_gamma_size(&amdgpu_crtc->base, 256);
 	amdgpu_crtc->crtc_id = index;
@@ -2621,6 +2652,13 @@ static int dce_v8_0_crtc_init(struct amdgpu_device *adev, int index)
 	drm_plane_helper_add(amdgpu_crtc->base.primary, &dce_v8_0_drm_primary_plane_helper_funcs);
 
 	return 0;
+
+err_kfree:
+	kfree(amdgpu_crtc);
+err_drm_plane_cleanup:
+	drm_plane_cleanup(primary);
+	kfree(primary);
+	return ret;
 }
 
 static int dce_v8_0_early_init(struct amdgpu_ip_block *ip_block)
-- 
2.56.0

[PATCH 3/5] drm/amdgpu: dce_v10_0: Inline drm_crtc_init()

[Thomas Zimmermann]

Inline drm_crtc_init() into DCE v10 CRTC initialization. The helper is
only used by drivers for AMD hardware to implement legacy mode setting.

As in drm_crtc_init(), create a primary plane for the CRTC. Use the same
formats as with drm_crtc_init(). Format modifiers are still marked as
unsupported and handled internally by the plane. For the CRTC, call
drm_crtc_init_with_planes() for initialization. Clean up on errors.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 44 ++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index 05d7da888f49..0fe7f13cb8c1 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -25,6 +25,8 @@
 #include <drm/drm_fourcc.h>
 #include <drm/drm_modeset_helper.h>
 #include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_panic_helper.h>
+#include <drm/drm_plane_helper.h>
 #include <drm/drm_vblank.h>
 
 #include "amdgpu.h"
@@ -2639,6 +2641,18 @@ static const struct drm_crtc_helper_funcs dce_v10_0_crtc_helper_funcs = {
 	.get_scanout_position = amdgpu_crtc_get_scanout_position,
 };
 
+static const u32 dce_v10_0_drm_primary_plane_formats[] = {
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_ARGB8888,
+};
+
+static const struct drm_plane_funcs dce_v10_0_drm_primary_plane_funcs = {
+	.update_plane = drm_plane_helper_update_primary,
+	.disable_plane = drm_plane_helper_disable_primary,
+	.destroy = drm_plane_helper_destroy,
+	DRM_PANIC_PLANE_FUNCS,
+};
+
 static void dce_v10_0_panic_flush(struct drm_plane *plane)
 {
 	struct drm_framebuffer *fb;
@@ -2667,14 +2681,31 @@ static const struct drm_plane_helper_funcs dce_v10_0_drm_primary_plane_helper_fu
 
 static int dce_v10_0_crtc_init(struct amdgpu_device *adev, int index)
 {
+	struct drm_device *dev = adev_to_drm(adev);
+	struct drm_plane *primary;
 	struct amdgpu_crtc *amdgpu_crtc;
+	int ret;
+
+	/* possible_crtc's will be filled in later by crtc_init */
+	primary = __drm_universal_plane_alloc(dev, sizeof(*primary), 0, 0,
+					      &dce_v10_0_drm_primary_plane_funcs,
+					      dce_v10_0_drm_primary_plane_formats,
+					      ARRAY_SIZE(dce_v10_0_drm_primary_plane_formats),
+					      NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (IS_ERR(primary))
+		return PTR_ERR(primary);
 
 	amdgpu_crtc = kzalloc(sizeof(struct amdgpu_crtc) +
 			      (AMDGPUFB_CONN_LIMIT * sizeof(struct drm_connector *)), GFP_KERNEL);
-	if (amdgpu_crtc == NULL)
-		return -ENOMEM;
+	if (!amdgpu_crtc) {
+		ret = -ENOMEM;
+		goto err_drm_plane_cleanup;
+	}
 
-	drm_crtc_init(adev_to_drm(adev), &amdgpu_crtc->base, &dce_v10_0_crtc_funcs);
+	ret = drm_crtc_init_with_planes(dev, &amdgpu_crtc->base, primary, NULL,
+					&dce_v10_0_crtc_funcs, NULL);
+	if (ret)
+		goto err_kfree;
 
 	drm_mode_crtc_set_gamma_size(&amdgpu_crtc->base, 256);
 	amdgpu_crtc->crtc_id = index;
@@ -2715,6 +2746,13 @@ static int dce_v10_0_crtc_init(struct amdgpu_device *adev, int index)
 	drm_plane_helper_add(amdgpu_crtc->base.primary, &dce_v10_0_drm_primary_plane_helper_funcs);
 
 	return 0;
+
+err_kfree:
+	kfree(amdgpu_crtc);
+err_drm_plane_cleanup:
+	drm_plane_cleanup(primary);
+	kfree(primary);
+	return ret;
 }
 
 static int dce_v10_0_early_init(struct amdgpu_ip_block *ip_block)
-- 
2.56.0

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

[Thomas Zimmermann]

Inline drm_crtc_init() into radeon CRTC initialization. The helper is
only used by drivers for AMD hardware to implement legacy mode setting.

As in drm_crtc_init(), create a primary plane for the CRTC. Use the same
formats as with drm_crtc_init(). Format modifiers are still marked as
unsupported and handled internally by the plane. For the CRTC, call
drm_crtc_init_with_planes() for initialization. Clean up on errors.

Radeon does not support DRM panic helpers. Therefore do not initialize
the primary plane's panic-helper callbacks.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/radeon/radeon_display.c | 47 +++++++++++++++++++++----
 1 file changed, 40 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index aac6733ddd82..4b784df4679f 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);
+	if (IS_ERR(primary))
 		return;
 
+	radeon_crtc = kzalloc_obj(*radeon_crtc);
+	if (!radeon_crtc)
+		goto err_drm_plane_cleanup;
+
 	radeon_crtc->flip_queue = alloc_workqueue("radeon-crtc",
 						  WQ_HIGHPRI | WQ_PERCPU, 0);
-	if (!radeon_crtc->flip_queue) {
-		kfree(radeon_crtc);
-		return;
-	}
+	if (!radeon_crtc->flip_queue)
+		goto err_kfree;
 
-	drm_crtc_init(dev, &radeon_crtc->base, &radeon_crtc_funcs);
+	ret = drm_crtc_init_with_planes(dev, &radeon_crtc->base, primary, NULL,
+					&radeon_crtc_funcs, NULL);
+	if (ret)
+		goto err_destroy_workqueue;
 
 	drm_mode_crtc_set_gamma_size(&radeon_crtc->base, 256);
 	radeon_crtc->crtc_id = index;
@@ -713,6 +736,16 @@ 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_destroy_workqueue:
+	destroy_workqueue(radeon_crtc->flip_queue);
+err_kfree:
+	kfree(radeon_crtc);
+err_drm_plane_cleanup:
+	drm_plane_cleanup(primary);
+	kfree(primary);
 }
 
 static const char *encoder_names[38] = {
-- 
2.56.0

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

[sashiko-bot]

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

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

[Thomas Zimmermann]

Hi

Am 10.11.26 um 13:18 schrieb 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.

That's likely the case and amdgpu might also be affected. I found the 
old validation logic confusing. will be fixed in the next rev.


>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)

[PATCH 5/5] drm: Remove drm_crtc_init()

[Thomas Zimmermann]

There are no remaining users of drm_crtc_init(), so remove the helper
and the internal data structures for the primary plane that the helper
creates.

Also remove the format_default flag from struct drm_plane. Without
drm_crtc_init(), no drivers set it to true.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_crtc.c           | 26 +++-------
 drivers/gpu/drm/drm_modeset_helper.c | 74 ----------------------------
 include/drm/drm_modeset_helper.h     |  3 --
 include/drm/drm_plane.h              |  5 --
 4 files changed, 7 insertions(+), 101 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 5295ec122e0d..b7cf18b79b1d 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -334,10 +334,7 @@ static int __drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *
  * @funcs: callbacks for the new CRTC
  * @name: printf style format string for the CRTC name, or NULL for default name
  *
- * Inits a new object created as base part of a driver crtc object. Drivers
- * should use this function instead of drm_crtc_init(), which is only provided
- * for backwards compatibility with drivers which do not yet support universal
- * planes).
+ * Inits a new object created as base part of a driver crtc object.
  * The &drm_crtc_funcs.destroy hook should call drm_crtc_cleanup() and kfree()
  * the crtc structure. The crtc structure should not be allocated with
  * devm_kzalloc().
@@ -417,10 +414,7 @@ static int __drmm_crtc_init_with_planes(struct drm_device *dev,
  * @funcs: callbacks for the new CRTC
  * @name: printf style format string for the CRTC name, or NULL for default name
  *
- * Inits a new object created as base part of a driver crtc object. Drivers
- * should use this function instead of drm_crtc_init(), which is only provided
- * for backwards compatibility with drivers which do not yet support universal
- * planes).
+ * Inits a new object created as base part of a driver crtc object.
  *
  * Cleanup is automatically handled through registering
  * drmm_crtc_cleanup() with drmm_add_action(). The crtc structure should
@@ -806,18 +800,12 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 
 		/*
 		 * Check whether the primary plane supports the fb pixel format.
-		 * Drivers not implementing the universal planes API use a
-		 * default formats list provided by the DRM core which doesn't
-		 * match real hardware capabilities. Skip the check in that
-		 * case.
 		 */
-		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;
-			}
+		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;
 		}
 
 		ret = drm_crtc_check_viewport(crtc, crtc_req->x, crtc_req->y,
diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
index cca623eb23e5..d462b347bde6 100644
--- a/drivers/gpu/drm/drm_modeset_helper.c
+++ b/drivers/gpu/drm/drm_modeset_helper.c
@@ -101,80 +101,6 @@ void drm_helper_mode_fill_fb_struct(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_helper_mode_fill_fb_struct);
 
-/*
- * This is the minimal list of formats that seem to be safe for modeset use
- * with all current DRM drivers.  Most hardware can actually support more
- * formats than this and drivers may specify a more accurate list when
- * creating the primary plane.
- */
-static const uint32_t safe_modeset_formats[] = {
-	DRM_FORMAT_XRGB8888,
-	DRM_FORMAT_ARGB8888,
-};
-
-static const struct drm_plane_funcs primary_plane_funcs = {
-	DRM_PLANE_NON_ATOMIC_FUNCS,
-	DRM_PANIC_PLANE_FUNCS,
-};
-
-/**
- * drm_crtc_init - Legacy CRTC initialization function
- * @dev: DRM device
- * @crtc: CRTC object to init
- * @funcs: callbacks for the new CRTC
- *
- * Initialize a CRTC object with a default helper-provided primary plane and no
- * cursor plane.
- *
- * Note that we make some assumptions about hardware limitations that may not be
- * true for all hardware:
- *
- * 1. Primary plane cannot be repositioned.
- * 2. Primary plane cannot be scaled.
- * 3. Primary plane must cover the entire CRTC.
- * 4. Subpixel positioning is not supported.
- * 5. The primary plane must always be on if the CRTC is enabled.
- *
- * This is purely a backwards compatibility helper for old drivers. Drivers
- * should instead implement their own primary plane. Atomic drivers must do so.
- *
- * Returns:
- * Zero on success, error code on failure.
- */
-int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
-		  const struct drm_crtc_funcs *funcs)
-{
-	struct drm_plane *primary;
-	int ret;
-
-	/* possible_crtc's will be filled in later by crtc_init */
-	primary = __drm_universal_plane_alloc(dev, sizeof(*primary), 0, 0,
-					      &primary_plane_funcs,
-					      safe_modeset_formats,
-					      ARRAY_SIZE(safe_modeset_formats),
-					      NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
-	if (IS_ERR(primary))
-		return PTR_ERR(primary);
-
-	/*
-	 * Remove the format_default field from drm_plane when dropping
-	 * this helper.
-	 */
-	primary->format_default = true;
-
-	ret = drm_crtc_init_with_planes(dev, crtc, primary, NULL, funcs, NULL);
-	if (ret)
-		goto err_drm_plane_cleanup;
-
-	return 0;
-
-err_drm_plane_cleanup:
-	drm_plane_cleanup(primary);
-	kfree(primary);
-	return ret;
-}
-EXPORT_SYMBOL(drm_crtc_init);
-
 /**
  * drm_mode_config_helper_suspend - Modeset suspend helper
  * @dev: DRM device
diff --git a/include/drm/drm_modeset_helper.h b/include/drm/drm_modeset_helper.h
index 7e3d4c5a7f66..6b9421aee536 100644
--- a/include/drm/drm_modeset_helper.h
+++ b/include/drm/drm_modeset_helper.h
@@ -37,9 +37,6 @@ void drm_helper_mode_fill_fb_struct(struct drm_device *dev,
 				    const struct drm_format_info *info,
 				    const struct drm_mode_fb_cmd2 *mode_cmd);
 
-int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
-		  const struct drm_crtc_funcs *funcs);
-
 int drm_mode_config_helper_suspend(struct drm_device *dev);
 int drm_mode_config_helper_resume(struct drm_device *dev);
 
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 410d7b02b77a..3f6a1408cf5e 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -738,11 +738,6 @@ struct drm_plane {
 	uint32_t *format_types;
 	/** @format_count: Size of the array pointed at by @format_types. */
 	unsigned int format_count;
-	/**
-	 * @format_default: driver hasn't supplied supported formats for the
-	 * plane. Used by the non-atomic driver compatibility wrapper only.
-	 */
-	bool format_default;
 
 	/** @modifiers: array of modifiers supported by this plane */
 	uint64_t *modifiers;
-- 
2.56.0

Re: [PATCH 0/5] drm/amdgpu,radeon: Inline drm_crtc_init()

[Maxime Ripard]

On Tue, 10 Nov 2026 12:53:26 +0100, Thomas Zimmermann wrote:
> The helper drm_crtc_init() sets up a CRTC with a primary plane for
> non-atomic mode setting. Only drivers for some AMD hardware still
> use it.
> 
> Inline drm_crtc_init() into amdgpu and radeon and remove it from
> 
> [ ... ]

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

Re: [PATCH 0/5] drm/amdgpu,radeon: Inline drm_crtc_init()

[Alex Deucher]

On Tue, Nov 10, 2026 at 7:11 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> The helper drm_crtc_init() sets up a CRTC with a primary plane for
> non-atomic mode setting. Only drivers for some AMD hardware still
> use it.
>
> Inline drm_crtc_init() into amdgpu and radeon and remove it from
> DRM. All new code implements atomic mode setting, which does not
> use drm_crtc_init().
>

I'd like to drop all of the old amdgpu non-DC code since DC is now the
default, and we have an LTS kernel now.  I'm planning to send these
out in the near future:
https://gitlab.freedesktop.org/agd5f/linux/-/commits/remove-non-dc-display?ref_type=heads
So that just leaves radeon.

Alex

> DRM support for kernel panic is only implemented in amdgpu. Take
> the panic helpers from drm_crtc_init() in amdgpu, but clean it up
> in radeon.
>
> With this series in place, DRM's helpers for drawing panic screens
> can be further integrated with other drawing helpers, as outlined
> in [1].
>
> Tested on radeon with a Radeon HD 6570.
>
> [1] https://lore.kernel.org/dri-devel/20261019154612.777074-1-tzimmermann@suse.de/
>
> Thomas Zimmermann (5):
>   drm/amdgpu: dce_v6_0: Inline drm_crtc_init()
>   drm/amdgpu: dce_v8_0: Inline drm_crtc_init()
>   drm/amdgpu: dce_v10_0: Inline drm_crtc_init()
>   drm/radeon: Inline drm_crtc_init()
>   drm: Remove drm_crtc_init()
>
>  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c  | 44 ++++++++++++++-
>  drivers/gpu/drm/amd/amdgpu/dce_v6_0.c   | 44 ++++++++++++++-
>  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c   | 44 ++++++++++++++-
>  drivers/gpu/drm/drm_crtc.c              | 26 +++------
>  drivers/gpu/drm/drm_modeset_helper.c    | 74 -------------------------
>  drivers/gpu/drm/radeon/radeon_display.c | 47 +++++++++++++---
>  include/drm/drm_modeset_helper.h        |  3 -
>  include/drm/drm_plane.h                 |  5 --
>  8 files changed, 170 insertions(+), 117 deletions(-)
>
> --
> 2.56.0
>

Re: [PATCH 0/5] drm/amdgpu,radeon: Inline drm_crtc_init()

[Thomas Zimmermann]

Hi

Am 10.11.26 um 22:49 schrieb Alex Deucher:
> On Tue, Nov 10, 2026 at 7:11 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> The helper drm_crtc_init() sets up a CRTC with a primary plane for
>> non-atomic mode setting. Only drivers for some AMD hardware still
>> use it.
>>
>> Inline drm_crtc_init() into amdgpu and radeon and remove it from
>> DRM. All new code implements atomic mode setting, which does not
>> use drm_crtc_init().
>>
> I'd like to drop all of the old amdgpu non-DC code since DC is now the
> default, and we have an LTS kernel now.  I'm planning to send these
> out in the near future:
> https://gitlab.freedesktop.org/agd5f/linux/-/commits/remove-non-dc-display?ref_type=heads

That's even better. :) Do you have an ETA for this work? Resolving 
drm_crtc_init() is required for that series I referred to at [1].


> So that just leaves radeon.

I'll fix the Sashiko report and send this out in a separate patch.

Best regards
Thomas

>
> Alex
>
>> DRM support for kernel panic is only implemented in amdgpu. Take
>> the panic helpers from drm_crtc_init() in amdgpu, but clean it up
>> in radeon.
>>
>> With this series in place, DRM's helpers for drawing panic screens
>> can be further integrated with other drawing helpers, as outlined
>> in [1].
>>
>> Tested on radeon with a Radeon HD 6570.
>>
>> [1] https://lore.kernel.org/dri-devel/20261019154612.777074-1-tzimmermann@suse.de/
>>
>> Thomas Zimmermann (5):
>>    drm/amdgpu: dce_v6_0: Inline drm_crtc_init()
>>    drm/amdgpu: dce_v8_0: Inline drm_crtc_init()
>>    drm/amdgpu: dce_v10_0: Inline drm_crtc_init()
>>    drm/radeon: Inline drm_crtc_init()
>>    drm: Remove drm_crtc_init()
>>
>>   drivers/gpu/drm/amd/amdgpu/dce_v10_0.c  | 44 ++++++++++++++-
>>   drivers/gpu/drm/amd/amdgpu/dce_v6_0.c   | 44 ++++++++++++++-
>>   drivers/gpu/drm/amd/amdgpu/dce_v8_0.c   | 44 ++++++++++++++-
>>   drivers/gpu/drm/drm_crtc.c              | 26 +++------
>>   drivers/gpu/drm/drm_modeset_helper.c    | 74 -------------------------
>>   drivers/gpu/drm/radeon/radeon_display.c | 47 +++++++++++++---
>>   include/drm/drm_modeset_helper.h        |  3 -
>>   include/drm/drm_plane.h                 |  5 --
>>   8 files changed, 170 insertions(+), 117 deletions(-)
>>
>> --
>> 2.56.0
>>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)

Re: [PATCH 0/5] drm/amdgpu,radeon: Inline drm_crtc_init()

[Alex Deucher]

On Wed, Nov 11, 2026 at 4:35 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 10.11.26 um 22:49 schrieb Alex Deucher:
> > On Tue, Nov 10, 2026 at 7:11 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >> The helper drm_crtc_init() sets up a CRTC with a primary plane for
> >> non-atomic mode setting. Only drivers for some AMD hardware still
> >> use it.
> >>
> >> Inline drm_crtc_init() into amdgpu and radeon and remove it from
> >> DRM. All new code implements atomic mode setting, which does not
> >> use drm_crtc_init().
> >>
> > I'd like to drop all of the old amdgpu non-DC code since DC is now the
> > default, and we have an LTS kernel now.  I'm planning to send these
> > out in the near future:
> > https://gitlab.freedesktop.org/agd5f/linux/-/commits/remove-non-dc-display?ref_type=heads
>
> That's even better. :) Do you have an ETA for this work? Resolving
> drm_crtc_init() is required for that series I referred to at [1].

Just sent them out.

Thanks,

Alex

>
>
> > So that just leaves radeon.
>
> I'll fix the Sashiko report and send this out in a separate patch.
>
> Best regards
> Thomas
>
> >
> > Alex
> >
> >> DRM support for kernel panic is only implemented in amdgpu. Take
> >> the panic helpers from drm_crtc_init() in amdgpu, but clean it up
> >> in radeon.
> >>
> >> With this series in place, DRM's helpers for drawing panic screens
> >> can be further integrated with other drawing helpers, as outlined
> >> in [1].
> >>
> >> Tested on radeon with a Radeon HD 6570.
> >>
> >> [1] https://lore.kernel.org/dri-devel/20261019154612.777074-1-tzimmermann@suse.de/
> >>
> >> Thomas Zimmermann (5):
> >>    drm/amdgpu: dce_v6_0: Inline drm_crtc_init()
> >>    drm/amdgpu: dce_v8_0: Inline drm_crtc_init()
> >>    drm/amdgpu: dce_v10_0: Inline drm_crtc_init()
> >>    drm/radeon: Inline drm_crtc_init()
> >>    drm: Remove drm_crtc_init()
> >>
> >>   drivers/gpu/drm/amd/amdgpu/dce_v10_0.c  | 44 ++++++++++++++-
> >>   drivers/gpu/drm/amd/amdgpu/dce_v6_0.c   | 44 ++++++++++++++-
> >>   drivers/gpu/drm/amd/amdgpu/dce_v8_0.c   | 44 ++++++++++++++-
> >>   drivers/gpu/drm/drm_crtc.c              | 26 +++------
> >>   drivers/gpu/drm/drm_modeset_helper.c    | 74 -------------------------
> >>   drivers/gpu/drm/radeon/radeon_display.c | 47 +++++++++++++---
> >>   include/drm/drm_modeset_helper.h        |  3 -
> >>   include/drm/drm_plane.h                 |  5 --
> >>   8 files changed, 170 insertions(+), 117 deletions(-)
> >>
> >> --
> >> 2.56.0
> >>
>
> --
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
> GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)
>
>