diff mbox series

[v1,35/35] drm/modes: Introduce more named modes

Message ID 20220728-rpi-analog-tv-properties-v1-35-3d53ae722097@cerno.tech (mailing list archive)
State New, archived
Delegated to: Neil Armstrong
Headers show
Series drm: Analog TV Improvements | expand

Commit Message

Maxime Ripard July 29, 2022, 4:35 p.m. UTC 
Now that we can easily extend the named modes list, let's add a few more
analog TV modes that were used in the wild, and some unit tests to make
sure it works as intended.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Comments

Noralf Trønnes Aug. 21, 2022, 4:16 p.m. UTC | #1 
Den 29.07.2022 18.35, skrev Maxime Ripard:
> Now that we can easily extend the named modes list, let's add a few more
> analog TV modes that were used in the wild, and some unit tests to make
> sure it works as intended.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 3634ac9f787d..09ed5ce7746d 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1853,7 +1853,9 @@ struct drm_named_mode {
>  
>  static const struct drm_named_mode drm_named_modes[] = {
>  	{ "NTSC", &drm_mode_480i, DRM_MODE_TV_NORM_NTSC_M, },
> +	{ "NTSC_J", &drm_mode_480i, DRM_MODE_TV_NORM_NTSC_J, },
>  	{ "PAL", &drm_mode_576i, DRM_MODE_TV_NORM_PAL_B, },
> +	{ "PAL_M", &drm_mode_480i, DRM_MODE_TV_NORM_PAL_M, },

Please use a dash instead of an underscore to keep it consistent with
what's used elsewhere.

I tried to use PAL and set the connector as connected, but that didn't
work: video=Composite-1:PALe

Is this a bug and it should work or should it be phrased differently?
It would have been nice to get a warning in the log if the parser fails
to understand.

This very verbose version did work:
video=Composite-1:720x576@50ie,tv_mode=PAL-B

(it seems to me that userspace often treats connection status "unknown"
the same as "disconnected")

Noralf.

>  };
>  
>  static bool drm_mode_parse_cmdline_named_mode(const char *name,
> diff --git a/drivers/gpu/drm/tests/drm_mode_test.c b/drivers/gpu/drm/tests/drm_mode_test.c
> index 006b73a61fd4..991eb8ed687c 100644
> --- a/drivers/gpu/drm/tests/drm_mode_test.c
> +++ b/drivers/gpu/drm/tests/drm_mode_test.c
> @@ -156,6 +156,32 @@ static void drm_mode_named_ntsc(struct kunit *test)
>  	KUNIT_EXPECT_TRUE(test, drm_mode_equal(&drm_mode_480i, mode));
>  }
>  
> +static void drm_mode_named_ntsc_j(struct kunit *test)
> +{
> +	struct drm_mode_test_priv *priv = test->priv;
> +	struct drm_device *drm = priv->drm;
> +	struct drm_connector *connector = &priv->connector;
> +	struct drm_cmdline_mode *cmdline_mode = &connector->cmdline_mode;
> +	struct drm_display_mode *mode;
> +	const char *cmdline = "NTSC_J";
> +	int ret;
> +
> +	KUNIT_ASSERT_TRUE(test,
> +			  drm_mode_parse_command_line_for_connector(cmdline,
> +								    connector,
> +								    cmdline_mode));
> +
> +	mutex_lock(&drm->mode_config.mutex);
> +	ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
> +	mutex_unlock(&drm->mode_config.mutex);
> +	KUNIT_ASSERT_GT(test, ret, 0);
> +
> +	mode = drm_connector_pick_cmdline_mode(connector);
> +	KUNIT_ASSERT_PTR_NE(test, mode, NULL);
> +
> +	KUNIT_EXPECT_TRUE(test, drm_mode_equal(&drm_mode_480i, mode));
> +}
> +
>  static void drm_mode_named_pal(struct kunit *test)
>  {
>  	struct drm_mode_test_priv *priv = test->priv;
> @@ -182,10 +208,38 @@ static void drm_mode_named_pal(struct kunit *test)
>  	KUNIT_EXPECT_TRUE(test, drm_mode_equal(&drm_mode_576i, mode));
>  }
>  
> +static void drm_mode_named_pal_m(struct kunit *test)
> +{
> +	struct drm_mode_test_priv *priv = test->priv;
> +	struct drm_device *drm = priv->drm;
> +	struct drm_connector *connector = &priv->connector;
> +	struct drm_cmdline_mode *cmdline_mode = &connector->cmdline_mode;
> +	struct drm_display_mode *mode;
> +	const char *cmdline = "PAL_M";
> +	int ret;
> +
> +	KUNIT_ASSERT_TRUE(test,
> +			  drm_mode_parse_command_line_for_connector(cmdline,
> +								    connector,
> +								    cmdline_mode));
> +
> +	mutex_lock(&drm->mode_config.mutex);
> +	ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
> +	mutex_unlock(&drm->mode_config.mutex);
> +	KUNIT_ASSERT_GT(test, ret, 0);
> +
> +	mode = drm_connector_pick_cmdline_mode(connector);
> +	KUNIT_ASSERT_PTR_NE(test, mode, NULL);
> +
> +	KUNIT_EXPECT_TRUE(test, drm_mode_equal(&drm_mode_480i, mode));
> +}
> +
>  static struct kunit_case drm_mode_tests[] = {
>  	KUNIT_CASE(drm_mode_res_1920_1080_60),
>  	KUNIT_CASE(drm_mode_named_ntsc),
> +	KUNIT_CASE(drm_mode_named_ntsc_j),
>  	KUNIT_CASE(drm_mode_named_pal),
> +	KUNIT_CASE(drm_mode_named_pal_m),
>  	{}
>  };
>  
>
Maxime Ripard Aug. 25, 2022, 2:22 p.m. UTC | #2 
On Sun, Aug 21, 2022 at 06:16:15PM +0200, Noralf Trønnes wrote:
> 
> 
> Den 29.07.2022 18.35, skrev Maxime Ripard:
> > Now that we can easily extend the named modes list, let's add a few more
> > analog TV modes that were used in the wild, and some unit tests to make
> > sure it works as intended.
> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > 
> > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > index 3634ac9f787d..09ed5ce7746d 100644
> > --- a/drivers/gpu/drm/drm_modes.c
> > +++ b/drivers/gpu/drm/drm_modes.c
> > @@ -1853,7 +1853,9 @@ struct drm_named_mode {
> >  
> >  static const struct drm_named_mode drm_named_modes[] = {
> >  	{ "NTSC", &drm_mode_480i, DRM_MODE_TV_NORM_NTSC_M, },
> > +	{ "NTSC_J", &drm_mode_480i, DRM_MODE_TV_NORM_NTSC_J, },
> >  	{ "PAL", &drm_mode_576i, DRM_MODE_TV_NORM_PAL_B, },
> > +	{ "PAL_M", &drm_mode_480i, DRM_MODE_TV_NORM_PAL_M, },
> 
> Please use a dash instead of an underscore to keep it consistent with
> what's used elsewhere.

Yeah, I tried not to use it since I didn't want to cripple the parser
with bad heuristics, but Geert has done a much better job than my
attempts here:
https://lore.kernel.org/dri-devel/2eb205da88c3cb19ddf04d167ece4e16a330948b.1657788997.git.geert@linux-m68k.org/

I've taken that patch in, renamed the modes and we already have unit
tests in other patches to cover that part of the parser.

> I tried to use PAL and set the connector as connected, but that didn't
> work: video=Composite-1:PALe
> 
> Is this a bug and it should work or should it be phrased differently?

I'm not sure. We never supported something like that so far, because the
expectation is that if it starts with a letter, it's a named mode. Thus
the named mode is PALe in your case, which doesn't match.

I'm not even sure what would be a good heuristic to support it, since
interpreting any e, E, or D as the last letter seems like a recipe for
disaster when some modes might legitimately end with e or d.

> It would have been nice to get a warning in the log if the parser fails
> to understand.
> 
> This very verbose version did work:
> video=Composite-1:720x576@50ie,tv_mode=PAL-B

I think that's what we should be using in that case.

Maxime
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 3634ac9f787d..09ed5ce7746d 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1853,7 +1853,9 @@  struct drm_named_mode {
 
 static const struct drm_named_mode drm_named_modes[] = {
 	{ "NTSC", &drm_mode_480i, DRM_MODE_TV_NORM_NTSC_M, },
+	{ "NTSC_J", &drm_mode_480i, DRM_MODE_TV_NORM_NTSC_J, },
 	{ "PAL", &drm_mode_576i, DRM_MODE_TV_NORM_PAL_B, },
+	{ "PAL_M", &drm_mode_480i, DRM_MODE_TV_NORM_PAL_M, },
 };
 
 static bool drm_mode_parse_cmdline_named_mode(const char *name,
diff --git a/drivers/gpu/drm/tests/drm_mode_test.c b/drivers/gpu/drm/tests/drm_mode_test.c
index 006b73a61fd4..991eb8ed687c 100644
--- a/drivers/gpu/drm/tests/drm_mode_test.c
+++ b/drivers/gpu/drm/tests/drm_mode_test.c
@@ -156,6 +156,32 @@  static void drm_mode_named_ntsc(struct kunit *test)
 	KUNIT_EXPECT_TRUE(test, drm_mode_equal(&drm_mode_480i, mode));
 }
 
+static void drm_mode_named_ntsc_j(struct kunit *test)
+{
+	struct drm_mode_test_priv *priv = test->priv;
+	struct drm_device *drm = priv->drm;
+	struct drm_connector *connector = &priv->connector;
+	struct drm_cmdline_mode *cmdline_mode = &connector->cmdline_mode;
+	struct drm_display_mode *mode;
+	const char *cmdline = "NTSC_J";
+	int ret;
+
+	KUNIT_ASSERT_TRUE(test,
+			  drm_mode_parse_command_line_for_connector(cmdline,
+								    connector,
+								    cmdline_mode));
+
+	mutex_lock(&drm->mode_config.mutex);
+	ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
+	mutex_unlock(&drm->mode_config.mutex);
+	KUNIT_ASSERT_GT(test, ret, 0);
+
+	mode = drm_connector_pick_cmdline_mode(connector);
+	KUNIT_ASSERT_PTR_NE(test, mode, NULL);
+
+	KUNIT_EXPECT_TRUE(test, drm_mode_equal(&drm_mode_480i, mode));
+}
+
 static void drm_mode_named_pal(struct kunit *test)
 {
 	struct drm_mode_test_priv *priv = test->priv;
@@ -182,10 +208,38 @@  static void drm_mode_named_pal(struct kunit *test)
 	KUNIT_EXPECT_TRUE(test, drm_mode_equal(&drm_mode_576i, mode));
 }
 
+static void drm_mode_named_pal_m(struct kunit *test)
+{
+	struct drm_mode_test_priv *priv = test->priv;
+	struct drm_device *drm = priv->drm;
+	struct drm_connector *connector = &priv->connector;
+	struct drm_cmdline_mode *cmdline_mode = &connector->cmdline_mode;
+	struct drm_display_mode *mode;
+	const char *cmdline = "PAL_M";
+	int ret;
+
+	KUNIT_ASSERT_TRUE(test,
+			  drm_mode_parse_command_line_for_connector(cmdline,
+								    connector,
+								    cmdline_mode));
+
+	mutex_lock(&drm->mode_config.mutex);
+	ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
+	mutex_unlock(&drm->mode_config.mutex);
+	KUNIT_ASSERT_GT(test, ret, 0);
+
+	mode = drm_connector_pick_cmdline_mode(connector);
+	KUNIT_ASSERT_PTR_NE(test, mode, NULL);
+
+	KUNIT_EXPECT_TRUE(test, drm_mode_equal(&drm_mode_480i, mode));
+}
+
 static struct kunit_case drm_mode_tests[] = {
 	KUNIT_CASE(drm_mode_res_1920_1080_60),
 	KUNIT_CASE(drm_mode_named_ntsc),
+	KUNIT_CASE(drm_mode_named_ntsc_j),
 	KUNIT_CASE(drm_mode_named_pal),
+	KUNIT_CASE(drm_mode_named_pal_m),
 	{}
 };