Re: [PATCH v7 15/23] drm/modes: Introduce more named modes

[Maxime Ripard <maxime@cerno.tech>]

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 <maxime@cerno.tech>
> >>>
> >>> ---
> >>> 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