Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933142AbcKPRNX (ORCPT ); Wed, 16 Nov 2016 12:13:23 -0500 Received: from mail-yw0-f173.google.com ([209.85.161.173]:34273 "EHLO mail-yw0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752899AbcKPRNR (ORCPT ); Wed, 16 Nov 2016 12:13:17 -0500 MIME-Version: 1.0 In-Reply-To: References: From: Sean Paul Date: Wed, 16 Nov 2016 12:12:53 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 1/5] drm/modes: Rewrite the command line parser To: Maxime Ripard Cc: Daniel Vetter , David Airlie , dri-devel , Laurent Pinchart , Boris Brezillon , Thomas Petazzoni , Linux ARM Kernel , Hans de Goede , Chen-Yu Tsai , Linux Kernel Mailing List Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 14823 Lines: 390 On Tue, Oct 18, 2016 at 4:29 AM, Maxime Ripard wrote: > Rewrite the command line parser in order to get away from the state machine > parsing the video mode lines. > > Hopefully, this will allow to extend it more easily to support named modes > and / or properties set directly on the command line. > > Signed-off-by: Maxime Ripard > --- > drivers/gpu/drm/drm_modes.c | 305 +++++++++++++++++++++++-------------- > 1 file changed, 190 insertions(+), 115 deletions(-) > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c > index 53f07ac7c174..7d5bdca276f2 100644 > --- a/drivers/gpu/drm/drm_modes.c > +++ b/drivers/gpu/drm/drm_modes.c > @@ -30,6 +30,7 @@ > * authorization from the copyright holder(s) and author(s). > */ > > +#include > #include > #include > #include > @@ -1261,6 +1262,131 @@ void drm_mode_connector_list_update(struct drm_connector *connector) > } > EXPORT_SYMBOL(drm_mode_connector_list_update); > > +static int drm_mode_parse_cmdline_bpp(const char *str, char **end_ptr, > + struct drm_cmdline_mode *mode) > +{ > + if (str[0] != '-') > + return -EINVAL; > + > + mode->bpp = simple_strtol(str + 1, end_ptr, 10); > + mode->bpp_specified = true; > + > + return 0; > +} > + > +static int drm_mode_parse_cmdline_refresh(const char *str, char **end_ptr, > + struct drm_cmdline_mode *mode) > +{ > + if (str[0] != '@') > + return -EINVAL; > + > + mode->refresh = simple_strtol(str + 1, end_ptr, 10); > + mode->refresh_specified = true; > + > + return 0; > +} > + > +static int drm_mode_parse_cmdline_extra(const char *str, int length, > + struct drm_connector *connector, > + struct drm_cmdline_mode *mode) > +{ > + int i; > + > + for (i = 0; i < length; i++) { > + switch (str[i]) { > + case 'i': > + mode->interlace = true; > + break; > + case 'm': > + mode->margins = true; > + break; > + case 'D': > + if (mode->force != DRM_FORCE_UNSPECIFIED) > + return -EINVAL; > + > + if ((connector->connector_type != DRM_MODE_CONNECTOR_DVII) && > + (connector->connector_type != DRM_MODE_CONNECTOR_HDMIB)) > + mode->force = DRM_FORCE_ON; > + else > + mode->force = DRM_FORCE_ON_DIGITAL; > + break; > + case 'd': > + if (mode->force != DRM_FORCE_UNSPECIFIED) > + return -EINVAL; > + > + mode->force = DRM_FORCE_OFF; > + break; > + case 'e': > + if (mode->force != DRM_FORCE_UNSPECIFIED) > + return -EINVAL; > + > + mode->force = DRM_FORCE_ON; > + break; > + default: > + return -EINVAL; > + } > + } > + > + return 0; > +} > + > +static int drm_mode_parse_cmdline_res_mode(const char *str, unsigned int length, > + bool extras, > + struct drm_connector *connector, > + struct drm_cmdline_mode *mode) > +{ > + bool rb = false, cvt = false; > + int xres = 0, yres = 0; > + int remaining, i; > + char *end_ptr; > + > + xres = simple_strtol(str, &end_ptr, 10); > + checkpatch is telling me to use kstrtol instead, as simple_strtol is deprecated > + if (end_ptr[0] != 'x') check that end_ptr != NULL? you should probably also check that xres isn't an error (ie: -ERANGE or -EINVAL) > + return -EINVAL; > + end_ptr++; > + > + yres = simple_strtol(end_ptr, &end_ptr, 10); check end_ptr != NULL and yres sane > + > + remaining = length - (end_ptr - str); > + if (remaining < 0) right, so if end_ptr is NULL here, we'll end up with a huge positive value for remaining :) > + return -EINVAL; > + > + for (i = 0; i < remaining; i++) { > + switch (end_ptr[i]) { > + case 'M': > + cvt = true; the previous code ensured proper ordering as well as parsing, whereas yours will take these in any order (and accepts duplicates). i don't think this should cause any issues, but perhaps something to verify. > + break; > + case 'R': > + rb = true; > + break; > + default: > + /* > + * Try to pass that to our extras parsing > + * function to handle the case where the > + * extras are directly after the resolution > + */ > + if (extras) { > + int ret = drm_mode_parse_cmdline_extra(end_ptr + i, > + 1, > + connector, > + mode); > + if (ret) > + return ret; > + } else { > + return -EINVAL; > + } > + } > + } > + > + mode->xres = xres; > + mode->yres = yres; > + mode->cvt = cvt; > + mode->rb = rb; > + > + return 0; > +} > + > /** > * drm_mode_parse_command_line_for_connector - parse command line modeline for connector > * @mode_option: optional per connector mode option > @@ -1287,13 +1413,12 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, > struct drm_cmdline_mode *mode) > { > const char *name; > - unsigned int namelen; > - bool res_specified = false, bpp_specified = false, refresh_specified = false; > - unsigned int xres = 0, yres = 0, bpp = 32, refresh = 0; > - bool yres_specified = false, cvt = false, rb = false; > - bool interlace = false, margins = false, was_digit = false; > - int i; > - enum drm_connector_force force = DRM_FORCE_UNSPECIFIED; > + bool 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; > + char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL; > + int ret; > > #ifdef CONFIG_FB > if (!mode_option) > @@ -1306,127 +1431,77 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, > } > > name = mode_option; > - namelen = strlen(name); > - for (i = namelen-1; i >= 0; i--) { > - switch (name[i]) { > - case '@': > - if (!refresh_specified && !bpp_specified && > - !yres_specified && !cvt && !rb && was_digit) { > - refresh = simple_strtol(&name[i+1], NULL, 10); > - refresh_specified = true; > - was_digit = false; > - } else > - goto done; > - break; > - case '-': > - if (!bpp_specified && !yres_specified && !cvt && > - !rb && was_digit) { > - bpp = simple_strtol(&name[i+1], NULL, 10); > - bpp_specified = true; > - was_digit = false; > - } else > - goto done; > - break; > - case 'x': > - if (!yres_specified && was_digit) { > - yres = simple_strtol(&name[i+1], NULL, 10); > - yres_specified = true; > - was_digit = false; > - } else > - goto done; > - break; > - case '0' ... '9': > - was_digit = true; > - break; > - case 'M': > - if (yres_specified || cvt || was_digit) > - goto done; > - cvt = true; > - break; > - case 'R': > - if (yres_specified || cvt || rb || was_digit) > - goto done; > - rb = true; > - break; > - case 'm': > - if (cvt || yres_specified || was_digit) > - goto done; > - margins = true; > - break; > - case 'i': > - if (cvt || yres_specified || was_digit) > - goto done; > - interlace = true; > - break; > - case 'e': > - if (yres_specified || bpp_specified || refresh_specified || > - was_digit || (force != DRM_FORCE_UNSPECIFIED)) > - goto done; > > - force = DRM_FORCE_ON; > - break; > - case 'D': > - if (yres_specified || bpp_specified || refresh_specified || > - was_digit || (force != DRM_FORCE_UNSPECIFIED)) > - goto done; > + if (!isdigit(name[0])) > + return false; > > - if ((connector->connector_type != DRM_MODE_CONNECTOR_DVII) && > - (connector->connector_type != DRM_MODE_CONNECTOR_HDMIB)) > - force = DRM_FORCE_ON; > - else > - force = DRM_FORCE_ON_DIGITAL; > - break; > - case 'd': > - if (yres_specified || bpp_specified || refresh_specified || > - was_digit || (force != DRM_FORCE_UNSPECIFIED)) > - goto done; > + /* Try to locate the bpp and refresh specifiers, if any */ > + bpp_ptr = strchr(name, '-'); > + if (bpp_ptr) { > + bpp_off = bpp_ptr - name; > + mode->bpp_specified = true; > + } > > - force = DRM_FORCE_OFF; > - break; > - default: > - goto done; > - } > + refresh_ptr = strchr(name, '@'); > + if (refresh_ptr) { > + refresh_off = refresh_ptr - name; > + mode->refresh_specified = true; > } > > - if (i < 0 && yres_specified) { > - char *ch; > - xres = simple_strtol(name, &ch, 10); > - if ((ch != NULL) && (*ch == 'x')) > - res_specified = true; > - else > - i = ch - name; > - } else if (!yres_specified && was_digit) { > - /* catch mode that begins with digits but has no 'x' */ > - i = 0; > + /* Locate the end of the name / resolution, and parse it */ > + if (bpp_ptr && refresh_ptr) { > + mode_end = min(bpp_off, refresh_off); > + } else if (bpp_ptr) { > + mode_end = bpp_off; > + } else if (refresh_ptr) { > + mode_end = refresh_off; > + } else { > + mode_end = strlen(name); > + parse_extras = true; > } > -done: > - if (i >= 0) { > - pr_warn("[drm] parse error at position %i in video mode '%s'\n", > - i, name); > - mode->specified = false; > + > + ret = drm_mode_parse_cmdline_res_mode(name, mode_end, > + parse_extras, > + connector, > + mode); > + if (ret) > return false; > - } > + mode->specified = true; > > - if (res_specified) { > - mode->specified = true; > - mode->xres = xres; > - mode->yres = yres; > + if (bpp_ptr) { > + ret = drm_mode_parse_cmdline_bpp(bpp_ptr, &bpp_end_ptr, mode); > + if (ret) > + return false; > } > > - if (refresh_specified) { > - mode->refresh_specified = true; > - mode->refresh = refresh; > + if (refresh_ptr) { > + ret = drm_mode_parse_cmdline_refresh(refresh_ptr, > + &refresh_end_ptr, mode); > + if (ret) > + return false; > } > > - if (bpp_specified) { > - mode->bpp_specified = true; > - mode->bpp = bpp; > + /* > + * Locate the end of the bpp / refresh, and parse the extras > + * if relevant > + */ > + if (bpp_ptr && refresh_ptr) Perhaps I'm paranoid, but I think it'd be better to check bpp_end_ptr && refresh_end_ptr in this conditional. Sean > + extra_ptr = max(bpp_end_ptr, refresh_end_ptr); > + else if (bpp_ptr) > + extra_ptr = bpp_end_ptr; > + else if (refresh_ptr) > + extra_ptr = refresh_end_ptr; > + > + if (extra_ptr) { > + int remaining = strlen(name) - (extra_ptr - name); > + > + /* > + * We still have characters to process, while > + * we shouldn't have any > + */ > + if (remaining > 0) > + return false; > } > - mode->rb = rb; > - mode->cvt = cvt; > - mode->interlace = interlace; > - mode->margins = margins; > - mode->force = force; > > return true; > } > -- > git-series 0.8.10