Den 07.11.2022 15.16, 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 <[email protected]>
>
> ---
> Changes in v6:
> - Renamed the tests to follow DRM test naming convention
>
> Changes in v5:
> - Switched to KUNIT_ASSERT_NOT_NULL
> ---
> drivers/gpu/drm/drm_modes.c | 2 +
> drivers/gpu/drm/tests/drm_client_modeset_test.c | 54 +++++++++++++++++++++++++
> 2 files changed, 56 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 49441cabdd9d..17c5b6108103 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -2272,7 +2272,9 @@ struct drm_named_mode {
>
> static const struct drm_named_mode drm_named_modes[] = {
> NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE, DRM_MODE_TV_MODE_NTSC),
> + NAMED_MODE("NTSC-J", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE, DRM_MODE_TV_MODE_NTSC_J),
> NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE, DRM_MODE_TV_MODE_PAL),
> + NAMED_MODE("PAL-M", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE, DRM_MODE_TV_MODE_PAL_M),
> };
I'm now having second thoughts about the tv_mode commandline option. Can
we just add all the variants to this table and drop the tv_mode option?
IMO this will be more user friendly and less confusing.
The named modes needs to be documented in modedb.rst.
Noralf.
>
> static int drm_mode_parse_cmdline_named_mode(const char *name,
> diff --git a/drivers/gpu/drm/tests/drm_client_modeset_test.c b/drivers/gpu/drm/tests/drm_client_modeset_test.c
> index fdfe9e20702e..b3820d25beca 100644
> --- a/drivers/gpu/drm/tests/drm_client_modeset_test.c
> +++ b/drivers/gpu/drm/tests/drm_client_modeset_test.c
> @@ -133,6 +133,32 @@ static void drm_test_pick_cmdline_named_ntsc(struct kunit *test)
> KUNIT_EXPECT_TRUE(test, drm_mode_equal(drm_mode_analog_ntsc_480i(drm), mode));
> }
>
> +static void drm_test_pick_cmdline_named_ntsc_j(struct kunit *test)
> +{
> + struct drm_client_modeset_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_NOT_NULL(test, mode);
> +
> + KUNIT_EXPECT_TRUE(test, drm_mode_equal(drm_mode_analog_ntsc_480i(drm), mode));
> +}
> +
> static void drm_test_pick_cmdline_named_pal(struct kunit *test)
> {
> struct drm_client_modeset_test_priv *priv = test->priv;
> @@ -159,10 +185,38 @@ static void drm_test_pick_cmdline_named_pal(struct kunit *test)
> KUNIT_EXPECT_TRUE(test, drm_mode_equal(drm_mode_analog_pal_576i(drm), mode));
> }
>
> +static void drm_test_pick_cmdline_named_pal_m(struct kunit *test)
> +{
> + struct drm_client_modeset_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_NOT_NULL(test, mode);
> +
> + KUNIT_EXPECT_TRUE(test, drm_mode_equal(drm_mode_analog_ntsc_480i(drm), mode));
> +}
> +
> static struct kunit_case drm_test_pick_cmdline_tests[] = {
> KUNIT_CASE(drm_test_pick_cmdline_res_1920_1080_60),
> KUNIT_CASE(drm_test_pick_cmdline_named_ntsc),
> + KUNIT_CASE(drm_test_pick_cmdline_named_ntsc_j),
> KUNIT_CASE(drm_test_pick_cmdline_named_pal),
> + KUNIT_CASE(drm_test_pick_cmdline_named_pal_m),
> {}
> };
>
>
Den 07.11.2022 19.03, skrev Noralf Trønnes:
>
>
> Den 07.11.2022 15.16, 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 <[email protected]>
>>
>> ---
>> Changes in v6:
>> - Renamed the tests to follow DRM test naming convention
>>
>> Changes in v5:
>> - Switched to KUNIT_ASSERT_NOT_NULL
>> ---
>> drivers/gpu/drm/drm_modes.c | 2 +
>> drivers/gpu/drm/tests/drm_client_modeset_test.c | 54 +++++++++++++++++++++++++
>> 2 files changed, 56 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>> index 49441cabdd9d..17c5b6108103 100644
>> --- a/drivers/gpu/drm/drm_modes.c
>> +++ b/drivers/gpu/drm/drm_modes.c
>> @@ -2272,7 +2272,9 @@ struct drm_named_mode {
>>
>> static const struct drm_named_mode drm_named_modes[] = {
>> NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE, DRM_MODE_TV_MODE_NTSC),
>> + NAMED_MODE("NTSC-J", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE, DRM_MODE_TV_MODE_NTSC_J),
>> NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE, DRM_MODE_TV_MODE_PAL),
>> + NAMED_MODE("PAL-M", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE, DRM_MODE_TV_MODE_PAL_M),
>> };
>
> I'm now having second thoughts about the tv_mode commandline option. Can
> we just add all the variants to this table and drop the tv_mode option?
> IMO this will be more user friendly and less confusing.
>
One downside of this is that it's not possible to force connector status
when using named modes, but I think it would be better to have a force
option than a tv_mode option. A lot of userspace treats unknown status
as disconnected.
Anyone know if it's possible to set the connector status sysfs file
using a udev rule?
Noralf.
Hi Noralf,
W dniu 8.11.2022 o 10:38, Noralf Trønnes pisze:
>
> Den 07.11.2022 19.03, skrev Noralf Trønnes:
>>
>> Den 07.11.2022 15.16, 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 <[email protected]>
>>>
>>> ---
>>> Changes in v6:
>>> - Renamed the tests to follow DRM test naming convention
>>>
>>> Changes in v5:
>>> - Switched to KUNIT_ASSERT_NOT_NULL
>>> ---
>>> drivers/gpu/drm/drm_modes.c | 2 +
>>> drivers/gpu/drm/tests/drm_client_modeset_test.c | 54 +++++++++++++++++++++++++
>>> 2 files changed, 56 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>>> index 49441cabdd9d..17c5b6108103 100644
>>> --- a/drivers/gpu/drm/drm_modes.c
>>> +++ b/drivers/gpu/drm/drm_modes.c
>>> @@ -2272,7 +2272,9 @@ struct drm_named_mode {
>>>
>>> static const struct drm_named_mode drm_named_modes[] = {
>>> NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE, DRM_MODE_TV_MODE_NTSC),
>>> + NAMED_MODE("NTSC-J", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE, DRM_MODE_TV_MODE_NTSC_J),
>>> NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE, DRM_MODE_TV_MODE_PAL),
>>> + NAMED_MODE("PAL-M", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE, DRM_MODE_TV_MODE_PAL_M),
>>> };
>> I'm now having second thoughts about the tv_mode commandline option. Can
>> we just add all the variants to this table and drop the tv_mode option?
>> IMO this will be more user friendly and less confusing.
>>
> One downside of this is that it's not possible to force connector status
> when using named modes, but I think it would be better to have a force
> option than a tv_mode option. A lot of userspace treats unknown status
> as disconnected.
>
> Anyone know if it's possible to set the connector status sysfs file
> using a udev rule?
>
> Noralf.
I think that leaving named modes only would be a bit limiting. There are use
cases for custom modes, e.g. we might want progressive 240p "NTSC" (like 80s/90s
home computers and video game consoles) or the modes with non-13.5MHz pixel
clock that Geert requested with Amiga in mind.
I'm not sure if the current cmdline-to-drm_mode conversion is flexible enough
to meaningfully facilitate those, but we're at least getting the syntax down.
Best regards,
Mateusz Kwiatkowski
On Tue, Nov 08, 2022 at 10:27:17PM +0100, Mateusz Kwiatkowski wrote:
> Hi Noralf,
>
> W dniu 8.11.2022 o 10:38, Noralf Tr?nnes pisze:
> >
> > Den 07.11.2022 19.03, skrev Noralf Tr?nnes:
> >>
> >> Den 07.11.2022 15.16, 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 <[email protected]>
> >>>
> >>> ---
> >>> Changes in v6:
> >>> - Renamed the tests to follow DRM test naming convention
> >>>
> >>> Changes in v5:
> >>> - Switched to KUNIT_ASSERT_NOT_NULL
> >>> ---
> >>> drivers/gpu/drm/drm_modes.c | 2 +
> >>> drivers/gpu/drm/tests/drm_client_modeset_test.c | 54 +++++++++++++++++++++++++
> >>> 2 files changed, 56 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> >>> index 49441cabdd9d..17c5b6108103 100644
> >>> --- a/drivers/gpu/drm/drm_modes.c
> >>> +++ b/drivers/gpu/drm/drm_modes.c
> >>> @@ -2272,7 +2272,9 @@ struct drm_named_mode {
> >>>
> >>> static const struct drm_named_mode drm_named_modes[] = {
> >>> NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE, DRM_MODE_TV_MODE_NTSC),
> >>> + NAMED_MODE("NTSC-J", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE, DRM_MODE_TV_MODE_NTSC_J),
> >>> NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE, DRM_MODE_TV_MODE_PAL),
> >>> + NAMED_MODE("PAL-M", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE, DRM_MODE_TV_MODE_PAL_M),
> >>> };
> >> I'm now having second thoughts about the tv_mode commandline option. Can
> >> we just add all the variants to this table and drop the tv_mode option?
> >> IMO this will be more user friendly and less confusing.
> >>
> > One downside of this is that it's not possible to force connector status
> > when using named modes, but I think it would be better to have a force
> > option than a tv_mode option. A lot of userspace treats unknown status
> > as disconnected.
> >
> > Anyone know if it's possible to set the connector status sysfs file
> > using a udev rule?
> >
> > Noralf.
>
> I think that leaving named modes only would be a bit limiting. There are use
> cases for custom modes, e.g. we might want progressive 240p "NTSC" (like 80s/90s
> home computers and video game consoles) or the modes with non-13.5MHz pixel
> clock that Geert requested with Amiga in mind.
Yeah, it was one of the early requirements that we would be allowed to
fill in any analog mode on the command line, so just having the named
modes with the 480i and 576i modes won't really work for that.
> I'm not sure if the current cmdline-to-drm_mode conversion is flexible enough
> to meaningfully facilitate those, but we're at least getting the syntax down.
It might require a bit of plumbing to get
drm_mode_create_from_cmdline_mode() to add the mode if tv_mode_specified
is set, but it's probably it.
Maxime