diff mbox series

[v5,05/12] drm/modes: Rewrite the command line parser

Message ID e32cd4009153b184103554009135c7bf7c9975d7.1560783090.git-series.maxime.ripard@bootlin.com
State Accepted
Commit e08ab74bd4c7a5fe311bc05f32dbb4f1e7fa3428
Headers show
Series None | expand

Commit Message

Maxime Ripard June 17, 2019, 2:51 p.m. UTC
From: Maxime Ripard <maxime.ripard@free-electrons.com>

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.

Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/gpu/drm/drm_modes.c | 325 +++++++++++++++++++++++--------------
 1 file changed, 210 insertions(+), 115 deletions(-)

Comments

Dmitry Osipenko July 5, 2019, 4:54 p.m. UTC | #1
17.06.2019 17:51, Maxime Ripard пишет:
> From: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> 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.
> 
> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/gpu/drm/drm_modes.c | 325 +++++++++++++++++++++++--------------
>  1 file changed, 210 insertions(+), 115 deletions(-)

Hello,

I have a Tegra device that uses a stock android bootloader which passes "video=tegrafb" in
the kernels cmdline. That wasn't a problem before this patch, but now Tegra DRM driver
fails to probe because the mode is 0x0:0 and hence framebuffer allocation fails. Is it a
legit regression that should be fixed in upstream?
Maxime Ripard July 9, 2019, 12:45 p.m. UTC | #2
Hi,

On Fri, Jul 05, 2019 at 07:54:47PM +0300, Dmitry Osipenko wrote:
> 17.06.2019 17:51, Maxime Ripard пишет:

> > From: Maxime Ripard <maxime.ripard@free-electrons.com>

> >

> > 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.

> >

> > Reviewed-by: Noralf Trønnes <noralf@tronnes.org>

> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

> > ---

> >  drivers/gpu/drm/drm_modes.c | 325 +++++++++++++++++++++++--------------

> >  1 file changed, 210 insertions(+), 115 deletions(-)

>

> I have a Tegra device that uses a stock android bootloader which

> passes "video=tegrafb" in the kernels cmdline. That wasn't a problem

> before this patch, but now Tegra DRM driver fails to probe because

> the mode is 0x0:0 and hence framebuffer allocation fails. Is it a

> legit regression that should be fixed in upstream?


Thierry indeed reported that issue, but the discussion pretty much
stalled since then.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Dmitry Osipenko July 9, 2019, 12:52 p.m. UTC | #3
09.07.2019 15:45, Maxime Ripard пишет:
> Hi,
> 
> On Fri, Jul 05, 2019 at 07:54:47PM +0300, Dmitry Osipenko wrote:
>> 17.06.2019 17:51, Maxime Ripard пишет:
>>> From: Maxime Ripard <maxime.ripard@free-electrons.com>
>>>
>>> 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.
>>>
>>> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>>> ---
>>>  drivers/gpu/drm/drm_modes.c | 325 +++++++++++++++++++++++--------------
>>>  1 file changed, 210 insertions(+), 115 deletions(-)
>>
>> I have a Tegra device that uses a stock android bootloader which
>> passes "video=tegrafb" in the kernels cmdline. That wasn't a problem
>> before this patch, but now Tegra DRM driver fails to probe because
>> the mode is 0x0:0 and hence framebuffer allocation fails. Is it a
>> legit regression that should be fixed in upstream?
> 
> Thierry indeed reported that issue, but the discussion pretty much
> stalled since then.

Sorry, this doesn't answer my question. Where it was reported?

If it's a valid regression (my device is broken), then the patch should either be fixed or
reverted.
Jon Hunter July 9, 2019, 1:26 p.m. UTC | #4
On 09/07/2019 13:52, Dmitry Osipenko wrote:
> 09.07.2019 15:45, Maxime Ripard пишет:
>> Hi,
>>
>> On Fri, Jul 05, 2019 at 07:54:47PM +0300, Dmitry Osipenko wrote:
>>> 17.06.2019 17:51, Maxime Ripard пишет:
>>>> From: Maxime Ripard <maxime.ripard@free-electrons.com>
>>>>
>>>> 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.
>>>>
>>>> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
>>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_modes.c | 325 +++++++++++++++++++++++--------------
>>>>  1 file changed, 210 insertions(+), 115 deletions(-)
>>>
>>> I have a Tegra device that uses a stock android bootloader which
>>> passes "video=tegrafb" in the kernels cmdline. That wasn't a problem
>>> before this patch, but now Tegra DRM driver fails to probe because
>>> the mode is 0x0:0 and hence framebuffer allocation fails. Is it a
>>> legit regression that should be fixed in upstream?
>>
>> Thierry indeed reported that issue, but the discussion pretty much
>> stalled since then.

Yes Thierry is currently out and hence has not responded. I had been
planning to look at this this week and responded.

> Sorry, this doesn't answer my question. Where it was reported?

Same thread [0]. Dmitry, are you able to respond to Maxime's response to
Thierry?

Cheers
Jon

[0] https://patchwork.kernel.org/patch/10999393/
Jon Hunter July 9, 2019, 1:28 p.m. UTC | #5
On 09/07/2019 14:26, Jon Hunter wrote:
> 
> On 09/07/2019 13:52, Dmitry Osipenko wrote:
>> 09.07.2019 15:45, Maxime Ripard пишет:
>>> Hi,
>>>
>>> On Fri, Jul 05, 2019 at 07:54:47PM +0300, Dmitry Osipenko wrote:
>>>> 17.06.2019 17:51, Maxime Ripard пишет:
>>>>> From: Maxime Ripard <maxime.ripard@free-electrons.com>
>>>>>
>>>>> 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.
>>>>>
>>>>> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
>>>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>>>>> ---
>>>>>  drivers/gpu/drm/drm_modes.c | 325 +++++++++++++++++++++++--------------
>>>>>  1 file changed, 210 insertions(+), 115 deletions(-)
>>>>
>>>> I have a Tegra device that uses a stock android bootloader which
>>>> passes "video=tegrafb" in the kernels cmdline. That wasn't a problem
>>>> before this patch, but now Tegra DRM driver fails to probe because
>>>> the mode is 0x0:0 and hence framebuffer allocation fails. Is it a
>>>> legit regression that should be fixed in upstream?
>>>
>>> Thierry indeed reported that issue, but the discussion pretty much
>>> stalled since then.
> 
> Yes Thierry is currently out and hence has not responded. I had been
> planning to look at this this week and responded.
> 
>> Sorry, this doesn't answer my question. Where it was reported?
> 
> Same thread [0].

Correction, it was on patch 6/12 and not this one.

Jon
Dmitry Osipenko July 9, 2019, 1:40 p.m. UTC | #6
09.07.2019 16:28, Jon Hunter пишет:
> 
> On 09/07/2019 14:26, Jon Hunter wrote:
>>
>> On 09/07/2019 13:52, Dmitry Osipenko wrote:
>>> 09.07.2019 15:45, Maxime Ripard пишет:
>>>> Hi,
>>>>
>>>> On Fri, Jul 05, 2019 at 07:54:47PM +0300, Dmitry Osipenko wrote:
>>>>> 17.06.2019 17:51, Maxime Ripard пишет:
>>>>>> From: Maxime Ripard <maxime.ripard@free-electrons.com>
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
>>>>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/drm_modes.c | 325 +++++++++++++++++++++++--------------
>>>>>>  1 file changed, 210 insertions(+), 115 deletions(-)
>>>>>
>>>>> I have a Tegra device that uses a stock android bootloader which
>>>>> passes "video=tegrafb" in the kernels cmdline. That wasn't a problem
>>>>> before this patch, but now Tegra DRM driver fails to probe because
>>>>> the mode is 0x0:0 and hence framebuffer allocation fails. Is it a
>>>>> legit regression that should be fixed in upstream?
>>>>
>>>> Thierry indeed reported that issue, but the discussion pretty much
>>>> stalled since then.
>>
>> Yes Thierry is currently out and hence has not responded. I had been
>> planning to look at this this week and responded.
>>
>>> Sorry, this doesn't answer my question. Where it was reported?
>>
>> Same thread [0].
> 
> Correction, it was on patch 6/12 and not this one.

Thank you very much, Jon! So looks like this patch is simply broken because it silently
copies the "mode's name" and then drm_connector_get_cmdline_mode() doesn't even try to
validate the mode.
Jernej Škrabec Aug. 19, 2019, 6:54 p.m. UTC | #7
+CC: Thomas Graichen

Dne ponedeljek, 17. junij 2019 ob 16:51:32 CEST je Maxime Ripard napisal(a):
> From: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> 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.
> 
> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Thomas reported to me that this patch breaks "video=CONNECTOR:e" kernel 
parameter which he currently uses as a workaround for H6 HDMI monitor 
detection issue on one STB.

I suppose this is the same issue that Dmitry noticed.

Thomas Graichen (in CC) can provide more information if needed.

Best regards,
Jernej
Thomas Graichen Aug. 19, 2019, 7:20 p.m. UTC | #8
On Mon, Aug 19, 2019 at 8:54 PM Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
>
> +CC: Thomas Graichen
>
> Dne ponedeljek, 17. junij 2019 ob 16:51:32 CEST je Maxime Ripard napisal(a):
> > From: Maxime Ripard <maxime.ripard@free-electrons.com>
> >
> > 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.
> >
> > Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>
> Thomas reported to me that this patch breaks "video=CONNECTOR:e" kernel
> parameter which he currently uses as a workaround for H6 HDMI monitor
> detection issue on one STB.
>
> I suppose this is the same issue that Dmitry noticed.
>
> Thomas Graichen (in CC) can provide more information if needed.
>
> Best regards,
> Jernej

as jernej already mentioned i am currently having to use the kernel
cmdline option video=HDMI-A-1:e to get a working hdmi output on an
eachlink h6 mini tv box and was wondering that i did not get any hdmi
output even with this option when switching from the
https://github.com/megous/linux oprange-pi-5.2 to the orange-pi-5.3
branch which seems to contain this patch. as i had no idea what might
have caused the breakage of the hdmi output and did a full bisect of
the kernel between those two versions, which ended reliably at exactly
this patch - so i guess there is a regression at least with the
video=CONNECTOR:e option (maybe others too?) with this patches code
which makes it not working anymore.

best wishes - thomas
Maxime Ripard Aug. 20, 2019, 3 p.m. UTC | #9
Hi,

On Mon, Aug 19, 2019 at 09:20:00PM +0200, Thomas Graichen wrote:
> On Mon, Aug 19, 2019 at 8:54 PM Jernej Škrabec <jernej.skrabec@gmail.com> wrote:

> >

> > +CC: Thomas Graichen

> >

> > Dne ponedeljek, 17. junij 2019 ob 16:51:32 CEST je Maxime Ripard napisal(a):

> > > From: Maxime Ripard <maxime.ripard@free-electrons.com>

> > >

> > > 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.

> > >

> > > Reviewed-by: Noralf Trønnes <noralf@tronnes.org>

> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

> >

> > Thomas reported to me that this patch breaks "video=CONNECTOR:e" kernel

> > parameter which he currently uses as a workaround for H6 HDMI monitor

> > detection issue on one STB.

> >

> > I suppose this is the same issue that Dmitry noticed.

> >

> > Thomas Graichen (in CC) can provide more information if needed.

>

> as jernej already mentioned i am currently having to use the kernel

> cmdline option video=HDMI-A-1:e to get a working hdmi output on an

> eachlink h6 mini tv box and was wondering that i did not get any hdmi

> output even with this option when switching from the

> https://github.com/megous/linux oprange-pi-5.2 to the orange-pi-5.3

> branch which seems to contain this patch.


Which kernel version is that based on?

> as i had no idea what might have caused the breakage of the hdmi

> output and did a full bisect of the kernel between those two

> versions, which ended reliably at exactly this patch - so i guess

> there is a regression at least with the video=CONNECTOR:e option

> (maybe others too?) with this patches code which makes it not

> working anymore.


I'm not sure I'll have the time to look into it this week (or the
next, unfortunately). However, the e parameter is supposed to be
parsed by drm_mode_parse_cmdline_extra, which in turn is supposed to
be called there:
https://elixir.bootlin.com/linux/v5.3-rc5/source/drivers/gpu/drm/drm_modes.c#L1810

If you can test that, having an idea of if that function is called,
which return code it returns, and if it isn't if why would be super
helpful.

Thanks!
Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Thomas Graichen Aug. 23, 2019, 2:04 p.m. UTC | #10
hi maxim,

On Tue, Aug 20, 2019 at 5:00 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> Hi,
>
> On Mon, Aug 19, 2019 at 09:20:00PM +0200, Thomas Graichen wrote:
> > On Mon, Aug 19, 2019 at 8:54 PM Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
> > >
> > > +CC: Thomas Graichen
> > >
> > > Dne ponedeljek, 17. junij 2019 ob 16:51:32 CEST je Maxime Ripard napisal(a):
> > > > From: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > >
> > > > 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.
> > > >
> > > > Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > >
> > > Thomas reported to me that this patch breaks "video=CONNECTOR:e" kernel
> > > parameter which he currently uses as a workaround for H6 HDMI monitor
> > > detection issue on one STB.
> > >
> > > I suppose this is the same issue that Dmitry noticed.
> > >
> > > Thomas Graichen (in CC) can provide more information if needed.
> >
> > as jernej already mentioned i am currently having to use the kernel
> > cmdline option video=HDMI-A-1:e to get a working hdmi output on an
> > eachlink h6 mini tv box and was wondering that i did not get any hdmi
> > output even with this option when switching from the
> > https://github.com/megous/linux oprange-pi-5.2 to the orange-pi-5.3
> > branch which seems to contain this patch.
>
> Which kernel version is that based on?

5.3-rc3

> > as i had no idea what might have caused the breakage of the hdmi
> > output and did a full bisect of the kernel between those two
> > versions, which ended reliably at exactly this patch - so i guess
> > there is a regression at least with the video=CONNECTOR:e option
> > (maybe others too?) with this patches code which makes it not
> > working anymore.
>
> I'm not sure I'll have the time to look into it this week (or the
> next, unfortunately). However, the e parameter is supposed to be
> parsed by drm_mode_parse_cmdline_extra, which in turn is supposed to
> be called there:
> https://elixir.bootlin.com/linux/v5.3-rc5/source/drivers/gpu/drm/drm_modes.c#L1810
>
> If you can test that, having an idea of if that function is called,
> which return code it returns, and if it isn't if why would be super
> helpful.

i just added a printk and it looks like it is not getting called:

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index b0369e690f36..4c58fdb1d7be 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1813,6 +1813,7 @@ bool
drm_mode_parse_command_line_for_connector(const char *mode_option,

                ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
                                                   connector, mode);
+               printk(KERN_WARNING "DEBUG -
drm_mode_parse_cmdline_extra %d", ret);
                if (ret)
                        return false;
        }

no output from it in dmesg (my loglevel=8 and on the kernel cmdline
and in /proc/cmdline i have "video=HDMI-A-1:e") - so looks like it
really gets lost somewhere along the way ...

best wishes - thomas
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 5a07a28fec6d..6debbd6c1763 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 <linux/ctype.h>
 #include <linux/list.h>
 #include <linux/list_sort.h>
 #include <linux/export.h>
@@ -1408,6 +1409,151 @@  void drm_connector_list_update(struct drm_connector *connector)
 }
 EXPORT_SYMBOL(drm_connector_list_update);
 
+static int drm_mode_parse_cmdline_bpp(const char *str, char **end_ptr,
+				      struct drm_cmdline_mode *mode)
+{
+	unsigned int bpp;
+
+	if (str[0] != '-')
+		return -EINVAL;
+
+	str++;
+	bpp = simple_strtol(str, end_ptr, 10);
+	if (*end_ptr == str)
+		return -EINVAL;
+
+	mode->bpp = bpp;
+	mode->bpp_specified = true;
+
+	return 0;
+}
+
+static int drm_mode_parse_cmdline_refresh(const char *str, char **end_ptr,
+					  struct drm_cmdline_mode *mode)
+{
+	unsigned int refresh;
+
+	if (str[0] != '@')
+		return -EINVAL;
+
+	str++;
+	refresh = simple_strtol(str, end_ptr, 10);
+	if (*end_ptr == str)
+		return -EINVAL;
+
+	mode->refresh = refresh;
+	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)
+{
+	const char *str_start = str;
+	bool rb = false, cvt = false;
+	int xres = 0, yres = 0;
+	int remaining, i;
+	char *end_ptr;
+
+	xres = simple_strtol(str, &end_ptr, 10);
+	if (end_ptr == str)
+		return -EINVAL;
+
+	if (end_ptr[0] != 'x')
+		return -EINVAL;
+	end_ptr++;
+
+	str = end_ptr;
+	yres = simple_strtol(str, &end_ptr, 10);
+	if (end_ptr == str)
+		return -EINVAL;
+
+	remaining = length - (end_ptr - str_start);
+	if (remaining < 0)
+		return -EINVAL;
+
+	for (i = 0; i < remaining; i++) {
+		switch (end_ptr[i]) {
+		case 'M':
+			cvt = true;
+			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
@@ -1434,13 +1580,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)
@@ -1453,127 +1598,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)
+		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;
 }