Re: [PATCH v5 22/22] drm/sun4i: tv: Convert to the new TV mode property

["Jernej Škrabec" <jernej.skrabec@gmail.com>]

Hi Maxime,

Dne četrtek, 13. oktober 2022 ob 15:19:06 CEST je Maxime Ripard napisal(a):
> Now that the core can deal fine with analog TV modes, let's convert the
> sun4i TV driver to leverage those new features.
> 
> Acked-by: Noralf Trønnes <noralf@tronnes.org>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> 
> ---
> Changes in v5:
> - Removed the count variable in get_modes
> - Removed spurious vc4 change
> ---
>  drivers/gpu/drm/sun4i/sun4i_tv.c | 145
> +++++++++++++-------------------------- 1 file changed, 48 insertions(+),
> 97 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tv.c
> b/drivers/gpu/drm/sun4i/sun4i_tv.c index c65f0a89b6b0..4f07aff11551 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tv.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tv.c
> @@ -141,23 +141,14 @@ struct resync_parameters {
>  struct tv_mode {
>  	char		*name;
> 
> +	unsigned int	tv_mode;
> +
>  	u32		mode;
>  	u32		chroma_freq;
>  	u16		back_porch;
>  	u16		front_porch;
> -	u16		line_number;
>  	u16		vblank_level;

isn't there a way to get or calculate back_porch, front_porch and vblank_level 
from mode? From quick glance over removed values below, I would say that at 
least back_porch can be removed too?

Otherwise this patch looks ok.

Best regards,
Jernej

> 
> -	u32		hdisplay;
> -	u16		hfront_porch;
> -	u16		hsync_len;
> -	u16		hback_porch;
> -
> -	u32		vdisplay;
> -	u16		vfront_porch;
> -	u16		vsync_len;
> -	u16		vback_porch;
> -
>  	bool		yc_en;
>  	bool		dac3_en;
>  	bool		dac_bit25_en;
> @@ -213,7 +204,7 @@ static const struct resync_parameters
> pal_resync_parameters = {
> 
>  static const struct tv_mode tv_modes[] = {
>  	{
> -		.name		= "NTSC",
> +		.tv_mode	= DRM_MODE_TV_MODE_NTSC,
>  		.mode		= SUN4I_TVE_CFG0_RES_480i,
>  		.chroma_freq	= 0x21f07c1f,
>  		.yc_en		= true,
> @@ -222,17 +213,6 @@ static const struct tv_mode tv_modes[] = {
> 
>  		.back_porch	= 118,
>  		.front_porch	= 32,
> -		.line_number	= 525,
> -
> -		.hdisplay	= 720,
> -		.hfront_porch	= 18,
> -		.hsync_len	= 2,
> -		.hback_porch	= 118,
> -
> -		.vdisplay	= 480,
> -		.vfront_porch	= 26,
> -		.vsync_len	= 2,
> -		.vback_porch	= 17,
> 
>  		.vblank_level	= 240,
> 
> @@ -242,23 +222,12 @@ static const struct tv_mode tv_modes[] = {
>  		.resync_params	= &ntsc_resync_parameters,
>  	},
>  	{
> -		.name		= "PAL",
> +		.tv_mode	= DRM_MODE_TV_MODE_PAL,
>  		.mode		= SUN4I_TVE_CFG0_RES_576i,
>  		.chroma_freq	= 0x2a098acb,
> 
>  		.back_porch	= 138,
>  		.front_porch	= 24,
> -		.line_number	= 625,
> -
> -		.hdisplay	= 720,
> -		.hfront_porch	= 3,
> -		.hsync_len	= 2,
> -		.hback_porch	= 139,
> -
> -		.vdisplay	= 576,
> -		.vfront_porch	= 28,
> -		.vsync_len	= 2,
> -		.vback_porch	= 19,
> 
>  		.vblank_level	= 252,
> 
> @@ -276,63 +245,21 @@ drm_encoder_to_sun4i_tv(struct drm_encoder *encoder)
>  			    encoder);
>  }
> 
> -/*
> - * FIXME: If only the drm_display_mode private field was usable, this
> - * could go away...
> - *
> - * So far, it doesn't seem to be preserved when the mode is passed by
> - * to mode_set for some reason.
> - */
> -static const struct tv_mode *sun4i_tv_find_tv_by_mode(const struct
> drm_display_mode *mode) +static const struct tv_mode *
> +sun4i_tv_find_tv_by_mode(unsigned int mode)
>  {
>  	int i;
> 
> -	/* First try to identify the mode by name */
>  	for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
>  		const struct tv_mode *tv_mode = &tv_modes[i];
> 
> -		DRM_DEBUG_DRIVER("Comparing mode %s vs %s",
> -				 mode->name, tv_mode->name);
> -
> -		if (!strcmp(mode->name, tv_mode->name))
> -			return tv_mode;
> -	}
> -
> -	/* Then by number of lines */
> -	for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
> -		const struct tv_mode *tv_mode = &tv_modes[i];
> -
> -		DRM_DEBUG_DRIVER("Comparing mode %s vs %s (X: %d vs 
%d)",
> -				 mode->name, tv_mode->name,
> -				 mode->vdisplay, tv_mode-
>vdisplay);
> -
> -		if (mode->vdisplay == tv_mode->vdisplay)
> +		if (tv_mode->tv_mode == mode)
>  			return tv_mode;
>  	}
> 
>  	return NULL;
>  }
> 
> -static void sun4i_tv_mode_to_drm_mode(const struct tv_mode *tv_mode,
> -				      struct drm_display_mode 
*mode)
> -{
> -	DRM_DEBUG_DRIVER("Creating mode %s\n", mode->name);
> -
> -	mode->type = DRM_MODE_TYPE_DRIVER;
> -	mode->clock = 13500;
> -	mode->flags = DRM_MODE_FLAG_INTERLACE;
> -
> -	mode->hdisplay = tv_mode->hdisplay;
> -	mode->hsync_start = mode->hdisplay + tv_mode->hfront_porch;
> -	mode->hsync_end = mode->hsync_start + tv_mode->hsync_len;
> -	mode->htotal = mode->hsync_end  + tv_mode->hback_porch;
> -
> -	mode->vdisplay = tv_mode->vdisplay;
> -	mode->vsync_start = mode->vdisplay + tv_mode->vfront_porch;
> -	mode->vsync_end = mode->vsync_start + tv_mode->vsync_len;
> -	mode->vtotal = mode->vsync_end  + tv_mode->vback_porch;
> -}
> -
>  static void sun4i_tv_disable(struct drm_encoder *encoder,
>  			    struct drm_atomic_state *state)
>  {
> @@ -356,7 +283,11 @@ static void sun4i_tv_enable(struct drm_encoder
> *encoder, struct drm_crtc_state *crtc_state =
>  		drm_atomic_get_new_crtc_state(state, encoder->crtc);
>  	struct drm_display_mode *mode = &crtc_state->mode;
> -	const struct tv_mode *tv_mode = sun4i_tv_find_tv_by_mode(mode);
> +	struct drm_connector *connector = &tv->connector;
> +	struct drm_connector_state *conn_state =
> +		drm_atomic_get_new_connector_state(state, connector);
> +	const struct tv_mode *tv_mode =
> +		sun4i_tv_find_tv_by_mode(conn_state->tv.mode);
> 
>  	DRM_DEBUG_DRIVER("Enabling the TV Output\n");
> 
> @@ -404,7 +335,7 @@ static void sun4i_tv_enable(struct drm_encoder *encoder,
> /* Set the lines setup */
>  	regmap_write(tv->regs, SUN4I_TVE_LINE_REG,
>  		     SUN4I_TVE_LINE_FIRST(22) |
> -		     SUN4I_TVE_LINE_NUMBER(tv_mode->line_number));
> +		     SUN4I_TVE_LINE_NUMBER(mode->vtotal));
> 
>  	regmap_write(tv->regs, SUN4I_TVE_LEVEL_REG,
>  		     SUN4I_TVE_LEVEL_BLANK(tv_mode->video_levels-
>blank) |
> @@ -467,35 +398,43 @@ static const struct drm_encoder_helper_funcs
> sun4i_tv_helper_funcs = {
> 
>  static int sun4i_tv_comp_get_modes(struct drm_connector *connector)
>  {
> -	int i;
> +	struct drm_display_mode *mode;
> 
> -	for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
> -		struct drm_display_mode *mode;
> -		const struct tv_mode *tv_mode = &tv_modes[i];
> -
> -		mode = drm_mode_create(connector->dev);
> -		if (!mode) {
> -			DRM_ERROR("Failed to create a new display 
mode\n");
> -			return 0;
> -		}
> +	mode = drm_mode_analog_ntsc_480i(connector->dev);
> +	if (!mode) {
> +		DRM_ERROR("Failed to create a new display mode\n");
> +		return -ENOMEM;
> +	}
> 
> -		strcpy(mode->name, tv_mode->name);
> +	mode->type |= DRM_MODE_TYPE_PREFERRED;
> +	drm_mode_probed_add(connector, mode);
> 
> -		sun4i_tv_mode_to_drm_mode(tv_mode, mode);
> -		drm_mode_probed_add(connector, mode);
> +	mode = drm_mode_analog_pal_576i(connector->dev);
> +	if (!mode) {
> +		DRM_ERROR("Failed to create a new display mode\n");
> +		return -ENOMEM;
>  	}
> 
> -	return i;
> +	drm_mode_probed_add(connector, mode);
> +
> +	return 2;
>  }
> 
>  static const struct drm_connector_helper_funcs
> sun4i_tv_comp_connector_helper_funcs = { +	.atomic_check	=
> drm_atomic_helper_connector_tv_check,
>  	.get_modes	= sun4i_tv_comp_get_modes,
>  };
> 
> +static void sun4i_tv_connector_reset(struct drm_connector *connector)
> +{
> +	drm_atomic_helper_connector_reset(connector);
> +	drm_atomic_helper_connector_tv_reset(connector);
> +}
> +
>  static const struct drm_connector_funcs sun4i_tv_comp_connector_funcs = {
>  	.fill_modes		= drm_helper_probe_single_connector_modes,
>  	.destroy		= drm_connector_cleanup,
> -	.reset			= 
drm_atomic_helper_connector_reset,
> +	.reset			= sun4i_tv_connector_reset,
>  	.atomic_duplicate_state	= 
drm_atomic_helper_connector_duplicate_state,
>  	.atomic_destroy_state	= 
drm_atomic_helper_connector_destroy_state,
>  };
> @@ -587,8 +526,20 @@ static int sun4i_tv_bind(struct device *dev, struct
> device *master,
> 
>  	drm_connector_attach_encoder(&tv->connector, &tv->encoder);
> 
> +	ret = drm_mode_create_tv_properties(drm,
> +					    
BIT(DRM_MODE_TV_MODE_NTSC) |
> +					    
BIT(DRM_MODE_TV_MODE_PAL));
> +	if (ret)
> +		goto err_cleanup_connector;
> +
> +	drm_object_attach_property(&tv->connector.base,
> +				   drm-
>mode_config.tv_mode_property,
> +				   DRM_MODE_TV_MODE_NTSC);
> +
>  	return 0;
> 
> +err_cleanup_connector:
> +	drm_connector_cleanup(&tv->connector);
>  err_cleanup_encoder:
>  	drm_encoder_cleanup(&tv->encoder);
>  err_disable_clk:
> 
> --
> b4 0.11.0-dev-7da52