[PATCH 1/4] drm/modes: Add a switch to differentiate free standing options

[PATCH 1/4] drm/modes: Add a switch to differentiate free standing options

[Maxime Ripard]

From: Maxime Ripard <maxime.ripard@bootlin.com>

Some extra command line options can be either specified without anything
else on the command line (basically all the force connection options), but
some other are only relevant when matched with a resolution (margin and
interlace).

Let's add a switch to restrict if needed the available option set.

Fixes: e08ab74bd4c7 ("drm/modes: Rewrite the command line parser")
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/drm_modes.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 11acc9581977..e5997f35b779 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1454,6 +1454,7 @@ static int drm_mode_parse_cmdline_refresh(const char *str, char **end_ptr,
 }
 
 static int drm_mode_parse_cmdline_extra(const char *str, int length,
+					bool freestanding,
 					const struct drm_connector *connector,
 					struct drm_cmdline_mode *mode)
 {
@@ -1462,9 +1463,15 @@ static int drm_mode_parse_cmdline_extra(const char *str, int length,
 	for (i = 0; i < length; i++) {
 		switch (str[i]) {
 		case 'i':
+			if (freestanding)
+				return -EINVAL;
+
 			mode->interlace = true;
 			break;
 		case 'm':
+			if (freestanding)
+				return -EINVAL;
+
 			mode->margins = true;
 			break;
 		case 'D':
@@ -1542,6 +1549,7 @@ static int drm_mode_parse_cmdline_res_mode(const char *str, unsigned int length,
 			if (extras) {
 				int ret = drm_mode_parse_cmdline_extra(end_ptr + i,
 								       1,
+								       false,
 								       connector,
 								       mode);
 				if (ret)
@@ -1811,7 +1819,7 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 	    extra_ptr != options_ptr) {
 		int len = strlen(name) - (extra_ptr - name);
 
-		ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
+		ret = drm_mode_parse_cmdline_extra(extra_ptr, len, false,
 						   connector, mode);
 		if (ret)
 			return false;
-- 
2.21.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] drm/modes: Fix the command line parser to take force options into account

[Maxime Ripard]

From: Maxime Ripard <maxime.ripard@bootlin.com>

The command line parser when it has been rewritten introduced a regression
when the only thing on the command line is an option to force the detection
of a connector (such as video=HDMI-A-1:d), which are completely valid.

It's been further broken by the support for the named modes which take
anything that is not a resolution as a named mode.

Let's fix this by running the extra command line option parser on the named
modes if they only take a single character.

Fixes: e08ab74bd4c7 ("drm/modes: Rewrite the command line parser")
Reported-by: Jernej Škrabec <jernej.skrabec@gmail.com>
Reported-by: Thomas Graichen <thomas.graichen@googlemail.com>
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/drm_modes.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index e5997f35b779..ea7e6c8c8318 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1733,16 +1733,30 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 	 * bunch of things:
 	 *   - We need to make sure that the first character (which
 	 *     would be our resolution in X) is a digit.
-	 *   - However, if the X resolution is missing, then we end up
-	 *     with something like x<yres>, with our first character
-	 *     being an alpha-numerical character, which would be
-	 *     considered a named mode.
+	 *   - If not, then it's either a named mode or a force on/off.
+	 *     To distinguish between the two, we need to run the
+	 *     extra parsing function, and if not, then we consider it
+	 *     a named mode.
 	 *
 	 * If this isn't enough, we should add more heuristics here,
 	 * and matching unit-tests.
 	 */
-	if (!isdigit(name[0]) && name[0] != 'x')
+	if (!isdigit(name[0]) && name[0] != 'x') {
+		unsigned int namelen = strlen(name);
+
+		/*
+		 * Only the force on/off options can be in that case,
+		 * and they all take a single character.
+		 */
+		if (namelen == 1) {
+			ret = drm_mode_parse_cmdline_extra(name, namelen, true,
+							   connector, mode);
+			if (!ret)
+				return true;
+		}
+
 		named_mode = true;
+	}
 
 	/* Try to locate the bpp and refresh specifiers, if any */
 	bpp_ptr = strchr(name, '-');
-- 
2.21.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/4] drm/modes: Introduce a whitelist for the named modes

[Maxime Ripard]

From: Maxime Ripard <maxime.ripard@bootlin.com>

The named modes support has introduced a number of glitches that were in
part due to the fact that the parser will take any string as a named mode.

Since we shouldn't have a lot of options there (and they should be pretty
standard), let's introduce a whitelist of the available named modes so that
the kernel can differentiate between a poorly formed command line and a
named mode.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/drm_modes.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index ea7e6c8c8318..988797d8080a 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1677,6 +1677,22 @@ static int drm_mode_parse_cmdline_options(char *str, size_t len,
 	return 0;
 }
 
+const char *drm_named_modes_whitelist[] = {
+	"NTSC",
+	"PAL",
+};
+
+static bool drm_named_mode_is_in_whitelist(const char *mode, unsigned int size)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++)
+		if (!strncmp(mode, drm_named_modes_whitelist[i], size))
+			return true;
+
+	return false;
+}
+
 /**
  * drm_mode_parse_command_line_for_connector - parse command line modeline for connector
  * @mode_option: optional per connector mode option
@@ -1794,6 +1810,10 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 	if (named_mode) {
 		if (mode_end + 1 > DRM_DISPLAY_MODE_LEN)
 			return false;
+
+		if (!drm_named_mode_is_in_whitelist(name, mode_end))
+			return false;
+
 		strscpy(mode->name, name, mode_end + 1);
 	} else {
 		ret = drm_mode_parse_cmdline_res_mode(name, mode_end,
-- 
2.21.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/4] drm/selftests: modes: Add more unit tests for the cmdline parser

[Maxime Ripard]

From: Maxime Ripard <maxime.ripard@bootlin.com>

Let's add some unit tests for the recent bugs we just fixed.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 .../gpu/drm/selftests/drm_cmdline_selftests.h |   7 +
 .../drm/selftests/test-drm_cmdline_parser.c   | 130 ++++++++++++++++++
 2 files changed, 137 insertions(+)

diff --git a/drivers/gpu/drm/selftests/drm_cmdline_selftests.h b/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
index b45824ec7c8f..6d61a0eb5d64 100644
--- a/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
+++ b/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
@@ -9,6 +9,13 @@
 
 #define cmdline_test(test)	selftest(test, test)
 
+cmdline_test(drm_cmdline_test_force_d_only)
+cmdline_test(drm_cmdline_test_force_D_only_dvi)
+cmdline_test(drm_cmdline_test_force_D_only_hdmi)
+cmdline_test(drm_cmdline_test_force_D_only_not_digital)
+cmdline_test(drm_cmdline_test_force_e_only)
+cmdline_test(drm_cmdline_test_margin_only)
+cmdline_test(drm_cmdline_test_interlace_only)
 cmdline_test(drm_cmdline_test_res)
 cmdline_test(drm_cmdline_test_res_missing_x)
 cmdline_test(drm_cmdline_test_res_missing_y)
diff --git a/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c b/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c
index 14c96edb13df..013de9d27c35 100644
--- a/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c
+++ b/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c
@@ -17,6 +17,136 @@
 
 static const struct drm_connector no_connector = {};
 
+static int drm_cmdline_test_force_e_only(void *ignored)
+{
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("e",
+							   &no_connector,
+							   &mode));
+	FAIL_ON(mode.specified);
+	FAIL_ON(mode.refresh_specified);
+	FAIL_ON(mode.bpp_specified);
+
+	FAIL_ON(mode.rb);
+	FAIL_ON(mode.cvt);
+	FAIL_ON(mode.interlace);
+	FAIL_ON(mode.margins);
+	FAIL_ON(mode.force != DRM_FORCE_ON);
+
+	return 0;
+}
+
+static int drm_cmdline_test_force_D_only_not_digital(void *ignored)
+{
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("D",
+							   &no_connector,
+							   &mode));
+	FAIL_ON(mode.specified);
+	FAIL_ON(mode.refresh_specified);
+	FAIL_ON(mode.bpp_specified);
+
+	FAIL_ON(mode.rb);
+	FAIL_ON(mode.cvt);
+	FAIL_ON(mode.interlace);
+	FAIL_ON(mode.margins);
+	FAIL_ON(mode.force != DRM_FORCE_ON);
+
+	return 0;
+}
+
+static const struct drm_connector connector_hdmi = {
+	.connector_type	= DRM_MODE_CONNECTOR_HDMIB,
+};
+
+static int drm_cmdline_test_force_D_only_hdmi(void *ignored)
+{
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("D",
+							   &connector_hdmi,
+							   &mode));
+	FAIL_ON(mode.specified);
+	FAIL_ON(mode.refresh_specified);
+	FAIL_ON(mode.bpp_specified);
+
+	FAIL_ON(mode.rb);
+	FAIL_ON(mode.cvt);
+	FAIL_ON(mode.interlace);
+	FAIL_ON(mode.margins);
+	FAIL_ON(mode.force != DRM_FORCE_ON_DIGITAL);
+
+	return 0;
+}
+
+static const struct drm_connector connector_dvi = {
+	.connector_type	= DRM_MODE_CONNECTOR_DVII,
+};
+
+static int drm_cmdline_test_force_D_only_dvi(void *ignored)
+{
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("D",
+							   &connector_dvi,
+							   &mode));
+	FAIL_ON(mode.specified);
+	FAIL_ON(mode.refresh_specified);
+	FAIL_ON(mode.bpp_specified);
+
+	FAIL_ON(mode.rb);
+	FAIL_ON(mode.cvt);
+	FAIL_ON(mode.interlace);
+	FAIL_ON(mode.margins);
+	FAIL_ON(mode.force != DRM_FORCE_ON_DIGITAL);
+
+	return 0;
+}
+
+static int drm_cmdline_test_force_d_only(void *ignored)
+{
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("d",
+							   &no_connector,
+							   &mode));
+	FAIL_ON(mode.specified);
+	FAIL_ON(mode.refresh_specified);
+	FAIL_ON(mode.bpp_specified);
+
+	FAIL_ON(mode.rb);
+	FAIL_ON(mode.cvt);
+	FAIL_ON(mode.interlace);
+	FAIL_ON(mode.margins);
+	FAIL_ON(mode.force != DRM_FORCE_OFF);
+
+	return 0;
+}
+
+static int drm_cmdline_test_margin_only(void *ignored)
+{
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(drm_mode_parse_command_line_for_connector("m",
+							  &no_connector,
+							  &mode));
+
+	return 0;
+}
+
+static int drm_cmdline_test_interlace_only(void *ignored)
+{
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(drm_mode_parse_command_line_for_connector("i",
+							  &no_connector,
+							  &mode));
+
+	return 0;
+}
+
 static int drm_cmdline_test_res(void *ignored)
 {
 	struct drm_cmdline_mode mode = { };
-- 
2.21.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/modes: Add a switch to differentiate free standing options

[Thomas Graichen]

hi maxime,

On Tue, Aug 27, 2019 at 1:58 PM Maxime Ripard <mripard@kernel.org> wrote:
>
> From: Maxime Ripard <maxime.ripard@bootlin.com>
>
> Some extra command line options can be either specified without anything
> else on the command line (basically all the force connection options), but
> some other are only relevant when matched with a resolution (margin and
> interlace).
>
> Let's add a switch to restrict if needed the available option set.
>
> Fixes: e08ab74bd4c7 ("drm/modes: Rewrite the command line parser")
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Tested-by: thomas graichen <thomas.graichen@gmail.com>

BEFORE (only "[PATCH v5 05/12] drm/modes: Rewrite the command line
parser " applied):
my eachlink h6 mini tv box (which requires the video=HDMI-A-1:e
cmdline option to give any output on hdmi) did not show any hdmi
output any more (in 5.2 it still worked)

AFTER (the above patch plus this patch set here applied):
my eachlink h6 mini tv box gives output on hdmi again. i also did
check it the other way around: if i remove the video=HDMI-A-1:e option
i no longer get any hdmi output as expected. as a result this patch
series fixes the problem/regression for me.

best wishes - thomas

> ---
>  drivers/gpu/drm/drm_modes.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 11acc9581977..e5997f35b779 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1454,6 +1454,7 @@ static int drm_mode_parse_cmdline_refresh(const char *str, char **end_ptr,
>  }
>
>  static int drm_mode_parse_cmdline_extra(const char *str, int length,
> +                                       bool freestanding,
>                                         const struct drm_connector *connector,
>                                         struct drm_cmdline_mode *mode)
>  {
> @@ -1462,9 +1463,15 @@ static int drm_mode_parse_cmdline_extra(const char *str, int length,
>         for (i = 0; i < length; i++) {
>                 switch (str[i]) {
>                 case 'i':
> +                       if (freestanding)
> +                               return -EINVAL;
> +
>                         mode->interlace = true;
>                         break;
>                 case 'm':
> +                       if (freestanding)
> +                               return -EINVAL;
> +
>                         mode->margins = true;
>                         break;
>                 case 'D':
> @@ -1542,6 +1549,7 @@ static int drm_mode_parse_cmdline_res_mode(const char *str, unsigned int length,
>                         if (extras) {
>                                 int ret = drm_mode_parse_cmdline_extra(end_ptr + i,
>                                                                        1,
> +                                                                      false,
>                                                                        connector,
>                                                                        mode);
>                                 if (ret)
> @@ -1811,7 +1819,7 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>             extra_ptr != options_ptr) {
>                 int len = strlen(name) - (extra_ptr - name);
>
> -               ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
> +               ret = drm_mode_parse_cmdline_extra(extra_ptr, len, false,
>                                                    connector, mode);
>                 if (ret)
>                         return false;
> --
> 2.21.0
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/modes: Fix the command line parser to take force options into account

[Thomas Graichen]

hi maxime,

On Tue, Aug 27, 2019 at 1:59 PM Maxime Ripard <mripard@kernel.org> wrote:
>
> From: Maxime Ripard <maxime.ripard@bootlin.com>
>
> The command line parser when it has been rewritten introduced a regression
> when the only thing on the command line is an option to force the detection
> of a connector (such as video=HDMI-A-1:d), which are completely valid.
>
> It's been further broken by the support for the named modes which take
> anything that is not a resolution as a named mode.
>
> Let's fix this by running the extra command line option parser on the named
> modes if they only take a single character.
>
> Fixes: e08ab74bd4c7 ("drm/modes: Rewrite the command line parser")
> Reported-by: Jernej Škrabec <jernej.skrabec@gmail.com>
> Reported-by: Thomas Graichen <thomas.graichen@googlemail.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Tested-by: thomas graichen <thomas.graichen@gmail.com>

BEFORE (only "[PATCH v5 05/12] drm/modes: Rewrite the command line
parser " applied):
my eachlink h6 mini tv box (which requires the video=HDMI-A-1:e
cmdline option to give any output on hdmi) did not show any hdmi
output any more (in 5.2 it still worked)

AFTER (the above patch plus this patch set here applied):
my eachlink h6 mini tv box gives output on hdmi again. i also did
check it the other way around: if i remove the video=HDMI-A-1:e option
i no longer get any hdmi output as expected. as a result this patch
series fixes the problem/regression for me.

best wishes - thomas

> ---
>  drivers/gpu/drm/drm_modes.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index e5997f35b779..ea7e6c8c8318 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1733,16 +1733,30 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>          * bunch of things:
>          *   - We need to make sure that the first character (which
>          *     would be our resolution in X) is a digit.
> -        *   - However, if the X resolution is missing, then we end up
> -        *     with something like x<yres>, with our first character
> -        *     being an alpha-numerical character, which would be
> -        *     considered a named mode.
> +        *   - If not, then it's either a named mode or a force on/off.
> +        *     To distinguish between the two, we need to run the
> +        *     extra parsing function, and if not, then we consider it
> +        *     a named mode.
>          *
>          * If this isn't enough, we should add more heuristics here,
>          * and matching unit-tests.
>          */
> -       if (!isdigit(name[0]) && name[0] != 'x')
> +       if (!isdigit(name[0]) && name[0] != 'x') {
> +               unsigned int namelen = strlen(name);
> +
> +               /*
> +                * Only the force on/off options can be in that case,
> +                * and they all take a single character.
> +                */
> +               if (namelen == 1) {
> +                       ret = drm_mode_parse_cmdline_extra(name, namelen, true,
> +                                                          connector, mode);
> +                       if (!ret)
> +                               return true;
> +               }
> +
>                 named_mode = true;
> +       }
>
>         /* Try to locate the bpp and refresh specifiers, if any */
>         bpp_ptr = strchr(name, '-');
> --
> 2.21.0
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/modes: Introduce a whitelist for the named modes

[Thomas Graichen]

hi maxime,

On Tue, Aug 27, 2019 at 1:59 PM Maxime Ripard <mripard@kernel.org> wrote:
>
> From: Maxime Ripard <maxime.ripard@bootlin.com>
>
> The named modes support has introduced a number of glitches that were in
> part due to the fact that the parser will take any string as a named mode.
>
> Since we shouldn't have a lot of options there (and they should be pretty
> standard), let's introduce a whitelist of the available named modes so that
> the kernel can differentiate between a poorly formed command line and a
> named mode.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Tested-by: thomas graichen <thomas.graichen@gmail.com>

BEFORE (only "[PATCH v5 05/12] drm/modes: Rewrite the command line
parser " applied):
my eachlink h6 mini tv box (which requires the video=HDMI-A-1:e
cmdline option to give any output on hdmi) did not show any hdmi
output any more (in 5.2 it still worked)

AFTER (the above patch plus this patch set here applied):
my eachlink h6 mini tv box gives output on hdmi again. i also did
check it the other way around: if i remove the video=HDMI-A-1:e option
i no longer get any hdmi output as expected. as a result this patch
series fixes the problem/regression for me.

best wishes - thomas

> ---
>  drivers/gpu/drm/drm_modes.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index ea7e6c8c8318..988797d8080a 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1677,6 +1677,22 @@ static int drm_mode_parse_cmdline_options(char *str, size_t len,
>         return 0;
>  }
>
> +const char *drm_named_modes_whitelist[] = {
> +       "NTSC",
> +       "PAL",
> +};
> +
> +static bool drm_named_mode_is_in_whitelist(const char *mode, unsigned int size)
> +{
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++)
> +               if (!strncmp(mode, drm_named_modes_whitelist[i], size))
> +                       return true;
> +
> +       return false;
> +}
> +
>  /**
>   * drm_mode_parse_command_line_for_connector - parse command line modeline for connector
>   * @mode_option: optional per connector mode option
> @@ -1794,6 +1810,10 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>         if (named_mode) {
>                 if (mode_end + 1 > DRM_DISPLAY_MODE_LEN)
>                         return false;
> +
> +               if (!drm_named_mode_is_in_whitelist(name, mode_end))
> +                       return false;
> +
>                 strscpy(mode->name, name, mode_end + 1);
>         } else {
>                 ret = drm_mode_parse_cmdline_res_mode(name, mode_end,
> --
> 2.21.0
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/selftests: modes: Add more unit tests for the cmdline parser

[Thomas Graichen]

hi maxime,

On Tue, Aug 27, 2019 at 1:59 PM Maxime Ripard <mripard@kernel.org> wrote:
>
> From: Maxime Ripard <maxime.ripard@bootlin.com>
>
> Let's add some unit tests for the recent bugs we just fixed.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Tested-by: thomas graichen <thomas.graichen@gmail.com>

BEFORE (only "[PATCH v5 05/12] drm/modes: Rewrite the command line
parser " applied):
my eachlink h6 mini tv box (which requires the video=HDMI-A-1:e
cmdline option to give any output on hdmi) did not show any hdmi
output any more (in 5.2 it still worked)

AFTER (the above patch plus this patch set here applied):
my eachlink h6 mini tv box gives output on hdmi again. i also did
check it the other way around: if i remove the video=HDMI-A-1:e option
i no longer get any hdmi output as expected. as a result this patch
series fixes the problem/regression for me.

best wishes - thomas

> ---
>  .../gpu/drm/selftests/drm_cmdline_selftests.h |   7 +
>  .../drm/selftests/test-drm_cmdline_parser.c   | 130 ++++++++++++++++++
>  2 files changed, 137 insertions(+)
>
> diff --git a/drivers/gpu/drm/selftests/drm_cmdline_selftests.h b/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
> index b45824ec7c8f..6d61a0eb5d64 100644
> --- a/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
> +++ b/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
> @@ -9,6 +9,13 @@
>
>  #define cmdline_test(test)     selftest(test, test)
>
> +cmdline_test(drm_cmdline_test_force_d_only)
> +cmdline_test(drm_cmdline_test_force_D_only_dvi)
> +cmdline_test(drm_cmdline_test_force_D_only_hdmi)
> +cmdline_test(drm_cmdline_test_force_D_only_not_digital)
> +cmdline_test(drm_cmdline_test_force_e_only)
> +cmdline_test(drm_cmdline_test_margin_only)
> +cmdline_test(drm_cmdline_test_interlace_only)
>  cmdline_test(drm_cmdline_test_res)
>  cmdline_test(drm_cmdline_test_res_missing_x)
>  cmdline_test(drm_cmdline_test_res_missing_y)
> diff --git a/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c b/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c
> index 14c96edb13df..013de9d27c35 100644
> --- a/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c
> +++ b/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c
> @@ -17,6 +17,136 @@
>
>  static const struct drm_connector no_connector = {};
>
> +static int drm_cmdline_test_force_e_only(void *ignored)
> +{
> +       struct drm_cmdline_mode mode = { };
> +
> +       FAIL_ON(!drm_mode_parse_command_line_for_connector("e",
> +                                                          &no_connector,
> +                                                          &mode));
> +       FAIL_ON(mode.specified);
> +       FAIL_ON(mode.refresh_specified);
> +       FAIL_ON(mode.bpp_specified);
> +
> +       FAIL_ON(mode.rb);
> +       FAIL_ON(mode.cvt);
> +       FAIL_ON(mode.interlace);
> +       FAIL_ON(mode.margins);
> +       FAIL_ON(mode.force != DRM_FORCE_ON);
> +
> +       return 0;
> +}
> +
> +static int drm_cmdline_test_force_D_only_not_digital(void *ignored)
> +{
> +       struct drm_cmdline_mode mode = { };
> +
> +       FAIL_ON(!drm_mode_parse_command_line_for_connector("D",
> +                                                          &no_connector,
> +                                                          &mode));
> +       FAIL_ON(mode.specified);
> +       FAIL_ON(mode.refresh_specified);
> +       FAIL_ON(mode.bpp_specified);
> +
> +       FAIL_ON(mode.rb);
> +       FAIL_ON(mode.cvt);
> +       FAIL_ON(mode.interlace);
> +       FAIL_ON(mode.margins);
> +       FAIL_ON(mode.force != DRM_FORCE_ON);
> +
> +       return 0;
> +}
> +
> +static const struct drm_connector connector_hdmi = {
> +       .connector_type = DRM_MODE_CONNECTOR_HDMIB,
> +};
> +
> +static int drm_cmdline_test_force_D_only_hdmi(void *ignored)
> +{
> +       struct drm_cmdline_mode mode = { };
> +
> +       FAIL_ON(!drm_mode_parse_command_line_for_connector("D",
> +                                                          &connector_hdmi,
> +                                                          &mode));
> +       FAIL_ON(mode.specified);
> +       FAIL_ON(mode.refresh_specified);
> +       FAIL_ON(mode.bpp_specified);
> +
> +       FAIL_ON(mode.rb);
> +       FAIL_ON(mode.cvt);
> +       FAIL_ON(mode.interlace);
> +       FAIL_ON(mode.margins);
> +       FAIL_ON(mode.force != DRM_FORCE_ON_DIGITAL);
> +
> +       return 0;
> +}
> +
> +static const struct drm_connector connector_dvi = {
> +       .connector_type = DRM_MODE_CONNECTOR_DVII,
> +};
> +
> +static int drm_cmdline_test_force_D_only_dvi(void *ignored)
> +{
> +       struct drm_cmdline_mode mode = { };
> +
> +       FAIL_ON(!drm_mode_parse_command_line_for_connector("D",
> +                                                          &connector_dvi,
> +                                                          &mode));
> +       FAIL_ON(mode.specified);
> +       FAIL_ON(mode.refresh_specified);
> +       FAIL_ON(mode.bpp_specified);
> +
> +       FAIL_ON(mode.rb);
> +       FAIL_ON(mode.cvt);
> +       FAIL_ON(mode.interlace);
> +       FAIL_ON(mode.margins);
> +       FAIL_ON(mode.force != DRM_FORCE_ON_DIGITAL);
> +
> +       return 0;
> +}
> +
> +static int drm_cmdline_test_force_d_only(void *ignored)
> +{
> +       struct drm_cmdline_mode mode = { };
> +
> +       FAIL_ON(!drm_mode_parse_command_line_for_connector("d",
> +                                                          &no_connector,
> +                                                          &mode));
> +       FAIL_ON(mode.specified);
> +       FAIL_ON(mode.refresh_specified);
> +       FAIL_ON(mode.bpp_specified);
> +
> +       FAIL_ON(mode.rb);
> +       FAIL_ON(mode.cvt);
> +       FAIL_ON(mode.interlace);
> +       FAIL_ON(mode.margins);
> +       FAIL_ON(mode.force != DRM_FORCE_OFF);
> +
> +       return 0;
> +}
> +
> +static int drm_cmdline_test_margin_only(void *ignored)
> +{
> +       struct drm_cmdline_mode mode = { };
> +
> +       FAIL_ON(drm_mode_parse_command_line_for_connector("m",
> +                                                         &no_connector,
> +                                                         &mode));
> +
> +       return 0;
> +}
> +
> +static int drm_cmdline_test_interlace_only(void *ignored)
> +{
> +       struct drm_cmdline_mode mode = { };
> +
> +       FAIL_ON(drm_mode_parse_command_line_for_connector("i",
> +                                                         &no_connector,
> +                                                         &mode));
> +
> +       return 0;
> +}
> +
>  static int drm_cmdline_test_res(void *ignored)
>  {
>         struct drm_cmdline_mode mode = { };
> --
> 2.21.0
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/modes: Add a switch to differentiate free standing options

[Jernej Škrabec]

Hi!

Dne torek, 27. avgust 2019 ob 13:58:47 CEST je Maxime Ripard napisal(a):
> From: Maxime Ripard <maxime.ripard@bootlin.com>
> 
> Some extra command line options can be either specified without anything
> else on the command line (basically all the force connection options), but
> some other are only relevant when matched with a resolution (margin and
> interlace).
> 
> Let's add a switch to restrict if needed the available option set.
> 
> Fixes: e08ab74bd4c7 ("drm/modes: Rewrite the command line parser")
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Reviewed-by: Jernej Skrabec <jernej.skrabec@siol.net>

Best regards,
Jernej


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/modes: Fix the command line parser to take force options into account

[Jernej Škrabec]

Hi!

Dne torek, 27. avgust 2019 ob 13:58:48 CEST je Maxime Ripard napisal(a):
> From: Maxime Ripard <maxime.ripard@bootlin.com>
> 
> The command line parser when it has been rewritten introduced a regression
> when the only thing on the command line is an option to force the detection
> of a connector (such as video=HDMI-A-1:d), which are completely valid.
> 
> It's been further broken by the support for the named modes which take
> anything that is not a resolution as a named mode.
> 
> Let's fix this by running the extra command line option parser on the named
> modes if they only take a single character.
> 
> Fixes: e08ab74bd4c7 ("drm/modes: Rewrite the command line parser")
> Reported-by: Jernej Škrabec <jernej.skrabec@gmail.com>
> Reported-by: Thomas Graichen <thomas.graichen@googlemail.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Reviewed-by: Jernej Skrabec <jernej.skrabec@siol.net>

Best regards,
Jernej


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/modes: Introduce a whitelist for the named modes

[Jernej Škrabec]

Hi!

Dne torek, 27. avgust 2019 ob 13:58:49 CEST je Maxime Ripard napisal(a):
> From: Maxime Ripard <maxime.ripard@bootlin.com>
> 
> The named modes support has introduced a number of glitches that were in
> part due to the fact that the parser will take any string as a named mode.
> 
> Since we shouldn't have a lot of options there (and they should be pretty
> standard), let's introduce a whitelist of the available named modes so that
> the kernel can differentiate between a poorly formed command line and a
> named mode.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  drivers/gpu/drm/drm_modes.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index ea7e6c8c8318..988797d8080a 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1677,6 +1677,22 @@ static int drm_mode_parse_cmdline_options(char *str,
> size_t len, return 0;
>  }
> 
> +const char *drm_named_modes_whitelist[] = {
> +	"NTSC",
> +	"PAL",
> +};

That array should be static. With that fixed:

Reviewed-by: Jernej Skrabec <jernej.skrabec@siol.net>

Best regards,
Jernej

> +
> +static bool drm_named_mode_is_in_whitelist(const char *mode, unsigned int
> size) +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++)
> +		if (!strncmp(mode, drm_named_modes_whitelist[i], size))
> +			return true;
> +
> +	return false;
> +}
> +
>  /**
>   * drm_mode_parse_command_line_for_connector - parse command line modeline
> for connector * @mode_option: optional per connector mode option
> @@ -1794,6 +1810,10 @@ bool drm_mode_parse_command_line_for_connector(const
> char *mode_option, if (named_mode) {
>  		if (mode_end + 1 > DRM_DISPLAY_MODE_LEN)
>  			return false;
> +
> +		if (!drm_named_mode_is_in_whitelist(name, mode_end))
> +			return false;
> +
>  		strscpy(mode->name, name, mode_end + 1);
>  	} else {
>  		ret = drm_mode_parse_cmdline_res_mode(name, mode_end,




_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/selftests: modes: Add more unit tests for the cmdline parser

[Jernej Škrabec]

Hi!

Dne torek, 27. avgust 2019 ob 13:58:50 CEST je Maxime Ripard napisal(a):
> From: Maxime Ripard <maxime.ripard@bootlin.com>
> 
> Let's add some unit tests for the recent bugs we just fixed.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  .../gpu/drm/selftests/drm_cmdline_selftests.h |   7 +
>  .../drm/selftests/test-drm_cmdline_parser.c   | 130 ++++++++++++++++++
>  2 files changed, 137 insertions(+)
> 
> diff --git a/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
> b/drivers/gpu/drm/selftests/drm_cmdline_selftests.h index

Reviewed-by: Jernej Skrabec <jernej.skrabec@siol.net>

Best regards,
Jernej


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/modes: Introduce a whitelist for the named modes

[Jani Nikula]

On Thu, 29 Aug 2019, Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
> Hi!
>
> Dne torek, 27. avgust 2019 ob 13:58:49 CEST je Maxime Ripard napisal(a):
>> From: Maxime Ripard <maxime.ripard@bootlin.com>
>> 
>> The named modes support has introduced a number of glitches that were in
>> part due to the fact that the parser will take any string as a named mode.
>> 
>> Since we shouldn't have a lot of options there (and they should be pretty
>> standard), let's introduce a whitelist of the available named modes so that
>> the kernel can differentiate between a poorly formed command line and a
>> named mode.
>> 
>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>> ---
>>  drivers/gpu/drm/drm_modes.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>> index ea7e6c8c8318..988797d8080a 100644
>> --- a/drivers/gpu/drm/drm_modes.c
>> +++ b/drivers/gpu/drm/drm_modes.c
>> @@ -1677,6 +1677,22 @@ static int drm_mode_parse_cmdline_options(char *str,
>> size_t len, return 0;
>>  }
>> 
>> +const char *drm_named_modes_whitelist[] = {
>> +	"NTSC",
>> +	"PAL",
>> +};
>
> That array should be static. With that fixed:

And add more const for good measure:

static const char * const drm_named_modes_whitelist[] = {

BR,
Jani.

>
> Reviewed-by: Jernej Skrabec <jernej.skrabec@siol.net>
>
> Best regards,
> Jernej
>
>> +
>> +static bool drm_named_mode_is_in_whitelist(const char *mode, unsigned int
>> size) +{
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++)
>> +		if (!strncmp(mode, drm_named_modes_whitelist[i], size))
>> +			return true;
>> +
>> +	return false;
>> +}
>> +
>>  /**
>>   * drm_mode_parse_command_line_for_connector - parse command line modeline
>> for connector * @mode_option: optional per connector mode option
>> @@ -1794,6 +1810,10 @@ bool drm_mode_parse_command_line_for_connector(const
>> char *mode_option, if (named_mode) {
>>  		if (mode_end + 1 > DRM_DISPLAY_MODE_LEN)
>>  			return false;
>> +
>> +		if (!drm_named_mode_is_in_whitelist(name, mode_end))
>> +			return false;
>> +
>>  		strscpy(mode->name, name, mode_end + 1);
>>  	} else {
>>  		ret = drm_mode_parse_cmdline_res_mode(name, mode_end,
>
>
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel