Re: [PATCH 2/5] drm/modes: Support modes names on the command line

From: Maxime Ripard
Date: Mon Nov 21 2016 - 02:41:00 EST

Hi Sean,

On Wed, Nov 16, 2016 at 12:21:42PM -0500, Sean Paul wrote:
> On Tue, Oct 18, 2016 at 4:29 AM, Maxime Ripard
> <maxime.ripard@xxxxxxxxxxxxxxxxxx> wrote:
> > The drm subsystem also uses the video= kernel parameter, and in the
> > documentation refers to the fbdev documentation for that parameter.
> >
> > However, that documentation also says that instead of giving the mode using
> > its resolution we can also give a name. However, DRM doesn't handle that
> > case at the moment. Even though in most case it shouldn't make any
> > difference, it might be useful for analog modes, where different standards
> > might have the same resolution, but still have a few different parameters
> > that are not encoded in the modes (NTSC vs NTSC-J vs PAL-M for example).
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/drm_connector.c | 3 +-
> > drivers/gpu/drm/drm_fb_helper.c | 4 +++-
> > drivers/gpu/drm/drm_modes.c | 49 +++++++++++++++++++++++-----------
> > include/drm/drm_connector.h | 1 +-
> > 4 files changed, 41 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index 2db7fb510b6c..27a8a511257c 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -147,8 +147,9 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
> > connector->force = mode->force;
> > }
> >
> > - DRM_DEBUG_KMS("cmdline mode for connector %s %dx%d@%dHz%s%s%s\n",
> > + DRM_DEBUG_KMS("cmdline mode for connector %s %s %dx%d@%dHz%s%s%s\n",
> > connector->name,
> > + mode->name ? mode->name : "",
> > mode->xres, mode->yres,
> > mode->refresh_specified ? mode->refresh : 60,
> > mode->rb ? " reduced blanking" : "",
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index 03414bde1f15..20a68305fb45 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -1748,6 +1748,10 @@ struct drm_display_mode *drm_pick_cmdline_mode(struct drm_fb_helper_connector *f
> > prefer_non_interlace = !cmdline_mode->interlace;
> > again:
> > list_for_each_entry(mode, &fb_helper_conn->connector->modes, head) {
> > + /* Check (optional) mode name first */
> > + if (!strcmp(mode->name, cmdline_mode->name))
> > + return mode;
> > +
> > /* check width/height */
> > if (mode->hdisplay != cmdline_mode->xres ||
> > mode->vdisplay != cmdline_mode->yres)
> > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > index 7d5bdca276f2..fdbf541a5978 100644
> > --- a/drivers/gpu/drm/drm_modes.c
> > +++ b/drivers/gpu/drm/drm_modes.c
> > @@ -1413,7 +1413,7 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
> > struct drm_cmdline_mode *mode)
> > {
> > const char *name;
> > - bool parse_extras = false;
> > + bool named_mode = false, parse_extras = false;
> > unsigned int bpp_off = 0, refresh_off = 0;
> > unsigned int mode_end = 0;
> > char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
> > @@ -1432,8 +1432,14 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
> >
> > name = mode_option;
> >
> > + /*
> > + * If the first character is not a digit, then it means that
> > + * we have a named mode.
> > + */
> > if (!isdigit(name[0]))
> > - return false;
> > + named_mode = true;
> > + else
> > + named_mode = false;
>
> named_mode = isalpha(name[0]); might be more succinct (and covers
> special characters).
>
> >
> > /* Try to locate the bpp and refresh specifiers, if any */
> > bpp_ptr = strchr(name, '-');
> > @@ -1460,12 +1466,16 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
> > parse_extras = true;
> > }
> >
> > - ret = drm_mode_parse_cmdline_res_mode(name, mode_end,
> > - parse_extras,
> > - connector,
> > - mode);
> > - if (ret)
> > - return false;
> > + if (named_mode) {
> > + strncpy(mode->name, name, mode_end);
> > + } else {
> > + ret = drm_mode_parse_cmdline_res_mode(name, mode_end,
> > + parse_extras,
> > + connector,
> > + mode);
> > + if (ret)
> > + return false;
> > + }
> > mode->specified = true;
> >
> > if (bpp_ptr) {
> > @@ -1493,14 +1503,23 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
> > extra_ptr = refresh_end_ptr;
> >
> > if (extra_ptr) {
> > - int remaining = strlen(name) - (extra_ptr - name);
> > + if (!named_mode) {
> > + int len = strlen(name) - (extra_ptr - name);
> >
> > - /*
> > - * We still have characters to process, while
> > - * we shouldn't have any
> > - */
> > - if (remaining > 0)
> > - return false;
> > + ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
> > + connector, mode);
> > + if (ret)
> > + return false;
> > + } else {
> > + int remaining = strlen(name) - (extra_ptr - name);
> > +
> > + /*
> > + * We still have characters to process, while
> > + * we shouldn't have any
> > + */
> > + if (remaining > 0)
> > + return false;
>
> Correct me if I'm wrong, but this shouldn't ever happen. AFAICT, the
> only way it could would be if we parsed bpp or refresh in the named
> mode (since those are the only cases where we don't copy the whole
> string over). Shouldn't that be invalid anyways?

It's supposed to be supported by the video
string. Documentation/fb/modedb.txt defines:

Valid mode specifiers (mode_option argument):

<xres>x<yres>[M][R][-<bpp>][@<refresh>][i][m][eDd]
<name>[-<bpp>][@<refresh>]

So we can't just copy the string over, and we need to parse it :/

Thanks!
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Attachment: signature.asc
Description: PGP signature