Date: Tue, 2 Jan 2024 15:12:26 +0000
From: Dave Stevenson <dave.stevenson@...pberrypi.com>
To: Maxime Ripard <maxime@...no.tech>, Maxime Ripard <mripard@...nel.org>
Cc: Karol Herbst <kherbst@...hat.com>, Samuel Holland <samuel@...lland.org>, 
	Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>, Daniel Vetter <daniel@...ll.ch>, 
	Chen-Yu Tsai <wens@...e.org>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Lyude Paul <lyude@...hat.com>, Thomas Zimmermann <tzimmermann@...e.de>, 
	Jani Nikula <jani.nikula@...ux.intel.com>, Rodrigo Vivi <rodrigo.vivi@...el.com>, 
	Ben Skeggs <bskeggs@...hat.com>, Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>, 
	David Airlie <airlied@...ux.ie>, Jernej Skrabec <jernej.skrabec@...il.com>, 
	Dom Cobley <dom@...pberrypi.com>, Phil Elwell <phil@...pberrypi.com>, 
	Mateusz Kwiatkowski <kfyatek+publicgit@...il.com>, nouveau@...ts.freedesktop.org, 
	Hans de Goede <hdegoede@...hat.com>, intel-gfx@...ts.freedesktop.org, 
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, Noralf Trønnes <noralf@...nnes.org>, 
	Geert Uytterhoeven <geert@...ux-m68k.org>, linux-sunxi@...ts.linux.dev
Subject: Re: [PATCH v9 09/25] drm/modes: Move named modes parsing to a
 separate function

Hi Maxime

On Mon, 14 Nov 2022 at 13:00, Maxime Ripard <maxime@...no.tech> 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 <noralf@...nnes.org>
> Tested-by: Mateusz Kwiatkowski <kfyatek+publicgit@...il.com>
> Signed-off-by: Maxime Ripard <maxime@...no.tech>
>
> ---
> 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

Powered by blists - more mailing lists