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

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

[Jernej Škrabec]

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

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

[Maxime Ripard]

Hi Jernej,

On Thu, Oct 13, 2022 at 08:23:51PM +0200, Jernej Škrabec wrote:
> 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?

I tried actually, but I'm not sure what the front porch and back porch
parameters actually are. They are called that way by Allwinner, but it
doesn't match the PAL or NTSC timings at all.

For example, back_porch is 118 for NTSC and 138 for PAL. Actual back
porches would be around 12 and 16, respectively. Actually, the entire
blanking area are 138 and 144. This is close enough for PAL, but pretty
far off for NTSC.

Allwinner has the habit of integrating the sync period into one of the
porches, but still there's no obvious match.

front_porch is pretty much in the same case.

Since it affected the display output quite a lot, I chose to keep it for
now unfortunately.

> Otherwise this patch looks ok.

Can I add your Acked-by/Reviewed-by then?

Thanks!
Maxime

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

[Jernej Škrabec]

Dne petek, 14. oktober 2022 ob 09:38:10 CEST je Maxime Ripard napisal(a):
> Hi Jernej,
> 
> On Thu, Oct 13, 2022 at 08:23:51PM +0200, Jernej Škrabec wrote:
> > 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?
> 
> I tried actually, but I'm not sure what the front porch and back porch
> parameters actually are. They are called that way by Allwinner, but it
> doesn't match the PAL or NTSC timings at all.
> 
> For example, back_porch is 118 for NTSC and 138 for PAL. Actual back
> porches would be around 12 and 16, respectively. Actually, the entire
> blanking area are 138 and 144. This is close enough for PAL, but pretty
> far off for NTSC.
> 
> Allwinner has the habit of integrating the sync period into one of the
> porches, but still there's no obvious match.
> 
> front_porch is pretty much in the same case.

Ok then.

> 
> Since it affected the display output quite a lot, I chose to keep it for
> now unfortunately.
> 
> > Otherwise this patch looks ok.
> 
> Can I add your Acked-by/Reviewed-by then?

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

Best regards,
Jernej

Re: [PATCH v5 01/22] drm/tests: Add Kunit Helpers

[Noralf Trønnes]

Den 13.10.2022 15.18, skrev Maxime Ripard:
> As the number of kunit tests in KMS grows further, we start to have
> multiple test suites that, for example, need to register a mock DRM
> driver to interact with the KMS function they are supposed to test.
> 
> Let's add a file meant to provide those kind of helpers to avoid
> duplication.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> 
> ---

Reviewed-by: Noralf Trønnes <noralf@tronnes.org>

Re: [PATCH v5 07/22] drm/client: Add some tests for drm_connector_pick_cmdline_mode()

[Noralf Trønnes]

Den 13.10.2022 15.18, skrev Maxime Ripard:
> drm_connector_pick_cmdline_mode() is in charge of finding a proper
> drm_display_mode from the definition we got in the video= command line
> argument.
> 
> Let's add some unit tests to make sure we're not getting any regressions
> there.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> 
> ---
> Changes in v5:
> - Removed useless (for now) count and modes intermediate variables in
>   get_modes
> - Switched to kunit assertions in test init, and to KUNIT_ASSERT_NOT_NULL
>   instead of KUNIT_ASSERT_PTR_NE(..., NULL)
> 
> Changes in v4:
> - Removed MODULE macros
> ---
>  drivers/gpu/drm/drm_client_modeset.c            |  4 +
>  drivers/gpu/drm/tests/drm_client_modeset_test.c | 99 +++++++++++++++++++++++++
>  2 files changed, 103 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> index bbc535cc50dd..d553e793e673 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -1237,3 +1237,7 @@ int drm_client_modeset_dpms(struct drm_client_dev *client, int mode)
>  	return ret;
>  }
>  EXPORT_SYMBOL(drm_client_modeset_dpms);
> +
> +#ifdef CONFIG_DRM_KUNIT_TEST
> +#include "tests/drm_client_modeset_test.c"
> +#endif

I can't say I like including the file like this, but exporting the
static function for testing isn't attractive either and doing it like
this is shown in the kunit docs, so:

Acked-by: Noralf Trønnes <noralf@tronnes.org>

Re: [PATCH v5 08/22] drm/modes: Move named modes parsing to a separate function

[Noralf Trønnes]

Den 13.10.2022 15.18, skrev Maxime Ripard:
> The current construction of the named mode parsing doesn't allow to extend
> it easily. Let's move it to a separate function so we can add more
> parameters and modes.
> 
> In order for the tests to still pass, some extra checks are needed, so
> it's not a 1:1 move.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> 

I was hoping that someone else would step up and review these parser
patches since the parser code is rather difficult to read, for me at
least. I have studied it now, so I'll give it a try.

> ---
> Changes in v4:
> - Fold down all the named mode patches that were split into a single
>   patch again to maintain bisectability
> ---
>  drivers/gpu/drm/drm_modes.c | 73 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 62 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index c0dceff51cac..2f020ef2ddf2 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -2229,6 +2229,55 @@ static const char * const drm_named_modes_whitelist[] = {
>  	"PAL",
>  };
>  
> +static int drm_mode_parse_cmdline_named_mode(const char *name,
> +					     unsigned int name_end,
> +					     struct drm_cmdline_mode *cmdline_mode)
> +{
> +	unsigned int i;
> +
> +	if (!name_end)
> +		return 0;

name_end can't be zero since the argument is checked before calling this
function.

> +
> +	/* If the name starts with a digit, it's not a named mode */
> +	if (isdigit(name[0]))
> +		return 0;
> +
> +	/*
> +	 * If there's an equal sign in the name, the command-line
> +	 * contains only an option and no mode.
> +	 */
> +	if (strnchr(name, name_end, '='))
> +		return 0;

I think this check actually belongs in
drm_mode_parse_command_line_for_connector() after options_off is set. If
theres an equal sign it should skip all mode parsing and skip down to
drm_mode_parse_cmdline_options(). Which probably means that the mode
parsing should have been moved out to separate function to avoid using a
goto.
But that's probably beyond the scope of this patchset :)

> +
> +#define STR_STRICT_EQ(str, len, cmp) \
> +	(str_has_prefix(str, cmp) == len)
> +
> +	/* The connection status extras can be set without a mode. */
> +	if (STR_STRICT_EQ(name, name_end, "d") ||
> +	    STR_STRICT_EQ(name, name_end, "D") ||
> +	    STR_STRICT_EQ(name, name_end, "e"))
> +		return 0;

It took me a while to understand what is going on here.
If str_has_prefix() finds a match it returns strlen(prefix). Since
prefix is always of length 1, name_end has to always be 1 for the
statement to be true.

I would have written it like this:

	/* The connection status extras can be set without a mode. */
	if (name_end == 1) {
		if (name[0] == "d" || name[0] == "D" || name[0] == "e")
			return 0;
	}

> +
> +	/*
> +	 * We're sure we're a named mode at that point, iterate over the

that -> this ?

> +	 * list of modes we're aware of.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
> +		int ret;
> +
> +		ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
> +		if (ret != name_end)
> +			continue;
> +
> +		strcpy(cmdline_mode->name, drm_named_modes_whitelist[i]);
> +		cmdline_mode->specified = true;
> +
> +		return 1;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  /**
>   * drm_mode_parse_command_line_for_connector - parse command line modeline for connector
>   * @mode_option: optional per connector mode option
> @@ -2265,7 +2314,7 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>  	const char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
>  	const char *options_ptr = NULL;
>  	char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
> -	int i, len, ret;
> +	int len, ret;
>  
>  	memset(mode, 0, sizeof(*mode));
>  	mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> @@ -2306,17 +2355,19 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>  		parse_extras = true;
>  	}
>  
> -	/* First check for a named mode */
> -	for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
> -		ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
> -		if (ret == mode_end) {
> -			if (refresh_ptr)
> -				return false; /* named + refresh is invalid */
>  
> -			strcpy(mode->name, drm_named_modes_whitelist[i]);
> -			mode->specified = true;
> -			break;
> -		}
> +	if (mode_end) {

Shouldn't this be:

	if (!mode_end)
		return false;

I can't see how mode_end == 0 can produce anything valid.
Scenarios for mode_option that gives mode_end == 0:
- zero length
- starts with a comma
- starts with -bpp
- starts with @refresh

Maybe it's a separate patch with matching tests ;)

Noralf.

> +		ret = drm_mode_parse_cmdline_named_mode(name, mode_end, mode);
> +		if (ret < 0)
> +			return false;
> +
> +		/*
> +		 * Having a mode that starts by a letter (and thus is named)
> +		 * and an at-sign (used to specify a refresh rate) is
> +		 * disallowed.
> +		 */
> +		if (ret && refresh_ptr)
> +			return false;
>  	}
>  
>  	/* No named mode? Check for a normal mode argument, e.g. 1024x768 */
> 

Re: [PATCH v5 06/22] drm/modes: Add a function to generate analog display modes

[Mateusz Kwiatkowski]

Hi Maxime & everyone,

Sorry for being inactive in the discussions about this patchset for the last
couple of weeks.

> +const static struct analog_parameters tv_modes_parameters[] = {
> +	TV_MODE_PARAMETER(DRM_MODE_ANALOG_NTSC,
> +			  NTSC_LINES_NUMBER,
> +			  NTSC_LINE_DURATION_NS,
> +			  PARAM_RANGE(NTSC_HACT_DURATION_MIN_NS,
> +				      NTSC_HACT_DURATION_TYP_NS,
> +				      NTSC_HACT_DURATION_MAX_NS),
> +			  PARAM_RANGE(NTSC_HFP_DURATION_MIN_NS,
> +				      NTSC_HFP_DURATION_TYP_NS,
> +				      NTSC_HFP_DURATION_MAX_NS),
> +			  PARAM_RANGE(NTSC_HSLEN_DURATION_MIN_NS,
> +				      NTSC_HSLEN_DURATION_TYP_NS,
> +				      NTSC_HSLEN_DURATION_MAX_NS),
> +			  PARAM_RANGE(NTSC_HBP_DURATION_MIN_NS,
> +				      NTSC_HBP_DURATION_TYP_NS,
> +				      NTSC_HBP_DURATION_MAX_NS),
> +			  PARAM_RANGE(NTSC_HBLK_DURATION_MIN_NS,
> +				      NTSC_HBLK_DURATION_TYP_NS,
> +				      NTSC_HBLK_DURATION_MAX_NS),
> +			  16,
> +			  PARAM_FIELD(3, 3),
> +			  PARAM_FIELD(3, 3),
> +			  PARAM_FIELD(16, 17)),
> +	TV_MODE_PARAMETER(DRM_MODE_ANALOG_PAL,
> +			  PAL_LINES_NUMBER,
> +			  PAL_LINE_DURATION_NS,
> +			  PARAM_RANGE(PAL_HACT_DURATION_MIN_NS,
> +				      PAL_HACT_DURATION_TYP_NS,
> +				      PAL_HACT_DURATION_MAX_NS),
> +			  PARAM_RANGE(PAL_HFP_DURATION_MIN_NS,
> +				      PAL_HFP_DURATION_TYP_NS,
> +				      PAL_HFP_DURATION_MAX_NS),
> +			  PARAM_RANGE(PAL_HSLEN_DURATION_MIN_NS,
> +				      PAL_HSLEN_DURATION_TYP_NS,
> +				      PAL_HSLEN_DURATION_MAX_NS),
> +			  PARAM_RANGE(PAL_HBP_DURATION_MIN_NS,
> +				      PAL_HBP_DURATION_TYP_NS,
> +				      PAL_HBP_DURATION_MAX_NS),
> +			  PARAM_RANGE(PAL_HBLK_DURATION_MIN_NS,
> +				      PAL_HBLK_DURATION_TYP_NS,
> +				      PAL_HBLK_DURATION_MAX_NS),
> +			  12,
> +
> +			  /*
> +			   * The front porch is actually 6 short sync
> +			   * pulses for the even field, and 5 for the
> +			   * odd field. Each sync takes half a life so
> +			   * the odd field front porch is shorter by
> +			   * half a line.
> +			   *
> +			   * In progressive, we're supposed to use 6
> +			   * pulses, so we're fine there
> +			   */
> +			  PARAM_FIELD(3, 2),
> +
> +			  /*
> +			   * The vsync length is 5 long sync pulses,
> +			   * each field taking half a line. We're
> +			   * shorter for both fields by half a line.
> +			   *
> +			   * In progressive, we're supposed to use 5
> +			   * pulses, so we're off by half
> +			   * a line.
> +			   *
> +			   * In interlace, we're now off by half a line
> +			   * for the even field and one line for the odd
> +			   * field.
> +			   */
> +			  PARAM_FIELD(3, 3),
> +
> +			  /*
> +			   * The back porch starts with post-equalizing
> +			   * pulses, consisting in 5 short sync pulses
> +			   * for the even field, 4 for the odd field. In
> +			   * progressive, it's 5 short syncs.
> +			   *
> +			   * In progressive, we thus have 2.5 lines,
> +			   * plus the 0.5 line we were missing
> +			   * previously, so we should use 3 lines.
> +			   *
> +			   * In interlace, the even field is in the
> +			   * exact same case than progressive. For the
> +			   * odd field, we should be using 2 lines but
> +			   * we're one line short, so we'll make up for
> +			   * it here by using 3.
> +			   *
> +			   * The entire blanking area is supposed to
> +			   * take 25 lines, so we also need to account
> +			   * for the rest of the blanking area that
> +			   * can't be in either the front porch or sync
> +			   * period.
> +			   */
> +			  PARAM_FIELD(19, 20)),
> +};

Nit: setting vbp limits like that makes it impossible to use
drm_analog_tv_mode() to generate modes that include the VBI for e.g. emitting
teletext.

This probably doesn't matter, as it can still be created as a custom mode from
userspace, hence I'm mentioning it as a nit.

> +		 * By convention, NSTC (aka 525/60) systems start with

Typo: s/NSTC/NTSC/

Best regards,
Mateusz Kwiatkowski

Re: [PATCH v5 13/22] drm/modes: Introduce the tv_mode property as a command-line option

[Mateusz Kwiatkowski]

Hi Maxime, Noralf & everyone,

I'd like to address Noralf here in particular, and refer to these discussions
from the past:

- https://lore.kernel.org/linux-arm-kernel/2f607c7d-6da1-c8df-1c02-8dd344a92343@gmail.com/
- https://lore.kernel.org/linux-arm-kernel/9e76a508-f469-a54d-ecd7-b5868ca99af4@tronnes.org/

> @@ -2230,20 +2256,22 @@ struct drm_named_mode {
>  	unsigned int xres;
>  	unsigned int yres;
>  	unsigned int flags;
> +	unsigned int tv_mode;
>  };

I saw that you (Noralf) opposed my suggestion about the DRM_MODE_TV_MODE_NONE
enum value in enum drm drm_connector_tv_mode. I get your argumentation, and I'm
not gonna argue, but I still don't like the fact that struct drm_named_mode now
includes a field that is only relevant for analog TV modes, has no "none" value,
and yet the type is supposed to be generic enough to be usable for other types
of outputs as well.

It's true that it can just be ignored (as Maxime mentioned in his response to
my e-mail linked above), and now the value of 0 corresponds to
DRM_MODE_TV_MODE_NTSC, which is a rather sane default, but it still feels messy
to me.

I'm not gonna force my opinion here, but I wanted to bring your attention to
this issue, maybe you have some other solution in mind for this problem. Or if
you don't see that as a problem at all, that's fine, too.

Best regards,
Mateusz Kwiatkowski

Re: [PATCH v5 19/22] drm/vc4: vec: Check for VEC output constraints

[Mateusz Kwiatkowski]

Hi Maxime,

W dniu 13.10.2022 o 15:19, Maxime Ripard pisze:
> From: Mateusz Kwiatkowski > > The VEC can accept pretty much any relatively reasonable mode, but still > has a bunch of constraints to meet. > > Let's create an atomic_check() implementation that will make sure we > don't end up accepting a non-functional mode. > > Acked-by: Noralf Trønnes > Signed-off-by: Mateusz Kwiatkowski > Signed-off-by: Maxime Ripard > --- > drivers/gpu/drm/vc4/vc4_vec.c | 48 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c > index 90e375a8a8f9..1fcb7baf874e 100644 > --- a/drivers/gpu/drm/vc4/vc4_vec.c > +++ b/drivers/gpu/drm/vc4/vc4_vec.c > @@ -453,6 +453,7 @@ static int vc4_vec_encoder_atomic_check(struct drm_encoder *encoder, > struct drm_crtc_state *crtc_state, > struct drm_connector_state *conn_state) > { > + const struct drm_display_mode *mode = &crtc_state->adjusted_mode; > const struct vc4_vec_tv_mode *vec_mode; > > vec_mode =
&vc4_vec_tv_modes[conn_state->tv.legacy_mode]; > @@ -461,6 +462,53 @@ static int vc4_vec_encoder_atomic_check(struct drm_encoder *encoder, > !drm_mode_equal(vec_mode->mode, &crtc_state->adjusted_mode)) > return -EINVAL; > > + if (mode->crtc_hdisplay % 4) > + return -EINVAL; > + > + if (!(mode->crtc_hsync_end - mode->crtc_hsync_start)) > + return -EINVAL; > + > + switch (mode->vtotal) { > + case 525: > + if (mode->crtc_vtotal > 262) > + return -EINVAL; > + > + if (mode->crtc_vdisplay < 1 || mode->crtc_vdisplay > 253) > + return -EINVAL; > + > + if (!(mode->crtc_vsync_start - mode->crtc_vdisplay)) > + return -EINVAL; > + > + if ((mode->crtc_vsync_end - mode->crtc_vsync_start) != 3) > + return -EINVAL; > + > + if ((mode->crtc_vtotal - mode->crtc_vsync_end) < 4) > + return -EINVAL; > + > + break; > + > + case 625: > + if (mode->crtc_vtotal > 312) > + return -EINVAL; > + > + if (mode->crtc_vdisplay < 1 || mode->crtc_vdisplay > 305) > + return -EINVAL; > + > + if
(!(mode->crtc_vsync_start - mode->crtc_vdisplay)) > + return -EINVAL; > + > + if ((mode->crtc_vsync_end - mode->crtc_vsync_start) != 3) > + return -EINVAL; > + > + if ((mode->crtc_vtotal - mode->crtc_vsync_end) < 2) > + return -EINVAL; > + > + break; > + > + default: > + return -EINVAL; > + } > + > return 0; > } In my original version of this function (https://github.com/raspberrypi/linux/pull/4406/files) the switch is over reference_mode->vtotal, not mode->vtotal. This was intended to explicitly allow a different value of mode->vtotal, to support non-standard modes, such as "fake" 525 lines with SECAM encoding, or the progressive modes. You're switching over mode->vtotal, which makes specifying those impossible. I don't think we should limit the users like that. We're removing reference_mode in patch 20/22, so adding a switch over reference_mode->vtotal is probably not a good idea -- in that case I'd switch over mode->htotal instead: 858 for "NTSC" and 864 for "PAL". This
may seem a bit weird, but any other value of htotal causes the VEC to output garbage anyway. Best regards, Mateusz Kwiatkowski

Re: [PATCH v5 19/22] drm/vc4: vec: Check for VEC output constraints

[Mateusz Kwiatkowski]

Hi Maxime,

Sorry about the mess that happened to the previous message. I hope this one
will be delivered more cleanly.

W dniu 13.10.2022 o 15:19, Maxime Ripard pisze:
> From: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>
>
> The VEC can accept pretty much any relatively reasonable mode, but still
> has a bunch of constraints to meet.
>
> Let's create an atomic_check() implementation that will make sure we
> don't end up accepting a non-functional mode.
>
> Acked-by: Noralf Trønnes <noralf@tronnes.org>
> Signed-off-by: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/gpu/drm/vc4/vc4_vec.c | 48 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
> index 90e375a8a8f9..1fcb7baf874e 100644
> --- a/drivers/gpu/drm/vc4/vc4_vec.c
> +++ b/drivers/gpu/drm/vc4/vc4_vec.c
> @@ -453,6 +453,7 @@ static int vc4_vec_encoder_atomic_check(struct drm_encoder *encoder,
>                      struct drm_crtc_state *crtc_state,
>                      struct drm_connector_state *conn_state)
>  {
> +    const struct drm_display_mode *mode = &crtc_state->adjusted_mode;
>      const struct vc4_vec_tv_mode *vec_mode;
>  
>      vec_mode = &vc4_vec_tv_modes[conn_state->tv.legacy_mode];
> @@ -461,6 +462,53 @@ static int vc4_vec_encoder_atomic_check(struct drm_encoder *encoder,
>          !drm_mode_equal(vec_mode->mode, &crtc_state->adjusted_mode))
>          return -EINVAL;
>  
> +    if (mode->crtc_hdisplay % 4)
> +        return -EINVAL;
> +
> +    if (!(mode->crtc_hsync_end - mode->crtc_hsync_start))
> +        return -EINVAL;
> +
> +    switch (mode->vtotal) {
> +    case 525:
> +        if (mode->crtc_vtotal > 262)
> +            return -EINVAL;
> +
> +        if (mode->crtc_vdisplay < 1 || mode->crtc_vdisplay > 253)
> +            return -EINVAL;
> +
> +        if (!(mode->crtc_vsync_start - mode->crtc_vdisplay))
> +            return -EINVAL;
> +
> +        if ((mode->crtc_vsync_end - mode->crtc_vsync_start) != 3)
> +            return -EINVAL;
> +
> +        if ((mode->crtc_vtotal - mode->crtc_vsync_end) < 4)
> +            return -EINVAL;
> +
> +        break;
> +
> +    case 625:
> +        if (mode->crtc_vtotal > 312)
> +            return -EINVAL;
> +
> +        if (mode->crtc_vdisplay < 1 || mode->crtc_vdisplay > 305)
> +            return -EINVAL;
> +
> +        if (!(mode->crtc_vsync_start - mode->crtc_vdisplay))
> +            return -EINVAL;
> +
> +        if ((mode->crtc_vsync_end - mode->crtc_vsync_start) != 3)
> +            return -EINVAL;
> +
> +        if ((mode->crtc_vtotal - mode->crtc_vsync_end) < 2)
> +            return -EINVAL;
> +
> +        break;
> +
> +    default:
> +        return -EINVAL;
> +    }
> +
>      return 0;
>  }
>  
>

In my original version of this function
(https://github.com/raspberrypi/linux/pull/4406/files) the switch is over
reference_mode->vtotal, not mode->vtotal. This was intended to explicitly allow
a different value of mode->vtotal, to support non-standard modes, such as "fake"
525 lines with SECAM encoding, or the progressive modes.

You're switching over mode->vtotal, which makes specifying those impossible.
I don't think we should limit the users like that.

We're removing reference_mode in patch 20/22, so adding a switch over
reference_mode->vtotal is probably not a good idea -- in that case I'd switch
over mode->htotal instead: 858 for "NTSC" and 864 for "PAL". This may seem a bit
weird, but any other value of htotal causes the VEC to output garbage anyway.

Best regards,
Mateusz Kwiatkowski

Re: [PATCH v5 20/22] drm/vc4: vec: Convert to the new TV mode property

[Mateusz Kwiatkowski]

Hi Maxime,

>  static int vc4_vec_connector_get_modes(struct drm_connector *connector)
>  {
> -	struct drm_connector_state *state = connector->state;
>  	struct drm_display_mode *mode;
>  
> -	mode = drm_mode_duplicate(connector->dev,
> -				  vc4_vec_tv_modes[state->tv.legacy_mode].mode);
> +	mode = drm_mode_analog_ntsc_480i(connector->dev);
>  	if (!mode) {
>  		DRM_ERROR("Failed to create a new display mode\n");
>  		return -ENOMEM;
>  	}
>  
> +	mode->type |= DRM_MODE_TYPE_PREFERRED;
>  	drm_mode_probed_add(connector, mode);
>  
> -	return 1;
> +	mode = drm_mode_analog_pal_576i(connector->dev);
> +	if (!mode) {
> +		DRM_ERROR("Failed to create a new display mode\n");
> +		return -ENOMEM;
> +	}
> +
> +	drm_mode_probed_add(connector, mode);
> +
> +	return 2;
> +}

Referencing those previous discussions:
- https://lore.kernel.org/dri-devel/0255f7c6-0484-6456-350d-cf24f3fee5d6@tronnes.org/
- https://lore.kernel.org/dri-devel/c8f8015a-75da-afa8-ca7f-b2b134cacd16@gmail.com/

Unconditionally setting the 480i mode as DRM_MODE_TYPE_PREFERRED causes Xorg
(at least on current Raspberry Pi OS) to display garbage when
video=Composite1:PAL is specified on the command line, so I'm afraid this won't
do.

As I see it, there are three viable solutions for this issue:

a) Somehow query the video= command line option from this function, and set
   DRM_MODE_TYPE_PREFERRED appropriately. This would break the abstraction
   provided by global DRM code, but should work fine.

b) Modify drm_helper_probe_add_cmdline_mode() so that it sets
   DRM_MODE_TYPE_PREFERRED in addition to DRM_MODE_TYPE_USERDEF. This seems
   pretty robust, but affects the entire DRM subsystem, which may break
   userspace in different ways.

   - Maybe this could be mitigated by adding some additional conditions, e.g.
     setting the PREFERRED flag only if no modes are already flagged as such
     and/or only if the cmdline mode is a named one (~= analog TV mode)

c) Forcing userspace (Xorg / Raspberry Pi OS) to get fixed and honor the USERDEF
   flag.

Either way, hardcoding 480i as PREFERRED does not seem right.

Note: this also applies to the sun4i version (patch 22/22).

> @@ -366,13 +472,16 @@ static void vc4_vec_encoder_enable(struct drm_encoder *encoder,
>  	struct drm_connector *connector = &vec->connector;
>  	struct drm_connector_state *conn_state =
>  		drm_atomic_get_new_connector_state(state, connector);
> -	const struct vc4_vec_tv_mode *tv_mode =
> -		&vc4_vec_tv_modes[conn_state->tv.legacy_mode];
> +	const struct vc4_vec_tv_mode *tv_mode;
>  	int idx, ret;
>  
>  	if (!drm_dev_enter(drm, &idx))
>  		return;
>  
> +	tv_mode = vc4_vec_tv_mode_lookup(conn_state->tv.mode);
> +	if (!tv_mode)
> +		goto err_dev_exit;
> +
>  	ret = pm_runtime_get_sync(&vec->pdev->dev);
>  	if (ret < 0) {
>  		DRM_ERROR("Failed to retain power domain: %d\n", ret);

If this (!tv_mode) condition is somehow triggered, the power management goes
somewhat crazy. vc4_vec_encoder_enable() cannot return an error, so when
vc4_vec_encoder_disable() is eventually called after a failed enable, it hangs
in pm_runtime_put() for quite a bit.

At least I think that's what's happening. Anyway, to solve this, I'd propose
this thing below:

Best regards,
Mateusz Kwiatkowski

Re: [PATCH v5 20/22] drm/vc4: vec: Convert to the new TV mode property

[Mateusz Kwiatkowski]

Hi Maxime,

Urgh. I cannot send e-mails apparently today, as I removed the second half of
the previous message. Here goes:

> @@ -454,13 +563,6 @@ static int vc4_vec_encoder_atomic_check(struct drm_encoder *encoder,
>  					struct drm_connector_state *conn_state)
>  {
>  	const struct drm_display_mode *mode = &crtc_state->adjusted_mode;

You could add here something like:

+	const struct vc4_vec_tv_mode *tv_mode =
+		vc4_vec_tv_mode_lookup(conn_state->tv.mode);
+
+	if (!tv_mode)
+		return -EINVAL;

This should explicitly make it impossible to enter the equivalent condition in
vc4_vec_encoder_enable() that causes the problem mentioned in the previous
e-mail.

This is probably basically impossible already, but I triggered that when testing
a follow-up change I'd like to post shortly.

> -	const struct vc4_vec_tv_mode *vec_mode;
> -
> -	vec_mode = &vc4_vec_tv_modes[conn_state->tv.legacy_mode];
> -
> -	if (conn_state->crtc &&
> -	    !drm_mode_equal(vec_mode->mode, &crtc_state->adjusted_mode))
> -		return -EINVAL;

If you're removing the reference mode, then I think you should at least add
checks that the crtc_clock is set to 13.5 MHz (it's otherwise ignored) and that
crtc_htotal is either 858 or 864 (using a switch over reference_mode->htotal as
I proposed in my comment to patch 19/22 would double as such check), as all
other values causes VEC to output garbage.

Best regards,
Mateusz Kwiatkowski

Re: [PATCH v5 21/22] drm/vc4: vec: Add support for more analog TV standards

[Mateusz Kwiatkowski]

Hi Maxime,

W dniu 13.10.2022 o 15:19, Maxime Ripard pisze:
> From: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>
>
> Add support for the following composite output modes (all of them are
> somewhat more obscure than the previously defined ones):
>
> - NTSC_443 - NTSC-style signal with the chroma subcarrier shifted to
>   4.43361875 MHz (the PAL subcarrier frequency). Never used for
>   broadcasting, but sometimes used as a hack to play NTSC content in PAL
>   regions (e.g. on VCRs).
> - PAL_N - PAL with alternative chroma subcarrier frequency,
>   3.58205625 MHz. Used as a broadcast standard in Argentina, Paraguay
>   and Uruguay to fit 576i50 with colour in 6 MHz channel raster.
> - PAL60 - 480i60 signal with PAL-style color at normal European PAL
>   frequency. Another non-standard, non-broadcast mode, used in similar
>   contexts as NTSC_443. Some displays support one but not the other.

The current version actually does not support PAL-60. Proper PAL-60 output from
VEC requires configuring it differently than for regular PAL. We have unified
the PAL and PAL-60 modes for the "TV mode" property, but the code here has not
been adjusted appropriately.

I'll try to submit an additional patch that fixes this shortly.

> - SECAM - French frequency-modulated analog color standard; also have
>   been broadcast in Eastern Europe and various parts of Africa and Asia.
>   Uses the same 576i50 timings as PAL.
>
> Also added some comments explaining color subcarrier frequency
> registers.
>
> Acked-by: Noralf Trønnes <noralf@tronnes.org>
> Signed-off-by: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
(snip)

Best regards,
Mateusz Kwiatkowski

[PATCH] drm/vc4: vec: Add support for PAL-60

[Mateusz Kwiatkowski]

Add support for the PAL-60 mode. Because there is no separate TV mode
property value for PAL-60, this requires matching the settings based on
the modeline in addition to just that property alone.

Signed-off-by: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>
---
This patch depends on patch
'[PATCH v5 21/22] drm/vc4: vec: Add support for more analog TV standards'
submitted by Maxime Ripard
(https://lore.kernel.org/dri-devel/20220728-rpi-analog-tv-properties-v5-21-d841cc64fe4b@cerno.tech/).

To Maxime: if you decide to post v6, feel free to include this in your patchset
instead if you want.
---
 drivers/gpu/drm/vc4/vc4_vec.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
index 88b4330bfa39..bbc41e502cc3 100644
--- a/drivers/gpu/drm/vc4/vc4_vec.c
+++ b/drivers/gpu/drm/vc4/vc4_vec.c
@@ -235,6 +235,7 @@ enum vc4_vec_tv_mode_id {
 
 struct vc4_vec_tv_mode {
 	unsigned int mode;
+	u16 expected_htotal;
 	u32 config0;
 	u32 config1;
 	u32 custom_freq;
@@ -270,37 +271,52 @@ static const struct debugfs_reg32 vec_regs[] = {
 static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = {
 	{
 		.mode = DRM_MODE_TV_MODE_NTSC,
+		.expected_htotal = 858,
 		.config0 = VEC_CONFIG0_NTSC_STD | VEC_CONFIG0_PDEN,
 		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
 	},
 	{
 		.mode = DRM_MODE_TV_MODE_NTSC_443,
+		.expected_htotal = 858,
 		.config0 = VEC_CONFIG0_NTSC_STD,
 		.config1 = VEC_CONFIG1_C_CVBS_CVBS | VEC_CONFIG1_CUSTOM_FREQ,
 		.custom_freq = 0x2a098acb,
 	},
 	{
 		.mode = DRM_MODE_TV_MODE_NTSC_J,
+		.expected_htotal = 858,
 		.config0 = VEC_CONFIG0_NTSC_STD,
 		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
 	},
 	{
 		.mode = DRM_MODE_TV_MODE_PAL,
+		.expected_htotal = 864,
 		.config0 = VEC_CONFIG0_PAL_BDGHI_STD,
 		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
 	},
+	{
+		/* PAL-60 */
+		.mode = DRM_MODE_TV_MODE_PAL,
+		.expected_htotal = 858,
+		.config0 = VEC_CONFIG0_PAL_M_STD,
+		.config1 = VEC_CONFIG1_C_CVBS_CVBS | VEC_CONFIG1_CUSTOM_FREQ,
+		.custom_freq = 0x2a098acb,
+	},
 	{
 		.mode = DRM_MODE_TV_MODE_PAL_M,
+		.expected_htotal = 858,
 		.config0 = VEC_CONFIG0_PAL_M_STD,
 		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
 	},
 	{
 		.mode = DRM_MODE_TV_MODE_PAL_N,
+		.expected_htotal = 864,
 		.config0 = VEC_CONFIG0_PAL_N_STD,
 		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
 	},
 	{
 		.mode = DRM_MODE_TV_MODE_SECAM,
+		.expected_htotal = 864,
 		.config0 = VEC_CONFIG0_SECAM_STD,
 		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
 		.custom_freq = 0x29c71c72,
@@ -308,14 +324,15 @@ static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = {
 };
 
 static inline const struct vc4_vec_tv_mode *
-vc4_vec_tv_mode_lookup(unsigned int mode)
+vc4_vec_tv_mode_lookup(unsigned int mode, u16 htotal)
 {
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(vc4_vec_tv_modes); i++) {
 		const struct vc4_vec_tv_mode *tv_mode = &vc4_vec_tv_modes[i];
 
-		if (tv_mode->mode == mode)
+		if (tv_mode->mode == mode &&
+		    tv_mode->expected_htotal == htotal)
 			return tv_mode;
 	}
 
@@ -394,6 +411,7 @@ vc4_vec_connector_set_property(struct drm_connector *connector,
 		break;
 
 	case VC4_VEC_TV_MODE_PAL:
+	case VC4_VEC_TV_MODE_PAL_60:
 		state->tv.mode = DRM_MODE_TV_MODE_PAL;
 		break;
 
@@ -551,13 +569,16 @@ static void vc4_vec_encoder_enable(struct drm_encoder *encoder,
 	struct drm_connector *connector = &vec->connector;
 	struct drm_connector_state *conn_state =
 		drm_atomic_get_new_connector_state(state, connector);
+	struct drm_display_mode *adjusted_mode =
+		&encoder->crtc->state->adjusted_mode;
 	const struct vc4_vec_tv_mode *tv_mode;
 	int idx, ret;
 
 	if (!drm_dev_enter(drm, &idx))
 		return;
 
-	tv_mode = vc4_vec_tv_mode_lookup(conn_state->tv.mode);
+	tv_mode = vc4_vec_tv_mode_lookup(conn_state->tv.mode,
+					 adjusted_mode->htotal);
 	if (!tv_mode)
 		goto err_dev_exit;
 

base-commit: e16415e3ddae9abb14a00793554a162403f9af6d
-- 
2.34.1

Re: [PATCH v5 13/22] drm/modes: Introduce the tv_mode property as a command-line option

[Noralf Trønnes]

Den 16.10.2022 19.51, skrev Mateusz Kwiatkowski:
> Hi Maxime, Noralf & everyone,
> 
> I'd like to address Noralf here in particular, and refer to these discussions
> from the past:
> 
> - https://lore.kernel.org/linux-arm-kernel/2f607c7d-6da1-c8df-1c02-8dd344a92343@gmail.com/
> - https://lore.kernel.org/linux-arm-kernel/9e76a508-f469-a54d-ecd7-b5868ca99af4@tronnes.org/
> 
>> @@ -2230,20 +2256,22 @@ struct drm_named_mode {
>>  	unsigned int xres;
>>  	unsigned int yres;
>>  	unsigned int flags;
>> +	unsigned int tv_mode;
>>  };
> 
> I saw that you (Noralf) opposed my suggestion about the DRM_MODE_TV_MODE_NONE
> enum value in enum drm drm_connector_tv_mode. I get your argumentation, and I'm
> not gonna argue, but I still don't like the fact that struct drm_named_mode now
> includes a field that is only relevant for analog TV modes, has no "none" value,
> and yet the type is supposed to be generic enough to be usable for other types
> of outputs as well.
> 
> It's true that it can just be ignored (as Maxime mentioned in his response to
> my e-mail linked above), and now the value of 0 corresponds to
> DRM_MODE_TV_MODE_NTSC, which is a rather sane default, but it still feels messy
> to me.
> 
> I'm not gonna force my opinion here, but I wanted to bring your attention to
> this issue, maybe you have some other solution in mind for this problem. Or if
> you don't see that as a problem at all, that's fine, too.
> 

I hadn't looked at this patch in detail before, but you're right this,
together with drm_atomic_helper_connector_tv_reset(), will overwrite
tv.mode unconditionally regardless of tv_mode being present in video= or
not. We need a tv_mode_specified flag like we have for bpp and refresh.

Noralf.

Re: [PATCH v5 20/22] drm/vc4: vec: Convert to the new TV mode property

[Noralf Trønnes]

Den 16.10.2022 20.52, skrev Mateusz Kwiatkowski:
> Hi Maxime,
> 
>>  static int vc4_vec_connector_get_modes(struct drm_connector *connector)
>>  {
>> -	struct drm_connector_state *state = connector->state;
>>  	struct drm_display_mode *mode;
>>  
>> -	mode = drm_mode_duplicate(connector->dev,
>> -				  vc4_vec_tv_modes[state->tv.legacy_mode].mode);
>> +	mode = drm_mode_analog_ntsc_480i(connector->dev);
>>  	if (!mode) {
>>  		DRM_ERROR("Failed to create a new display mode\n");
>>  		return -ENOMEM;
>>  	}
>>  
>> +	mode->type |= DRM_MODE_TYPE_PREFERRED;
>>  	drm_mode_probed_add(connector, mode);
>>  
>> -	return 1;
>> +	mode = drm_mode_analog_pal_576i(connector->dev);
>> +	if (!mode) {
>> +		DRM_ERROR("Failed to create a new display mode\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	drm_mode_probed_add(connector, mode);
>> +
>> +	return 2;
>> +}
> 
> Referencing those previous discussions:
> - https://lore.kernel.org/dri-devel/0255f7c6-0484-6456-350d-cf24f3fee5d6@tronnes.org/
> - https://lore.kernel.org/dri-devel/c8f8015a-75da-afa8-ca7f-b2b134cacd16@gmail.com/
> 
> Unconditionally setting the 480i mode as DRM_MODE_TYPE_PREFERRED causes Xorg
> (at least on current Raspberry Pi OS) to display garbage when
> video=Composite1:PAL is specified on the command line, so I'm afraid this won't
> do.
> 
> As I see it, there are three viable solutions for this issue:
> 
> a) Somehow query the video= command line option from this function, and set
>    DRM_MODE_TYPE_PREFERRED appropriately. This would break the abstraction
>    provided by global DRM code, but should work fine.
> 
> b) Modify drm_helper_probe_add_cmdline_mode() so that it sets
>    DRM_MODE_TYPE_PREFERRED in addition to DRM_MODE_TYPE_USERDEF. This seems
>    pretty robust, but affects the entire DRM subsystem, which may break
>    userspace in different ways.
> 
>    - Maybe this could be mitigated by adding some additional conditions, e.g.
>      setting the PREFERRED flag only if no modes are already flagged as such
>      and/or only if the cmdline mode is a named one (~= analog TV mode)
> 
> c) Forcing userspace (Xorg / Raspberry Pi OS) to get fixed and honor the USERDEF
>    flag.
> 
> Either way, hardcoding 480i as PREFERRED does not seem right.
> 

My solution for this is to look at tv.mode to know which mode to mark as
preferred. Maxime didn't like this since it changes things behind
userspace's back. I don't see how that can cause any problems for userspace.

If userspace uses atomic and sets tv_mode, it has to know which mode to
use before hand, so it doesn't look at the preferreded flag.

If it uses legacy and sets tv_mode, it can end up with a stale preferred
flag, but no worse than not having the flag or that ntsc is always
preferred.

If it doesn't change tv_mode, there's no problem, the preferred flag
doesn't change.

Noralf.

Re: [PATCH v5 16/22] drm/atomic-helper: Add a TV properties reset helper

[Noralf Trønnes]

Den 13.10.2022 15.19, skrev Maxime Ripard:
> The drm_tv_create_properties() function will create a bunch of properties,
> but it's up to each and every driver using that function to properly reset
> the state of these properties leading to inconsistent behaviours.
> 
> Let's create a helper that will take care of it.
> 
> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/gpu/drm/drm_atomic_state_helper.c | 75 +++++++++++++++++++++++++++++++
>  include/drm/drm_atomic_state_helper.h     |  1 +
>  2 files changed, 76 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index dfb57217253b..0373c3dc824b 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -481,6 +481,81 @@ void drm_atomic_helper_connector_tv_margins_reset(struct drm_connector *connecto
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_connector_tv_margins_reset);
>  
> +/**
> + * drm_atomic_helper_connector_tv_reset - Resets Analog TV connector properties
> + * @connector: DRM connector
> + *
> + * Resets the analog TV properties attached to a connector
> + */
> +void drm_atomic_helper_connector_tv_reset(struct drm_connector *connector)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_cmdline_mode *cmdline = &connector->cmdline_mode;
> +	struct drm_connector_state *state = connector->state;
> +	struct drm_property *prop;
> +	uint64_t val;
> +
> +	prop = dev->mode_config.tv_mode_property;
> +	if (prop)
> +		if (!drm_object_property_get_default_value(&connector->base,
> +							   prop, &val))
> +			state->tv.mode = val;
> +
> +	if (cmdline->tv_mode)
> +		state->tv.mode = cmdline->tv_mode;

This can't set ntsc now that the none value is gone.
But we need a tv_mode_specified flag as mentioned in the other patch.

Noralf.

> +
> +	prop = dev->mode_config.tv_select_subconnector_property;
> +	if (prop)
> +		if (!drm_object_property_get_default_value(&connector->base,
> +							   prop, &val))
> +			state->tv.select_subconnector = val;
> +
> +	prop = dev->mode_config.tv_subconnector_property;
> +	if (prop)
> +		if (!drm_object_property_get_default_value(&connector->base,
> +							   prop, &val))
> +			state->tv.subconnector = val;
> +
> +	prop = dev->mode_config.tv_brightness_property;
> +	if (prop)
> +		if (!drm_object_property_get_default_value(&connector->base,
> +							   prop, &val))
> +			state->tv.brightness = val;
> +
> +	prop = dev->mode_config.tv_contrast_property;
> +	if (prop)
> +		if (!drm_object_property_get_default_value(&connector->base,
> +							   prop, &val))
> +			state->tv.contrast = val;
> +
> +	prop = dev->mode_config.tv_flicker_reduction_property;
> +	if (prop)
> +		if (!drm_object_property_get_default_value(&connector->base,
> +							   prop, &val))
> +			state->tv.flicker_reduction = val;
> +
> +	prop = dev->mode_config.tv_overscan_property;
> +	if (prop)
> +		if (!drm_object_property_get_default_value(&connector->base,
> +							   prop, &val))
> +			state->tv.overscan = val;
> +
> +	prop = dev->mode_config.tv_saturation_property;
> +	if (prop)
> +		if (!drm_object_property_get_default_value(&connector->base,
> +							   prop, &val))
> +			state->tv.saturation = val;
> +
> +	prop = dev->mode_config.tv_hue_property;
> +	if (prop)
> +		if (!drm_object_property_get_default_value(&connector->base,
> +							   prop, &val))
> +			state->tv.hue = val;
> +
> +	drm_atomic_helper_connector_tv_margins_reset(connector);
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_connector_tv_reset);
> +
>  /**
>   * __drm_atomic_helper_connector_duplicate_state - copy atomic connector state
>   * @connector: connector object
> diff --git a/include/drm/drm_atomic_state_helper.h b/include/drm/drm_atomic_state_helper.h
> index 192766656b88..c8fbce795ee7 100644
> --- a/include/drm/drm_atomic_state_helper.h
> +++ b/include/drm/drm_atomic_state_helper.h
> @@ -70,6 +70,7 @@ void __drm_atomic_helper_connector_state_reset(struct drm_connector_state *conn_
>  void __drm_atomic_helper_connector_reset(struct drm_connector *connector,
>  					 struct drm_connector_state *conn_state);
>  void drm_atomic_helper_connector_reset(struct drm_connector *connector);
> +void drm_atomic_helper_connector_tv_reset(struct drm_connector *connector);
>  void drm_atomic_helper_connector_tv_margins_reset(struct drm_connector *connector);
>  void
>  __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
> 

Re: [PATCH v5 12/22] drm/connector: Add a function to lookup a TV mode by its name

[Noralf Trønnes]

Den 13.10.2022 15.18, skrev Maxime Ripard:
> As part of the command line parsing rework coming in the next patches,
> we'll need to lookup drm_connector_tv_mode values by their name, already
> defined in drm_tv_mode_enum_list.
> 
> In order to avoid any code duplication, let's do a function that will
> perform a lookup of a TV mode name and return its value.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/gpu/drm/drm_connector.c | 24 ++++++++++++++++++++++++
>  include/drm/drm_connector.h     |  2 ++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 820f4c730b38..30611c616435 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -991,6 +991,30 @@ static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = {
>  };
>  DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list)
>  
> +/**
> + * drm_get_tv_mode_from_name - Translates a TV mode name into its enum value
> + * @name: TV Mode name we want to convert
> + * @len: Length of @name
> + *
> + * Translates @name into an enum drm_connector_tv_mode.
> + *
> + * Returns: the enum value on success, a negative errno otherwise.
> + */
> +int drm_get_tv_mode_from_name(const char *name, size_t len)

Do we really need to pass in length here?
item->name has to always be NUL terminated otherwise things would break
elsewhere, so it shouldn't be necessary AFAICS.

Noralf.

> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(drm_tv_mode_enum_list); i++) {
> +		const struct drm_prop_enum_list *item = &drm_tv_mode_enum_list[i];
> +
> +		if (strlen(item->name) == len && !strncmp(item->name, name, len))
> +			return item->type;
> +	}
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(drm_get_tv_mode_from_name);
> +
>  static const struct drm_prop_enum_list drm_tv_select_enum_list[] = {
>  	{ DRM_MODE_SUBCONNECTOR_Automatic, "Automatic" }, /* DVI-I and TV-out */
>  	{ DRM_MODE_SUBCONNECTOR_Composite, "Composite" }, /* TV-out */
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index a501db7d2222..a33f24a76738 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1864,6 +1864,8 @@ const char *drm_get_dp_subconnector_name(int val);
>  const char *drm_get_content_protection_name(int val);
>  const char *drm_get_hdcp_content_type_name(int val);
>  
> +int drm_get_tv_mode_from_name(const char *name, size_t len);
> +
>  int drm_mode_create_dvi_i_properties(struct drm_device *dev);
>  void drm_connector_attach_dp_subconnector_property(struct drm_connector *connector);
>  
> 

Re: [PATCH v5 20/22] drm/vc4: vec: Convert to the new TV mode property

[Noralf Trønnes]

Den 13.10.2022 15.19, skrev Maxime Ripard:
> Now that the core can deal fine with analog TV modes, let's convert the vc4
> VEC driver to leverage those new features.
> 
> We've added some backward compatibility to support the old TV mode property
> and translate it into the new TV norm property. We're also making use of
> the new analog TV atomic_check helper to make sure we trigger a modeset
> whenever the TV mode is updated.
> 
> Acked-by: Noralf Trønnes <noralf@tronnes.org>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> 
> ---

> @@ -276,19 +292,96 @@ static void vc4_vec_connector_reset(struct drm_connector *connector)
>  
>  static int vc4_vec_connector_get_modes(struct drm_connector *connector)
>  {
> -	struct drm_connector_state *state = connector->state;
>  	struct drm_display_mode *mode;
>  
> -	mode = drm_mode_duplicate(connector->dev,
> -				  vc4_vec_tv_modes[state->tv.legacy_mode].mode);
> +	mode = drm_mode_analog_ntsc_480i(connector->dev);
>  	if (!mode) {
>  		DRM_ERROR("Failed to create a new display mode\n");
>  		return -ENOMEM;
>  	}
>  
> +	mode->type |= DRM_MODE_TYPE_PREFERRED;
>  	drm_mode_probed_add(connector, mode);
>  
> -	return 1;
> +	mode = drm_mode_analog_pal_576i(connector->dev);
> +	if (!mode) {
> +		DRM_ERROR("Failed to create a new display mode\n");
> +		return -ENOMEM;

I just remembered that you can't return an error from .get_modes, it
should only return the number of modes added. From doc:

	 * RETURNS:
	 *
	 * The number of modes added by calling drm_mode_probed_add().

See also the use of count in drm_helper_probe_single_connector_modes().

Patch 14 and 22 has the same issue.

Noralf.

> +	}
> +
> +	drm_mode_probed_add(connector, mode);
> +
> +	return 2;
> +}
> +
> +static int
> +vc4_vec_connector_set_property(struct drm_connector *connector,
> +			       struct drm_connector_state *state,
> +			       struct drm_property *property,
> +			       uint64_t val)
> +{
> +	struct vc4_vec *vec = connector_to_vc4_vec(connector);
> +
> +	if (property != vec->legacy_tv_mode_property)
> +		return -EINVAL;
> +
> +	switch (val) {
> +	case VC4_VEC_TV_MODE_NTSC:
> +		state->tv.mode = DRM_MODE_TV_MODE_NTSC;
> +		break;
> +
> +	case VC4_VEC_TV_MODE_NTSC_J:
> +		state->tv.mode = DRM_MODE_TV_MODE_NTSC_J;
> +		break;
> +
> +	case VC4_VEC_TV_MODE_PAL:
> +		state->tv.mode = DRM_MODE_TV_MODE_PAL;
> +		break;
> +
> +	case VC4_VEC_TV_MODE_PAL_M:
> +		state->tv.mode = DRM_MODE_TV_MODE_PAL_M;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +vc4_vec_connector_get_property(struct drm_connector *connector,
> +			       const struct drm_connector_state *state,
> +			       struct drm_property *property,
> +			       uint64_t *val)
> +{
> +	struct vc4_vec *vec = connector_to_vc4_vec(connector);
> +
> +	if (property != vec->legacy_tv_mode_property)
> +		return -EINVAL;
> +
> +	switch (state->tv.mode) {
> +	case DRM_MODE_TV_MODE_NTSC:
> +		*val = VC4_VEC_TV_MODE_NTSC;
> +		break;
> +
> +	case DRM_MODE_TV_MODE_NTSC_J:
> +		*val = VC4_VEC_TV_MODE_NTSC_J;
> +		break;
> +
> +	case DRM_MODE_TV_MODE_PAL:
> +		*val = VC4_VEC_TV_MODE_PAL;
> +		break;
> +
> +	case DRM_MODE_TV_MODE_PAL_M:
> +		*val = VC4_VEC_TV_MODE_PAL_M;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
>  }
>  
>  static const struct drm_connector_funcs vc4_vec_connector_funcs = {
> @@ -297,15 +390,19 @@ static const struct drm_connector_funcs vc4_vec_connector_funcs = {
>  	.reset = vc4_vec_connector_reset,
>  	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +	.atomic_get_property = vc4_vec_connector_get_property,
> +	.atomic_set_property = vc4_vec_connector_set_property,
>  };
>  
>  static const struct drm_connector_helper_funcs vc4_vec_connector_helper_funcs = {
> +	.atomic_check = drm_atomic_helper_connector_tv_check,
>  	.get_modes = vc4_vec_connector_get_modes,
>  };
>  
>  static int vc4_vec_connector_init(struct drm_device *dev, struct vc4_vec *vec)
>  {
>  	struct drm_connector *connector = &vec->connector;
> +	struct drm_property *prop;
>  	int ret;
>  
>  	connector->interlace_allowed = true;
> @@ -318,8 +415,17 @@ static int vc4_vec_connector_init(struct drm_device *dev, struct vc4_vec *vec)
>  	drm_connector_helper_add(connector, &vc4_vec_connector_helper_funcs);
>  
>  	drm_object_attach_property(&connector->base,
> -				   dev->mode_config.legacy_tv_mode_property,
> -				   VC4_VEC_TV_MODE_NTSC);
> +				   dev->mode_config.tv_mode_property,
> +				   DRM_MODE_TV_MODE_NTSC);
> +
> +	prop = drm_property_create_enum(dev, 0, "mode",
> +					legacy_tv_mode_names,
> +					ARRAY_SIZE(legacy_tv_mode_names));
> +	if (!prop)
> +		return -ENOMEM;
> +	vec->legacy_tv_mode_property = prop;
> +
> +	drm_object_attach_property(&connector->base, prop, VC4_VEC_TV_MODE_NTSC);
>  
>  	drm_connector_attach_encoder(connector, &vec->encoder.base);
>  
> @@ -366,13 +472,16 @@ static void vc4_vec_encoder_enable(struct drm_encoder *encoder,
>  	struct drm_connector *connector = &vec->connector;
>  	struct drm_connector_state *conn_state =
>  		drm_atomic_get_new_connector_state(state, connector);
> -	const struct vc4_vec_tv_mode *tv_mode =
> -		&vc4_vec_tv_modes[conn_state->tv.legacy_mode];
> +	const struct vc4_vec_tv_mode *tv_mode;
>  	int idx, ret;
>  
>  	if (!drm_dev_enter(drm, &idx))
>  		return;
>  
> +	tv_mode = vc4_vec_tv_mode_lookup(conn_state->tv.mode);
> +	if (!tv_mode)
> +		goto err_dev_exit;
> +
>  	ret = pm_runtime_get_sync(&vec->pdev->dev);
>  	if (ret < 0) {
>  		DRM_ERROR("Failed to retain power domain: %d\n", ret);
> @@ -454,13 +563,6 @@ static int vc4_vec_encoder_atomic_check(struct drm_encoder *encoder,
>  					struct drm_connector_state *conn_state)
>  {
>  	const struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> -	const struct vc4_vec_tv_mode *vec_mode;
> -
> -	vec_mode = &vc4_vec_tv_modes[conn_state->tv.legacy_mode];
> -
> -	if (conn_state->crtc &&
> -	    !drm_mode_equal(vec_mode->mode, &crtc_state->adjusted_mode))
> -		return -EINVAL;
>  
>  	if (mode->crtc_hdisplay % 4)
>  		return -EINVAL;
> @@ -554,13 +656,6 @@ static const struct of_device_id vc4_vec_dt_match[] = {
>  	{ /* sentinel */ },
>  };
>  
> -static const char * const tv_mode_names[] = {
> -	[VC4_VEC_TV_MODE_NTSC] = "NTSC",
> -	[VC4_VEC_TV_MODE_NTSC_J] = "NTSC-J",
> -	[VC4_VEC_TV_MODE_PAL] = "PAL",
> -	[VC4_VEC_TV_MODE_PAL_M] = "PAL-M",
> -};
> -
>  static int vc4_vec_bind(struct device *dev, struct device *master, void *data)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> @@ -568,9 +663,11 @@ static int vc4_vec_bind(struct device *dev, struct device *master, void *data)
>  	struct vc4_vec *vec;
>  	int ret;
>  
> -	ret = drm_mode_create_tv_properties_legacy(drm,
> -						   ARRAY_SIZE(tv_mode_names),
> -						   tv_mode_names);
> +	ret = drm_mode_create_tv_properties(drm,
> +					    BIT(DRM_MODE_TV_MODE_NTSC) |
> +					    BIT(DRM_MODE_TV_MODE_NTSC_J) |
> +					    BIT(DRM_MODE_TV_MODE_PAL) |
> +					    BIT(DRM_MODE_TV_MODE_PAL_M));
>  	if (ret)
>  		return ret;
>  
> 

Re: [PATCH v5 08/22] drm/modes: Move named modes parsing to a separate function

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1995 bytes --]

On Sun, Oct 16, 2022 at 06:11:21PM +0200, Noralf Trønnes wrote:
> Den 13.10.2022 15.18, skrev Maxime Ripard:
> > The current construction of the named mode parsing doesn't allow to extend
> > it easily. Let's move it to a separate function so we can add more
> > parameters and modes.
> > 
> > In order for the tests to still pass, some extra checks are needed, so
> > it's not a 1:1 move.
> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > 
> 
> I was hoping that someone else would step up and review these parser
> patches since the parser code is rather difficult to read, for me at
> least. I have studied it now, so I'll give it a try.
> 
> > ---
> > Changes in v4:
> > - Fold down all the named mode patches that were split into a single
> >   patch again to maintain bisectability
> > ---
> >  drivers/gpu/drm/drm_modes.c | 73 ++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 62 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > index c0dceff51cac..2f020ef2ddf2 100644
> > --- a/drivers/gpu/drm/drm_modes.c
> > +++ b/drivers/gpu/drm/drm_modes.c
> > @@ -2229,6 +2229,55 @@ static const char * const drm_named_modes_whitelist[] = {
> >  	"PAL",
> >  };
> >  
> > +static int drm_mode_parse_cmdline_named_mode(const char *name,
> > +					     unsigned int name_end,
> > +					     struct drm_cmdline_mode *cmdline_mode)
> > +{
> > +	unsigned int i;
> > +
> > +	if (!name_end)
> > +		return 0;
> 
> name_end can't be zero since the argument is checked before calling this
> function.

I'd really like to keep it in though. At least, we know by looking at
this small function that we're going to be safe all the time, no matter
what the caller does.

And if the caller wants to check it as well, fine, it's only a simple
comparison ran once or twice at boot, it's not like it's in a hot-path.

I've addressed your other comments, thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v5 06/22] drm/modes: Add a function to generate analog display modes

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4518 bytes --]

Hi,

On Sun, Oct 16, 2022 at 07:34:12PM +0200, Mateusz Kwiatkowski wrote:
> Hi Maxime & everyone,
> 
> Sorry for being inactive in the discussions about this patchset for the last
> couple of weeks.
> 
> > +const static struct analog_parameters tv_modes_parameters[] = {
> > +	TV_MODE_PARAMETER(DRM_MODE_ANALOG_NTSC,
> > +			  NTSC_LINES_NUMBER,
> > +			  NTSC_LINE_DURATION_NS,
> > +			  PARAM_RANGE(NTSC_HACT_DURATION_MIN_NS,
> > +				      NTSC_HACT_DURATION_TYP_NS,
> > +				      NTSC_HACT_DURATION_MAX_NS),
> > +			  PARAM_RANGE(NTSC_HFP_DURATION_MIN_NS,
> > +				      NTSC_HFP_DURATION_TYP_NS,
> > +				      NTSC_HFP_DURATION_MAX_NS),
> > +			  PARAM_RANGE(NTSC_HSLEN_DURATION_MIN_NS,
> > +				      NTSC_HSLEN_DURATION_TYP_NS,
> > +				      NTSC_HSLEN_DURATION_MAX_NS),
> > +			  PARAM_RANGE(NTSC_HBP_DURATION_MIN_NS,
> > +				      NTSC_HBP_DURATION_TYP_NS,
> > +				      NTSC_HBP_DURATION_MAX_NS),
> > +			  PARAM_RANGE(NTSC_HBLK_DURATION_MIN_NS,
> > +				      NTSC_HBLK_DURATION_TYP_NS,
> > +				      NTSC_HBLK_DURATION_MAX_NS),
> > +			  16,
> > +			  PARAM_FIELD(3, 3),
> > +			  PARAM_FIELD(3, 3),
> > +			  PARAM_FIELD(16, 17)),
> > +	TV_MODE_PARAMETER(DRM_MODE_ANALOG_PAL,
> > +			  PAL_LINES_NUMBER,
> > +			  PAL_LINE_DURATION_NS,
> > +			  PARAM_RANGE(PAL_HACT_DURATION_MIN_NS,
> > +				      PAL_HACT_DURATION_TYP_NS,
> > +				      PAL_HACT_DURATION_MAX_NS),
> > +			  PARAM_RANGE(PAL_HFP_DURATION_MIN_NS,
> > +				      PAL_HFP_DURATION_TYP_NS,
> > +				      PAL_HFP_DURATION_MAX_NS),
> > +			  PARAM_RANGE(PAL_HSLEN_DURATION_MIN_NS,
> > +				      PAL_HSLEN_DURATION_TYP_NS,
> > +				      PAL_HSLEN_DURATION_MAX_NS),
> > +			  PARAM_RANGE(PAL_HBP_DURATION_MIN_NS,
> > +				      PAL_HBP_DURATION_TYP_NS,
> > +				      PAL_HBP_DURATION_MAX_NS),
> > +			  PARAM_RANGE(PAL_HBLK_DURATION_MIN_NS,
> > +				      PAL_HBLK_DURATION_TYP_NS,
> > +				      PAL_HBLK_DURATION_MAX_NS),
> > +			  12,
> > +
> > +			  /*
> > +			   * The front porch is actually 6 short sync
> > +			   * pulses for the even field, and 5 for the
> > +			   * odd field. Each sync takes half a life so
> > +			   * the odd field front porch is shorter by
> > +			   * half a line.
> > +			   *
> > +			   * In progressive, we're supposed to use 6
> > +			   * pulses, so we're fine there
> > +			   */
> > +			  PARAM_FIELD(3, 2),
> > +
> > +			  /*
> > +			   * The vsync length is 5 long sync pulses,
> > +			   * each field taking half a line. We're
> > +			   * shorter for both fields by half a line.
> > +			   *
> > +			   * In progressive, we're supposed to use 5
> > +			   * pulses, so we're off by half
> > +			   * a line.
> > +			   *
> > +			   * In interlace, we're now off by half a line
> > +			   * for the even field and one line for the odd
> > +			   * field.
> > +			   */
> > +			  PARAM_FIELD(3, 3),
> > +
> > +			  /*
> > +			   * The back porch starts with post-equalizing
> > +			   * pulses, consisting in 5 short sync pulses
> > +			   * for the even field, 4 for the odd field. In
> > +			   * progressive, it's 5 short syncs.
> > +			   *
> > +			   * In progressive, we thus have 2.5 lines,
> > +			   * plus the 0.5 line we were missing
> > +			   * previously, so we should use 3 lines.
> > +			   *
> > +			   * In interlace, the even field is in the
> > +			   * exact same case than progressive. For the
> > +			   * odd field, we should be using 2 lines but
> > +			   * we're one line short, so we'll make up for
> > +			   * it here by using 3.
> > +			   *
> > +			   * The entire blanking area is supposed to
> > +			   * take 25 lines, so we also need to account
> > +			   * for the rest of the blanking area that
> > +			   * can't be in either the front porch or sync
> > +			   * period.
> > +			   */
> > +			  PARAM_FIELD(19, 20)),
> > +};
> 
> Nit: setting vbp limits like that makes it impossible to use
> drm_analog_tv_mode() to generate modes that include the VBI for e.g. emitting
> teletext.
> 
> This probably doesn't matter, as it can still be created as a custom mode from
> userspace, hence I'm mentioning it as a nit.

Yeah, I think it's out of scope at least for now. Also, the compositor
should probably be aware of the margins being used to put the VBI data,
so expecting userspace to come up with the mode is probably best?

> > +		 * By convention, NSTC (aka 525/60) systems start with
> 
> Typo: s/NSTC/NTSC/

Fixed, thanks
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v5 19/22] drm/vc4: vec: Check for VEC output constraints

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4104 bytes --]

Hi,

On Sun, Oct 16, 2022 at 08:16:32PM +0200, Mateusz Kwiatkowski wrote:
> W dniu 13.10.2022 o 15:19, Maxime Ripard pisze:
> > From: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>
> >
> > The VEC can accept pretty much any relatively reasonable mode, but still
> > has a bunch of constraints to meet.
> >
> > Let's create an atomic_check() implementation that will make sure we
> > don't end up accepting a non-functional mode.
> >
> > Acked-by: Noralf Trønnes <noralf@tronnes.org>
> > Signed-off-by: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >  drivers/gpu/drm/vc4/vc4_vec.c | 48 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 48 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
> > index 90e375a8a8f9..1fcb7baf874e 100644
> > --- a/drivers/gpu/drm/vc4/vc4_vec.c
> > +++ b/drivers/gpu/drm/vc4/vc4_vec.c
> > @@ -453,6 +453,7 @@ static int vc4_vec_encoder_atomic_check(struct drm_encoder *encoder,
> >                      struct drm_crtc_state *crtc_state,
> >                      struct drm_connector_state *conn_state)
> >  {
> > +    const struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> >      const struct vc4_vec_tv_mode *vec_mode;
> >  
> >      vec_mode = &vc4_vec_tv_modes[conn_state->tv.legacy_mode];
> > @@ -461,6 +462,53 @@ static int vc4_vec_encoder_atomic_check(struct drm_encoder *encoder,
> >          !drm_mode_equal(vec_mode->mode, &crtc_state->adjusted_mode))
> >          return -EINVAL;
> >  
> > +    if (mode->crtc_hdisplay % 4)
> > +        return -EINVAL;
> > +
> > +    if (!(mode->crtc_hsync_end - mode->crtc_hsync_start))
> > +        return -EINVAL;
> > +
> > +    switch (mode->vtotal) {
> > +    case 525:
> > +        if (mode->crtc_vtotal > 262)
> > +            return -EINVAL;
> > +
> > +        if (mode->crtc_vdisplay < 1 || mode->crtc_vdisplay > 253)
> > +            return -EINVAL;
> > +
> > +        if (!(mode->crtc_vsync_start - mode->crtc_vdisplay))
> > +            return -EINVAL;
> > +
> > +        if ((mode->crtc_vsync_end - mode->crtc_vsync_start) != 3)
> > +            return -EINVAL;
> > +
> > +        if ((mode->crtc_vtotal - mode->crtc_vsync_end) < 4)
> > +            return -EINVAL;
> > +
> > +        break;
> > +
> > +    case 625:
> > +        if (mode->crtc_vtotal > 312)
> > +            return -EINVAL;
> > +
> > +        if (mode->crtc_vdisplay < 1 || mode->crtc_vdisplay > 305)
> > +            return -EINVAL;
> > +
> > +        if (!(mode->crtc_vsync_start - mode->crtc_vdisplay))
> > +            return -EINVAL;
> > +
> > +        if ((mode->crtc_vsync_end - mode->crtc_vsync_start) != 3)
> > +            return -EINVAL;
> > +
> > +        if ((mode->crtc_vtotal - mode->crtc_vsync_end) < 2)
> > +            return -EINVAL;
> > +
> > +        break;
> > +
> > +    default:
> > +        return -EINVAL;
> > +    }
> > +
> >      return 0;
> >  }
> >  
> >
> 
> In my original version of this function
> (https://github.com/raspberrypi/linux/pull/4406/files) the switch is over
> reference_mode->vtotal, not mode->vtotal. This was intended to explicitly allow
> a different value of mode->vtotal, to support non-standard modes, such as "fake"
> 525 lines with SECAM encoding, or the progressive modes.
>
> You're switching over mode->vtotal, which makes specifying those impossible.
> I don't think we should limit the users like that.
>
> We're removing reference_mode in patch 20/22, so adding a switch over
> reference_mode->vtotal is probably not a good idea in that case I'd switch
> over mode->htotal instead: 858 for "NTSC" and 864 for "PAL". This may seem a bit
> weird, but any other value of htotal causes the VEC to output garbage anyway.

Ack, I'll change it.

If it ever causes an issue, we can always switch back to a reference
mode anyway. We'd just have to call drm_mode_analog_tv at each
atomic_check so I'd rather avoid the overhead if we can

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] drm/vc4: vec: Add support for PAL-60

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 738 bytes --]

Hi,

On Sun, Oct 16, 2022 at 09:46:49PM +0200, Mateusz Kwiatkowski wrote:
> @@ -308,14 +324,15 @@ static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = {
>  };
>  
>  static inline const struct vc4_vec_tv_mode *
> -vc4_vec_tv_mode_lookup(unsigned int mode)
> +vc4_vec_tv_mode_lookup(unsigned int mode, u16 htotal)
>  {
>  	unsigned int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(vc4_vec_tv_modes); i++) {
>  		const struct vc4_vec_tv_mode *tv_mode = &vc4_vec_tv_modes[i];
>  
> -		if (tv_mode->mode == mode)
> +		if (tv_mode->mode == mode &&
> +		    tv_mode->expected_htotal == htotal)
>  			return tv_mode;

Is there any reason we're not using the refresh rate to filter this? It
seems more natural to me.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v5 12/22] drm/connector: Add a function to lookup a TV mode by its name

[Maxime Ripard]

On Mon, Oct 17, 2022 at 12:44:45PM +0200, Noralf Trønnes wrote:
> Den 13.10.2022 15.18, skrev Maxime Ripard:
> > As part of the command line parsing rework coming in the next patches,
> > we'll need to lookup drm_connector_tv_mode values by their name, already
> > defined in drm_tv_mode_enum_list.
> > 
> > In order to avoid any code duplication, let's do a function that will
> > perform a lookup of a TV mode name and return its value.
> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >  drivers/gpu/drm/drm_connector.c | 24 ++++++++++++++++++++++++
> >  include/drm/drm_connector.h     |  2 ++
> >  2 files changed, 26 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index 820f4c730b38..30611c616435 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -991,6 +991,30 @@ static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = {
> >  };
> >  DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list)
> >  
> > +/**
> > + * drm_get_tv_mode_from_name - Translates a TV mode name into its enum value
> > + * @name: TV Mode name we want to convert
> > + * @len: Length of @name
> > + *
> > + * Translates @name into an enum drm_connector_tv_mode.
> > + *
> > + * Returns: the enum value on success, a negative errno otherwise.
> > + */
> > +int drm_get_tv_mode_from_name(const char *name, size_t len)
> 
> Do we really need to pass in length here? item->name has to always be
> NUL terminated otherwise things would break elsewhere, so it shouldn't
> be necessary AFAICS.

The only user so far is the command-line parsing code, and we might very
well have an option after the tv_mode, something like
720x480i,tv_mode=NTSC,rotate=180

In this case, we won't get a NULL-terminated name.

Maxime

Re: [PATCH v5 20/22] drm/vc4: vec: Convert to the new TV mode property

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3927 bytes --]

On Mon, Oct 17, 2022 at 12:31:31PM +0200, Noralf Trønnes wrote:
> Den 16.10.2022 20.52, skrev Mateusz Kwiatkowski:
> >>  static int vc4_vec_connector_get_modes(struct drm_connector *connector)
> >>  {
> >> -	struct drm_connector_state *state = connector->state;
> >>  	struct drm_display_mode *mode;
> >>  
> >> -	mode = drm_mode_duplicate(connector->dev,
> >> -				  vc4_vec_tv_modes[state->tv.legacy_mode].mode);
> >> +	mode = drm_mode_analog_ntsc_480i(connector->dev);
> >>  	if (!mode) {
> >>  		DRM_ERROR("Failed to create a new display mode\n");
> >>  		return -ENOMEM;
> >>  	}
> >>  
> >> +	mode->type |= DRM_MODE_TYPE_PREFERRED;
> >>  	drm_mode_probed_add(connector, mode);
> >>  
> >> -	return 1;
> >> +	mode = drm_mode_analog_pal_576i(connector->dev);
> >> +	if (!mode) {
> >> +		DRM_ERROR("Failed to create a new display mode\n");
> >> +		return -ENOMEM;
> >> +	}
> >> +
> >> +	drm_mode_probed_add(connector, mode);
> >> +
> >> +	return 2;
> >> +}
> > 
> > Referencing those previous discussions:
> > - https://lore.kernel.org/dri-devel/0255f7c6-0484-6456-350d-cf24f3fee5d6@tronnes.org/
> > - https://lore.kernel.org/dri-devel/c8f8015a-75da-afa8-ca7f-b2b134cacd16@gmail.com/
> > 
> > Unconditionally setting the 480i mode as DRM_MODE_TYPE_PREFERRED causes Xorg
> > (at least on current Raspberry Pi OS) to display garbage when
> > video=Composite1:PAL is specified on the command line, so I'm afraid this won't
> > do.
> > 
> > As I see it, there are three viable solutions for this issue:
> > 
> > a) Somehow query the video= command line option from this function, and set
> >    DRM_MODE_TYPE_PREFERRED appropriately. This would break the abstraction
> >    provided by global DRM code, but should work fine.
> > 
> > b) Modify drm_helper_probe_add_cmdline_mode() so that it sets
> >    DRM_MODE_TYPE_PREFERRED in addition to DRM_MODE_TYPE_USERDEF. This seems
> >    pretty robust, but affects the entire DRM subsystem, which may break
> >    userspace in different ways.
> > 
> >    - Maybe this could be mitigated by adding some additional conditions, e.g.
> >      setting the PREFERRED flag only if no modes are already flagged as such
> >      and/or only if the cmdline mode is a named one (~= analog TV mode)
> > 
> > c) Forcing userspace (Xorg / Raspberry Pi OS) to get fixed and honor the USERDEF
> >    flag.
> > 
> > Either way, hardcoding 480i as PREFERRED does not seem right.
> > 
> 
> My solution for this is to look at tv.mode to know which mode to mark as
> preferred. Maxime didn't like this since it changes things behind
> userspace's back. I don't see how that can cause any problems for userspace.
> 
> If userspace uses atomic and sets tv_mode, it has to know which mode to
> use before hand, so it doesn't look at the preferreded flag.
> 
> If it uses legacy and sets tv_mode, it can end up with a stale preferred
> flag, but no worse than not having the flag or that ntsc is always
> preferred.
> 
> If it doesn't change tv_mode, there's no problem, the preferred flag
> doesn't change.

I don't like it because I just see no way to make this reliable. When we
set tv_mode, we're not only going to change the preferred flag, but also
the order of the modes to make the preferred mode first.

Since we just changed the mode lists, we also want to send a hotplug
event to userspace so that it gets notified of it. It will pick up the
new preferred mode, great.

But what if it doesn't? There's no requirement for userspace to handle
hotplug events, and Kodi won't for example. So we just changed the TV
mode but not the actual mode, and that's it. It's just as broken for
Kodi as it is for X11 right now.

If we can't get a bullet-proof solution, then I'm not convinced it's
worth addressing. Especially since it's already the current state, and
it doesn't seem to bother a lot of people.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v5 12/22] drm/connector: Add a function to lookup a TV mode by its name

[Noralf Trønnes]

Den 18.10.2022 11.33, skrev Maxime Ripard:
> On Mon, Oct 17, 2022 at 12:44:45PM +0200, Noralf Trønnes wrote:
>> Den 13.10.2022 15.18, skrev Maxime Ripard:
>>> As part of the command line parsing rework coming in the next patches,
>>> we'll need to lookup drm_connector_tv_mode values by their name, already
>>> defined in drm_tv_mode_enum_list.
>>>
>>> In order to avoid any code duplication, let's do a function that will
>>> perform a lookup of a TV mode name and return its value.
>>>
>>> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>>> ---
>>>  drivers/gpu/drm/drm_connector.c | 24 ++++++++++++++++++++++++
>>>  include/drm/drm_connector.h     |  2 ++
>>>  2 files changed, 26 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>>> index 820f4c730b38..30611c616435 100644
>>> --- a/drivers/gpu/drm/drm_connector.c
>>> +++ b/drivers/gpu/drm/drm_connector.c
>>> @@ -991,6 +991,30 @@ static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = {
>>>  };
>>>  DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list)
>>>  
>>> +/**
>>> + * drm_get_tv_mode_from_name - Translates a TV mode name into its enum value
>>> + * @name: TV Mode name we want to convert
>>> + * @len: Length of @name
>>> + *
>>> + * Translates @name into an enum drm_connector_tv_mode.
>>> + *
>>> + * Returns: the enum value on success, a negative errno otherwise.
>>> + */
>>> +int drm_get_tv_mode_from_name(const char *name, size_t len)
>>
>> Do we really need to pass in length here? item->name has to always be
>> NUL terminated otherwise things would break elsewhere, so it shouldn't
>> be necessary AFAICS.
> 
> The only user so far is the command-line parsing code, and we might very
> well have an option after the tv_mode, something like
> 720x480i,tv_mode=NTSC,rotate=180
> 
> In this case, we won't get a NULL-terminated name.
> 

My point is that item->name will always be NUL terminated so strcmp()
will never look past that.

Noralf.

Re: [PATCH] drm/vc4: vec: Add support for PAL-60

[Mateusz Kwiatkowski]

Hi Maxime,

W dniu 18.10.2022 o 10:31, Maxime Ripard pisze:
> Hi,
>
> On Sun, Oct 16, 2022 at 09:46:49PM +0200, Mateusz Kwiatkowski wrote:
>> @@ -308,14 +324,15 @@ static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = {
>>  };
>>  
>>  static inline const struct vc4_vec_tv_mode *
>> -vc4_vec_tv_mode_lookup(unsigned int mode)
>> +vc4_vec_tv_mode_lookup(unsigned int mode, u16 htotal)
>>  {
>>  	unsigned int i;
>>  
>>  	for (i = 0; i < ARRAY_SIZE(vc4_vec_tv_modes); i++) {
>>  		const struct vc4_vec_tv_mode *tv_mode = &vc4_vec_tv_modes[i];
>>  
>> -		if (tv_mode->mode == mode)
>> +		if (tv_mode->mode == mode &&
>> +		    tv_mode->expected_htotal == htotal)
>>  			return tv_mode;
>
> Is there any reason we're not using the refresh rate to filter this? It
> seems more natural to me.

Let me give you an example first.

There are actually two ways to configure PAL-60-ish mode on VC4/VEC:

a) Modeline 13.5 720 734 798 858 480 487 493 525 Interlace, standard registers
   set to VEC_CONFIG0_PAL_M_STD, custom frequency enabled and set to 0x2a098acb;
   Setting the standard registers to "PAL-M" puts the VEC in true 59.94 Hz mode,
   so the video timings are identical as for NTSC (or PAL-M), and the custom
   frequency makes the color subcarrier compatible with regular PAL receivers.
   This is the "true" PAL-60, thanks to the true System M timings.

a) Modeline 13.5 720 740 804 864 480 486 492 525 Interlace, standards registers
   set to VEC_CONFIG0_PAL with standard frequency; This is a "fake" PAL-60 mode,
   the refresh rate is actually ~59.524 Hz. Most "NTSC" sets will be able to
   sync with this mode no problem, but the VEC is actually operating in its
   50 Hz mode - it's just the "premature" vertical sync signal causes it to
   output something that is similar to the 525-line system, however strictly
   speaking non-standard due to lower horizontal sync frequency.

This comes down to the fact that:

- When VEC's standard registers are set to VEC_CONFIG0_NTSC_STD or
  VEC_CONFIG0_PAL_M_STD, it operates in the "CCIR System M" mode, expects htotal
  to be exactly 858 pixels (and it will generate horizontal sync pulse every 858
  pixels on its own regardless of what comes out of the PV - so there will be
  garbage on screen if you set it to anything else), and vtotal to be 525 lines.
  It will not accept vtotal that's any higher (it will generate its own vertical
  sync as demanded by System M if not triggered by the PV), but it can be lower
  - resulting in modes that are non-standard, but otherwise valid.

- Likewise, when the registers are set to VEC_CONFIG0_PAL_BDGHI_STD,
  VEC_CONFIG0_PAL_N_STD or VEC_CONFIG0_SECAM_STD (SECAM is a bit special, but
  that's irrelevant here), it operates in the "CCIR System B/D/G/H/I/N" mode,
  and likewise, expects htotal to be exactly 864 pixels (garbage on screen
  otherwise), vtotal limit is 625 lines, etc.

Checking for the refresh rate would only work for standard-compliant modes and
have the potential of completely breaking on any custom modes. Conversely,
checking for htotal aligns perfectly with the limitations of the hardware, and
allows the user to set any modeline that the hardware is able to output with
any level of sanity.

Footnote: all this information on VEC's behavior comes from my own
experimentation, messing around with its registers and seeing what happens
(both on screen and on an oscilloscope). I've never seen any Broadcom docs on
this chip, so it's by no means official.

Best regards,
Mateusz Kwiatkowski

Re: [PATCH v5 12/22] drm/connector: Add a function to lookup a TV mode by its name

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2338 bytes --]

On Tue, Oct 18, 2022 at 02:29:00PM +0200, Noralf Trønnes wrote:
> 
> 
> Den 18.10.2022 11.33, skrev Maxime Ripard:
> > On Mon, Oct 17, 2022 at 12:44:45PM +0200, Noralf Trønnes wrote:
> >> Den 13.10.2022 15.18, skrev Maxime Ripard:
> >>> As part of the command line parsing rework coming in the next patches,
> >>> we'll need to lookup drm_connector_tv_mode values by their name, already
> >>> defined in drm_tv_mode_enum_list.
> >>>
> >>> In order to avoid any code duplication, let's do a function that will
> >>> perform a lookup of a TV mode name and return its value.
> >>>
> >>> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> >>> ---
> >>>  drivers/gpu/drm/drm_connector.c | 24 ++++++++++++++++++++++++
> >>>  include/drm/drm_connector.h     |  2 ++
> >>>  2 files changed, 26 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> >>> index 820f4c730b38..30611c616435 100644
> >>> --- a/drivers/gpu/drm/drm_connector.c
> >>> +++ b/drivers/gpu/drm/drm_connector.c
> >>> @@ -991,6 +991,30 @@ static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = {
> >>>  };
> >>>  DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list)
> >>>  
> >>> +/**
> >>> + * drm_get_tv_mode_from_name - Translates a TV mode name into its enum value
> >>> + * @name: TV Mode name we want to convert
> >>> + * @len: Length of @name
> >>> + *
> >>> + * Translates @name into an enum drm_connector_tv_mode.
> >>> + *
> >>> + * Returns: the enum value on success, a negative errno otherwise.
> >>> + */
> >>> +int drm_get_tv_mode_from_name(const char *name, size_t len)
> >>
> >> Do we really need to pass in length here? item->name has to always be
> >> NUL terminated otherwise things would break elsewhere, so it shouldn't
> >> be necessary AFAICS.
> > 
> > The only user so far is the command-line parsing code, and we might very
> > well have an option after the tv_mode, something like
> > 720x480i,tv_mode=NTSC,rotate=180
> > 
> > In this case, we won't get a NULL-terminated name.
>
> My point is that item->name will always be NUL terminated so strcmp()
> will never look past that.

Right, but we don't have the guarantee that strlen(item->name) <
strlen(name), and we could thus just access after the end of our name

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v5 12/22] drm/connector: Add a function to lookup a TV mode by its name

[Noralf Trønnes]

Den 19.10.2022 10.48, skrev Maxime Ripard:
> On Tue, Oct 18, 2022 at 02:29:00PM +0200, Noralf Trønnes wrote:
>>
>>
>> Den 18.10.2022 11.33, skrev Maxime Ripard:
>>> On Mon, Oct 17, 2022 at 12:44:45PM +0200, Noralf Trønnes wrote:
>>>> Den 13.10.2022 15.18, skrev Maxime Ripard:
>>>>> As part of the command line parsing rework coming in the next patches,
>>>>> we'll need to lookup drm_connector_tv_mode values by their name, already
>>>>> defined in drm_tv_mode_enum_list.
>>>>>
>>>>> In order to avoid any code duplication, let's do a function that will
>>>>> perform a lookup of a TV mode name and return its value.
>>>>>
>>>>> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>>>>> ---
>>>>>  drivers/gpu/drm/drm_connector.c | 24 ++++++++++++++++++++++++
>>>>>  include/drm/drm_connector.h     |  2 ++
>>>>>  2 files changed, 26 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>>>>> index 820f4c730b38..30611c616435 100644
>>>>> --- a/drivers/gpu/drm/drm_connector.c
>>>>> +++ b/drivers/gpu/drm/drm_connector.c
>>>>> @@ -991,6 +991,30 @@ static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = {
>>>>>  };
>>>>>  DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list)
>>>>>  
>>>>> +/**
>>>>> + * drm_get_tv_mode_from_name - Translates a TV mode name into its enum value
>>>>> + * @name: TV Mode name we want to convert
>>>>> + * @len: Length of @name
>>>>> + *
>>>>> + * Translates @name into an enum drm_connector_tv_mode.
>>>>> + *
>>>>> + * Returns: the enum value on success, a negative errno otherwise.
>>>>> + */
>>>>> +int drm_get_tv_mode_from_name(const char *name, size_t len)
>>>>
>>>> Do we really need to pass in length here? item->name has to always be
>>>> NUL terminated otherwise things would break elsewhere, so it shouldn't
>>>> be necessary AFAICS.
>>>
>>> The only user so far is the command-line parsing code, and we might very
>>> well have an option after the tv_mode, something like
>>> 720x480i,tv_mode=NTSC,rotate=180
>>>
>>> In this case, we won't get a NULL-terminated name.
>>
>> My point is that item->name will always be NUL terminated so strcmp()
>> will never look past that.
> 
> Right, but we don't have the guarantee that strlen(item->name) <
> strlen(name), and we could thus just access after the end of our name
> 

Ok, using the length limiting str funtions is the safe thing to do, so
len needs to stay. But I don't get the 'strlen(item->name) == len'
check. strncmp() will stop when it reaches a NUL in either string so no
need for the length check?

Anyways:

Reviewed-by: Noralf Trønnes <noralf@tronnes.org>

Re: [PATCH v5 07/22] drm/client: Add some tests for drm_connector_pick_cmdline_mode()

[Michał Winiarski]

On Thu, Oct 13, 2022 at 03:18:51PM +0200, Maxime Ripard wrote:
> +static struct kunit_case drm_pick_cmdline_tests[] = {
> +	KUNIT_CASE(drm_pick_cmdline_res_1920_1080_60),
> +	{}
> +};

drm_test_pick_cmdline_res_1920_1080_60, since we adopted a consistent naming
convention for test cases in DRM.

-Michał

> +
> +static struct kunit_suite drm_pick_cmdline_test_suite = {
> +	.name = "drm_pick_cmdline",
> +	.init = drm_client_modeset_test_init,
> +	.test_cases = drm_pick_cmdline_tests
> +};
> +
> +kunit_test_suite(drm_pick_cmdline_test_suite);
> 
> -- 
> b4 0.11.0-dev-7da52
> 

Re: [PATCH v5 12/22] drm/connector: Add a function to lookup a TV mode by its name

[maxime]

[-- Attachment #1: Type: text/plain, Size: 3154 bytes --]

Hi Noralf,

On Wed, Oct 19, 2022 at 12:43:19PM +0200, Noralf Trønnes wrote:
> 
> 
> Den 19.10.2022 10.48, skrev Maxime Ripard:
> > On Tue, Oct 18, 2022 at 02:29:00PM +0200, Noralf Trønnes wrote:
> >>
> >>
> >> Den 18.10.2022 11.33, skrev Maxime Ripard:
> >>> On Mon, Oct 17, 2022 at 12:44:45PM +0200, Noralf Trønnes wrote:
> >>>> Den 13.10.2022 15.18, skrev Maxime Ripard:
> >>>>> As part of the command line parsing rework coming in the next patches,
> >>>>> we'll need to lookup drm_connector_tv_mode values by their name, already
> >>>>> defined in drm_tv_mode_enum_list.
> >>>>>
> >>>>> In order to avoid any code duplication, let's do a function that will
> >>>>> perform a lookup of a TV mode name and return its value.
> >>>>>
> >>>>> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> >>>>> ---
> >>>>>  drivers/gpu/drm/drm_connector.c | 24 ++++++++++++++++++++++++
> >>>>>  include/drm/drm_connector.h     |  2 ++
> >>>>>  2 files changed, 26 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> >>>>> index 820f4c730b38..30611c616435 100644
> >>>>> --- a/drivers/gpu/drm/drm_connector.c
> >>>>> +++ b/drivers/gpu/drm/drm_connector.c
> >>>>> @@ -991,6 +991,30 @@ static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = {
> >>>>>  };
> >>>>>  DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list)
> >>>>>  
> >>>>> +/**
> >>>>> + * drm_get_tv_mode_from_name - Translates a TV mode name into its enum value
> >>>>> + * @name: TV Mode name we want to convert
> >>>>> + * @len: Length of @name
> >>>>> + *
> >>>>> + * Translates @name into an enum drm_connector_tv_mode.
> >>>>> + *
> >>>>> + * Returns: the enum value on success, a negative errno otherwise.
> >>>>> + */
> >>>>> +int drm_get_tv_mode_from_name(const char *name, size_t len)
> >>>>
> >>>> Do we really need to pass in length here? item->name has to always be
> >>>> NUL terminated otherwise things would break elsewhere, so it shouldn't
> >>>> be necessary AFAICS.
> >>>
> >>> The only user so far is the command-line parsing code, and we might very
> >>> well have an option after the tv_mode, something like
> >>> 720x480i,tv_mode=NTSC,rotate=180
> >>>
> >>> In this case, we won't get a NULL-terminated name.
> >>
> >> My point is that item->name will always be NUL terminated so strcmp()
> >> will never look past that.
> > 
> > Right, but we don't have the guarantee that strlen(item->name) <
> > strlen(name), and we could thus just access after the end of our name
> > 
> 
> Ok, using the length limiting str funtions is the safe thing to do, so
> len needs to stay. But I don't get the 'strlen(item->name) == len'
> check. strncmp() will stop when it reaches a NUL in either string so no
> need for the length check?

Yeah, but if the cmdline is truncated, we'll pass a shorter len than
strlen(item->name), and it will consider the string as equal.

For example strncmp("NTS", "NTSC", strlen("NTS"))) == 0, while it obviously
isn't for us.

> Anyways:
> 
> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v5 20/22] drm/vc4: vec: Convert to the new TV mode property

[maxime]

[-- Attachment #1: Type: text/plain, Size: 6201 bytes --]

Hi,

On Tue, Oct 18, 2022 at 11:28:35PM +0200, Mateusz Kwiatkowski wrote:
> W dniu 18.10.2022 o 12:00, Maxime Ripard pisze:
> > On Mon, Oct 17, 2022 at 12:31:31PM +0200, Noralf Trønnes wrote:
> >> Den 16.10.2022 20.52, skrev Mateusz Kwiatkowski:
> >>>>  static int vc4_vec_connector_get_modes(struct drm_connector *connector)
> >>>>  {
> >>>> -	struct drm_connector_state *state = connector->state;
> >>>>  	struct drm_display_mode *mode;
> >>>>  
> >>>> -	mode = drm_mode_duplicate(connector->dev,
> >>>> -				  vc4_vec_tv_modes[state->tv.legacy_mode].mode);
> >>>> +	mode = drm_mode_analog_ntsc_480i(connector->dev);
> >>>>  	if (!mode) {
> >>>>  		DRM_ERROR("Failed to create a new display mode\n");
> >>>>  		return -ENOMEM;
> >>>>  	}
> >>>>  
> >>>> +	mode->type |= DRM_MODE_TYPE_PREFERRED;
> >>>>  	drm_mode_probed_add(connector, mode);
> >>>>  
> >>>> -	return 1;
> >>>> +	mode = drm_mode_analog_pal_576i(connector->dev);
> >>>> +	if (!mode) {
> >>>> +		DRM_ERROR("Failed to create a new display mode\n");
> >>>> +		return -ENOMEM;
> >>>> +	}
> >>>> +
> >>>> +	drm_mode_probed_add(connector, mode);
> >>>> +
> >>>> +	return 2;
> >>>> +}
> >>>
> >>> Referencing those previous discussions:
> >>> - https://lore.kernel.org/dri-devel/0255f7c6-0484-6456-350d-cf24f3fee5d6@tronnes.org/
> >>> - https://lore.kernel.org/dri-devel/c8f8015a-75da-afa8-ca7f-b2b134cacd16@gmail.com/
> >>>
> >>> Unconditionally setting the 480i mode as DRM_MODE_TYPE_PREFERRED causes Xorg
> >>> (at least on current Raspberry Pi OS) to display garbage when
> >>> video=Composite1:PAL is specified on the command line, so I'm afraid this won't
> >>> do.
> >>>
> >>> As I see it, there are three viable solutions for this issue:
> >>>
> >>> a) Somehow query the video= command line option from this function, and set
> >>>    DRM_MODE_TYPE_PREFERRED appropriately. This would break the abstraction
> >>>    provided by global DRM code, but should work fine.
> >>>
> >>> b) Modify drm_helper_probe_add_cmdline_mode() so that it sets
> >>>    DRM_MODE_TYPE_PREFERRED in addition to DRM_MODE_TYPE_USERDEF. This seems
> >>>    pretty robust, but affects the entire DRM subsystem, which may break
> >>>    userspace in different ways.
> >>>
> >>>    - Maybe this could be mitigated by adding some additional conditions, e.g.
> >>>      setting the PREFERRED flag only if no modes are already flagged as such
> >>>      and/or only if the cmdline mode is a named one (~= analog TV mode)
> >>>
> >>> c) Forcing userspace (Xorg / Raspberry Pi OS) to get fixed and honor the USERDEF
> >>>    flag.
> >>>
> >>> Either way, hardcoding 480i as PREFERRED does not seem right.
> >>>
> >>
> >> My solution for this is to look at tv.mode to know which mode to mark as
> >> preferred. Maxime didn't like this since it changes things behind
> >> userspace's back. I don't see how that can cause any problems for userspace.
> >>
> >> If userspace uses atomic and sets tv_mode, it has to know which mode to
> >> use before hand, so it doesn't look at the preferreded flag.
> >>
> >> If it uses legacy and sets tv_mode, it can end up with a stale preferred
> >> flag, but no worse than not having the flag or that ntsc is always
> >> preferred.
> >>
> >> If it doesn't change tv_mode, there's no problem, the preferred flag
> >> doesn't change.
> >
> > I don't like it because I just see no way to make this reliable. When we
> > set tv_mode, we're not only going to change the preferred flag, but also
> > the order of the modes to make the preferred mode first.
> >
> > Since we just changed the mode lists, we also want to send a hotplug
> > event to userspace so that it gets notified of it. It will pick up the
> > new preferred mode, great.
> >
> > But what if it doesn't? There's no requirement for userspace to handle
> > hotplug events, and Kodi won't for example. So we just changed the TV
> > mode but not the actual mode, and that's it. It's just as broken for
> > Kodi as it is for X11 right now.
> >
> > If we can't get a bullet-proof solution, then I'm not convinced it's
> > worth addressing. Especially since it's already the current state, and
> > it doesn't seem to bother a lot of people.
> 
> I wouldn't rely on the "doesn't seem to bother a lot of people" bit too much.
> Here's why:
> 
> - Analog TV output is a relatively obscure feature in this day and age in the
>   first place.
> 
> - Out of the people interested in using it with VC4/VEC, many are actually using
>   the downstream kernel from https://github.com/raspberrypi/linux instead of the
>   upstream kernel, and/or firmware mode-switching instead of proper KMS.
> 
>   - The downstream kernel only reports modes that match the TV mode set at boot
>     either via vc4.tv_norm=, or implied by the resolution set via video=; note
>     that video= is also set appropriately at boot by Pi firmware, based on the
>     value of sdtv_mode set in config.txt. See also the
>     vc4_vec_connector_get_modes() and vc4_vec_get_default_mode() functions in
>     https://github.com/raspberrypi/linux/blob/dbd073e4028580a09b6ee507e0c137441cb52650/drivers/gpu/drm/vc4/vc4_vec.c
> 
>   - When firmware mode-switching is used, it sets the appropriate TV mode and
>     resolution based on the sdtv_mode set in config.txt.
> 
> So, all in all, the number of people who would use 1. analog TV out with VC4,
> 2. the upstream kernel, 3. full KMS (and thus the vc4_vec.c code) is rather
> small, so the fact that you're not hearing too many complaints doesn't mean that
> the current behavior is OK. If anybody ran into problems and was bothered by
> that, they likely migrated to the downstream kernel and/or firmware
> mode-switching.
>
> That being said, I completely forgot that there's a cmdline_mode field in
> struct drm_connector, even though I actually added code that examines it inside
> vc4_vec_connector_get_modes() that's in the downstream kernel. So... what do
> you think about just examining connector->cmdline_mode.tv_mode there? It seems
> to solve all the problems.

It's a very good idea, I'll work on it, thanks :)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] drm/vc4: vec: Add support for PAL-60

[maxime]

[-- Attachment #1: Type: text/plain, Size: 3709 bytes --]

On Tue, Oct 18, 2022 at 10:57:04PM +0200, Mateusz Kwiatkowski wrote:
> Hi Maxime,
> 
> W dniu 18.10.2022 o 10:31, Maxime Ripard pisze:
> > Hi,
> >
> > On Sun, Oct 16, 2022 at 09:46:49PM +0200, Mateusz Kwiatkowski wrote:
> >> @@ -308,14 +324,15 @@ static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = {
> >>  };
> >>  
> >>  static inline const struct vc4_vec_tv_mode *
> >> -vc4_vec_tv_mode_lookup(unsigned int mode)
> >> +vc4_vec_tv_mode_lookup(unsigned int mode, u16 htotal)
> >>  {
> >>  	unsigned int i;
> >>  
> >>  	for (i = 0; i < ARRAY_SIZE(vc4_vec_tv_modes); i++) {
> >>  		const struct vc4_vec_tv_mode *tv_mode = &vc4_vec_tv_modes[i];
> >>  
> >> -		if (tv_mode->mode == mode)
> >> +		if (tv_mode->mode == mode &&
> >> +		    tv_mode->expected_htotal == htotal)
> >>  			return tv_mode;
> >
> > Is there any reason we're not using the refresh rate to filter this? It
> > seems more natural to me.
> 
> Let me give you an example first.
> 
> There are actually two ways to configure PAL-60-ish mode on VC4/VEC:
> 
> a) Modeline 13.5 720 734 798 858 480 487 493 525 Interlace, standard registers
>    set to VEC_CONFIG0_PAL_M_STD, custom frequency enabled and set to 0x2a098acb;
>    Setting the standard registers to "PAL-M" puts the VEC in true 59.94 Hz mode,
>    so the video timings are identical as for NTSC (or PAL-M), and the custom
>    frequency makes the color subcarrier compatible with regular PAL receivers.
>    This is the "true" PAL-60, thanks to the true System M timings.

That's the one I would expect, and I assume we could just do that by
selecting the 480i mode + PAL TV Mode property, right?

> a) Modeline 13.5 720 740 804 864 480 486 492 525 Interlace, standards registers
>    set to VEC_CONFIG0_PAL with standard frequency; This is a "fake" PAL-60 mode,
>    the refresh rate is actually ~59.524 Hz. Most "NTSC" sets will be able to
>    sync with this mode no problem, but the VEC is actually operating in its
>    50 Hz mode - it's just the "premature" vertical sync signal causes it to
>    output something that is similar to the 525-line system, however strictly
>    speaking non-standard due to lower horizontal sync frequency.

But it's not really clear to me why we should support both.

> This comes down to the fact that:
> 
> - When VEC's standard registers are set to VEC_CONFIG0_NTSC_STD or
>   VEC_CONFIG0_PAL_M_STD, it operates in the "CCIR System M" mode, expects htotal
>   to be exactly 858 pixels (and it will generate horizontal sync pulse every 858
>   pixels on its own regardless of what comes out of the PV - so there will be
>   garbage on screen if you set it to anything else), and vtotal to be 525 lines.
>   It will not accept vtotal that's any higher (it will generate its own vertical
>   sync as demanded by System M if not triggered by the PV), but it can be lower
>   - resulting in modes that are non-standard, but otherwise valid.
> 
> - Likewise, when the registers are set to VEC_CONFIG0_PAL_BDGHI_STD,
>   VEC_CONFIG0_PAL_N_STD or VEC_CONFIG0_SECAM_STD (SECAM is a bit special, but
>   that's irrelevant here), it operates in the "CCIR System B/D/G/H/I/N" mode,
>   and likewise, expects htotal to be exactly 864 pixels (garbage on screen
>   otherwise), vtotal limit is 625 lines, etc.
> 
> Checking for the refresh rate would only work for standard-compliant modes and
> have the potential of completely breaking on any custom modes. Conversely,
> checking for htotal aligns perfectly with the limitations of the hardware, and
> allows the user to set any modeline that the hardware is able to output with
> any level of sanity.

OK

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]