Re: [PATCH 01/15] drm/exynos: remove dependency on DRM simple helpers

Hi

Am 19.07.26 um 01:35 schrieb Diogo Silva:
> The simple KMS helpers are deprecated because they only add an
> intermediate layer between drivers and atomic modesetting.
>
> Open-code drm_simple_encoder_init() by calling drm_encoder_init()
> directly and providing driver-local drm_encoder_funcs.
> Also check the return value from drm_encoder_init() to avoid silent
> failures.
>
> Signed-off-by: Diogo Silva <[email protected]>
> ---
>   drivers/gpu/drm/exynos/exynos_dp.c       | 13 +++++++++++--
>   drivers/gpu/drm/exynos/exynos_drm_dpi.c  | 14 ++++++++++++--
>   drivers/gpu/drm/exynos/exynos_drm_dsi.c  | 11 +++++++++--
>   drivers/gpu/drm/exynos/exynos_drm_vidi.c | 14 ++++++++++++--
>   drivers/gpu/drm/exynos/exynos_hdmi.c     | 15 +++++++++++++--
>   5 files changed, 57 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
> index b80540328150..1598892c602b 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp.c
> @@ -24,11 +24,11 @@
>   #include <drm/drm_bridge.h>
>   #include <drm/drm_bridge_connector.h>
>   #include <drm/drm_crtc.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 <drm/exynos_drm.h>
>   
>   #include "exynos_drm_crtc.h"
> @@ -79,6 +79,10 @@ static void exynos_dp_nop(struct drm_encoder *encoder)
>   	/* do nothing */
>   }
>   
> +static const struct drm_encoder_funcs exynos_dp_encoder_funcs = {
> +	.destroy = drm_encoder_cleanup,
> +};
> +
>   static const struct drm_encoder_helper_funcs exynos_dp_encoder_helper_funcs = {
>   	.mode_set = exynos_dp_mode_set,
>   	.enable = exynos_dp_nop,
> @@ -95,7 +99,12 @@ static int exynos_dp_bind(struct device *dev, struct device *master, void *data)
>   
>   	dp->drm_dev = drm_dev;
>   
> -	drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_TMDS);
> +	ret = drm_encoder_init(drm_dev, encoder, &exynos_dp_encoder_funcs,
> +			       DRM_MODE_ENCODER_TMDS, NULL);
> +	if (ret) {
> +		dev_err(dp->dev, "Failed to initialize encoder\n");

Preferably use drm_err and drm_dev.

> +		return ret;
> +	}
>   
>   	drm_encoder_helper_add(encoder, &exynos_dp_encoder_helper_funcs);
>   
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dpi.c b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
> index 0dc36df6ada3..4e42a1da81d1 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dpi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
> @@ -12,10 +12,10 @@
>   #include <linux/regulator/consumer.h>
>   
>   #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_encoder.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 <video/of_videomode.h>
>   #include <video/videomode.h>
> @@ -140,6 +140,10 @@ static void exynos_dpi_disable(struct drm_encoder *encoder)
>   	}
>   }
>   
> +static const struct drm_encoder_funcs exynos_dpi_encoder_funcs = {
> +	.destroy = drm_encoder_cleanup,
> +};
> +
>   static const struct drm_encoder_helper_funcs exynos_dpi_encoder_helper_funcs = {
>   	.mode_set = exynos_dpi_mode_set,
>   	.enable = exynos_dpi_enable,
> @@ -194,7 +198,13 @@ int exynos_dpi_bind(struct drm_device *dev, struct drm_encoder *encoder)
>   {
>   	int ret;
>   
> -	drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_TMDS);
> +	ret = drm_encoder_init(dev, encoder, &exynos_dpi_encoder_funcs,
> +			       DRM_MODE_ENCODER_TMDS, NULL);
> +	if (ret) {
> +		DRM_DEV_ERROR(encoder_to_dpi(encoder)->dev,

It just failed to init the encoder, so better not use it here. The 
device should be the same as passed to the function via 'dev'.

> +			      "failed to create encoder ret = %d\n", ret);
> +		return ret;
> +	}
>   
>   	drm_encoder_helper_add(encoder, &exynos_dpi_encoder_helper_funcs);
>   
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index c4d098ab7863..6b7561ac9bb0 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -13,7 +13,7 @@
>   
>   #include <drm/bridge/samsung-dsim.h>
>   #include <drm/drm_probe_helper.h>
> -#include <drm/drm_simple_kms_helper.h>
> +#include <drm/drm_encoder.h>
>   
>   #include "exynos_drm_crtc.h"
>   #include "exynos_drm_drv.h"
> @@ -22,6 +22,10 @@ struct exynos_dsi {
>   	struct drm_encoder encoder;
>   };
>   
> +static const struct drm_encoder_funcs exynos_drm_dsi_encoder_funcs = {
> +	.destroy = drm_encoder_cleanup,
> +};
> +
>   static irqreturn_t exynos_dsi_te_irq_handler(struct samsung_dsim *dsim)
>   {
>   	struct exynos_dsi *dsi = dsim->priv;
> @@ -79,7 +83,10 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, void *data
>   	struct drm_device *drm_dev = data;
>   	int ret;
>   
> -	drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_TMDS);
> +	ret = drm_encoder_init(drm_dev, encoder, &exynos_drm_dsi_encoder_funcs,
> +			       DRM_MODE_ENCODER_TMDS, NULL);
> +	if (ret)
> +		return ret;
>   
>   	ret = exynos_drm_set_possible_crtcs(encoder, EXYNOS_DISPLAY_TYPE_LCD);
>   	if (ret < 0)
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> index 67bbf9b8bc0e..59dea853d364 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> @@ -13,10 +13,10 @@
>   
>   #include <drm/drm_atomic_helper.h>
>   #include <drm/drm_edid.h>
> +#include <drm/drm_encoder.h>
>   #include <drm/drm_framebuffer.h>
>   #include <drm/drm_print.h>
>   #include <drm/drm_probe_helper.h>
> -#include <drm/drm_simple_kms_helper.h>
>   #include <drm/drm_vblank.h>
>   #include <drm/exynos_drm.h>
>   
> @@ -403,6 +403,10 @@ static void exynos_vidi_disable(struct drm_encoder *encoder)
>   {
>   }
>   
> +static const struct drm_encoder_funcs exynos_vidi_encoder_funcs = {
> +	.destroy = drm_encoder_cleanup,
> +};
> +
>   static const struct drm_encoder_helper_funcs exynos_vidi_encoder_helper_funcs = {
>   	.mode_set = exynos_vidi_mode_set,
>   	.enable = exynos_vidi_enable,
> @@ -445,7 +449,13 @@ static int vidi_bind(struct device *dev, struct device *master, void *data)
>   		return PTR_ERR(ctx->crtc);
>   	}
>   
> -	drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_TMDS);
> +	ret = drm_encoder_init(drm_dev, encoder, &exynos_vidi_encoder_funcs,
> +			       DRM_MODE_ENCODER_TMDS, NULL);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "failed to initialize encoder ret = %d\n",
> +			      ret);

Again, you rather want drm_err() with drm_dev here.

I've briefly looked over the series and many patches seem affected. 
Please prefer drm_ logging functions and DRM devices over the plain 
device equivalents.

Best regards
Thomas


> +		return ret;
> +	}
>   
>   	drm_encoder_helper_add(encoder, &exynos_vidi_encoder_helper_funcs);
>   
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index 09b2cabb236f..f44586ce0fdf 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -36,9 +36,9 @@
>   #include <drm/drm_atomic_helper.h>
>   #include <drm/drm_bridge.h>
>   #include <drm/drm_edid.h>
> +#include <drm/drm_encoder.h>
>   #include <drm/drm_print.h>
>   #include <drm/drm_probe_helper.h>
> -#include <drm/drm_simple_kms_helper.h>
>   
>   #include "exynos_drm_crtc.h"
>   #include "regs-hdmi.h"
> @@ -1575,6 +1575,11 @@ static void hdmi_disable(struct drm_encoder *encoder)
>   	mutex_unlock(&hdata->mutex);
>   }
>   
> +static const struct drm_encoder_funcs exynos_hdmi_encoder_funcs = {
> +	.destroy = drm_encoder_cleanup,
> +};
> +
> +
>   static const struct drm_encoder_helper_funcs exynos_hdmi_encoder_helper_funcs = {
>   	.mode_fixup	= hdmi_mode_fixup,
>   	.enable		= hdmi_enable,
> @@ -1862,7 +1867,13 @@ static int hdmi_bind(struct device *dev, struct device *master, void *data)
>   
>   	hdata->phy_clk.enable = hdmiphy_clk_enable;
>   
> -	drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_TMDS);
> +	ret = drm_encoder_init(drm_dev, encoder, &exynos_hdmi_encoder_funcs,
> +			       DRM_MODE_ENCODER_TMDS, NULL);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "failed to initialize encoder ret = %d\n",
> +			      ret);
> +		return ret;
> +	}
>   
>   	drm_encoder_helper_add(encoder, &exynos_hdmi_encoder_helper_funcs);
>   
>

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