Re: [PATCH v1 34/35] drm/modes: Introduce the tv_mode property as a command-line option

[Maxime Ripard]

On Fri, Aug 12, 2022 at 03:31:19PM +0200, Geert Uytterhoeven wrote:
> Hi Maxime,
> 
> On Fri, Jul 29, 2022 at 6:37 PM Maxime Ripard <maxime@xxxxxxxxxx> 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@xxxxxxxxxx>
> 
> Thanks for your patch!
> 
> > --- a/drivers/gpu/drm/drm_modes.c
> > +++ b/drivers/gpu/drm/drm_modes.c
> > @@ -1677,6 +1677,80 @@ static int drm_mode_parse_panel_orientation(const char *delim,
> >         return 0;
> >  }
> >
> > +#define TV_OPTION_EQUAL(value, len, option) \
> > +       ((strlen(option) == len) && !strncmp(value, option, len))
> > +
> > +static int drm_mode_parse_tv_mode(const char *delim,
> > +                                 struct drm_cmdline_mode *mode)
> > +{
> > +       const char *value;
> > +       unsigned int len;
> > +
> > +       if (*delim != '=')
> > +               return -EINVAL;
> > +
> > +       value = delim + 1;
> > +       delim = strchr(value, ',');
> > +       if (!delim)
> > +               delim = value + strlen(value);
> > +
> > +       len = delim - value;
> > +       if (TV_OPTION_EQUAL(value, len, "NTSC-443"))
> > +               mode->tv_mode = DRM_MODE_TV_NORM_NTSC_443;
> > +       else if (TV_OPTION_EQUAL(value, len, "NTSC-J"))
> > +               mode->tv_mode = DRM_MODE_TV_NORM_NTSC_J;
> > +       else if (TV_OPTION_EQUAL(value, len, "NTSC-M"))
> > +               mode->tv_mode = DRM_MODE_TV_NORM_NTSC_M;
> 
> [...]
> 
> You already have the array tv_norm_values[] from "[PATCH v1 05/35]
> drm/connector: Add TV standard property". Can't you export that, and
> loop over that array instead?

I'm not sure, the command line doesn't follow the same conventions than
the property names for a number of conventions, but at the same time we
should try to keep it as consistent as possible...

Then again, Jani and Thomas didn't seem too fond about exposing data as
part of the API, so I'm not sure how we could expose that properly.

> > +       else if (TV_OPTION_EQUAL(value, len, "HD480I"))
> > +               mode->tv_mode = DRM_MODE_TV_NORM_HD480I;
> > +       else if (TV_OPTION_EQUAL(value, len, "HD480P"))
> > +               mode->tv_mode = DRM_MODE_TV_NORM_HD480P;
> > +       else if (TV_OPTION_EQUAL(value, len, "HD576I"))
> > +               mode->tv_mode = DRM_MODE_TV_NORM_HD576I;
> > +       else if (TV_OPTION_EQUAL(value, len, "HD576P"))
> > +               mode->tv_mode = DRM_MODE_TV_NORM_HD576P;
> > +       else if (TV_OPTION_EQUAL(value, len, "HD720P"))
> > +               mode->tv_mode = DRM_MODE_TV_NORM_HD720P;
> > +       else if (TV_OPTION_EQUAL(value, len, "HD1080I"))
> > +               mode->tv_mode = DRM_MODE_TV_NORM_HD1080I;
> 
> The names in tv_norm_values[] use lower-case, while you use upper-case
> here.

Indeed, I'll fix it, thanks!
Maxime

Attachment: signature.asc
Description: PGP signature