diff mbox series

drm/sun4i: Remove dependency on DRM simple helpers

Message ID 20260623-sun4i_simple-v1-1-fa6696df1fad@gmail.com (mailing list archive)
State New
Headers
Series drm/sun4i: Remove dependency on DRM simple helpers |

Commit Message

Diogo June 23, 2026, 8:34 p.m. UTC 
Simple KMS helper are deprecated since they only add an intermediate
layer between drivers and the atomic modesetting.
This patch removes the dependency on drm simple helpers from sun4i
DRM drivers.

Signed-off-by: Diogo Silva <diogompaissilva@gmail.com>
---
 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c |  9 ++++++---
 drivers/gpu/drm/sun4i/sun4i_lvds.c     | 10 +++++++---
 drivers/gpu/drm/sun4i/sun4i_rgb.c      | 10 +++++++---
 drivers/gpu/drm/sun4i/sun4i_tv.c       | 10 +++++++---
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 10 +++++++---
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c  |  8 ++++++--
 6 files changed, 40 insertions(+), 17 deletions(-)


---
base-commit: e2a6e04d045a56c6dc61e1eb9e2a6d7cf86512cd
change-id: 20260623-sun4i_simple-25c68885e359

Best regards,

Comments

sashiko-bot@kernel.org June 23, 2026, 9:23 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the return value of `drm_encoder_init()` is ignored in `sun8i_dw_hdmi_bind()`, which leads to a potential kernel panic on the error path.
--

commit 2d6b8f73fd3be1e78f02ca32197b62b5da0fb174
Author: Diogo Silva <diogompaissilva@gmail.com>

drm/sun4i: Remove dependency on DRM simple helpers

Simple KMS helper are deprecated since they only add an intermediate layer
between drivers and the atomic modesetting. This patch removes the dependency
on drm simple helpers from sun4i DRM drivers.

> diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> index 96532709c2a7e..9694ad142a7de 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
[ ... ]
> @@ -180,7 +183,8 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
>  		goto err_disable_clk_tmds;
>  
>  	drm_encoder_helper_add(encoder, &sun8i_dw_hdmi_encoder_helper_funcs);
> -	drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
> +	drm_encoder_init(drm, encoder, &sun8i_dw_hdmi_encoder_funcs,
> +			 DRM_MODE_ENCODER_TMDS, NULL);

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

If drm_encoder_init() fails, the encoder remains uninitialized. If a
subsequent initialization step like dw_hdmi_bind() then fails, the
execution jumps to the cleanup_encoder error path:

    cleanup_encoder:
            drm_encoder_cleanup(encoder);

Could calling drm_encoder_cleanup() on an uninitialized encoder cause a
kernel panic due to dereferencing NULL pointers (such as encoder->dev or a
zero-initialized list_head)? Should the return code of drm_encoder_init()
be checked and handled here?

>  
>  	plat_data->mode_valid = hdmi->quirks->mode_valid;
>  	plat_data->use_drm_infoframe = hdmi->quirks->use_drm_infoframe;
Jernej Škrabec June 27, 2026, 6:55 a.m. UTC | #2 
Dne torek, 23. junij 2026 ob 22:34:40 Srednjeevropski poletni čas je Diogo Silva napisal(a):
> Simple KMS helper are deprecated since they only add an intermediate
> layer between drivers and the atomic modesetting.
> This patch removes the dependency on drm simple helpers from sun4i
> DRM drivers.
> 
> Signed-off-by: Diogo Silva <diogompaissilva@gmail.com>

This doesn't look as useful change. Is this tree-wide attempt? If so, it
should be done in one go in one patch series, coordinated with DRM
maintainers.

Best regards,
Jernej
Diogo June 27, 2026, 1:07 p.m. UTC | #3 
Hey,

I saw the task to open-code the drm_simple_encoder_init in the DRM tasks, and thought that sending individual patches for each platform would be the easiest.
Thomas Zimmermann, is it ok if I do all drivers at once as a patch series?

BR,
Diogo

On 27 June 2026 08:55:04 CEST, "Jernej Škrabec" <jernej.skrabec@gmail.com> wrote:
>Dne torek, 23. junij 2026 ob 22:34:40 Srednjeevropski poletni čas je Diogo Silva napisal(a):
>> Simple KMS helper are deprecated since they only add an intermediate
>> layer between drivers and the atomic modesetting.
>> This patch removes the dependency on drm simple helpers from sun4i
>> DRM drivers.
>> 
>> Signed-off-by: Diogo Silva <diogompaissilva@gmail.com>
>
>This doesn't look as useful change. Is this tree-wide attempt? If so, it
>should be done in one go in one patch series, coordinated with DRM
>maintainers.
>
>Best regards,
>Jernej
>
>
Thomas Zimmermann June 29, 2026, 6:51 a.m. UTC | #4 
Hi

Am 27.06.26 um 08:55 schrieb Jernej Škrabec:
> Dne torek, 23. junij 2026 ob 22:34:40 Srednjeevropski poletni čas je Diogo Silva napisal(a):
>> Simple KMS helper are deprecated since they only add an intermediate
>> layer between drivers and the atomic modesetting.
>> This patch removes the dependency on drm simple helpers from sun4i
>> DRM drivers.
>>
>> Signed-off-by: Diogo Silva <diogompaissilva@gmail.com>
> This doesn't look as useful change. Is this tree-wide attempt? If so, it
> should be done in one go in one patch series, coordinated with DRM
> maintainers.

Yes, the interface is supposed to go away because it is fairly useless.  
It's a low-effort task for beginners. Hence many people send out such 
clean-up patches for the various drivers.  If the change is acceptable 
to you, it would be appreciated if you could merge it into the driver.

Best regards
Thomas

>
> Best regards,
> Jernej
>
>
Jernej Škrabec July 2, 2026, 2:39 p.m. UTC | #5 
Dne ponedeljek, 29. junij 2026 ob 08:51:52 Srednjeevropski poletni čas je Thomas Zimmermann napisal(a):
> Hi
> 
> Am 27.06.26 um 08:55 schrieb Jernej Škrabec:
> > Dne torek, 23. junij 2026 ob 22:34:40 Srednjeevropski poletni čas je Diogo Silva napisal(a):
> >> Simple KMS helper are deprecated since they only add an intermediate
> >> layer between drivers and the atomic modesetting.
> >> This patch removes the dependency on drm simple helpers from sun4i
> >> DRM drivers.
> >>
> >> Signed-off-by: Diogo Silva <diogompaissilva@gmail.com>
> > This doesn't look as useful change. Is this tree-wide attempt? If so, it
> > should be done in one go in one patch series, coordinated with DRM
> > maintainers.
> 
> Yes, the interface is supposed to go away because it is fairly useless.  
> It's a low-effort task for beginners. Hence many people send out such 
> clean-up patches for the various drivers.  If the change is acceptable 
> to you, it would be appreciated if you could merge it into the driver.

Ok.

Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej
Chen-Yu Tsai July 6, 2026, 4:50 p.m. UTC | #6 
On Tue, 23 Jun 2026 22:34:40 +0200, Diogo Silva wrote:
> Simple KMS helper are deprecated since they only add an intermediate
> layer between drivers and the atomic modesetting.
> This patch removes the dependency on drm simple helpers from sun4i
> DRM drivers.
> 
> 

Applied to drm-misc-next in drm-misc, thanks!

[1/1] drm/sun4i: Remove dependency on DRM simple helpers
      commit: 34d85c43b6cb3abec03d6f9d8128fb28a7e2bfdf

Best regards,
diff mbox series

Patch

diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
index 8f64464621c9..e99f52ebb26f 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
@@ -24,7 +24,6 @@ 
 #include <drm/drm_panel.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
 
 #include <drm/display/drm_hdmi_helper.h>
 #include <drm/display/drm_hdmi_state_helper.h>
@@ -172,6 +171,10 @@  static void sun4i_hdmi_enable(struct drm_encoder *encoder,
 	writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
 }
 
+static const struct drm_encoder_funcs sun4i_hdmi_funcs = {
+	.destroy = drm_encoder_cleanup,
+};
+
 static const struct drm_encoder_helper_funcs sun4i_hdmi_helper_funcs = {
 	.atomic_disable	= sun4i_hdmi_disable,
 	.atomic_enable	= sun4i_hdmi_enable,
@@ -624,8 +627,8 @@  static int sun4i_hdmi_bind(struct device *dev, struct device *master,
 
 	drm_encoder_helper_add(&hdmi->encoder,
 			       &sun4i_hdmi_helper_funcs);
-	ret = drm_simple_encoder_init(drm, &hdmi->encoder,
-				      DRM_MODE_ENCODER_TMDS);
+	ret = drm_encoder_init(drm, &hdmi->encoder, &sun4i_hdmi_funcs,
+			       DRM_MODE_ENCODER_TMDS, NULL);
 	if (ret) {
 		dev_err(dev, "Couldn't initialise the HDMI encoder\n");
 		goto err_put_ddc_i2c;
diff --git a/drivers/gpu/drm/sun4i/sun4i_lvds.c b/drivers/gpu/drm/sun4i/sun4i_lvds.c
index 6716e895ae8a..35a3f987c37a 100644
--- a/drivers/gpu/drm/sun4i/sun4i_lvds.c
+++ b/drivers/gpu/drm/sun4i/sun4i_lvds.c
@@ -8,11 +8,11 @@ 
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
+#include <drm/drm_encoder.h>
 #include <drm/drm_of.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
 
 #include "sun4i_crtc.h"
 #include "sun4i_tcon.h"
@@ -89,6 +89,10 @@  static void sun4i_lvds_encoder_disable(struct drm_encoder *encoder)
 	}
 }
 
+static const struct drm_encoder_funcs sun4i_lvds_enc_funcs = {
+	.destroy = drm_encoder_cleanup,
+};
+
 static const struct drm_encoder_helper_funcs sun4i_lvds_enc_helper_funcs = {
 	.disable	= sun4i_lvds_encoder_disable,
 	.enable		= sun4i_lvds_encoder_enable,
@@ -115,8 +119,8 @@  int sun4i_lvds_init(struct drm_device *drm, struct sun4i_tcon *tcon)
 
 	drm_encoder_helper_add(&lvds->encoder,
 			       &sun4i_lvds_enc_helper_funcs);
-	ret = drm_simple_encoder_init(drm, &lvds->encoder,
-				      DRM_MODE_ENCODER_LVDS);
+	ret = drm_encoder_init(drm, &lvds->encoder, &sun4i_lvds_enc_funcs,
+			       DRM_MODE_ENCODER_LVDS, NULL);
 	if (ret) {
 		dev_err(drm->dev, "Couldn't initialise the lvds encoder\n");
 		goto err_out;
diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index dfb6acc42f02..9c3fbf1b949e 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
@@ -10,11 +10,11 @@ 
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
+#include <drm/drm_encoder.h>
 #include <drm/drm_of.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
 
 #include "sun4i_crtc.h"
 #include "sun4i_tcon.h"
@@ -180,6 +180,10 @@  static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder)
 	}
 }
 
+static const struct drm_encoder_funcs sun4i_rgb_enc_funcs = {
+	.destroy = drm_encoder_cleanup,
+};
+
 static const struct drm_encoder_helper_funcs sun4i_rgb_enc_helper_funcs = {
 	.disable	= sun4i_rgb_encoder_disable,
 	.enable		= sun4i_rgb_encoder_enable,
@@ -207,8 +211,8 @@  int sun4i_rgb_init(struct drm_device *drm, struct sun4i_tcon *tcon)
 
 	drm_encoder_helper_add(&rgb->encoder,
 			       &sun4i_rgb_enc_helper_funcs);
-	ret = drm_simple_encoder_init(drm, &rgb->encoder,
-				      DRM_MODE_ENCODER_NONE);
+	ret = drm_encoder_init(drm, &rgb->encoder, &sun4i_rgb_enc_funcs,
+			       DRM_MODE_ENCODER_NONE, NULL);
 	if (ret) {
 		dev_err(drm->dev, "Couldn't initialise the rgb encoder\n");
 		goto err_out;
diff --git a/drivers/gpu/drm/sun4i/sun4i_tv.c b/drivers/gpu/drm/sun4i/sun4i_tv.c
index 814b77f278f6..ce22f1662c0c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tv.c
@@ -16,11 +16,11 @@ 
 
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_encoder.h>
 #include <drm/drm_of.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
 
 #include "sun4i_crtc.h"
 #include "sun4i_drv.h"
@@ -391,6 +391,10 @@  static void sun4i_tv_enable(struct drm_encoder *encoder,
 			   SUN4I_TVE_EN_ENABLE);
 }
 
+static const struct drm_encoder_funcs sun4i_tv_funcs = {
+	.destroy = drm_encoder_cleanup,
+};
+
 static const struct drm_encoder_helper_funcs sun4i_tv_helper_funcs = {
 	.atomic_disable	= sun4i_tv_disable,
 	.atomic_enable	= sun4i_tv_enable,
@@ -474,8 +478,8 @@  static int sun4i_tv_bind(struct device *dev, struct device *master,
 
 	drm_encoder_helper_add(&tv->encoder,
 			       &sun4i_tv_helper_funcs);
-	ret = drm_simple_encoder_init(drm, &tv->encoder,
-				      DRM_MODE_ENCODER_TVDAC);
+	ret = drm_encoder_init(drm, &tv->encoder, &sun4i_tv_funcs,
+			       DRM_MODE_ENCODER_TVDAC, NULL);
 	if (ret) {
 		dev_err(dev, "Couldn't initialise the TV encoder\n");
 		goto err_disable_clk;
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index c35b70d83e53..f0c9f0e573d2 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -20,11 +20,11 @@ 
 #include <linux/slab.h>
 
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_encoder.h>
 #include <drm/drm_mipi_dsi.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
 
 #include "sun4i_crtc.h"
 #include "sun4i_tcon.h"
@@ -842,6 +842,10 @@  static const struct drm_connector_funcs sun6i_dsi_connector_funcs = {
 	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
 };
 
+static const struct drm_encoder_funcs sun6i_dsi_enc_funcs = {
+	.destroy = drm_encoder_cleanup,
+};
+
 static const struct drm_encoder_helper_funcs sun6i_dsi_enc_helper_funcs = {
 	.disable	= sun6i_dsi_encoder_disable,
 	.enable		= sun6i_dsi_encoder_enable,
@@ -1056,8 +1060,8 @@  static int sun6i_dsi_bind(struct device *dev, struct device *master,
 
 	drm_encoder_helper_add(&dsi->encoder,
 			       &sun6i_dsi_enc_helper_funcs);
-	ret = drm_simple_encoder_init(drm, &dsi->encoder,
-				      DRM_MODE_ENCODER_DSI);
+	ret = drm_encoder_init(drm, &dsi->encoder, &sun6i_dsi_enc_funcs,
+			       DRM_MODE_ENCODER_DSI, NULL);
 	if (ret) {
 		dev_err(dsi->dev, "Couldn't initialise the DSI encoder\n");
 		return ret;
diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
index 96532709c2a7..9694ad142a7d 100644
--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
+++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
@@ -9,8 +9,8 @@ 
 #include <linux/platform_device.h>
 
 #include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_encoder.h>
 #include <drm/drm_of.h>
-#include <drm/drm_simple_kms_helper.h>
 
 #include "sun8i_dw_hdmi.h"
 #include "sun8i_tcon_top.h"
@@ -23,6 +23,9 @@  static void sun8i_dw_hdmi_encoder_mode_set(struct drm_encoder *encoder,
 
 	clk_set_rate(hdmi->clk_tmds, mode->crtc_clock * 1000);
 }
+static const struct drm_encoder_funcs sun8i_dw_hdmi_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
+};
 
 static const struct drm_encoder_helper_funcs
 sun8i_dw_hdmi_encoder_helper_funcs = {
@@ -180,7 +183,8 @@  static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
 		goto err_disable_clk_tmds;
 
 	drm_encoder_helper_add(encoder, &sun8i_dw_hdmi_encoder_helper_funcs);
-	drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
+	drm_encoder_init(drm, encoder, &sun8i_dw_hdmi_encoder_funcs,
+			 DRM_MODE_ENCODER_TMDS, NULL);
 
 	plat_data->mode_valid = hdmi->quirks->mode_valid;
 	plat_data->use_drm_infoframe = hdmi->quirks->use_drm_infoframe;