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

[maxime.ripard@free-electrons.com (Maxime Ripard)]

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@free-electrons.com> 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@free-electrons.com>
> > ---
> >  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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161121/e27ca461/attachment.sig>