Maxime Ripard
2024-Jan-03 13:32 UTC
[PATCH v9 09/25] drm/modes: Move named modes parsing to a separate function
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 <maxime at cerno.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 at tronnes.org> > > Tested-by: Mateusz Kwiatkowski <kfyatek+publicgit at gmail.com> > > Signed-off-by: Maxime Ripard <maxime at cerno.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?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 -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 228 bytes Desc: not available URL: <https://lists.freedesktop.org/archives/nouveau/attachments/20240103/17d192e6/attachment.sig>
Dave Stevenson
2024-Jan-03 14:02 UTC
[PATCH v9 09/25] drm/modes: Move named modes parsing to a separate function
Hi Maxime On Wed, 3 Jan 2024 at 13:33, Maxime Ripard <maxime at cerno.tech> 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 <maxime at cerno.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 at tronnes.org> > > > Tested-by: Mateusz Kwiatkowski <kfyatek+publicgit at gmail.com> > > > Signed-off-by: Maxime Ripard <maxime at cerno.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? > > 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