| Message ID | 20220728-rpi-analog-tv-properties-v2-19-459522d653a7@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Analog TV Improvements | expand |
Hi Maxime, On 8/29/22 10:11, Maxime Ripard wrote: > Our new tv mode option allows to specify the TV mode from a property. > However, it can still be useful, for example to avoid any boot time > artifact, to set that property directly from the kernel command line. > > Let's add some code to allow it, and some unit tests to exercise that code. > > 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 73d01e755496..a759a4ba0036 100644 > --- a/drivers/gpu/drm/drm_modes.c > +++ b/drivers/gpu/drm/drm_modes.c > @@ -2115,6 +2115,30 @@ static int drm_mode_parse_panel_orientation(const char *delim, > return 0; > } > > +static int drm_mode_parse_tv_mode(const char *delim, > + struct drm_cmdline_mode *mode) > +{ > + const char *value; > + unsigned int len; Looks like this variable len is not being used and is producing the following warning: ../drivers/gpu/drm/drm_modes.c:2122:15: warning: unused variable 'len' [-Wunused-variable] unsigned int len; ^ Best Regards, - Maíra Canal > + int ret; > + > + if (*delim != '=') > + return -EINVAL; > + > + value = delim + 1; > + delim = strchr(value, ','); > + if (!delim) > + delim = value + strlen(value); > + > + ret = drm_get_tv_mode_from_name(value, delim - value); > + if (ret < 0) > + return ret; > + > + mode->tv_mode = ret; > + > + return 0; > +} > + > static int drm_mode_parse_cmdline_options(const char *str, > bool freestanding, > const struct drm_connector *connector, > @@ -2184,6 +2208,9 @@ static int drm_mode_parse_cmdline_options(const char *str, > } else if (!strncmp(option, "panel_orientation", delim - option)) { > if (drm_mode_parse_panel_orientation(delim, mode)) > return -EINVAL; > + } else if (!strncmp(option, "tv_mode", delim - option)) { > + if (drm_mode_parse_tv_mode(delim, mode)) > + return -EINVAL; > } else { > return -EINVAL; > } > @@ -2212,20 +2239,22 @@ struct drm_named_mode { > unsigned int xres; > unsigned int yres; > unsigned int flags; > + unsigned int tv_mode; > }; > > -#define NAMED_MODE(_name, _pclk, _x, _y, _flags) \ > +#define NAMED_MODE(_name, _pclk, _x, _y, _flags, _mode) \ > { \ > .name = _name, \ > .pixel_clock_khz = _pclk, \ > .xres = _x, \ > .yres = _y, \ > .flags = _flags, \ > + .tv_mode = _mode, \ > } > > static const struct drm_named_mode drm_named_modes[] = { > - NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE), > - NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE), > + NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE, DRM_MODE_TV_MODE_NTSC_M), > + NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE, DRM_MODE_TV_MODE_PAL_B), > }; > > static int drm_mode_parse_cmdline_named_mode(const char *name, > @@ -2271,6 +2300,7 @@ static int drm_mode_parse_cmdline_named_mode(const char *name, > cmdline_mode->xres = mode->xres; > cmdline_mode->yres = mode->yres; > cmdline_mode->interlace = !!(mode->flags & DRM_MODE_FLAG_INTERLACE); > + cmdline_mode->tv_mode = mode->tv_mode; > cmdline_mode->specified = true; > > return 1; > diff --git a/drivers/gpu/drm/tests/drm_cmdline_parser_test.c b/drivers/gpu/drm/tests/drm_cmdline_parser_test.c > index 59b29cdfdd35..f1e73ed65be0 100644 > --- a/drivers/gpu/drm/tests/drm_cmdline_parser_test.c > +++ b/drivers/gpu/drm/tests/drm_cmdline_parser_test.c > @@ -885,6 +885,201 @@ static void drm_cmdline_test_multiple_options(struct kunit *test) > KUNIT_EXPECT_EQ(test, mode.force, DRM_FORCE_UNSPECIFIED); > } > > +static void drm_cmdline_test_tv_options(struct kunit *test, > + const char *cmdline, > + const struct drm_display_mode *expected_mode, > + unsigned int expected_tv_mode) > +{ > + struct drm_cmdline_mode mode = { }; > + > + KUNIT_EXPECT_TRUE(test, drm_mode_parse_command_line_for_connector(cmdline, > + &no_connector, &mode)); > + KUNIT_EXPECT_TRUE(test, mode.specified); > + KUNIT_EXPECT_EQ(test, mode.xres, expected_mode->hdisplay); > + KUNIT_EXPECT_EQ(test, mode.yres, expected_mode->vdisplay); > + KUNIT_EXPECT_EQ(test, mode.tv_mode, expected_tv_mode); > + > + KUNIT_EXPECT_FALSE(test, mode.refresh_specified); > + > + KUNIT_EXPECT_FALSE(test, mode.bpp_specified); > + > + KUNIT_EXPECT_FALSE(test, mode.rb); > + KUNIT_EXPECT_FALSE(test, mode.cvt); > + KUNIT_EXPECT_EQ(test, mode.interlace, !!(expected_mode->flags & DRM_MODE_FLAG_INTERLACE)); > + KUNIT_EXPECT_FALSE(test, mode.margins); > + KUNIT_EXPECT_EQ(test, mode.force, DRM_FORCE_UNSPECIFIED); > +} > + > +static void drm_cmdline_test_tv_option_ntsc_443(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x480i,tv_mode=NTSC-443", > + drm_mode_analog_ntsc_480i(NULL), > + DRM_MODE_TV_MODE_NTSC_443); > +} > + > +static void drm_cmdline_test_tv_option_ntsc_j(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x480i,tv_mode=NTSC-J", > + drm_mode_analog_ntsc_480i(NULL), > + DRM_MODE_TV_MODE_NTSC_J); > +} > + > +static void drm_cmdline_test_tv_option_ntsc_m(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x480i,tv_mode=NTSC-M", > + drm_mode_analog_ntsc_480i(NULL), > + DRM_MODE_TV_MODE_NTSC_M); > +} > + > +static void drm_cmdline_test_tv_option_pal_60(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=PAL-60", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_PAL_60); > +} > + > +static void drm_cmdline_test_tv_option_pal_b(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=PAL-B", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_PAL_B); > +} > + > +static void drm_cmdline_test_tv_option_pal_d(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=PAL-D", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_PAL_D); > +} > + > +static void drm_cmdline_test_tv_option_pal_g(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=PAL-G", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_PAL_G); > +} > + > +static void drm_cmdline_test_tv_option_pal_h(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=PAL-H", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_PAL_H); > +} > + > +static void drm_cmdline_test_tv_option_pal_i(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=PAL-I", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_PAL_I); > +} > + > +static void drm_cmdline_test_tv_option_pal_m(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x480i,tv_mode=PAL-M", > + drm_mode_analog_ntsc_480i(NULL), > + DRM_MODE_TV_MODE_PAL_M); > +} > + > +static void drm_cmdline_test_tv_option_pal_n(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=PAL-N", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_PAL_N); > +} > + > +static void drm_cmdline_test_tv_option_pal_nc(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=PAL-Nc", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_PAL_NC); > +} > + > +static void drm_cmdline_test_tv_option_secam_60(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=SECAM-60", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_SECAM_60); > +} > + > +static void drm_cmdline_test_tv_option_secam_b(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=SECAM-B", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_SECAM_B); > +} > + > +static void drm_cmdline_test_tv_option_secam_d(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=SECAM-D", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_SECAM_D); > +} > + > +static void drm_cmdline_test_tv_option_secam_g(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=SECAM-G", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_SECAM_G); > +} > + > +static void drm_cmdline_test_tv_option_secam_k(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=SECAM-K", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_SECAM_K); > +} > + > +static void drm_cmdline_test_tv_option_secam_k1(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=SECAM-K1", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_SECAM_K1); > +} > + > +static void drm_cmdline_test_tv_option_secam_l(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=SECAM-L", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_SECAM_L); > +} > + > +static void drm_cmdline_test_tv_option_invalid(struct kunit *test) > +{ > + struct drm_cmdline_mode mode = { }; > + const char *cmdline = "720x480i,tv_mode=invalid"; > + > + KUNIT_EXPECT_FALSE(test, drm_mode_parse_command_line_for_connector(cmdline, > + &no_connector, &mode)); > +} > + > +static void drm_cmdline_test_tv_option_truncated(struct kunit *test) > +{ > + struct drm_cmdline_mode mode = { }; > + const char *cmdline = "720x480i,tv_mode=NTSC"; > + > + KUNIT_EXPECT_FALSE(test, drm_mode_parse_command_line_for_connector(cmdline, > + &no_connector, &mode)); > +} > + > static void drm_cmdline_test_invalid_option(struct kunit *test) > { > struct drm_cmdline_mode mode = { }; > @@ -1047,6 +1242,27 @@ static struct kunit_case drm_cmdline_parser_tests[] = { > KUNIT_CASE(drm_cmdline_test_name_refresh_invalid_mode), > KUNIT_CASE(drm_cmdline_test_name_option), > KUNIT_CASE(drm_cmdline_test_name_bpp_option), > + KUNIT_CASE(drm_cmdline_test_tv_option_ntsc_443), > + KUNIT_CASE(drm_cmdline_test_tv_option_ntsc_j), > + KUNIT_CASE(drm_cmdline_test_tv_option_ntsc_m), > + KUNIT_CASE(drm_cmdline_test_tv_option_pal_60), > + KUNIT_CASE(drm_cmdline_test_tv_option_pal_b), > + KUNIT_CASE(drm_cmdline_test_tv_option_pal_d), > + KUNIT_CASE(drm_cmdline_test_tv_option_pal_g), > + KUNIT_CASE(drm_cmdline_test_tv_option_pal_h), > + KUNIT_CASE(drm_cmdline_test_tv_option_pal_i), > + KUNIT_CASE(drm_cmdline_test_tv_option_pal_m), > + KUNIT_CASE(drm_cmdline_test_tv_option_pal_n), > + KUNIT_CASE(drm_cmdline_test_tv_option_pal_nc), > + KUNIT_CASE(drm_cmdline_test_tv_option_secam_60), > + KUNIT_CASE(drm_cmdline_test_tv_option_secam_b), > + KUNIT_CASE(drm_cmdline_test_tv_option_secam_d), > + KUNIT_CASE(drm_cmdline_test_tv_option_secam_g), > + KUNIT_CASE(drm_cmdline_test_tv_option_secam_k), > + KUNIT_CASE(drm_cmdline_test_tv_option_secam_k1), > + KUNIT_CASE(drm_cmdline_test_tv_option_secam_l), > + KUNIT_CASE(drm_cmdline_test_tv_option_invalid), > + KUNIT_CASE(drm_cmdline_test_tv_option_truncated), > KUNIT_CASE(drm_cmdline_test_rotate_0), > KUNIT_CASE(drm_cmdline_test_rotate_90), > KUNIT_CASE(drm_cmdline_test_rotate_180), > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index 49d261977d4e..9589108ba202 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -1447,6 +1447,11 @@ struct drm_cmdline_mode { > * @tv_margins: TV margins to apply to the mode. > */ > struct drm_connector_tv_margins tv_margins; > + > + /** > + * @tv_mode: TV mode standard. See DRM_MODE_TV_MODE_*. > + */ > + enum drm_connector_tv_mode tv_mode; > }; > > /** >
On 8/29/22 10:11, Maxime Ripard wrote: > Our new tv mode option allows to specify the TV mode from a property. > However, it can still be useful, for example to avoid any boot time > artifact, to set that property directly from the kernel command line. > > Let's add some code to allow it, and some unit tests to exercise that code. > > 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 73d01e755496..a759a4ba0036 100644 > --- a/drivers/gpu/drm/drm_modes.c > +++ b/drivers/gpu/drm/drm_modes.c > @@ -2115,6 +2115,30 @@ static int drm_mode_parse_panel_orientation(const char *delim, > return 0; > } > > +static int drm_mode_parse_tv_mode(const char *delim, > + struct drm_cmdline_mode *mode) > +{ > + const char *value; > + unsigned int len; > + int ret; > + > + if (*delim != '=') > + return -EINVAL; > + > + value = delim + 1; > + delim = strchr(value, ','); > + if (!delim) > + delim = value + strlen(value); > + > + ret = drm_get_tv_mode_from_name(value, delim - value); > + if (ret < 0) > + return ret; > + > + mode->tv_mode = ret; > + > + return 0; > +} > + > static int drm_mode_parse_cmdline_options(const char *str, > bool freestanding, > const struct drm_connector *connector, > @@ -2184,6 +2208,9 @@ static int drm_mode_parse_cmdline_options(const char *str, > } else if (!strncmp(option, "panel_orientation", delim - option)) { > if (drm_mode_parse_panel_orientation(delim, mode)) > return -EINVAL; > + } else if (!strncmp(option, "tv_mode", delim - option)) { > + if (drm_mode_parse_tv_mode(delim, mode)) > + return -EINVAL; > } else { > return -EINVAL; > } > @@ -2212,20 +2239,22 @@ struct drm_named_mode { > unsigned int xres; > unsigned int yres; > unsigned int flags; > + unsigned int tv_mode; > }; > > -#define NAMED_MODE(_name, _pclk, _x, _y, _flags) \ > +#define NAMED_MODE(_name, _pclk, _x, _y, _flags, _mode) \ > { \ > .name = _name, \ > .pixel_clock_khz = _pclk, \ > .xres = _x, \ > .yres = _y, \ > .flags = _flags, \ > + .tv_mode = _mode, \ > } > > static const struct drm_named_mode drm_named_modes[] = { > - NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE), > - NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE), > + NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE, DRM_MODE_TV_MODE_NTSC_M), > + NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE, DRM_MODE_TV_MODE_PAL_B), > }; > > static int drm_mode_parse_cmdline_named_mode(const char *name, > @@ -2271,6 +2300,7 @@ static int drm_mode_parse_cmdline_named_mode(const char *name, > cmdline_mode->xres = mode->xres; > cmdline_mode->yres = mode->yres; > cmdline_mode->interlace = !!(mode->flags & DRM_MODE_FLAG_INTERLACE); > + cmdline_mode->tv_mode = mode->tv_mode; > cmdline_mode->specified = true; > > return 1; > diff --git a/drivers/gpu/drm/tests/drm_cmdline_parser_test.c b/drivers/gpu/drm/tests/drm_cmdline_parser_test.c > index 59b29cdfdd35..f1e73ed65be0 100644 > --- a/drivers/gpu/drm/tests/drm_cmdline_parser_test.c > +++ b/drivers/gpu/drm/tests/drm_cmdline_parser_test.c > @@ -885,6 +885,201 @@ static void drm_cmdline_test_multiple_options(struct kunit *test) > KUNIT_EXPECT_EQ(test, mode.force, DRM_FORCE_UNSPECIFIED); > } > > +static void drm_cmdline_test_tv_options(struct kunit *test, > + const char *cmdline, > + const struct drm_display_mode *expected_mode, > + unsigned int expected_tv_mode) > +{ > + struct drm_cmdline_mode mode = { }; > + > + KUNIT_EXPECT_TRUE(test, drm_mode_parse_command_line_for_connector(cmdline, > + &no_connector, &mode)); > + KUNIT_EXPECT_TRUE(test, mode.specified); > + KUNIT_EXPECT_EQ(test, mode.xres, expected_mode->hdisplay); > + KUNIT_EXPECT_EQ(test, mode.yres, expected_mode->vdisplay); > + KUNIT_EXPECT_EQ(test, mode.tv_mode, expected_tv_mode); > + > + KUNIT_EXPECT_FALSE(test, mode.refresh_specified); > + > + KUNIT_EXPECT_FALSE(test, mode.bpp_specified); > + > + KUNIT_EXPECT_FALSE(test, mode.rb); > + KUNIT_EXPECT_FALSE(test, mode.cvt); > + KUNIT_EXPECT_EQ(test, mode.interlace, !!(expected_mode->flags & DRM_MODE_FLAG_INTERLACE)); > + KUNIT_EXPECT_FALSE(test, mode.margins); > + KUNIT_EXPECT_EQ(test, mode.force, DRM_FORCE_UNSPECIFIED); > +} > + > +static void drm_cmdline_test_tv_option_ntsc_443(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x480i,tv_mode=NTSC-443", > + drm_mode_analog_ntsc_480i(NULL), > + DRM_MODE_TV_MODE_NTSC_443); > +} > + > +static void drm_cmdline_test_tv_option_ntsc_j(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x480i,tv_mode=NTSC-J", > + drm_mode_analog_ntsc_480i(NULL), > + DRM_MODE_TV_MODE_NTSC_J); > +} > + > +static void drm_cmdline_test_tv_option_ntsc_m(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x480i,tv_mode=NTSC-M", > + drm_mode_analog_ntsc_480i(NULL), > + DRM_MODE_TV_MODE_NTSC_M); > +} > + > +static void drm_cmdline_test_tv_option_pal_60(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=PAL-60", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_PAL_60); > +} > + > +static void drm_cmdline_test_tv_option_pal_b(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=PAL-B", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_PAL_B); > +} > + > +static void drm_cmdline_test_tv_option_pal_d(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=PAL-D", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_PAL_D); > +} > + > +static void drm_cmdline_test_tv_option_pal_g(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=PAL-G", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_PAL_G); > +} > + > +static void drm_cmdline_test_tv_option_pal_h(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=PAL-H", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_PAL_H); > +} > + > +static void drm_cmdline_test_tv_option_pal_i(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=PAL-I", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_PAL_I); > +} > + > +static void drm_cmdline_test_tv_option_pal_m(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x480i,tv_mode=PAL-M", > + drm_mode_analog_ntsc_480i(NULL), > + DRM_MODE_TV_MODE_PAL_M); > +} > + > +static void drm_cmdline_test_tv_option_pal_n(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=PAL-N", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_PAL_N); > +} > + > +static void drm_cmdline_test_tv_option_pal_nc(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=PAL-Nc", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_PAL_NC); > +} > + > +static void drm_cmdline_test_tv_option_secam_60(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=SECAM-60", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_SECAM_60); > +} > + > +static void drm_cmdline_test_tv_option_secam_b(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=SECAM-B", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_SECAM_B); > +} > + > +static void drm_cmdline_test_tv_option_secam_d(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=SECAM-D", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_SECAM_D); > +} > + > +static void drm_cmdline_test_tv_option_secam_g(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=SECAM-G", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_SECAM_G); > +} > + > +static void drm_cmdline_test_tv_option_secam_k(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=SECAM-K", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_SECAM_K); > +} > + > +static void drm_cmdline_test_tv_option_secam_k1(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=SECAM-K1", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_SECAM_K1); > +} > + > +static void drm_cmdline_test_tv_option_secam_l(struct kunit *test) > +{ > + drm_cmdline_test_tv_options(test, > + "720x576i,tv_mode=SECAM-L", > + drm_mode_analog_pal_576i(NULL), > + DRM_MODE_TV_MODE_SECAM_L); > +} Instead of creating a function to each drm_cmdline_test_tv_options test, you can create a parameterized test [1] for this function. This will help the readability of the tests. [1] https://docs.kernel.org/dev-tools/kunit/usage.html#parameterized-testing > + > +static void drm_cmdline_test_tv_option_invalid(struct kunit *test) > +{ > + struct drm_cmdline_mode mode = { }; > + const char *cmdline = "720x480i,tv_mode=invalid"; > + > + KUNIT_EXPECT_FALSE(test, drm_mode_parse_command_line_for_connector(cmdline, > + &no_connector, &mode)); > +} > + > +static void drm_cmdline_test_tv_option_truncated(struct kunit *test) > +{ > + struct drm_cmdline_mode mode = { }; > + const char *cmdline = "720x480i,tv_mode=NTSC"; > + > + KUNIT_EXPECT_FALSE(test, drm_mode_parse_command_line_for_connector(cmdline, > + &no_connector, &mode)); > +} > + I guess this tests can be incorporated in the future to the negative tests from Michał Winiarski [2]. I don't know which one will be merged first. [2] https://lore.kernel.org/dri-devel/20220817211236.252091-1-michal.winiarski@intel.com/ Best Regards, - Maíra Canal > static void drm_cmdline_test_invalid_option(struct kunit *test) > { > struct drm_cmdline_mode mode = { }; > @@ -1047,6 +1242,27 @@ static struct kunit_case drm_cmdline_parser_tests[] = { > KUNIT_CASE(drm_cmdline_test_name_refresh_invalid_mode), > KUNIT_CASE(drm_cmdline_test_name_option), > KUNIT_CASE(drm_cmdline_test_name_bpp_option), > + KUNIT_CASE(drm_cmdline_test_tv_option_ntsc_443), > + KUNIT_CASE(drm_cmdline_test_tv_option_ntsc_j), > + KUNIT_CASE(drm_cmdline_test_tv_option_ntsc_m), > + KUNIT_CASE(drm_cmdline_test_tv_option_pal_60), > + KUNIT_CASE(drm_cmdline_test_tv_option_pal_b), > + KUNIT_CASE(drm_cmdline_test_tv_option_pal_d), > + KUNIT_CASE(drm_cmdline_test_tv_option_pal_g), > + KUNIT_CASE(drm_cmdline_test_tv_option_pal_h), > + KUNIT_CASE(drm_cmdline_test_tv_option_pal_i), > + KUNIT_CASE(drm_cmdline_test_tv_option_pal_m), > + KUNIT_CASE(drm_cmdline_test_tv_option_pal_n), > + KUNIT_CASE(drm_cmdline_test_tv_option_pal_nc), > + KUNIT_CASE(drm_cmdline_test_tv_option_secam_60), > + KUNIT_CASE(drm_cmdline_test_tv_option_secam_b), > + KUNIT_CASE(drm_cmdline_test_tv_option_secam_d), > + KUNIT_CASE(drm_cmdline_test_tv_option_secam_g), > + KUNIT_CASE(drm_cmdline_test_tv_option_secam_k), > + KUNIT_CASE(drm_cmdline_test_tv_option_secam_k1), > + KUNIT_CASE(drm_cmdline_test_tv_option_secam_l), > + KUNIT_CASE(drm_cmdline_test_tv_option_invalid), > + KUNIT_CASE(drm_cmdline_test_tv_option_truncated), > KUNIT_CASE(drm_cmdline_test_rotate_0), > KUNIT_CASE(drm_cmdline_test_rotate_90), > KUNIT_CASE(drm_cmdline_test_rotate_180), > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index 49d261977d4e..9589108ba202 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -1447,6 +1447,11 @@ struct drm_cmdline_mode { > * @tv_margins: TV margins to apply to the mode. > */ > struct drm_connector_tv_margins tv_margins; > + > + /** > + * @tv_mode: TV mode standard. See DRM_MODE_TV_MODE_*. > + */ > + enum drm_connector_tv_mode tv_mode; > }; > > /** >
Hi Maxime, > @@ -2212,20 +2239,22 @@ struct drm_named_mode { > unsigned int xres; > unsigned int yres; > unsigned int flags; > + unsigned int tv_mode; > }; Are _all_ named modes supposed to be about analog TV? If so, then probably this structure should be renamed drm_named_analog_tv_mode or something. If not, then including tv_mode in all of them sounds almost dangrous. 0 is a valid value for enum drm_connector_tv_mode, corresponding to DRM_MODE_TV_MODE_NTSC_443. This is a very weird default (maybe it shouldn't be the one that has a numeric value of 0?) and if there ever is a named mode that is not related to analog TV, it looks that it will refer to NTSC-443. Not sure where could that actually propagate, and maybe what I'm saying can't happen, but I'm imagining weird scenarios where a GPU that has both a VGA/HDMI/whatever output, and a composite output, switches to NTSC-443 on the composite output by default because a named mode for the modern output is selected. Maybe something like DRM_MODE_TV_MODE_NONE = 0 would make sense? Maybe not. This is not an actual suggestion, just "thinking out loud". Best regards, Mateusz Kwiatkowski
On Fri, Sep 02, 2022 at 12:46:29AM +0200, Mateusz Kwiatkowski wrote: > > @@ -2212,20 +2239,22 @@ struct drm_named_mode { > > unsigned int xres; > > unsigned int yres; > > unsigned int flags; > > + unsigned int tv_mode; > > }; > > Are _all_ named modes supposed to be about analog TV? > > If so, then probably this structure should be renamed drm_named_analog_tv_mode > or something. I don't think they need to, but it's the only use case we've had so far. We could also imagine using UHD for 3840x2160 for example, so I wouldn't say it's limited to analog tv. > If not, then including tv_mode in all of them sounds almost dangrous. 0 is a > valid value for enum drm_connector_tv_mode, corresponding to > DRM_MODE_TV_MODE_NTSC_443. This is a very weird default (maybe it shouldn't be > the one that has a numeric value of 0?) and if there ever is a named mode that > is not related to analog TV, it looks that it will refer to NTSC-443. > > Not sure where could that actually propagate, and maybe what I'm saying can't > happen, but I'm imagining weird scenarios where a GPU that has both a > VGA/HDMI/whatever output, and a composite output, switches to NTSC-443 on the > composite output by default because a named mode for the modern output is > selected. So, named modes are per-connector so the fact that there's another output doesn't really matter. Then, the answer is quite simple actually, the HDMI driver wouldn't register and use the TV mode property at all, so it would completely ignore it, no matter what value it has. So it's not really a concern. > Maybe something like DRM_MODE_TV_MODE_NONE = 0 would make sense? But I guess we can add it still. Maxime
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 73d01e755496..a759a4ba0036 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -2115,6 +2115,30 @@ static int drm_mode_parse_panel_orientation(const char *delim, return 0; } +static int drm_mode_parse_tv_mode(const char *delim, + struct drm_cmdline_mode *mode) +{ + const char *value; + unsigned int len; + int ret; + + if (*delim != '=') + return -EINVAL; + + value = delim + 1; + delim = strchr(value, ','); + if (!delim) + delim = value + strlen(value); + + ret = drm_get_tv_mode_from_name(value, delim - value); + if (ret < 0) + return ret; + + mode->tv_mode = ret; + + return 0; +} + static int drm_mode_parse_cmdline_options(const char *str, bool freestanding, const struct drm_connector *connector, @@ -2184,6 +2208,9 @@ static int drm_mode_parse_cmdline_options(const char *str, } else if (!strncmp(option, "panel_orientation", delim - option)) { if (drm_mode_parse_panel_orientation(delim, mode)) return -EINVAL; + } else if (!strncmp(option, "tv_mode", delim - option)) { + if (drm_mode_parse_tv_mode(delim, mode)) + return -EINVAL; } else { return -EINVAL; } @@ -2212,20 +2239,22 @@ struct drm_named_mode { unsigned int xres; unsigned int yres; unsigned int flags; + unsigned int tv_mode; }; -#define NAMED_MODE(_name, _pclk, _x, _y, _flags) \ +#define NAMED_MODE(_name, _pclk, _x, _y, _flags, _mode) \ { \ .name = _name, \ .pixel_clock_khz = _pclk, \ .xres = _x, \ .yres = _y, \ .flags = _flags, \ + .tv_mode = _mode, \ } static const struct drm_named_mode drm_named_modes[] = { - NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE), - NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE), + NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE, DRM_MODE_TV_MODE_NTSC_M), + NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE, DRM_MODE_TV_MODE_PAL_B), }; static int drm_mode_parse_cmdline_named_mode(const char *name, @@ -2271,6 +2300,7 @@ static int drm_mode_parse_cmdline_named_mode(const char *name, cmdline_mode->xres = mode->xres; cmdline_mode->yres = mode->yres; cmdline_mode->interlace = !!(mode->flags & DRM_MODE_FLAG_INTERLACE); + cmdline_mode->tv_mode = mode->tv_mode; cmdline_mode->specified = true; return 1; diff --git a/drivers/gpu/drm/tests/drm_cmdline_parser_test.c b/drivers/gpu/drm/tests/drm_cmdline_parser_test.c index 59b29cdfdd35..f1e73ed65be0 100644 --- a/drivers/gpu/drm/tests/drm_cmdline_parser_test.c +++ b/drivers/gpu/drm/tests/drm_cmdline_parser_test.c @@ -885,6 +885,201 @@ static void drm_cmdline_test_multiple_options(struct kunit *test) KUNIT_EXPECT_EQ(test, mode.force, DRM_FORCE_UNSPECIFIED); } +static void drm_cmdline_test_tv_options(struct kunit *test, + const char *cmdline, + const struct drm_display_mode *expected_mode, + unsigned int expected_tv_mode) +{ + struct drm_cmdline_mode mode = { }; + + KUNIT_EXPECT_TRUE(test, drm_mode_parse_command_line_for_connector(cmdline, + &no_connector, &mode)); + KUNIT_EXPECT_TRUE(test, mode.specified); + KUNIT_EXPECT_EQ(test, mode.xres, expected_mode->hdisplay); + KUNIT_EXPECT_EQ(test, mode.yres, expected_mode->vdisplay); + KUNIT_EXPECT_EQ(test, mode.tv_mode, expected_tv_mode); + + KUNIT_EXPECT_FALSE(test, mode.refresh_specified); + + KUNIT_EXPECT_FALSE(test, mode.bpp_specified); + + KUNIT_EXPECT_FALSE(test, mode.rb); + KUNIT_EXPECT_FALSE(test, mode.cvt); + KUNIT_EXPECT_EQ(test, mode.interlace, !!(expected_mode->flags & DRM_MODE_FLAG_INTERLACE)); + KUNIT_EXPECT_FALSE(test, mode.margins); + KUNIT_EXPECT_EQ(test, mode.force, DRM_FORCE_UNSPECIFIED); +} + +static void drm_cmdline_test_tv_option_ntsc_443(struct kunit *test) +{ + drm_cmdline_test_tv_options(test, + "720x480i,tv_mode=NTSC-443", + drm_mode_analog_ntsc_480i(NULL), + DRM_MODE_TV_MODE_NTSC_443); +} + +static void drm_cmdline_test_tv_option_ntsc_j(struct kunit *test) +{ + drm_cmdline_test_tv_options(test, + "720x480i,tv_mode=NTSC-J", + drm_mode_analog_ntsc_480i(NULL), + DRM_MODE_TV_MODE_NTSC_J); +} + +static void drm_cmdline_test_tv_option_ntsc_m(struct kunit *test) +{ + drm_cmdline_test_tv_options(test, + "720x480i,tv_mode=NTSC-M", + drm_mode_analog_ntsc_480i(NULL), + DRM_MODE_TV_MODE_NTSC_M); +} + +static void drm_cmdline_test_tv_option_pal_60(struct kunit *test) +{ + drm_cmdline_test_tv_options(test, + "720x576i,tv_mode=PAL-60", + drm_mode_analog_pal_576i(NULL), + DRM_MODE_TV_MODE_PAL_60); +} + +static void drm_cmdline_test_tv_option_pal_b(struct kunit *test) +{ + drm_cmdline_test_tv_options(test, + "720x576i,tv_mode=PAL-B", + drm_mode_analog_pal_576i(NULL), + DRM_MODE_TV_MODE_PAL_B); +} + +static void drm_cmdline_test_tv_option_pal_d(struct kunit *test) +{ + drm_cmdline_test_tv_options(test, + "720x576i,tv_mode=PAL-D", + drm_mode_analog_pal_576i(NULL), + DRM_MODE_TV_MODE_PAL_D); +} + +static void drm_cmdline_test_tv_option_pal_g(struct kunit *test) +{ + drm_cmdline_test_tv_options(test, + "720x576i,tv_mode=PAL-G", + drm_mode_analog_pal_576i(NULL), + DRM_MODE_TV_MODE_PAL_G); +} + +static void drm_cmdline_test_tv_option_pal_h(struct kunit *test) +{ + drm_cmdline_test_tv_options(test, + "720x576i,tv_mode=PAL-H", + drm_mode_analog_pal_576i(NULL), + DRM_MODE_TV_MODE_PAL_H); +} + +static void drm_cmdline_test_tv_option_pal_i(struct kunit *test) +{ + drm_cmdline_test_tv_options(test, + "720x576i,tv_mode=PAL-I", + drm_mode_analog_pal_576i(NULL), + DRM_MODE_TV_MODE_PAL_I); +} + +static void drm_cmdline_test_tv_option_pal_m(struct kunit *test) +{ + drm_cmdline_test_tv_options(test, + "720x480i,tv_mode=PAL-M", + drm_mode_analog_ntsc_480i(NULL), + DRM_MODE_TV_MODE_PAL_M); +} + +static void drm_cmdline_test_tv_option_pal_n(struct kunit *test) +{ + drm_cmdline_test_tv_options(test, + "720x576i,tv_mode=PAL-N", + drm_mode_analog_pal_576i(NULL), + DRM_MODE_TV_MODE_PAL_N); +} + +static void drm_cmdline_test_tv_option_pal_nc(struct kunit *test) +{ + drm_cmdline_test_tv_options(test, + "720x576i,tv_mode=PAL-Nc", + drm_mode_analog_pal_576i(NULL), + DRM_MODE_TV_MODE_PAL_NC); +} + +static void drm_cmdline_test_tv_option_secam_60(struct kunit *test) +{ + drm_cmdline_test_tv_options(test, + "720x576i,tv_mode=SECAM-60", + drm_mode_analog_pal_576i(NULL), + DRM_MODE_TV_MODE_SECAM_60); +} + +static void drm_cmdline_test_tv_option_secam_b(struct kunit *test) +{ + drm_cmdline_test_tv_options(test, + "720x576i,tv_mode=SECAM-B", + drm_mode_analog_pal_576i(NULL), + DRM_MODE_TV_MODE_SECAM_B); +} + +static void drm_cmdline_test_tv_option_secam_d(struct kunit *test) +{ + drm_cmdline_test_tv_options(test, + "720x576i,tv_mode=SECAM-D", + drm_mode_analog_pal_576i(NULL), + DRM_MODE_TV_MODE_SECAM_D); +} + +static void drm_cmdline_test_tv_option_secam_g(struct kunit *test) +{ + drm_cmdline_test_tv_options(test, + "720x576i,tv_mode=SECAM-G", + drm_mode_analog_pal_576i(NULL), + DRM_MODE_TV_MODE_SECAM_G); +} + +static void drm_cmdline_test_tv_option_secam_k(struct kunit *test) +{ + drm_cmdline_test_tv_options(test, + "720x576i,tv_mode=SECAM-K", + drm_mode_analog_pal_576i(NULL), + DRM_MODE_TV_MODE_SECAM_K); +} + +static void drm_cmdline_test_tv_option_secam_k1(struct kunit *test) +{ + drm_cmdline_test_tv_options(test, + "720x576i,tv_mode=SECAM-K1", + drm_mode_analog_pal_576i(NULL), + DRM_MODE_TV_MODE_SECAM_K1); +} + +static void drm_cmdline_test_tv_option_secam_l(struct kunit *test) +{ + drm_cmdline_test_tv_options(test, + "720x576i,tv_mode=SECAM-L", + drm_mode_analog_pal_576i(NULL), + DRM_MODE_TV_MODE_SECAM_L); +} + +static void drm_cmdline_test_tv_option_invalid(struct kunit *test) +{ + struct drm_cmdline_mode mode = { }; + const char *cmdline = "720x480i,tv_mode=invalid"; + + KUNIT_EXPECT_FALSE(test, drm_mode_parse_command_line_for_connector(cmdline, + &no_connector, &mode)); +} + +static void drm_cmdline_test_tv_option_truncated(struct kunit *test) +{ + struct drm_cmdline_mode mode = { }; + const char *cmdline = "720x480i,tv_mode=NTSC"; + + KUNIT_EXPECT_FALSE(test, drm_mode_parse_command_line_for_connector(cmdline, + &no_connector, &mode)); +} + static void drm_cmdline_test_invalid_option(struct kunit *test) { struct drm_cmdline_mode mode = { }; @@ -1047,6 +1242,27 @@ static struct kunit_case drm_cmdline_parser_tests[] = { KUNIT_CASE(drm_cmdline_test_name_refresh_invalid_mode), KUNIT_CASE(drm_cmdline_test_name_option), KUNIT_CASE(drm_cmdline_test_name_bpp_option), + KUNIT_CASE(drm_cmdline_test_tv_option_ntsc_443), + KUNIT_CASE(drm_cmdline_test_tv_option_ntsc_j), + KUNIT_CASE(drm_cmdline_test_tv_option_ntsc_m), + KUNIT_CASE(drm_cmdline_test_tv_option_pal_60), + KUNIT_CASE(drm_cmdline_test_tv_option_pal_b), + KUNIT_CASE(drm_cmdline_test_tv_option_pal_d), + KUNIT_CASE(drm_cmdline_test_tv_option_pal_g), + KUNIT_CASE(drm_cmdline_test_tv_option_pal_h), + KUNIT_CASE(drm_cmdline_test_tv_option_pal_i), + KUNIT_CASE(drm_cmdline_test_tv_option_pal_m), + KUNIT_CASE(drm_cmdline_test_tv_option_pal_n), + KUNIT_CASE(drm_cmdline_test_tv_option_pal_nc), + KUNIT_CASE(drm_cmdline_test_tv_option_secam_60), + KUNIT_CASE(drm_cmdline_test_tv_option_secam_b), + KUNIT_CASE(drm_cmdline_test_tv_option_secam_d), + KUNIT_CASE(drm_cmdline_test_tv_option_secam_g), + KUNIT_CASE(drm_cmdline_test_tv_option_secam_k), + KUNIT_CASE(drm_cmdline_test_tv_option_secam_k1), + KUNIT_CASE(drm_cmdline_test_tv_option_secam_l), + KUNIT_CASE(drm_cmdline_test_tv_option_invalid), + KUNIT_CASE(drm_cmdline_test_tv_option_truncated), KUNIT_CASE(drm_cmdline_test_rotate_0), KUNIT_CASE(drm_cmdline_test_rotate_90), KUNIT_CASE(drm_cmdline_test_rotate_180), diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 49d261977d4e..9589108ba202 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -1447,6 +1447,11 @@ struct drm_cmdline_mode { * @tv_margins: TV margins to apply to the mode. */ struct drm_connector_tv_margins tv_margins; + + /** + * @tv_mode: TV mode standard. See DRM_MODE_TV_MODE_*. + */ + enum drm_connector_tv_mode tv_mode; }; /**
Our new tv mode option allows to specify the TV mode from a property. However, it can still be useful, for example to avoid any boot time artifact, to set that property directly from the kernel command line. Let's add some code to allow it, and some unit tests to exercise that code. Signed-off-by: Maxime Ripard <maxime@cerno.tech>