[v9,09/25] drm/modes: Move named modes parsing to a separate function

Commit Message

Maxime Ripard Nov. 14, 2022, 1 p.m. UTC

The current construction of the named mode parsing doesn't allow to extend
it easily. Let's move it to a separate function so we can add more
parameters and modes.

In order for the tests to still pass, some extra checks are needed, so
it's not a 1:1 move.

Reviewed-by: Noralf Trønnes <[e-mail]>
Tested-by: Mateusz Kwiatkowski <[e-mail]>
Signed-off-by: Maxime Ripard <[e-mail]>

---
Changes in v7:
- Add Noralf Reviewed-by

Changes in v6:
- Simplify the test for connection status extras
- Simplify the code path to call drm_mode_parse_cmdline_named_mode

Changes in v4:
- Fold down all the named mode patches that were split into a single
  patch again to maintain bisectability
---
 drivers/gpu/drm/drm_modes.c | 70 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 58 insertions(+), 12 deletions(-)

Comments

Maxime Ripard Nov. 15, 2022, 9:13 a.m. UTC | #1

On Mon, 14 Nov 2022 14:00:28 +0100, Maxime Ripard wrote:
> The current construction of the named mode parsing doesn't allow to extend
> it easily. Let's move it to a separate function so we can add more
> parameters and modes.
> 
> In order for the tests to still pass, some extra checks are needed, so
> it's not a 1:1 move.
> 
> [...]

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime

Dave Stevenson Jan. 2, 2024, 3:12 p.m. UTC | #2

Hi Maxime

On Mon, 14 Nov 2022 at 13:00, Maxime Ripard <[e-mail]> wrote:
>
> The current construction of the named mode parsing doesn't allow to extend
> it easily. Let's move it to a separate function so we can add more
> parameters and modes.
>
> In order for the tests to still pass, some extra checks are needed, so
> it's not a 1:1 move.
>
> Reviewed-by: Noralf Trønnes <[e-mail]>
> Tested-by: Mateusz Kwiatkowski <[e-mail]>
> Signed-off-by: Maxime Ripard <[e-mail]>
>
> ---
> Changes in v7:
> - Add Noralf Reviewed-by
>
> Changes in v6:
> - Simplify the test for connection status extras
> - Simplify the code path to call drm_mode_parse_cmdline_named_mode
>
> Changes in v4:
> - Fold down all the named mode patches that were split into a single
>   patch again to maintain bisectability
> ---
>  drivers/gpu/drm/drm_modes.c | 70 +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 58 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 71c050c3ee6b..37542612912b 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -2229,6 +2229,51 @@ static const char * const drm_named_modes_whitelist[] = {
>         "PAL",
>  };
>
> +static int drm_mode_parse_cmdline_named_mode(const char *name,
> +                                            unsigned int name_end,
> +                                            struct drm_cmdline_mode *cmdline_mode)
> +{
> +       unsigned int i;
> +
> +       if (!name_end)
> +               return 0;
> +
> +       /* If the name starts with a digit, it's not a named mode */
> +       if (isdigit(name[0]))
> +               return 0;
> +
> +       /*
> +        * If there's an equal sign in the name, the command-line
> +        * contains only an option and no mode.
> +        */
> +       if (strnchr(name, name_end, '='))
> +               return 0;
> +
> +       /* The connection status extras can be set without a mode. */
> +       if (name_end == 1 &&
> +           (name[0] == 'd' || name[0] == 'D' || name[0] == 'e'))
> +               return 0;
> +
> +       /*
> +        * We're sure we're a named mode at this point, iterate over the
> +        * list of modes we're aware of.
> +        */
> +       for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
> +               int ret;
> +
> +               ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
> +               if (ret != name_end)
> +                       continue;
> +
> +               strcpy(cmdline_mode->name, drm_named_modes_whitelist[i]);
> +               cmdline_mode->specified = true;
> +
> +               return 1;
> +       }
> +
> +       return -EINVAL;
> +}
> +
>  /**
>   * drm_mode_parse_command_line_for_connector - parse command line modeline for connector
>   * @mode_option: optional per connector mode option
> @@ -2265,7 +2310,7 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>         const char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
>         const char *options_ptr = NULL;
>         char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
> -       int i, len, ret;
> +       int len, ret;
>
>         memset(mode, 0, sizeof(*mode));
>         mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> @@ -2306,18 +2351,19 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>                 parse_extras = true;
>         }
>
> -       /* First check for a named mode */
> -       for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
> -               ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
> -               if (ret == mode_end) {
> -                       if (refresh_ptr)
> -                               return false; /* named + refresh is invalid */
> +       if (!mode_end)
> +               return false;

I'm chasing down a change in behaviour between 6.1 and 6.6, and this
patch seems to be at least part of the cause.

Since [1] we've had the emulated framebuffer on Pi being 16bpp to save
memory. All good.

It used to be possible to use "video=HDMI-A-1:-32" on the kernel
command line to set it back to 32bpp.

After this patch that is no longer possible. "mode_end = bpp_off", and
"bpp_off = bpp_ptr - name", so with bpp_ptr = name we get mode_end
being 0. That fails this conditional.
drm_mode_parse_cmdline_named_mode already aborts early but with no
error if name_end / mode_end is 0, so this "if" clause seems
redundant, and is a change in behaviour.

We do then get a second parsing failure due to the check if (bpp_ptr
|| refresh_ptr) at [2].
Prior to this patch my video= line would get mode->specified set via
"if (ret == mode_end)" removed above, as ret = mode_end = 0. We
therefore didn't evaluate the conditional that now fails.

So I guess my question is whether my command line is valid or not, and
therefore is this a regression?
If considered invalid, then presumably there is no way to update the
bpp without also having to specify the resolution. That is rather
annoying as almost everything else can be updated without having to
set the resolution, so this one property would be the odd one out.

Thanks, and Happy New Year.
  Dave

[1] https://github.com/torvalds/linux/commit/f741b28fb299263d2d03a0fb701bfe648927cd47
[2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_modes.c#L2441
[3] https://github.com/torvalds/linux/commit/a631bf30eb914affc0a574f44576833477346ad6

>
> -                       strcpy(mode->name, drm_named_modes_whitelist[i]);
> -                       mode->specified = true;
> -                       break;
> -               }
> -       }
> +       ret = drm_mode_parse_cmdline_named_mode(name, mode_end, mode);
> +       if (ret < 0)
> +               return false;
> +
> +       /*
> +        * Having a mode that starts by a letter (and thus is named) and
> +        * an at-sign (used to specify a refresh rate) is disallowed.
> +        */
> +       if (ret && refresh_ptr)
> +               return false;
>
>         /* No named mode? Check for a normal mode argument, e.g. 1024x768 */
>         if (!mode->specified && isdigit(name[0])) {
>
> --
> b4 0.11.0-dev-99e3a

Maxime Ripard Jan. 3, 2024, 1:32 p.m. UTC | #3

Hi Dave,

Happy new year :)

On Tue, Jan 02, 2024 at 03:12:26PM +0000, Dave Stevenson wrote:
> Hi Maxime
> 
> On Mon, 14 Nov 2022 at 13:00, Maxime Ripard <[e-mail]> wrote:
> >
> > The current construction of the named mode parsing doesn't allow to extend
> > it easily. Let's move it to a separate function so we can add more
> > parameters and modes.
> >
> > In order for the tests to still pass, some extra checks are needed, so
> > it's not a 1:1 move.
> >
> > Reviewed-by: Noralf Trønnes <[e-mail]>
> > Tested-by: Mateusz Kwiatkowski <[e-mail]>
> > Signed-off-by: Maxime Ripard <[e-mail]>
> >
> > ---
> > Changes in v7:
> > - Add Noralf Reviewed-by
> >
> > Changes in v6:
> > - Simplify the test for connection status extras
> > - Simplify the code path to call drm_mode_parse_cmdline_named_mode
> >
> > Changes in v4:
> > - Fold down all the named mode patches that were split into a single
> >   patch again to maintain bisectability
> > ---
> >  drivers/gpu/drm/drm_modes.c | 70 +++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 58 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > index 71c050c3ee6b..37542612912b 100644
> > --- a/drivers/gpu/drm/drm_modes.c
> > +++ b/drivers/gpu/drm/drm_modes.c
> > @@ -2229,6 +2229,51 @@ static const char * const drm_named_modes_whitelist[] = {
> >         "PAL",
> >  };
> >
> > +static int drm_mode_parse_cmdline_named_mode(const char *name,
> > +                                            unsigned int name_end,
> > +                                            struct drm_cmdline_mode *cmdline_mode)
> > +{
> > +       unsigned int i;
> > +
> > +       if (!name_end)
> > +               return 0;
> > +
> > +       /* If the name starts with a digit, it's not a named mode */
> > +       if (isdigit(name[0]))
> > +               return 0;
> > +
> > +       /*
> > +        * If there's an equal sign in the name, the command-line
> > +        * contains only an option and no mode.
> > +        */
> > +       if (strnchr(name, name_end, '='))
> > +               return 0;
> > +
> > +       /* The connection status extras can be set without a mode. */
> > +       if (name_end == 1 &&
> > +           (name[0] == 'd' || name[0] == 'D' || name[0] == 'e'))
> > +               return 0;
> > +
> > +       /*
> > +        * We're sure we're a named mode at this point, iterate over the
> > +        * list of modes we're aware of.
> > +        */
> > +       for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
> > +               int ret;
> > +
> > +               ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
> > +               if (ret != name_end)
> > +                       continue;
> > +
> > +               strcpy(cmdline_mode->name, drm_named_modes_whitelist[i]);
> > +               cmdline_mode->specified = true;
> > +
> > +               return 1;
> > +       }
> > +
> > +       return -EINVAL;
> > +}
> > +
> >  /**
> >   * drm_mode_parse_command_line_for_connector - parse command line modeline for connector
> >   * @mode_option: optional per connector mode option
> > @@ -2265,7 +2310,7 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
> >         const char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
> >         const char *options_ptr = NULL;
> >         char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
> > -       int i, len, ret;
> > +       int len, ret;
> >
> >         memset(mode, 0, sizeof(*mode));
> >         mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> > @@ -2306,18 +2351,19 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
> >                 parse_extras = true;
> >         }
> >
> > -       /* First check for a named mode */
> > -       for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
> > -               ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
> > -               if (ret == mode_end) {
> > -                       if (refresh_ptr)
> > -                               return false; /* named + refresh is invalid */
> > +       if (!mode_end)
> > +               return false;
> 
> I'm chasing down a change in behaviour between 6.1 and 6.6, and this
> patch seems to be at least part of the cause.
> 
> Since [1] we've had the emulated framebuffer on Pi being 16bpp to save
> memory. All good.
> 
> It used to be possible to use "video=HDMI-A-1:-32" on the kernel
> command line to set it back to 32bpp.
> 
> After this patch that is no longer possible. "mode_end = bpp_off", and
> "bpp_off = bpp_ptr - name", so with bpp_ptr = name we get mode_end
> being 0. That fails this conditional.
> drm_mode_parse_cmdline_named_mode already aborts early but with no
> error if name_end / mode_end is 0, so this "if" clause seems
> redundant, and is a change in behaviour.
> 
> We do then get a second parsing failure due to the check if (bpp_ptr
> || refresh_ptr) at [2].
> Prior to this patch my video= line would get mode->specified set via
> "if (ret == mode_end)" removed above, as ret = mode_end = 0. We
> therefore didn't evaluate the conditional that now fails.
> 
> So I guess my question is whether my command line is valid or not, and
> therefore is this a regression?

It's a mess :)

Documentation/fb/modedb.rst defines the video parameter syntax as:

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

And thus mandates the x and y resolution. I guess that's what I use as a
base, and thus -bpp alone would be invalid.

But then it contradicts itself some time later by documenting that
video=HDMI-1:D is ok.

I guess what you experienced was just an oversight: it was not
documented anywhere that it was valid, so we didn't really tested it
either. We should add a unit test for it and fix it.

Maxime

Dave Stevenson Jan. 3, 2024, 2:02 p.m. UTC | #4

Hi Maxime

On Wed, 3 Jan 2024 at 13:33, Maxime Ripard <[e-mail]> wrote:
>
> Hi Dave,
>
> Happy new year :)

And to you.

> On Tue, Jan 02, 2024 at 03:12:26PM +0000, Dave Stevenson wrote:
> > Hi Maxime
> >
> > On Mon, 14 Nov 2022 at 13:00, Maxime Ripard <[e-mail]> wrote:
> > >
> > > The current construction of the named mode parsing doesn't allow to extend
> > > it easily. Let's move it to a separate function so we can add more
> > > parameters and modes.
> > >
> > > In order for the tests to still pass, some extra checks are needed, so
> > > it's not a 1:1 move.
> > >
> > > Reviewed-by: Noralf Trønnes <[e-mail]>
> > > Tested-by: Mateusz Kwiatkowski <[e-mail]>
> > > Signed-off-by: Maxime Ripard <[e-mail]>
> > >
> > > ---
> > > Changes in v7:
> > > - Add Noralf Reviewed-by
> > >
> > > Changes in v6:
> > > - Simplify the test for connection status extras
> > > - Simplify the code path to call drm_mode_parse_cmdline_named_mode
> > >
> > > Changes in v4:
> > > - Fold down all the named mode patches that were split into a single
> > >   patch again to maintain bisectability
> > > ---
> > >  drivers/gpu/drm/drm_modes.c | 70 +++++++++++++++++++++++++++++++++++++--------
> > >  1 file changed, 58 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > > index 71c050c3ee6b..37542612912b 100644
> > > --- a/drivers/gpu/drm/drm_modes.c
> > > +++ b/drivers/gpu/drm/drm_modes.c
> > > @@ -2229,6 +2229,51 @@ static const char * const drm_named_modes_whitelist[] = {
> > >         "PAL",
> > >  };
> > >
> > > +static int drm_mode_parse_cmdline_named_mode(const char *name,
> > > +                                            unsigned int name_end,
> > > +                                            struct drm_cmdline_mode *cmdline_mode)
> > > +{
> > > +       unsigned int i;
> > > +
> > > +       if (!name_end)
> > > +               return 0;
> > > +
> > > +       /* If the name starts with a digit, it's not a named mode */
> > > +       if (isdigit(name[0]))
> > > +               return 0;
> > > +
> > > +       /*
> > > +        * If there's an equal sign in the name, the command-line
> > > +        * contains only an option and no mode.
> > > +        */
> > > +       if (strnchr(name, name_end, '='))
> > > +               return 0;
> > > +
> > > +       /* The connection status extras can be set without a mode. */
> > > +       if (name_end == 1 &&
> > > +           (name[0] == 'd' || name[0] == 'D' || name[0] == 'e'))
> > > +               return 0;
> > > +
> > > +       /*
> > > +        * We're sure we're a named mode at this point, iterate over the
> > > +        * list of modes we're aware of.
> > > +        */
> > > +       for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
> > > +               int ret;
> > > +
> > > +               ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
> > > +               if (ret != name_end)
> > > +                       continue;
> > > +
> > > +               strcpy(cmdline_mode->name, drm_named_modes_whitelist[i]);
> > > +               cmdline_mode->specified = true;
> > > +
> > > +               return 1;
> > > +       }
> > > +
> > > +       return -EINVAL;
> > > +}
> > > +
> > >  /**
> > >   * drm_mode_parse_command_line_for_connector - parse command line modeline for connector
> > >   * @mode_option: optional per connector mode option
> > > @@ -2265,7 +2310,7 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
> > >         const char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
> > >         const char *options_ptr = NULL;
> > >         char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
> > > -       int i, len, ret;
> > > +       int len, ret;
> > >
> > >         memset(mode, 0, sizeof(*mode));
> > >         mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> > > @@ -2306,18 +2351,19 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
> > >                 parse_extras = true;
> > >         }
> > >
> > > -       /* First check for a named mode */
> > > -       for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
> > > -               ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
> > > -               if (ret == mode_end) {
> > > -                       if (refresh_ptr)
> > > -                               return false; /* named + refresh is invalid */
> > > +       if (!mode_end)
> > > +               return false;
> >
> > I'm chasing down a change in behaviour between 6.1 and 6.6, and this
> > patch seems to be at least part of the cause.
> >
> > Since [1] we've had the emulated framebuffer on Pi being 16bpp to save
> > memory. All good.
> >
> > It used to be possible to use "video=HDMI-A-1:-32" on the kernel
> > command line to set it back to 32bpp.
> >
> > After this patch that is no longer possible. "mode_end = bpp_off", and
> > "bpp_off = bpp_ptr - name", so with bpp_ptr = name we get mode_end
> > being 0. That fails this conditional.
> > drm_mode_parse_cmdline_named_mode already aborts early but with no
> > error if name_end / mode_end is 0, so this "if" clause seems
> > redundant, and is a change in behaviour.
> >
> > We do then get a second parsing failure due to the check if (bpp_ptr
> > || refresh_ptr) at [2].
> > Prior to this patch my video= line would get mode->specified set via
> > "if (ret == mode_end)" removed above, as ret = mode_end = 0. We
> > therefore didn't evaluate the conditional that now fails.
> >
> > So I guess my question is whether my command line is valid or not, and
> > therefore is this a regression?
>
> It's a mess :)
>
> Documentation/fb/modedb.rst defines the video parameter syntax as:
>
> <xres>x<yres>[M][R][-<bpp>][@<refresh>][i][m][eDd]
>
> And thus mandates the x and y resolution. I guess that's what I use as a
> base, and thus -bpp alone would be invalid.
>
> But then it contradicts itself some time later by documenting that
> video=HDMI-1:D is ok.
>
> I guess what you experienced was just an oversight: it was not
> documented anywhere that it was valid, so we didn't really tested it
> either. We should add a unit test for it and fix it.

Does dropping this "if (!mode_end)" check, and at least the check for
bpp_ptr in the "No mode?" block below it, seem reasonable to you?

I guess there is also the question of whether a refresh rate without a
mode is valid. That one seems less useful, and all uses of
refresh_specified appear to be after some form of checking xres and
yres.

I can put a couple of patches together to deal with this if you're
happy with the principle.

Thanks
  Dave

Dave Stevenson Jan. 3, 2024, 5:14 p.m. UTC | #5

On Wed, 3 Jan 2024 at 14:02, Dave Stevenson
<[e-mail]> wrote:
>
> Hi Maxime
>
> On Wed, 3 Jan 2024 at 13:33, Maxime Ripard <[e-mail]> wrote:
> >
> > Hi Dave,
> >
> > Happy new year :)
>
> And to you.
>
> > On Tue, Jan 02, 2024 at 03:12:26PM +0000, Dave Stevenson wrote:
> > > Hi Maxime
> > >
> > > On Mon, 14 Nov 2022 at 13:00, Maxime Ripard <[e-mail]> wrote:
> > > >
> > > > The current construction of the named mode parsing doesn't allow to extend
> > > > it easily. Let's move it to a separate function so we can add more
> > > > parameters and modes.
> > > >
> > > > In order for the tests to still pass, some extra checks are needed, so
> > > > it's not a 1:1 move.
> > > >
> > > > Reviewed-by: Noralf Trønnes <[e-mail]>
> > > > Tested-by: Mateusz Kwiatkowski <[e-mail]>
> > > > Signed-off-by: Maxime Ripard <[e-mail]>
> > > >
> > > > ---
> > > > Changes in v7:
> > > > - Add Noralf Reviewed-by
> > > >
> > > > Changes in v6:
> > > > - Simplify the test for connection status extras
> > > > - Simplify the code path to call drm_mode_parse_cmdline_named_mode
> > > >
> > > > Changes in v4:
> > > > - Fold down all the named mode patches that were split into a single
> > > >   patch again to maintain bisectability
> > > > ---
> > > >  drivers/gpu/drm/drm_modes.c | 70 +++++++++++++++++++++++++++++++++++++--------
> > > >  1 file changed, 58 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > > > index 71c050c3ee6b..37542612912b 100644
> > > > --- a/drivers/gpu/drm/drm_modes.c
> > > > +++ b/drivers/gpu/drm/drm_modes.c
> > > > @@ -2229,6 +2229,51 @@ static const char * const drm_named_modes_whitelist[] = {
> > > >         "PAL",
> > > >  };
> > > >
> > > > +static int drm_mode_parse_cmdline_named_mode(const char *name,
> > > > +                                            unsigned int name_end,
> > > > +                                            struct drm_cmdline_mode *cmdline_mode)
> > > > +{
> > > > +       unsigned int i;
> > > > +
> > > > +       if (!name_end)
> > > > +               return 0;
> > > > +
> > > > +       /* If the name starts with a digit, it's not a named mode */
> > > > +       if (isdigit(name[0]))
> > > > +               return 0;
> > > > +
> > > > +       /*
> > > > +        * If there's an equal sign in the name, the command-line
> > > > +        * contains only an option and no mode.
> > > > +        */
> > > > +       if (strnchr(name, name_end, '='))
> > > > +               return 0;
> > > > +
> > > > +       /* The connection status extras can be set without a mode. */
> > > > +       if (name_end == 1 &&
> > > > +           (name[0] == 'd' || name[0] == 'D' || name[0] == 'e'))
> > > > +               return 0;
> > > > +
> > > > +       /*
> > > > +        * We're sure we're a named mode at this point, iterate over the
> > > > +        * list of modes we're aware of.
> > > > +        */
> > > > +       for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
> > > > +               int ret;
> > > > +
> > > > +               ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
> > > > +               if (ret != name_end)
> > > > +                       continue;
> > > > +
> > > > +               strcpy(cmdline_mode->name, drm_named_modes_whitelist[i]);
> > > > +               cmdline_mode->specified = true;
> > > > +
> > > > +               return 1;
> > > > +       }
> > > > +
> > > > +       return -EINVAL;
> > > > +}
> > > > +
> > > >  /**
> > > >   * drm_mode_parse_command_line_for_connector - parse command line modeline for connector
> > > >   * @mode_option: optional per connector mode option
> > > > @@ -2265,7 +2310,7 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
> > > >         const char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
> > > >         const char *options_ptr = NULL;
> > > >         char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
> > > > -       int i, len, ret;
> > > > +       int len, ret;
> > > >
> > > >         memset(mode, 0, sizeof(*mode));
> > > >         mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> > > > @@ -2306,18 +2351,19 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
> > > >                 parse_extras = true;
> > > >         }
> > > >
> > > > -       /* First check for a named mode */
> > > > -       for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
> > > > -               ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
> > > > -               if (ret == mode_end) {
> > > > -                       if (refresh_ptr)
> > > > -                               return false; /* named + refresh is invalid */
> > > > +       if (!mode_end)
> > > > +               return false;
> > >
> > > I'm chasing down a change in behaviour between 6.1 and 6.6, and this
> > > patch seems to be at least part of the cause.
> > >
> > > Since [1] we've had the emulated framebuffer on Pi being 16bpp to save
> > > memory. All good.
> > >
> > > It used to be possible to use "video=HDMI-A-1:-32" on the kernel
> > > command line to set it back to 32bpp.
> > >
> > > After this patch that is no longer possible. "mode_end = bpp_off", and
> > > "bpp_off = bpp_ptr - name", so with bpp_ptr = name we get mode_end
> > > being 0. That fails this conditional.
> > > drm_mode_parse_cmdline_named_mode already aborts early but with no
> > > error if name_end / mode_end is 0, so this "if" clause seems
> > > redundant, and is a change in behaviour.
> > >
> > > We do then get a second parsing failure due to the check if (bpp_ptr
> > > || refresh_ptr) at [2].
> > > Prior to this patch my video= line would get mode->specified set via
> > > "if (ret == mode_end)" removed above, as ret = mode_end = 0. We
> > > therefore didn't evaluate the conditional that now fails.
> > >
> > > So I guess my question is whether my command line is valid or not, and
> > > therefore is this a regression?
> >
> > It's a mess :)
> >
> > Documentation/fb/modedb.rst defines the video parameter syntax as:
> >
> > <xres>x<yres>[M][R][-<bpp>][@<refresh>][i][m][eDd]
> >
> > And thus mandates the x and y resolution. I guess that's what I use as a
> > base, and thus -bpp alone would be invalid.
> >
> > But then it contradicts itself some time later by documenting that
> > video=HDMI-1:D is ok.
> >
> > I guess what you experienced was just an oversight: it was not
> > documented anywhere that it was valid, so we didn't really tested it
> > either. We should add a unit test for it and fix it.
>
> Does dropping this "if (!mode_end)" check, and at least the check for
> bpp_ptr in the "No mode?" block below it, seem reasonable to you?
>
> I guess there is also the question of whether a refresh rate without a
> mode is valid. That one seems less useful, and all uses of
> refresh_specified appear to be after some form of checking xres and
> yres.
>
> I can put a couple of patches together to deal with this if you're
> happy with the principle.

In looking at this in more detail and writing the unit test, it does
become a bit of a mess.

drm_connector_get_cmdline_mode calls
drm_mode_parse_command_line_for_connector, passing in
connector->cmdline_mode as the mode to fill.
drm_mode_parse_command_line_for_connector fills in bits of the mode as
it goes along, so a failure late in the function leaves bits from the
parsing in connector->cmdline_mode.

My video=HDMI-A-1:-32 does fail later on due to
drm_mode_parse_cmdline_options, but as it had set bpp and
bpp_specified directly in connector->cmdline_mode I got the behaviour
I was desiring.

It feels like this should be an all or nothing thing. Allocate a
struct drm_cmdline_mode on the stack, and only copy it into
connector->cmdline_mode if parsing succeeds. The downside is that you
take out some command line entries that used to work (like mine).
Thoughts?

  Dave

Patch

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 71c050c3ee6b..37542612912b 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -2229,6 +2229,51 @@  static const char * const drm_named_modes_whitelist[] = {
 	"PAL",
 };
 
+static int drm_mode_parse_cmdline_named_mode(const char *name,
+					     unsigned int name_end,
+					     struct drm_cmdline_mode *cmdline_mode)
+{
+	unsigned int i;
+
+	if (!name_end)
+		return 0;
+
+	/* If the name starts with a digit, it's not a named mode */
+	if (isdigit(name[0]))
+		return 0;
+
+	/*
+	 * If there's an equal sign in the name, the command-line
+	 * contains only an option and no mode.
+	 */
+	if (strnchr(name, name_end, '='))
+		return 0;
+
+	/* The connection status extras can be set without a mode. */
+	if (name_end == 1 &&
+	    (name[0] == 'd' || name[0] == 'D' || name[0] == 'e'))
+		return 0;
+
+	/*
+	 * We're sure we're a named mode at this point, iterate over the
+	 * list of modes we're aware of.
+	 */
+	for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
+		int ret;
+
+		ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
+		if (ret != name_end)
+			continue;
+
+		strcpy(cmdline_mode->name, drm_named_modes_whitelist[i]);
+		cmdline_mode->specified = true;
+
+		return 1;
+	}
+
+	return -EINVAL;
+}
+
 /**
  * drm_mode_parse_command_line_for_connector - parse command line modeline for connector
  * @mode_option: optional per connector mode option
@@ -2265,7 +2310,7 @@  bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 	const char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
 	const char *options_ptr = NULL;
 	char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
-	int i, len, ret;
+	int len, ret;
 
 	memset(mode, 0, sizeof(*mode));
 	mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
@@ -2306,18 +2351,19 @@  bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 		parse_extras = true;
 	}
 
-	/* First check for a named mode */
-	for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
-		ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
-		if (ret == mode_end) {
-			if (refresh_ptr)
-				return false; /* named + refresh is invalid */
+	if (!mode_end)
+		return false;
 
-			strcpy(mode->name, drm_named_modes_whitelist[i]);
-			mode->specified = true;
-			break;
-		}
-	}
+	ret = drm_mode_parse_cmdline_named_mode(name, mode_end, mode);
+	if (ret < 0)
+		return false;
+
+	/*
+	 * Having a mode that starts by a letter (and thus is named) and
+	 * an at-sign (used to specify a refresh rate) is disallowed.
+	 */
+	if (ret && refresh_ptr)
+		return false;
 
 	/* No named mode? Check for a normal mode argument, e.g. 1024x768 */
 	if (!mode->specified && isdigit(name[0])) {