[PATCH v3 0/6] drm/vc4: Allow for more boot-time configuration

[PATCH v3 0/6] drm/vc4: Allow for more boot-time configuration

[Maxime Ripard]

Hi,

The proprietary stack for the RaspberryPi allows for a number of video
parameters widely used by their users, but yet don't have any equivalents
in the mainline kernel.

Those options are detailed here:
https://www.raspberrypi.org/documentation/configuration/config-txt/video.md

While not all of them are desirable to have in the mainline kernel, some of
them still have value, such as properties to initialise the overscan or
rotation parameters.

This series is an attempt to support those, and is based on a rewrite of
the command line parser I did a couple of years ago that never reached
upstream (due to a lack of time on my side). While this parser was
initially made to deal with named modes (in order to support TV modes), it
also allowed to extend it more easily, which is why it's resurrected.

Let me know what you think,
Maxime

Changes from v2:
  - Fixed some sparse warnings
  - Rebased on top of next and Noralf series
  - Moved the property initialisation to vc4 reset hook
  - Added documentation for the new drm_cmdline_mode
  - Renamed overscan to tv_margins to be consistent with the APIs

Changes from v1:
  - Dropped the patches to deal with EDID
  - Added the unit test as selftest
  - Rebased on next

Maxime Ripard (6):
  drm/modes: Rewrite the command line parser
  drm/modes: Support modes names on the command line
  drm/modes: Allow to specify rotation and reflection on the commandline
  drm/modes: Parse overscan properties
  drm/selftests: Add command line parser selftests
  drm/vc4: hdmi: Set default state margin at reset

 drivers/gpu/drm/drm_client_modeset.c                |  14 +-
 drivers/gpu/drm/drm_connector.c                     |   3 +-
 drivers/gpu/drm/drm_modes.c                         | 441 +++++--
 drivers/gpu/drm/selftests/Makefile                  |   2 +-
 drivers/gpu/drm/selftests/drm_cmdline_selftests.h   |  49 +-
 drivers/gpu/drm/selftests/test-drm_cmdline_parser.c | 846 +++++++++++++-
 drivers/gpu/drm/vc4/vc4_hdmi.c                      |  16 +-
 include/drm/drm_connector.h                         |  24 +-
 8 files changed, 1277 insertions(+), 118 deletions(-)
 create mode 100644 drivers/gpu/drm/selftests/drm_cmdline_selftests.h
 create mode 100644 drivers/gpu/drm/selftests/test-drm_cmdline_parser.c

base-commit: ea2c44e9e7b523296ff8e26943bfdd0758d60104
-- 
git-series 0.9.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 0/6] drm/vc4: Allow for more boot-time configuration

[Maxime Ripard]

Hi,

The proprietary stack for the RaspberryPi allows for a number of video
parameters widely used by their users, but yet don't have any equivalents
in the mainline kernel.

Those options are detailed here:
https://www.raspberrypi.org/documentation/configuration/config-txt/video.md

While not all of them are desirable to have in the mainline kernel, some of
them still have value, such as properties to initialise the overscan or
rotation parameters.

This series is an attempt to support those, and is based on a rewrite of
the command line parser I did a couple of years ago that never reached
upstream (due to a lack of time on my side). While this parser was
initially made to deal with named modes (in order to support TV modes), it
also allowed to extend it more easily, which is why it's resurrected.

Let me know what you think,
Maxime

Changes from v2:
  - Fixed some sparse warnings
  - Rebased on top of next and Noralf series
  - Moved the property initialisation to vc4 reset hook
  - Added documentation for the new drm_cmdline_mode
  - Renamed overscan to tv_margins to be consistent with the APIs

Changes from v1:
  - Dropped the patches to deal with EDID
  - Added the unit test as selftest
  - Rebased on next

Maxime Ripard (6):
  drm/modes: Rewrite the command line parser
  drm/modes: Support modes names on the command line
  drm/modes: Allow to specify rotation and reflection on the commandline
  drm/modes: Parse overscan properties
  drm/selftests: Add command line parser selftests
  drm/vc4: hdmi: Set default state margin at reset

 drivers/gpu/drm/drm_client_modeset.c                |  14 +-
 drivers/gpu/drm/drm_connector.c                     |   3 +-
 drivers/gpu/drm/drm_modes.c                         | 441 +++++--
 drivers/gpu/drm/selftests/Makefile                  |   2 +-
 drivers/gpu/drm/selftests/drm_cmdline_selftests.h   |  49 +-
 drivers/gpu/drm/selftests/test-drm_cmdline_parser.c | 846 +++++++++++++-
 drivers/gpu/drm/vc4/vc4_hdmi.c                      |  16 +-
 include/drm/drm_connector.h                         |  24 +-
 8 files changed, 1277 insertions(+), 118 deletions(-)
 create mode 100644 drivers/gpu/drm/selftests/drm_cmdline_selftests.h
 create mode 100644 drivers/gpu/drm/selftests/test-drm_cmdline_parser.c

base-commit: ea2c44e9e7b523296ff8e26943bfdd0758d60104
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 1/6] drm/modes: Rewrite the command line parser

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

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/gpu/drm/drm_modes.c | 305 +++++++++++++++++++++++--------------
 1 file changed, 190 insertions(+), 115 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 56f92a0bba62..3f89198f0891 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>
@@ -1405,6 +1406,131 @@ 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)
+{
+	if (str[0] != '-')
+		return -EINVAL;
+
+	mode->bpp = simple_strtol(str + 1, end_ptr, 10);
+	mode->bpp_specified = true;
+
+	return 0;
+}
+
+static int drm_mode_parse_cmdline_refresh(const char *str, char **end_ptr,
+					  struct drm_cmdline_mode *mode)
+{
+	if (str[0] != '@')
+		return -EINVAL;
+
+	mode->refresh = simple_strtol(str + 1, end_ptr, 10);
+	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)
+{
+	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[0] != 'x')
+		return -EINVAL;
+	end_ptr++;
+
+	yres = simple_strtol(end_ptr, &end_ptr, 10);
+
+	remaining = length - (end_ptr - str);
+	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
@@ -1431,13 +1557,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)
@@ -1450,127 +1575,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;
 }
-- 
git-series 0.9.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 1/6] drm/modes: Rewrite the command line parser

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

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/gpu/drm/drm_modes.c | 305 +++++++++++++++++++++++--------------
 1 file changed, 190 insertions(+), 115 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 56f92a0bba62..3f89198f0891 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>
@@ -1405,6 +1406,131 @@ 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)
+{
+	if (str[0] != '-')
+		return -EINVAL;
+
+	mode->bpp = simple_strtol(str + 1, end_ptr, 10);
+	mode->bpp_specified = true;
+
+	return 0;
+}
+
+static int drm_mode_parse_cmdline_refresh(const char *str, char **end_ptr,
+					  struct drm_cmdline_mode *mode)
+{
+	if (str[0] != '@')
+		return -EINVAL;
+
+	mode->refresh = simple_strtol(str + 1, end_ptr, 10);
+	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)
+{
+	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[0] != 'x')
+		return -EINVAL;
+	end_ptr++;
+
+	yres = simple_strtol(end_ptr, &end_ptr, 10);
+
+	remaining = length - (end_ptr - str);
+	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
@@ -1431,13 +1557,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)
@@ -1450,127 +1575,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;
 }
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 2/6] drm/modes: Support modes names on the command line

[Maxime Ripard]

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

The drm subsystem also uses the video= kernel parameter, and in the
documentation refers to the fbdev documentation for that parameter.

However, that documentation also says that instead of giving the mode using
its resolution we can also give a name. However, DRM doesn't handle that
case at the moment. Even though in most case it shouldn't make any
difference, it might be useful for analog modes, where different standards
might have the same resolution, but still have a few different parameters
that are not encoded in the modes (NTSC vs NTSC-J vs PAL-M for example).

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/gpu/drm/drm_client_modeset.c |  4 ++-
 drivers/gpu/drm/drm_connector.c      |  3 +-
 drivers/gpu/drm/drm_modes.c          | 52 ++++++++++++++++++++---------
 include/drm/drm_connector.h          |  1 +-
 4 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
index 2a428ac00930..f2869c82510c 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -149,6 +149,10 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector)
 	prefer_non_interlace = !cmdline_mode->interlace;
 again:
 	list_for_each_entry(mode, &connector->modes, head) {
+		/* Check (optional) mode name first */
+		if (!strcmp(mode->name, cmdline_mode->name))
+			return mode;
+
 		/* check width/height */
 		if (mode->hdisplay != cmdline_mode->xres ||
 		    mode->vdisplay != cmdline_mode->yres)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 2355124849db..e33814f5940e 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -139,8 +139,9 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
 		connector->force = mode->force;
 	}
 
-	DRM_DEBUG_KMS("cmdline mode for connector %s %dx%d@%dHz%s%s%s\n",
+	DRM_DEBUG_KMS("cmdline mode for connector %s %s %dx%d@%dHz%s%s%s\n",
 		      connector->name,
+		      mode->name ? mode->name : "",
 		      mode->xres, mode->yres,
 		      mode->refresh_specified ? mode->refresh : 60,
 		      mode->rb ? " reduced blanking" : "",
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 3f89198f0891..9613c1a28487 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1557,7 +1557,7 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 					       struct drm_cmdline_mode *mode)
 {
 	const char *name;
-	bool parse_extras = false;
+	bool named_mode = false, 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;
@@ -1576,8 +1576,14 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 
 	name = mode_option;
 
+	/*
+	 * If the first character is not a digit, then it means that
+	 * we have a named mode.
+	 */
 	if (!isdigit(name[0]))
-		return false;
+		named_mode = true;
+	else
+		named_mode = false;
 
 	/* Try to locate the bpp and refresh specifiers, if any */
 	bpp_ptr = strchr(name, '-');
@@ -1588,6 +1594,9 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 
 	refresh_ptr = strchr(name, '@');
 	if (refresh_ptr) {
+		if (named_mode)
+			return false;
+
 		refresh_off = refresh_ptr - name;
 		mode->refresh_specified = true;
 	}
@@ -1604,12 +1613,16 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 		parse_extras = true;
 	}
 
-	ret = drm_mode_parse_cmdline_res_mode(name, mode_end,
-					      parse_extras,
-					      connector,
-					      mode);
-	if (ret)
-		return false;
+	if (named_mode) {
+		strncpy(mode->name, name, mode_end);
+	} else {
+		ret = drm_mode_parse_cmdline_res_mode(name, mode_end,
+						      parse_extras,
+						      connector,
+						      mode);
+		if (ret)
+			return false;
+	}
 	mode->specified = true;
 
 	if (bpp_ptr) {
@@ -1637,14 +1650,23 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 		extra_ptr = refresh_end_ptr;
 
 	if (extra_ptr) {
-		int remaining = strlen(name) - (extra_ptr - name);
+		if (!named_mode) {
+			int len = strlen(name) - (extra_ptr - name);
 
-		/*
-		 * We still have characters to process, while
-		 * we shouldn't have any
-		 */
-		if (remaining > 0)
-			return false;
+			ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
+							   connector, mode);
+			if (ret)
+				return false;
+		} else {
+			int remaining = strlen(name) - (extra_ptr - name);
+
+			/*
+			 * We still have characters to process, while
+			 * we shouldn't have any
+			 */
+			if (remaining > 0)
+				return false;
+		}
 	}
 
 	return true;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 02a131202add..06aa3b9cb920 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -896,6 +896,7 @@ struct drm_connector_funcs {
 
 /* mode specified on the command line */
 struct drm_cmdline_mode {
+	char name[DRM_DISPLAY_MODE_LEN];
 	bool specified;
 	bool refresh_specified;
 	bool bpp_specified;
-- 
git-series 0.9.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 2/6] drm/modes: Support modes names on the command line

[Maxime Ripard]

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

The drm subsystem also uses the video= kernel parameter, and in the
documentation refers to the fbdev documentation for that parameter.

However, that documentation also says that instead of giving the mode using
its resolution we can also give a name. However, DRM doesn't handle that
case at the moment. Even though in most case it shouldn't make any
difference, it might be useful for analog modes, where different standards
might have the same resolution, but still have a few different parameters
that are not encoded in the modes (NTSC vs NTSC-J vs PAL-M for example).

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/gpu/drm/drm_client_modeset.c |  4 ++-
 drivers/gpu/drm/drm_connector.c      |  3 +-
 drivers/gpu/drm/drm_modes.c          | 52 ++++++++++++++++++++---------
 include/drm/drm_connector.h          |  1 +-
 4 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
index 2a428ac00930..f2869c82510c 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -149,6 +149,10 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector)
 	prefer_non_interlace = !cmdline_mode->interlace;
 again:
 	list_for_each_entry(mode, &connector->modes, head) {
+		/* Check (optional) mode name first */
+		if (!strcmp(mode->name, cmdline_mode->name))
+			return mode;
+
 		/* check width/height */
 		if (mode->hdisplay != cmdline_mode->xres ||
 		    mode->vdisplay != cmdline_mode->yres)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 2355124849db..e33814f5940e 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -139,8 +139,9 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
 		connector->force = mode->force;
 	}
 
-	DRM_DEBUG_KMS("cmdline mode for connector %s %dx%d@%dHz%s%s%s\n",
+	DRM_DEBUG_KMS("cmdline mode for connector %s %s %dx%d@%dHz%s%s%s\n",
 		      connector->name,
+		      mode->name ? mode->name : "",
 		      mode->xres, mode->yres,
 		      mode->refresh_specified ? mode->refresh : 60,
 		      mode->rb ? " reduced blanking" : "",
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 3f89198f0891..9613c1a28487 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1557,7 +1557,7 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 					       struct drm_cmdline_mode *mode)
 {
 	const char *name;
-	bool parse_extras = false;
+	bool named_mode = false, 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;
@@ -1576,8 +1576,14 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 
 	name = mode_option;
 
+	/*
+	 * If the first character is not a digit, then it means that
+	 * we have a named mode.
+	 */
 	if (!isdigit(name[0]))
-		return false;
+		named_mode = true;
+	else
+		named_mode = false;
 
 	/* Try to locate the bpp and refresh specifiers, if any */
 	bpp_ptr = strchr(name, '-');
@@ -1588,6 +1594,9 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 
 	refresh_ptr = strchr(name, '@');
 	if (refresh_ptr) {
+		if (named_mode)
+			return false;
+
 		refresh_off = refresh_ptr - name;
 		mode->refresh_specified = true;
 	}
@@ -1604,12 +1613,16 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 		parse_extras = true;
 	}
 
-	ret = drm_mode_parse_cmdline_res_mode(name, mode_end,
-					      parse_extras,
-					      connector,
-					      mode);
-	if (ret)
-		return false;
+	if (named_mode) {
+		strncpy(mode->name, name, mode_end);
+	} else {
+		ret = drm_mode_parse_cmdline_res_mode(name, mode_end,
+						      parse_extras,
+						      connector,
+						      mode);
+		if (ret)
+			return false;
+	}
 	mode->specified = true;
 
 	if (bpp_ptr) {
@@ -1637,14 +1650,23 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 		extra_ptr = refresh_end_ptr;
 
 	if (extra_ptr) {
-		int remaining = strlen(name) - (extra_ptr - name);
+		if (!named_mode) {
+			int len = strlen(name) - (extra_ptr - name);
 
-		/*
-		 * We still have characters to process, while
-		 * we shouldn't have any
-		 */
-		if (remaining > 0)
-			return false;
+			ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
+							   connector, mode);
+			if (ret)
+				return false;
+		} else {
+			int remaining = strlen(name) - (extra_ptr - name);
+
+			/*
+			 * We still have characters to process, while
+			 * we shouldn't have any
+			 */
+			if (remaining > 0)
+				return false;
+		}
 	}
 
 	return true;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 02a131202add..06aa3b9cb920 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -896,6 +896,7 @@ struct drm_connector_funcs {
 
 /* mode specified on the command line */
 struct drm_cmdline_mode {
+	char name[DRM_DISPLAY_MODE_LEN];
 	bool specified;
 	bool refresh_specified;
 	bool bpp_specified;
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 3/6] drm/modes: Allow to specify rotation and reflection on the commandline

[Maxime Ripard]

Rotations and reflections setup are needed in some scenarios to initialise
properly the initial framebuffer. Some drivers already had a bunch of
quirks to deal with this, such as either a private kernel command line
parameter (omapdss) or on the device tree (various panels).

In order to accomodate this, let's create a video mode parameter to deal
with the rotation and reflexion.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/drm_client_modeset.c |  10 +++-
 drivers/gpu/drm/drm_modes.c          | 110 ++++++++++++++++++++++------
 include/drm/drm_connector.h          |   9 ++-
 3 files changed, 109 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
index f2869c82510c..15145d2c86d5 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -823,6 +823,7 @@ EXPORT_SYMBOL(drm_client_modeset_probe);
 bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotation)
 {
 	struct drm_connector *connector = modeset->connectors[0];
+	struct drm_cmdline_mode *cmdline;
 	struct drm_plane *plane = modeset->crtc->primary;
 	u64 valid_mask = 0;
 	unsigned int i;
@@ -844,6 +845,15 @@ bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotat
 		*rotation = DRM_MODE_ROTATE_0;
 	}
 
+	/**
+	 * We want the rotation on the command line to overwrite
+	 * whatever comes from the panel.
+	 */
+	cmdline = &connector->cmdline_mode;
+	if (cmdline->specified &&
+	    cmdline->rotation != DRM_MODE_ROTATE_0)
+		*rotation = cmdline->rotation;
+
 	/*
 	 * TODO: support 90 / 270 degree hardware rotation,
 	 * depending on the hardware this may require the framebuffer
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 9613c1a28487..ac8d70b92b62 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1531,6 +1531,71 @@ static int drm_mode_parse_cmdline_res_mode(const char *str, unsigned int length,
 	return 0;
 }
 
+static int drm_mode_parse_cmdline_options(char *str, size_t len,
+					  struct drm_connector *connector,
+					  struct drm_cmdline_mode *mode)
+{
+	unsigned int rotation = 0;
+	char *sep = str;
+
+	while ((sep = strchr(sep, ','))) {
+		char *delim, *option;
+
+		option = sep + 1;
+		delim = strchr(option, '=');
+		if (!delim) {
+			delim = strchr(option, ',');
+
+			if (!delim)
+				delim = str + len;
+		}
+
+		if (!strncmp(option, "rotate", delim - option)) {
+			const char *value = delim + 1;
+			unsigned int deg;
+
+			deg = simple_strtol(value, &sep, 10);
+
+			/* Make sure we have parsed something */
+			if (sep == value)
+				return -EINVAL;
+
+			switch (deg) {
+			case 0:
+				rotation |= DRM_MODE_ROTATE_0;
+				break;
+
+			case 90:
+				rotation |= DRM_MODE_ROTATE_90;
+				break;
+
+			case 180:
+				rotation |= DRM_MODE_ROTATE_180;
+				break;
+
+			case 270:
+				rotation |= DRM_MODE_ROTATE_270;
+				break;
+
+			default:
+				return -EINVAL;
+			}
+		} else if (!strncmp(option, "reflect_x", delim - option)) {
+			rotation |= DRM_MODE_REFLECT_X;
+			sep = delim;
+		} else if (!strncmp(option, "reflect_y", delim - option)) {
+			rotation |= DRM_MODE_REFLECT_Y;
+			sep = delim;
+		} else {
+			return -EINVAL;
+		}
+	}
+
+	mode->rotation = rotation;
+
+	return 0;
+}
+
 /**
  * drm_mode_parse_command_line_for_connector - parse command line modeline for connector
  * @mode_option: optional per connector mode option
@@ -1558,9 +1623,10 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 {
 	const char *name;
 	bool named_mode = false, parse_extras = false;
-	unsigned int bpp_off = 0, refresh_off = 0;
+	unsigned int bpp_off = 0, refresh_off = 0, options_off = 0;
 	unsigned int mode_end = 0;
 	char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
+	char *options_ptr = NULL;
 	char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
 	int ret;
 
@@ -1601,13 +1667,18 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 		mode->refresh_specified = true;
 	}
 
+	/* Locate the start of named options */
+	options_ptr = strchr(name, ',');
+	if (options_ptr)
+		options_off = options_ptr - name;
+
 	/* 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) {
+	if (bpp_ptr) {
 		mode_end = bpp_off;
 	} else if (refresh_ptr) {
 		mode_end = refresh_off;
+	} else if (options_ptr) {
+		mode_end = options_off;
 	} else {
 		mode_end = strlen(name);
 		parse_extras = true;
@@ -1649,24 +1720,23 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 	else if (refresh_ptr)
 		extra_ptr = refresh_end_ptr;
 
-	if (extra_ptr) {
-		if (!named_mode) {
-			int len = strlen(name) - (extra_ptr - name);
+	if (extra_ptr &&
+	    extra_ptr != options_ptr) {
+		int len = strlen(name) - (extra_ptr - name);
 
-			ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
-							   connector, mode);
-			if (ret)
-				return false;
-		} else {
-			int remaining = strlen(name) - (extra_ptr - name);
+		ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
+						   connector, mode);
+		if (ret)
+			return false;
+	}
 
-			/*
-			 * We still have characters to process, while
-			 * we shouldn't have any
-			 */
-			if (remaining > 0)
-				return false;
-		}
+	if (options_ptr) {
+		int len = strlen(name) - (options_ptr - name);
+
+		ret = drm_mode_parse_cmdline_options(options_ptr, len,
+						     connector, mode);
+		if (ret)
+			return false;
 	}
 
 	return true;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 06aa3b9cb920..6f57c1a3afff 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -908,6 +908,15 @@ struct drm_cmdline_mode {
 	bool cvt;
 	bool margins;
 	enum drm_connector_force force;
+
+	/**
+	 * @rotation:
+	 *
+	 * Initial rotation of the mode setup from the command line.
+	 * See DRM_MODE_ROTATE_*. Only DRM_MODE_ROTATE_0 and
+	 * DRM_MODE_ROTATE_180 are supported at the moment.
+	 */
+	unsigned int rotation;
 };
 
 /**
-- 
git-series 0.9.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 3/6] drm/modes: Allow to specify rotation and reflection on the commandline

[Maxime Ripard]

Rotations and reflections setup are needed in some scenarios to initialise
properly the initial framebuffer. Some drivers already had a bunch of
quirks to deal with this, such as either a private kernel command line
parameter (omapdss) or on the device tree (various panels).

In order to accomodate this, let's create a video mode parameter to deal
with the rotation and reflexion.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/drm_client_modeset.c |  10 +++-
 drivers/gpu/drm/drm_modes.c          | 110 ++++++++++++++++++++++------
 include/drm/drm_connector.h          |   9 ++-
 3 files changed, 109 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
index f2869c82510c..15145d2c86d5 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -823,6 +823,7 @@ EXPORT_SYMBOL(drm_client_modeset_probe);
 bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotation)
 {
 	struct drm_connector *connector = modeset->connectors[0];
+	struct drm_cmdline_mode *cmdline;
 	struct drm_plane *plane = modeset->crtc->primary;
 	u64 valid_mask = 0;
 	unsigned int i;
@@ -844,6 +845,15 @@ bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotat
 		*rotation = DRM_MODE_ROTATE_0;
 	}
 
+	/**
+	 * We want the rotation on the command line to overwrite
+	 * whatever comes from the panel.
+	 */
+	cmdline = &connector->cmdline_mode;
+	if (cmdline->specified &&
+	    cmdline->rotation != DRM_MODE_ROTATE_0)
+		*rotation = cmdline->rotation;
+
 	/*
 	 * TODO: support 90 / 270 degree hardware rotation,
 	 * depending on the hardware this may require the framebuffer
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 9613c1a28487..ac8d70b92b62 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1531,6 +1531,71 @@ static int drm_mode_parse_cmdline_res_mode(const char *str, unsigned int length,
 	return 0;
 }
 
+static int drm_mode_parse_cmdline_options(char *str, size_t len,
+					  struct drm_connector *connector,
+					  struct drm_cmdline_mode *mode)
+{
+	unsigned int rotation = 0;
+	char *sep = str;
+
+	while ((sep = strchr(sep, ','))) {
+		char *delim, *option;
+
+		option = sep + 1;
+		delim = strchr(option, '=');
+		if (!delim) {
+			delim = strchr(option, ',');
+
+			if (!delim)
+				delim = str + len;
+		}
+
+		if (!strncmp(option, "rotate", delim - option)) {
+			const char *value = delim + 1;
+			unsigned int deg;
+
+			deg = simple_strtol(value, &sep, 10);
+
+			/* Make sure we have parsed something */
+			if (sep == value)
+				return -EINVAL;
+
+			switch (deg) {
+			case 0:
+				rotation |= DRM_MODE_ROTATE_0;
+				break;
+
+			case 90:
+				rotation |= DRM_MODE_ROTATE_90;
+				break;
+
+			case 180:
+				rotation |= DRM_MODE_ROTATE_180;
+				break;
+
+			case 270:
+				rotation |= DRM_MODE_ROTATE_270;
+				break;
+
+			default:
+				return -EINVAL;
+			}
+		} else if (!strncmp(option, "reflect_x", delim - option)) {
+			rotation |= DRM_MODE_REFLECT_X;
+			sep = delim;
+		} else if (!strncmp(option, "reflect_y", delim - option)) {
+			rotation |= DRM_MODE_REFLECT_Y;
+			sep = delim;
+		} else {
+			return -EINVAL;
+		}
+	}
+
+	mode->rotation = rotation;
+
+	return 0;
+}
+
 /**
  * drm_mode_parse_command_line_for_connector - parse command line modeline for connector
  * @mode_option: optional per connector mode option
@@ -1558,9 +1623,10 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 {
 	const char *name;
 	bool named_mode = false, parse_extras = false;
-	unsigned int bpp_off = 0, refresh_off = 0;
+	unsigned int bpp_off = 0, refresh_off = 0, options_off = 0;
 	unsigned int mode_end = 0;
 	char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
+	char *options_ptr = NULL;
 	char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
 	int ret;
 
@@ -1601,13 +1667,18 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 		mode->refresh_specified = true;
 	}
 
+	/* Locate the start of named options */
+	options_ptr = strchr(name, ',');
+	if (options_ptr)
+		options_off = options_ptr - name;
+
 	/* 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) {
+	if (bpp_ptr) {
 		mode_end = bpp_off;
 	} else if (refresh_ptr) {
 		mode_end = refresh_off;
+	} else if (options_ptr) {
+		mode_end = options_off;
 	} else {
 		mode_end = strlen(name);
 		parse_extras = true;
@@ -1649,24 +1720,23 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 	else if (refresh_ptr)
 		extra_ptr = refresh_end_ptr;
 
-	if (extra_ptr) {
-		if (!named_mode) {
-			int len = strlen(name) - (extra_ptr - name);
+	if (extra_ptr &&
+	    extra_ptr != options_ptr) {
+		int len = strlen(name) - (extra_ptr - name);
 
-			ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
-							   connector, mode);
-			if (ret)
-				return false;
-		} else {
-			int remaining = strlen(name) - (extra_ptr - name);
+		ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
+						   connector, mode);
+		if (ret)
+			return false;
+	}
 
-			/*
-			 * We still have characters to process, while
-			 * we shouldn't have any
-			 */
-			if (remaining > 0)
-				return false;
-		}
+	if (options_ptr) {
+		int len = strlen(name) - (options_ptr - name);
+
+		ret = drm_mode_parse_cmdline_options(options_ptr, len,
+						     connector, mode);
+		if (ret)
+			return false;
 	}
 
 	return true;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 06aa3b9cb920..6f57c1a3afff 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -908,6 +908,15 @@ struct drm_cmdline_mode {
 	bool cvt;
 	bool margins;
 	enum drm_connector_force force;
+
+	/**
+	 * @rotation:
+	 *
+	 * Initial rotation of the mode setup from the command line.
+	 * See DRM_MODE_ROTATE_*. Only DRM_MODE_ROTATE_0 and
+	 * DRM_MODE_ROTATE_180 are supported at the moment.
+	 */
+	unsigned int rotation;
 };
 
 /**
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 4/6] drm/modes: Parse overscan properties

[Maxime Ripard]

Properly configuring the overscan properties might be needed for the
initial setup of the framebuffer for display that still have overscan.
Let's allow for more properties on the kernel command line to setup each
margin.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/drm_modes.c | 44 ++++++++++++++++++++++++++++++++++++++-
 include/drm/drm_connector.h | 14 ++++++++++++-
 2 files changed, 58 insertions(+)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index ac8d70b92b62..d93c44a97ce9 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1586,6 +1586,50 @@ static int drm_mode_parse_cmdline_options(char *str, size_t len,
 		} else if (!strncmp(option, "reflect_y", delim - option)) {
 			rotation |= DRM_MODE_REFLECT_Y;
 			sep = delim;
+		} else if (!strncmp(option, "margin_right", delim - option)) {
+			const char *value = delim + 1;
+			unsigned int margin;
+
+			margin = simple_strtol(value, &sep, 10);
+
+			/* Make sure we have parsed something */
+			if (sep == value)
+				return -EINVAL;
+
+			mode->tv_margins.right = margin;
+		} else if (!strncmp(option, "margin_left", delim - option)) {
+			const char *value = delim + 1;
+			unsigned int margin;
+
+			margin = simple_strtol(value, &sep, 10);
+
+			/* Make sure we have parsed something */
+			if (sep == value)
+				return -EINVAL;
+
+			mode->tv_margins.left = margin;
+		} else if (!strncmp(option, "margin_top", delim - option)) {
+			const char *value = delim + 1;
+			unsigned int margin;
+
+			margin = simple_strtol(value, &sep, 10);
+
+			/* Make sure we have parsed something */
+			if (sep == value)
+				return -EINVAL;
+
+			mode->tv_margins.top = margin;
+		} else if (!strncmp(option, "margin_bottom", delim - option)) {
+			const char *value = delim + 1;
+			unsigned int margin;
+
+			margin = simple_strtol(value, &sep, 10);
+
+			/* Make sure we have parsed something */
+			if (sep == value)
+				return -EINVAL;
+
+			mode->tv_margins.bottom = margin;
 		} else {
 			return -EINVAL;
 		}
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 6f57c1a3afff..89bc6ac38043 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -917,6 +917,20 @@ struct drm_cmdline_mode {
 	 * DRM_MODE_ROTATE_180 are supported at the moment.
 	 */
 	unsigned int rotation;
+
+	/**
+	 * @tv_margins: TV margins (in pixels)
+	 * @tv_margins.left: left margin
+	 * @tv_margins.right: right margin
+	 * @tv_margins.top: top margin
+	 * @tv_margins.bottom: bottom margin
+	 */
+	struct {
+		unsigned int left;
+		unsigned int right;
+		unsigned int top;
+		unsigned int bottom;
+	} tv_margins;
 };
 
 /**
-- 
git-series 0.9.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 4/6] drm/modes: Parse overscan properties

[Maxime Ripard]

Properly configuring the overscan properties might be needed for the
initial setup of the framebuffer for display that still have overscan.
Let's allow for more properties on the kernel command line to setup each
margin.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/drm_modes.c | 44 ++++++++++++++++++++++++++++++++++++++-
 include/drm/drm_connector.h | 14 ++++++++++++-
 2 files changed, 58 insertions(+)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index ac8d70b92b62..d93c44a97ce9 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1586,6 +1586,50 @@ static int drm_mode_parse_cmdline_options(char *str, size_t len,
 		} else if (!strncmp(option, "reflect_y", delim - option)) {
 			rotation |= DRM_MODE_REFLECT_Y;
 			sep = delim;
+		} else if (!strncmp(option, "margin_right", delim - option)) {
+			const char *value = delim + 1;
+			unsigned int margin;
+
+			margin = simple_strtol(value, &sep, 10);
+
+			/* Make sure we have parsed something */
+			if (sep == value)
+				return -EINVAL;
+
+			mode->tv_margins.right = margin;
+		} else if (!strncmp(option, "margin_left", delim - option)) {
+			const char *value = delim + 1;
+			unsigned int margin;
+
+			margin = simple_strtol(value, &sep, 10);
+
+			/* Make sure we have parsed something */
+			if (sep == value)
+				return -EINVAL;
+
+			mode->tv_margins.left = margin;
+		} else if (!strncmp(option, "margin_top", delim - option)) {
+			const char *value = delim + 1;
+			unsigned int margin;
+
+			margin = simple_strtol(value, &sep, 10);
+
+			/* Make sure we have parsed something */
+			if (sep == value)
+				return -EINVAL;
+
+			mode->tv_margins.top = margin;
+		} else if (!strncmp(option, "margin_bottom", delim - option)) {
+			const char *value = delim + 1;
+			unsigned int margin;
+
+			margin = simple_strtol(value, &sep, 10);
+
+			/* Make sure we have parsed something */
+			if (sep == value)
+				return -EINVAL;
+
+			mode->tv_margins.bottom = margin;
 		} else {
 			return -EINVAL;
 		}
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 6f57c1a3afff..89bc6ac38043 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -917,6 +917,20 @@ struct drm_cmdline_mode {
 	 * DRM_MODE_ROTATE_180 are supported at the moment.
 	 */
 	unsigned int rotation;
+
+	/**
+	 * @tv_margins: TV margins (in pixels)
+	 * @tv_margins.left: left margin
+	 * @tv_margins.right: right margin
+	 * @tv_margins.top: top margin
+	 * @tv_margins.bottom: bottom margin
+	 */
+	struct {
+		unsigned int left;
+		unsigned int right;
+		unsigned int top;
+		unsigned int bottom;
+	} tv_margins;
 };
 
 /**
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 5/6] drm/selftests: Add command line parser selftests

[Maxime Ripard]

The command line parser is pretty tough to get right and very error prone,
so let's add a selftest to try to catch any regression.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/selftests/Makefile                  |   2 +-
 drivers/gpu/drm/selftests/drm_cmdline_selftests.h   |  49 +-
 drivers/gpu/drm/selftests/test-drm_cmdline_parser.c | 846 +++++++++++++-
 3 files changed, 896 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/selftests/drm_cmdline_selftests.h
 create mode 100644 drivers/gpu/drm/selftests/test-drm_cmdline_parser.c

diff --git a/drivers/gpu/drm/selftests/Makefile b/drivers/gpu/drm/selftests/Makefile
index 1bb73dc4c88c..fdd4238cb34a 100644
--- a/drivers/gpu/drm/selftests/Makefile
+++ b/drivers/gpu/drm/selftests/Makefile
@@ -2,4 +2,4 @@ test-drm_modeset-y := test-drm_modeset_common.o test-drm_plane_helper.o \
                       test-drm_format.o test-drm_framebuffer.o \
 		      test-drm_damage_helper.o
 
-obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o
+obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o test-drm_cmdline_parser.o
diff --git a/drivers/gpu/drm/selftests/drm_cmdline_selftests.h b/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
new file mode 100644
index 000000000000..6f9ba9459bec
--- /dev/null
+++ b/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* List each unit test as selftest(function)
+ *
+ * The name is used as both an enum and expanded as igt__name to create
+ * a module parameter. It must be unique and legal for a C identifier.
+ *
+ * Tests are executed in order by igt/drm_mm
+ */
+
+#define cmdline_test(test)	selftest(test, test)
+
+cmdline_test(drm_cmdline_test_res)
+cmdline_test(drm_cmdline_test_res_vesa)
+cmdline_test(drm_cmdline_test_res_vesa_rblank)
+cmdline_test(drm_cmdline_test_res_rblank)
+cmdline_test(drm_cmdline_test_res_bpp)
+cmdline_test(drm_cmdline_test_res_refresh)
+cmdline_test(drm_cmdline_test_res_bpp_refresh)
+cmdline_test(drm_cmdline_test_res_bpp_refresh_interlaced)
+cmdline_test(drm_cmdline_test_res_bpp_refresh_margins)
+cmdline_test(drm_cmdline_test_res_bpp_refresh_force_off)
+cmdline_test(drm_cmdline_test_res_bpp_refresh_force_on_off)
+cmdline_test(drm_cmdline_test_res_bpp_refresh_force_on)
+cmdline_test(drm_cmdline_test_res_bpp_refresh_force_on_analog)
+cmdline_test(drm_cmdline_test_res_bpp_refresh_force_on_digital)
+cmdline_test(drm_cmdline_test_res_bpp_refresh_interlaced_margins_force_on)
+cmdline_test(drm_cmdline_test_res_margins_force_on)
+cmdline_test(drm_cmdline_test_res_vesa_margins)
+cmdline_test(drm_cmdline_test_res_invalid_mode)
+cmdline_test(drm_cmdline_test_res_bpp_wrong_place_mode)
+cmdline_test(drm_cmdline_test_name)
+cmdline_test(drm_cmdline_test_name_bpp)
+cmdline_test(drm_cmdline_test_name_refresh)
+cmdline_test(drm_cmdline_test_name_bpp_refresh)
+cmdline_test(drm_cmdline_test_name_refresh_wrong_mode)
+cmdline_test(drm_cmdline_test_name_refresh_invalid_mode)
+cmdline_test(drm_cmdline_test_name_option)
+cmdline_test(drm_cmdline_test_name_bpp_option)
+cmdline_test(drm_cmdline_test_rotate_0)
+cmdline_test(drm_cmdline_test_rotate_90)
+cmdline_test(drm_cmdline_test_rotate_180)
+cmdline_test(drm_cmdline_test_rotate_270)
+cmdline_test(drm_cmdline_test_rotate_invalid_val)
+cmdline_test(drm_cmdline_test_rotate_truncated)
+cmdline_test(drm_cmdline_test_hmirror)
+cmdline_test(drm_cmdline_test_vmirror)
+cmdline_test(drm_cmdline_test_margin_options)
+cmdline_test(drm_cmdline_test_multiple_options)
+cmdline_test(drm_cmdline_test_invalid_option)
diff --git a/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c b/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c
new file mode 100644
index 000000000000..290d32cd9a07
--- /dev/null
+++ b/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c
@@ -0,0 +1,846 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019 Bootlin
+ */
+
+#define pr_fmt(fmt) "drm_cmdline: " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#include <drm/drm_connector.h>
+#include <drm/drm_modes.h>
+
+#define TESTS "drm_cmdline_selftests.h"
+#include "drm_selftest.h"
+#include "test-drm_modeset_common.h"
+
+static int drm_cmdline_test_res(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_vesa(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480M",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_vesa_rblank(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480MR",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_rblank(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480R",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_bpp(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480-24",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	FAIL_ON(mode.refresh_specified);
+
+	FAIL_ON(!mode.bpp_specified);
+	FAIL_ON(mode.bpp != 24);
+
+	FAIL_ON(mode.rb);
+	FAIL_ON(mode.cvt);
+	FAIL_ON(mode.interlace);
+	FAIL_ON(mode.margins);
+	FAIL_ON(mode.force != DRM_FORCE_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_refresh(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480@60",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	FAIL_ON(!mode.refresh_specified);
+	FAIL_ON(mode.refresh != 60);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_bpp_refresh(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480-24@60",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	FAIL_ON(!mode.refresh_specified);
+	FAIL_ON(mode.refresh != 60);
+
+	FAIL_ON(!mode.bpp_specified);
+	FAIL_ON(mode.bpp != 24);
+
+	FAIL_ON(mode.rb);
+	FAIL_ON(mode.cvt);
+	FAIL_ON(mode.interlace);
+	FAIL_ON(mode.margins);
+	FAIL_ON(mode.force != DRM_FORCE_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_bpp_refresh_interlaced(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480-24@60i",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	FAIL_ON(!mode.refresh_specified);
+	FAIL_ON(mode.refresh != 60);
+
+	FAIL_ON(!mode.bpp_specified);
+	FAIL_ON(mode.bpp != 24);
+
+	FAIL_ON(mode.rb);
+	FAIL_ON(mode.cvt);
+	FAIL_ON(!mode.interlace);
+	FAIL_ON(mode.margins);
+	FAIL_ON(mode.force != DRM_FORCE_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_bpp_refresh_margins(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480-24@60m",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	FAIL_ON(!mode.refresh_specified);
+	FAIL_ON(mode.refresh != 60);
+
+	FAIL_ON(!mode.bpp_specified);
+	FAIL_ON(mode.bpp != 24);
+
+	FAIL_ON(mode.rb);
+	FAIL_ON(mode.cvt);
+	FAIL_ON(mode.interlace);
+	FAIL_ON(!mode.margins);
+	FAIL_ON(mode.force != DRM_FORCE_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_bpp_refresh_force_off(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480-24@60d",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	FAIL_ON(!mode.refresh_specified);
+	FAIL_ON(mode.refresh != 60);
+
+	FAIL_ON(!mode.bpp_specified);
+	FAIL_ON(mode.bpp != 24);
+
+	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_res_bpp_refresh_force_on_off(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(drm_mode_parse_command_line_for_connector("720x480-24@60de",
+							  &connector,
+							  &mode));
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_bpp_refresh_force_on(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480-24@60e",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	FAIL_ON(!mode.refresh_specified);
+	FAIL_ON(mode.refresh != 60);
+
+	FAIL_ON(!mode.bpp_specified);
+	FAIL_ON(mode.bpp != 24);
+
+	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_res_bpp_refresh_force_on_analog(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480-24@60D",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	FAIL_ON(!mode.refresh_specified);
+	FAIL_ON(mode.refresh != 60);
+
+	FAIL_ON(!mode.bpp_specified);
+	FAIL_ON(mode.bpp != 24);
+
+	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_res_bpp_refresh_force_on_digital(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	connector.connector_type = DRM_MODE_CONNECTOR_DVII;
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480-24@60D",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	FAIL_ON(!mode.refresh_specified);
+	FAIL_ON(mode.refresh != 60);
+
+	FAIL_ON(!mode.bpp_specified);
+	FAIL_ON(mode.bpp != 24);
+
+	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_res_bpp_refresh_interlaced_margins_force_on(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480-24@60ime",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	FAIL_ON(!mode.refresh_specified);
+	FAIL_ON(mode.refresh != 60);
+
+	FAIL_ON(!mode.bpp_specified);
+	FAIL_ON(mode.bpp != 24);
+
+	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_res_margins_force_on(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480me",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	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_res_vesa_margins(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480Mm",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_invalid_mode(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(drm_mode_parse_command_line_for_connector("720x480f",
+							  &connector,
+							  &mode));
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_bpp_wrong_place_mode(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(drm_mode_parse_command_line_for_connector("720x480e-24",
+							  &connector,
+							  &mode));
+
+	return 0;
+}
+
+static int drm_cmdline_test_name(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("NTSC",
+							   &connector,
+							   &mode));
+	FAIL_ON(strcmp(mode.name, "NTSC"));
+	FAIL_ON(mode.refresh_specified);
+	FAIL_ON(mode.bpp_specified);
+
+	return 0;
+}
+
+static int drm_cmdline_test_name_bpp(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("NTSC-24",
+							   &connector,
+							   &mode));
+	FAIL_ON(strcmp(mode.name, "NTSC"));
+
+	FAIL_ON(mode.refresh_specified);
+
+	FAIL_ON(!mode.bpp_specified);
+	FAIL_ON(mode.bpp != 24);
+
+	return 0;
+}
+
+static int drm_cmdline_test_name_bpp_refresh(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(drm_mode_parse_command_line_for_connector("NTSC-24@60",
+							  &connector,
+							  &mode));
+
+	return 0;
+}
+
+static int drm_cmdline_test_name_refresh(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(drm_mode_parse_command_line_for_connector("NTSC@60",
+							  &connector,
+							  &mode));
+
+	return 0;
+}
+
+static int drm_cmdline_test_name_refresh_wrong_mode(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(drm_mode_parse_command_line_for_connector("NTSC@60m",
+							  &connector,
+							  &mode));
+
+	return 0;
+}
+
+static int drm_cmdline_test_name_refresh_invalid_mode(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(drm_mode_parse_command_line_for_connector("NTSC@60f",
+							  &connector,
+							  &mode));
+
+	return 0;
+}
+
+static int drm_cmdline_test_name_option(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("NTSC,rotate=180",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(strcmp(mode.name, "NTSC"));
+	FAIL_ON(mode.rotation != DRM_MODE_ROTATE_180);
+
+	return 0;
+}
+
+static int drm_cmdline_test_name_bpp_option(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("NTSC-24,rotate=180",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(strcmp(mode.name, "NTSC"));
+	FAIL_ON(mode.rotation != DRM_MODE_ROTATE_180);
+	FAIL_ON(!mode.bpp_specified);
+	FAIL_ON(mode.bpp != 24);
+
+	return 0;
+}
+
+static int drm_cmdline_test_rotate_0(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480,rotate=0",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+	FAIL_ON(mode.rotation != DRM_MODE_ROTATE_0);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_rotate_90(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480,rotate=90",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+	FAIL_ON(mode.rotation != DRM_MODE_ROTATE_90);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_rotate_180(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480,rotate=180",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+	FAIL_ON(mode.rotation != DRM_MODE_ROTATE_180);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_rotate_270(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480,rotate=270",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+	FAIL_ON(mode.rotation != DRM_MODE_ROTATE_270);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_rotate_invalid_val(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(drm_mode_parse_command_line_for_connector("720x480,rotate=42",
+							  &connector,
+							  &mode));
+
+	return 0;
+}
+
+static int drm_cmdline_test_rotate_truncated(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(drm_mode_parse_command_line_for_connector("720x480,rotate=",
+							  &connector,
+							  &mode));
+
+	return 0;
+}
+
+static int drm_cmdline_test_hmirror(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480,reflect_x",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+	FAIL_ON(mode.rotation != DRM_MODE_REFLECT_X);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_vmirror(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480,reflect_y",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+	FAIL_ON(mode.rotation != DRM_MODE_REFLECT_Y);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_margin_options(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480,margin_right=14,margin_left=24,margin_bottom=36,margin_top=42",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+	FAIL_ON(mode.tv_margins.right != 14);
+	FAIL_ON(mode.tv_margins.left != 24);
+	FAIL_ON(mode.tv_margins.bottom != 36);
+	FAIL_ON(mode.tv_margins.top != 42);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_multiple_options(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480,rotate=270,reflect_x",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+	FAIL_ON(mode.rotation != (DRM_MODE_ROTATE_270 | DRM_MODE_REFLECT_X));
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_invalid_option(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(drm_mode_parse_command_line_for_connector("720x480,test=42",
+							  &connector,
+							  &mode));
+
+	return 0;
+}
+
+#include "drm_selftest.c"
+
+static int __init test_drm_cmdline_init(void)
+{
+	int err;
+
+	err = run_selftests(selftests, ARRAY_SIZE(selftests), NULL);
+
+	return err > 0 ? 0 : err;
+}
+module_init(test_drm_cmdline_init);
+
+MODULE_AUTHOR("Maxime Ripard <maxime.ripard@bootlin.com>");
+MODULE_LICENSE("GPL");
-- 
git-series 0.9.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 5/6] drm/selftests: Add command line parser selftests

[Maxime Ripard]

The command line parser is pretty tough to get right and very error prone,
so let's add a selftest to try to catch any regression.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/selftests/Makefile                  |   2 +-
 drivers/gpu/drm/selftests/drm_cmdline_selftests.h   |  49 +-
 drivers/gpu/drm/selftests/test-drm_cmdline_parser.c | 846 +++++++++++++-
 3 files changed, 896 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/selftests/drm_cmdline_selftests.h
 create mode 100644 drivers/gpu/drm/selftests/test-drm_cmdline_parser.c

diff --git a/drivers/gpu/drm/selftests/Makefile b/drivers/gpu/drm/selftests/Makefile
index 1bb73dc4c88c..fdd4238cb34a 100644
--- a/drivers/gpu/drm/selftests/Makefile
+++ b/drivers/gpu/drm/selftests/Makefile
@@ -2,4 +2,4 @@ test-drm_modeset-y := test-drm_modeset_common.o test-drm_plane_helper.o \
                       test-drm_format.o test-drm_framebuffer.o \
 		      test-drm_damage_helper.o
 
-obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o
+obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o test-drm_cmdline_parser.o
diff --git a/drivers/gpu/drm/selftests/drm_cmdline_selftests.h b/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
new file mode 100644
index 000000000000..6f9ba9459bec
--- /dev/null
+++ b/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* List each unit test as selftest(function)
+ *
+ * The name is used as both an enum and expanded as igt__name to create
+ * a module parameter. It must be unique and legal for a C identifier.
+ *
+ * Tests are executed in order by igt/drm_mm
+ */
+
+#define cmdline_test(test)	selftest(test, test)
+
+cmdline_test(drm_cmdline_test_res)
+cmdline_test(drm_cmdline_test_res_vesa)
+cmdline_test(drm_cmdline_test_res_vesa_rblank)
+cmdline_test(drm_cmdline_test_res_rblank)
+cmdline_test(drm_cmdline_test_res_bpp)
+cmdline_test(drm_cmdline_test_res_refresh)
+cmdline_test(drm_cmdline_test_res_bpp_refresh)
+cmdline_test(drm_cmdline_test_res_bpp_refresh_interlaced)
+cmdline_test(drm_cmdline_test_res_bpp_refresh_margins)
+cmdline_test(drm_cmdline_test_res_bpp_refresh_force_off)
+cmdline_test(drm_cmdline_test_res_bpp_refresh_force_on_off)
+cmdline_test(drm_cmdline_test_res_bpp_refresh_force_on)
+cmdline_test(drm_cmdline_test_res_bpp_refresh_force_on_analog)
+cmdline_test(drm_cmdline_test_res_bpp_refresh_force_on_digital)
+cmdline_test(drm_cmdline_test_res_bpp_refresh_interlaced_margins_force_on)
+cmdline_test(drm_cmdline_test_res_margins_force_on)
+cmdline_test(drm_cmdline_test_res_vesa_margins)
+cmdline_test(drm_cmdline_test_res_invalid_mode)
+cmdline_test(drm_cmdline_test_res_bpp_wrong_place_mode)
+cmdline_test(drm_cmdline_test_name)
+cmdline_test(drm_cmdline_test_name_bpp)
+cmdline_test(drm_cmdline_test_name_refresh)
+cmdline_test(drm_cmdline_test_name_bpp_refresh)
+cmdline_test(drm_cmdline_test_name_refresh_wrong_mode)
+cmdline_test(drm_cmdline_test_name_refresh_invalid_mode)
+cmdline_test(drm_cmdline_test_name_option)
+cmdline_test(drm_cmdline_test_name_bpp_option)
+cmdline_test(drm_cmdline_test_rotate_0)
+cmdline_test(drm_cmdline_test_rotate_90)
+cmdline_test(drm_cmdline_test_rotate_180)
+cmdline_test(drm_cmdline_test_rotate_270)
+cmdline_test(drm_cmdline_test_rotate_invalid_val)
+cmdline_test(drm_cmdline_test_rotate_truncated)
+cmdline_test(drm_cmdline_test_hmirror)
+cmdline_test(drm_cmdline_test_vmirror)
+cmdline_test(drm_cmdline_test_margin_options)
+cmdline_test(drm_cmdline_test_multiple_options)
+cmdline_test(drm_cmdline_test_invalid_option)
diff --git a/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c b/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c
new file mode 100644
index 000000000000..290d32cd9a07
--- /dev/null
+++ b/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c
@@ -0,0 +1,846 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019 Bootlin
+ */
+
+#define pr_fmt(fmt) "drm_cmdline: " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#include <drm/drm_connector.h>
+#include <drm/drm_modes.h>
+
+#define TESTS "drm_cmdline_selftests.h"
+#include "drm_selftest.h"
+#include "test-drm_modeset_common.h"
+
+static int drm_cmdline_test_res(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_vesa(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480M",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_vesa_rblank(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480MR",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_rblank(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480R",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_bpp(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480-24",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	FAIL_ON(mode.refresh_specified);
+
+	FAIL_ON(!mode.bpp_specified);
+	FAIL_ON(mode.bpp != 24);
+
+	FAIL_ON(mode.rb);
+	FAIL_ON(mode.cvt);
+	FAIL_ON(mode.interlace);
+	FAIL_ON(mode.margins);
+	FAIL_ON(mode.force != DRM_FORCE_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_refresh(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480@60",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	FAIL_ON(!mode.refresh_specified);
+	FAIL_ON(mode.refresh != 60);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_bpp_refresh(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480-24@60",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	FAIL_ON(!mode.refresh_specified);
+	FAIL_ON(mode.refresh != 60);
+
+	FAIL_ON(!mode.bpp_specified);
+	FAIL_ON(mode.bpp != 24);
+
+	FAIL_ON(mode.rb);
+	FAIL_ON(mode.cvt);
+	FAIL_ON(mode.interlace);
+	FAIL_ON(mode.margins);
+	FAIL_ON(mode.force != DRM_FORCE_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_bpp_refresh_interlaced(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480-24@60i",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	FAIL_ON(!mode.refresh_specified);
+	FAIL_ON(mode.refresh != 60);
+
+	FAIL_ON(!mode.bpp_specified);
+	FAIL_ON(mode.bpp != 24);
+
+	FAIL_ON(mode.rb);
+	FAIL_ON(mode.cvt);
+	FAIL_ON(!mode.interlace);
+	FAIL_ON(mode.margins);
+	FAIL_ON(mode.force != DRM_FORCE_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_bpp_refresh_margins(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480-24@60m",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	FAIL_ON(!mode.refresh_specified);
+	FAIL_ON(mode.refresh != 60);
+
+	FAIL_ON(!mode.bpp_specified);
+	FAIL_ON(mode.bpp != 24);
+
+	FAIL_ON(mode.rb);
+	FAIL_ON(mode.cvt);
+	FAIL_ON(mode.interlace);
+	FAIL_ON(!mode.margins);
+	FAIL_ON(mode.force != DRM_FORCE_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_bpp_refresh_force_off(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480-24@60d",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	FAIL_ON(!mode.refresh_specified);
+	FAIL_ON(mode.refresh != 60);
+
+	FAIL_ON(!mode.bpp_specified);
+	FAIL_ON(mode.bpp != 24);
+
+	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_res_bpp_refresh_force_on_off(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(drm_mode_parse_command_line_for_connector("720x480-24@60de",
+							  &connector,
+							  &mode));
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_bpp_refresh_force_on(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480-24@60e",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	FAIL_ON(!mode.refresh_specified);
+	FAIL_ON(mode.refresh != 60);
+
+	FAIL_ON(!mode.bpp_specified);
+	FAIL_ON(mode.bpp != 24);
+
+	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_res_bpp_refresh_force_on_analog(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480-24@60D",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	FAIL_ON(!mode.refresh_specified);
+	FAIL_ON(mode.refresh != 60);
+
+	FAIL_ON(!mode.bpp_specified);
+	FAIL_ON(mode.bpp != 24);
+
+	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_res_bpp_refresh_force_on_digital(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	connector.connector_type = DRM_MODE_CONNECTOR_DVII;
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480-24@60D",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	FAIL_ON(!mode.refresh_specified);
+	FAIL_ON(mode.refresh != 60);
+
+	FAIL_ON(!mode.bpp_specified);
+	FAIL_ON(mode.bpp != 24);
+
+	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_res_bpp_refresh_interlaced_margins_force_on(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480-24@60ime",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	FAIL_ON(!mode.refresh_specified);
+	FAIL_ON(mode.refresh != 60);
+
+	FAIL_ON(!mode.bpp_specified);
+	FAIL_ON(mode.bpp != 24);
+
+	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_res_margins_force_on(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480me",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	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_res_vesa_margins(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480Mm",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_invalid_mode(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(drm_mode_parse_command_line_for_connector("720x480f",
+							  &connector,
+							  &mode));
+
+	return 0;
+}
+
+static int drm_cmdline_test_res_bpp_wrong_place_mode(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(drm_mode_parse_command_line_for_connector("720x480e-24",
+							  &connector,
+							  &mode));
+
+	return 0;
+}
+
+static int drm_cmdline_test_name(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("NTSC",
+							   &connector,
+							   &mode));
+	FAIL_ON(strcmp(mode.name, "NTSC"));
+	FAIL_ON(mode.refresh_specified);
+	FAIL_ON(mode.bpp_specified);
+
+	return 0;
+}
+
+static int drm_cmdline_test_name_bpp(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("NTSC-24",
+							   &connector,
+							   &mode));
+	FAIL_ON(strcmp(mode.name, "NTSC"));
+
+	FAIL_ON(mode.refresh_specified);
+
+	FAIL_ON(!mode.bpp_specified);
+	FAIL_ON(mode.bpp != 24);
+
+	return 0;
+}
+
+static int drm_cmdline_test_name_bpp_refresh(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(drm_mode_parse_command_line_for_connector("NTSC-24@60",
+							  &connector,
+							  &mode));
+
+	return 0;
+}
+
+static int drm_cmdline_test_name_refresh(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(drm_mode_parse_command_line_for_connector("NTSC@60",
+							  &connector,
+							  &mode));
+
+	return 0;
+}
+
+static int drm_cmdline_test_name_refresh_wrong_mode(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(drm_mode_parse_command_line_for_connector("NTSC@60m",
+							  &connector,
+							  &mode));
+
+	return 0;
+}
+
+static int drm_cmdline_test_name_refresh_invalid_mode(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(drm_mode_parse_command_line_for_connector("NTSC@60f",
+							  &connector,
+							  &mode));
+
+	return 0;
+}
+
+static int drm_cmdline_test_name_option(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("NTSC,rotate=180",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(strcmp(mode.name, "NTSC"));
+	FAIL_ON(mode.rotation != DRM_MODE_ROTATE_180);
+
+	return 0;
+}
+
+static int drm_cmdline_test_name_bpp_option(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("NTSC-24,rotate=180",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(strcmp(mode.name, "NTSC"));
+	FAIL_ON(mode.rotation != DRM_MODE_ROTATE_180);
+	FAIL_ON(!mode.bpp_specified);
+	FAIL_ON(mode.bpp != 24);
+
+	return 0;
+}
+
+static int drm_cmdline_test_rotate_0(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480,rotate=0",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+	FAIL_ON(mode.rotation != DRM_MODE_ROTATE_0);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_rotate_90(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480,rotate=90",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+	FAIL_ON(mode.rotation != DRM_MODE_ROTATE_90);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_rotate_180(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480,rotate=180",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+	FAIL_ON(mode.rotation != DRM_MODE_ROTATE_180);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_rotate_270(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480,rotate=270",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+	FAIL_ON(mode.rotation != DRM_MODE_ROTATE_270);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_rotate_invalid_val(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(drm_mode_parse_command_line_for_connector("720x480,rotate=42",
+							  &connector,
+							  &mode));
+
+	return 0;
+}
+
+static int drm_cmdline_test_rotate_truncated(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(drm_mode_parse_command_line_for_connector("720x480,rotate=",
+							  &connector,
+							  &mode));
+
+	return 0;
+}
+
+static int drm_cmdline_test_hmirror(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480,reflect_x",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+	FAIL_ON(mode.rotation != DRM_MODE_REFLECT_X);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_vmirror(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480,reflect_y",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+	FAIL_ON(mode.rotation != DRM_MODE_REFLECT_Y);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_margin_options(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480,margin_right=14,margin_left=24,margin_bottom=36,margin_top=42",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+	FAIL_ON(mode.tv_margins.right != 14);
+	FAIL_ON(mode.tv_margins.left != 24);
+	FAIL_ON(mode.tv_margins.bottom != 36);
+	FAIL_ON(mode.tv_margins.top != 42);
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_multiple_options(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480,rotate=270,reflect_x",
+							   &connector,
+							   &mode));
+	FAIL_ON(!mode.specified);
+	FAIL_ON(mode.xres != 720);
+	FAIL_ON(mode.yres != 480);
+	FAIL_ON(mode.rotation != (DRM_MODE_ROTATE_270 | DRM_MODE_REFLECT_X));
+
+	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_UNSPECIFIED);
+
+	return 0;
+}
+
+static int drm_cmdline_test_invalid_option(void *ignored)
+{
+	struct drm_connector connector = { };
+	struct drm_cmdline_mode mode = { };
+
+	FAIL_ON(drm_mode_parse_command_line_for_connector("720x480,test=42",
+							  &connector,
+							  &mode));
+
+	return 0;
+}
+
+#include "drm_selftest.c"
+
+static int __init test_drm_cmdline_init(void)
+{
+	int err;
+
+	err = run_selftests(selftests, ARRAY_SIZE(selftests), NULL);
+
+	return err > 0 ? 0 : err;
+}
+module_init(test_drm_cmdline_init);
+
+MODULE_AUTHOR("Maxime Ripard <maxime.ripard@bootlin.com>");
+MODULE_LICENSE("GPL");
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 6/6] drm/vc4: hdmi: Set default state margin at reset

[Maxime Ripard]

Now that the TV margins are properly parsed and filled into
drm_cmdline_mode, we just need to initialise the first state at reset to
get those values and start using them.

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

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 99fc8569e0f5..d86f154138f5 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -255,11 +255,25 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
 	return ret;
 }
 
+static void vc4_hdmi_connector_reset(struct drm_connector *connector)
+{
+	struct drm_cmdline_mode *cmdline = &connector->cmdline_mode;
+	struct drm_connector_state *state;
+
+	drm_atomic_helper_connector_reset(connector);
+
+	state = connector->state;
+	state->tv.margins.left = cmdline->tv_margins.left;
+	state->tv.margins.right = cmdline->tv_margins.right;
+	state->tv.margins.top = cmdline->tv_margins.top;
+	state->tv.margins.bottom = cmdline->tv_margins.bottom;
+}
+
 static const struct drm_connector_funcs vc4_hdmi_connector_funcs = {
 	.detect = vc4_hdmi_connector_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.destroy = vc4_hdmi_connector_destroy,
-	.reset = drm_atomic_helper_connector_reset,
+	.reset = vc4_hdmi_connector_reset,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
-- 
git-series 0.9.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 6/6] drm/vc4: hdmi: Set default state margin at reset

[Maxime Ripard]

Now that the TV margins are properly parsed and filled into
drm_cmdline_mode, we just need to initialise the first state at reset to
get those values and start using them.

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

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 99fc8569e0f5..d86f154138f5 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -255,11 +255,25 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
 	return ret;
 }
 
+static void vc4_hdmi_connector_reset(struct drm_connector *connector)
+{
+	struct drm_cmdline_mode *cmdline = &connector->cmdline_mode;
+	struct drm_connector_state *state;
+
+	drm_atomic_helper_connector_reset(connector);
+
+	state = connector->state;
+	state->tv.margins.left = cmdline->tv_margins.left;
+	state->tv.margins.right = cmdline->tv_margins.right;
+	state->tv.margins.top = cmdline->tv_margins.top;
+	state->tv.margins.bottom = cmdline->tv_margins.bottom;
+}
+
 static const struct drm_connector_funcs vc4_hdmi_connector_funcs = {
 	.detect = vc4_hdmi_connector_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.destroy = vc4_hdmi_connector_destroy,
-	.reset = drm_atomic_helper_connector_reset,
+	.reset = vc4_hdmi_connector_reset,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
-- 
git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/6] drm/modes: Rewrite the command line parser

[Noralf Trønnes]

Den 18.04.2019 14.41, skrev 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.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/gpu/drm/drm_modes.c | 305 +++++++++++++++++++++++--------------
>  1 file changed, 190 insertions(+), 115 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 56f92a0bba62..3f89198f0891 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>
> @@ -1405,6 +1406,131 @@ 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)
> +{
> +	if (str[0] != '-')
> +		return -EINVAL;
> +
> +	mode->bpp = simple_strtol(str + 1, end_ptr, 10);

What happens if this is not a number? I didn't find a test for that in
your unit tests. The same goes for _refresh().

> +	mode->bpp_specified = true;
> +
> +	return 0;
> +}
> +
> +static int drm_mode_parse_cmdline_refresh(const char *str, char **end_ptr,
> +					  struct drm_cmdline_mode *mode)
> +{
> +	if (str[0] != '@')
> +		return -EINVAL;
> +
> +	mode->refresh = simple_strtol(str + 1, end_ptr, 10);
> +	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)
> +{
> +	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[0] != 'x')

'x600' looks to be a valid resolution here, so I think:
	if (str == end_ptr || end_ptr[0] != 'x')

> +		return -EINVAL;
> +	end_ptr++;
> +
> +	yres = simple_strtol(end_ptr, &end_ptr, 10);
> +
> +	remaining = length - (end_ptr - str);
> +	if (remaining < 0)
> +		return -EINVAL;

Maybe some unit tests: '1024xy' and '1024x', to verify that this does
indeed require a number for yres.

Noralf.

> +
> +	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
> @@ -1431,13 +1557,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)
> @@ -1450,127 +1575,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;
>  }
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/6] drm/modes: Rewrite the command line parser

[Noralf Trønnes]

Den 18.04.2019 14.41, skrev 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.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/gpu/drm/drm_modes.c | 305 +++++++++++++++++++++++--------------
>  1 file changed, 190 insertions(+), 115 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 56f92a0bba62..3f89198f0891 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>
> @@ -1405,6 +1406,131 @@ 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)
> +{
> +	if (str[0] != '-')
> +		return -EINVAL;
> +
> +	mode->bpp = simple_strtol(str + 1, end_ptr, 10);

What happens if this is not a number? I didn't find a test for that in
your unit tests. The same goes for _refresh().

> +	mode->bpp_specified = true;
> +
> +	return 0;
> +}
> +
> +static int drm_mode_parse_cmdline_refresh(const char *str, char **end_ptr,
> +					  struct drm_cmdline_mode *mode)
> +{
> +	if (str[0] != '@')
> +		return -EINVAL;
> +
> +	mode->refresh = simple_strtol(str + 1, end_ptr, 10);
> +	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)
> +{
> +	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[0] != 'x')

'x600' looks to be a valid resolution here, so I think:
	if (str == end_ptr || end_ptr[0] != 'x')

> +		return -EINVAL;
> +	end_ptr++;
> +
> +	yres = simple_strtol(end_ptr, &end_ptr, 10);
> +
> +	remaining = length - (end_ptr - str);
> +	if (remaining < 0)
> +		return -EINVAL;

Maybe some unit tests: '1024xy' and '1024x', to verify that this does
indeed require a number for yres.

Noralf.

> +
> +	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
> @@ -1431,13 +1557,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)
> @@ -1450,127 +1575,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;
>  }
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 2/6] drm/modes: Support modes names on the command line

[Noralf Trønnes]

Den 18.04.2019 14.41, skrev Maxime Ripard:
> From: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> The drm subsystem also uses the video= kernel parameter, and in the
> documentation refers to the fbdev documentation for that parameter.
> 
> However, that documentation also says that instead of giving the mode using
> its resolution we can also give a name. However, DRM doesn't handle that
> case at the moment. Even though in most case it shouldn't make any
> difference, it might be useful for analog modes, where different standards
> might have the same resolution, but still have a few different parameters
> that are not encoded in the modes (NTSC vs NTSC-J vs PAL-M for example).
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/gpu/drm/drm_client_modeset.c |  4 ++-
>  drivers/gpu/drm/drm_connector.c      |  3 +-
>  drivers/gpu/drm/drm_modes.c          | 52 ++++++++++++++++++++---------
>  include/drm/drm_connector.h          |  1 +-
>  4 files changed, 44 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> index 2a428ac00930..f2869c82510c 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -149,6 +149,10 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector)
>  	prefer_non_interlace = !cmdline_mode->interlace;
>  again:
>  	list_for_each_entry(mode, &connector->modes, head) {
> +		/* Check (optional) mode name first */
> +		if (!strcmp(mode->name, cmdline_mode->name))
> +			return mode;
> +
>  		/* check width/height */
>  		if (mode->hdisplay != cmdline_mode->xres ||
>  		    mode->vdisplay != cmdline_mode->yres)
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 2355124849db..e33814f5940e 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -139,8 +139,9 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
>  		connector->force = mode->force;
>  	}
>  
> -	DRM_DEBUG_KMS("cmdline mode for connector %s %dx%d@%dHz%s%s%s\n",
> +	DRM_DEBUG_KMS("cmdline mode for connector %s %s %dx%d@%dHz%s%s%s\n",
>  		      connector->name,
> +		      mode->name ? mode->name : "",
>  		      mode->xres, mode->yres,
>  		      mode->refresh_specified ? mode->refresh : 60,
>  		      mode->rb ? " reduced blanking" : "",
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 3f89198f0891..9613c1a28487 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1557,7 +1557,7 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>  					       struct drm_cmdline_mode *mode)
>  {
>  	const char *name;
> -	bool parse_extras = false;
> +	bool named_mode = false, parse_extras = false;

You don't need to assign a value here, or if you do, you can drop the
else further down.

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

>  	unsigned int bpp_off = 0, refresh_off = 0;
>  	unsigned int mode_end = 0;
>  	char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
> @@ -1576,8 +1576,14 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>  
>  	name = mode_option;
>  
> +	/*
> +	 * If the first character is not a digit, then it means that
> +	 * we have a named mode.
> +	 */
>  	if (!isdigit(name[0]))
> -		return false;
> +		named_mode = true;
> +	else
> +		named_mode = false;
>  
>  	/* Try to locate the bpp and refresh specifiers, if any */
>  	bpp_ptr = strchr(name, '-');
> @@ -1588,6 +1594,9 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>  
>  	refresh_ptr = strchr(name, '@');
>  	if (refresh_ptr) {
> +		if (named_mode)
> +			return false;
> +
>  		refresh_off = refresh_ptr - name;
>  		mode->refresh_specified = true;
>  	}
> @@ -1604,12 +1613,16 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>  		parse_extras = true;
>  	}
>  
> -	ret = drm_mode_parse_cmdline_res_mode(name, mode_end,
> -					      parse_extras,
> -					      connector,
> -					      mode);
> -	if (ret)
> -		return false;
> +	if (named_mode) {
> +		strncpy(mode->name, name, mode_end);
> +	} else {
> +		ret = drm_mode_parse_cmdline_res_mode(name, mode_end,
> +						      parse_extras,
> +						      connector,
> +						      mode);
> +		if (ret)
> +			return false;
> +	}
>  	mode->specified = true;
>  
>  	if (bpp_ptr) {
> @@ -1637,14 +1650,23 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>  		extra_ptr = refresh_end_ptr;
>  
>  	if (extra_ptr) {
> -		int remaining = strlen(name) - (extra_ptr - name);
> +		if (!named_mode) {
> +			int len = strlen(name) - (extra_ptr - name);
>  
> -		/*
> -		 * We still have characters to process, while
> -		 * we shouldn't have any
> -		 */
> -		if (remaining > 0)
> -			return false;
> +			ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
> +							   connector, mode);
> +			if (ret)
> +				return false;
> +		} else {
> +			int remaining = strlen(name) - (extra_ptr - name);
> +
> +			/*
> +			 * We still have characters to process, while
> +			 * we shouldn't have any
> +			 */
> +			if (remaining > 0)
> +				return false;
> +		}
>  	}
>  
>  	return true;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 02a131202add..06aa3b9cb920 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -896,6 +896,7 @@ struct drm_connector_funcs {
>  
>  /* mode specified on the command line */
>  struct drm_cmdline_mode {
> +	char name[DRM_DISPLAY_MODE_LEN];
>  	bool specified;
>  	bool refresh_specified;
>  	bool bpp_specified;
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/6] drm/modes: Support modes names on the command line

[Noralf Trønnes]

Den 18.04.2019 14.41, skrev Maxime Ripard:
> From: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> The drm subsystem also uses the video= kernel parameter, and in the
> documentation refers to the fbdev documentation for that parameter.
> 
> However, that documentation also says that instead of giving the mode using
> its resolution we can also give a name. However, DRM doesn't handle that
> case at the moment. Even though in most case it shouldn't make any
> difference, it might be useful for analog modes, where different standards
> might have the same resolution, but still have a few different parameters
> that are not encoded in the modes (NTSC vs NTSC-J vs PAL-M for example).
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/gpu/drm/drm_client_modeset.c |  4 ++-
>  drivers/gpu/drm/drm_connector.c      |  3 +-
>  drivers/gpu/drm/drm_modes.c          | 52 ++++++++++++++++++++---------
>  include/drm/drm_connector.h          |  1 +-
>  4 files changed, 44 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> index 2a428ac00930..f2869c82510c 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -149,6 +149,10 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector)
>  	prefer_non_interlace = !cmdline_mode->interlace;
>  again:
>  	list_for_each_entry(mode, &connector->modes, head) {
> +		/* Check (optional) mode name first */
> +		if (!strcmp(mode->name, cmdline_mode->name))
> +			return mode;
> +
>  		/* check width/height */
>  		if (mode->hdisplay != cmdline_mode->xres ||
>  		    mode->vdisplay != cmdline_mode->yres)
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 2355124849db..e33814f5940e 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -139,8 +139,9 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
>  		connector->force = mode->force;
>  	}
>  
> -	DRM_DEBUG_KMS("cmdline mode for connector %s %dx%d@%dHz%s%s%s\n",
> +	DRM_DEBUG_KMS("cmdline mode for connector %s %s %dx%d@%dHz%s%s%s\n",
>  		      connector->name,
> +		      mode->name ? mode->name : "",
>  		      mode->xres, mode->yres,
>  		      mode->refresh_specified ? mode->refresh : 60,
>  		      mode->rb ? " reduced blanking" : "",
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 3f89198f0891..9613c1a28487 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1557,7 +1557,7 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>  					       struct drm_cmdline_mode *mode)
>  {
>  	const char *name;
> -	bool parse_extras = false;
> +	bool named_mode = false, parse_extras = false;

You don't need to assign a value here, or if you do, you can drop the
else further down.

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

>  	unsigned int bpp_off = 0, refresh_off = 0;
>  	unsigned int mode_end = 0;
>  	char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
> @@ -1576,8 +1576,14 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>  
>  	name = mode_option;
>  
> +	/*
> +	 * If the first character is not a digit, then it means that
> +	 * we have a named mode.
> +	 */
>  	if (!isdigit(name[0]))
> -		return false;
> +		named_mode = true;
> +	else
> +		named_mode = false;
>  
>  	/* Try to locate the bpp and refresh specifiers, if any */
>  	bpp_ptr = strchr(name, '-');
> @@ -1588,6 +1594,9 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>  
>  	refresh_ptr = strchr(name, '@');
>  	if (refresh_ptr) {
> +		if (named_mode)
> +			return false;
> +
>  		refresh_off = refresh_ptr - name;
>  		mode->refresh_specified = true;
>  	}
> @@ -1604,12 +1613,16 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>  		parse_extras = true;
>  	}
>  
> -	ret = drm_mode_parse_cmdline_res_mode(name, mode_end,
> -					      parse_extras,
> -					      connector,
> -					      mode);
> -	if (ret)
> -		return false;
> +	if (named_mode) {
> +		strncpy(mode->name, name, mode_end);
> +	} else {
> +		ret = drm_mode_parse_cmdline_res_mode(name, mode_end,
> +						      parse_extras,
> +						      connector,
> +						      mode);
> +		if (ret)
> +			return false;
> +	}
>  	mode->specified = true;
>  
>  	if (bpp_ptr) {
> @@ -1637,14 +1650,23 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>  		extra_ptr = refresh_end_ptr;
>  
>  	if (extra_ptr) {
> -		int remaining = strlen(name) - (extra_ptr - name);
> +		if (!named_mode) {
> +			int len = strlen(name) - (extra_ptr - name);
>  
> -		/*
> -		 * We still have characters to process, while
> -		 * we shouldn't have any
> -		 */
> -		if (remaining > 0)
> -			return false;
> +			ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
> +							   connector, mode);
> +			if (ret)
> +				return false;
> +		} else {
> +			int remaining = strlen(name) - (extra_ptr - name);
> +
> +			/*
> +			 * We still have characters to process, while
> +			 * we shouldn't have any
> +			 */
> +			if (remaining > 0)
> +				return false;
> +		}
>  	}
>  
>  	return true;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 02a131202add..06aa3b9cb920 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -896,6 +896,7 @@ struct drm_connector_funcs {
>  
>  /* mode specified on the command line */
>  struct drm_cmdline_mode {
> +	char name[DRM_DISPLAY_MODE_LEN];
>  	bool specified;
>  	bool refresh_specified;
>  	bool bpp_specified;
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/6] drm/modes: Allow to specify rotation and reflection on the commandline

[Noralf Trønnes]

Den 18.04.2019 14.41, skrev Maxime Ripard:
> Rotations and reflections setup are needed in some scenarios to initialise
> properly the initial framebuffer. Some drivers already had a bunch of
> quirks to deal with this, such as either a private kernel command line
> parameter (omapdss) or on the device tree (various panels).
> 
> In order to accomodate this, let's create a video mode parameter to deal
> with the rotation and reflexion.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  drivers/gpu/drm/drm_client_modeset.c |  10 +++-
>  drivers/gpu/drm/drm_modes.c          | 110 ++++++++++++++++++++++------
>  include/drm/drm_connector.h          |   9 ++-
>  3 files changed, 109 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> index f2869c82510c..15145d2c86d5 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -823,6 +823,7 @@ EXPORT_SYMBOL(drm_client_modeset_probe);
>  bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotation)
>  {
>  	struct drm_connector *connector = modeset->connectors[0];
> +	struct drm_cmdline_mode *cmdline;
>  	struct drm_plane *plane = modeset->crtc->primary;
>  	u64 valid_mask = 0;
>  	unsigned int i;
> @@ -844,6 +845,15 @@ bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotat
>  		*rotation = DRM_MODE_ROTATE_0;
>  	}
>  
> +	/**
> +	 * We want the rotation on the command line to overwrite
> +	 * whatever comes from the panel.
> +	 */
> +	cmdline = &connector->cmdline_mode;
> +	if (cmdline->specified &&
> +	    cmdline->rotation != DRM_MODE_ROTATE_0)

I believe you need to drop that second check, otherwise rotate=0 will
not overwrite panel rotation.

> +		*rotation = cmdline->rotation;
> +
>  	/*
>  	 * TODO: support 90 / 270 degree hardware rotation,
>  	 * depending on the hardware this may require the framebuffer
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 9613c1a28487..ac8d70b92b62 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1531,6 +1531,71 @@ static int drm_mode_parse_cmdline_res_mode(const char *str, unsigned int length,
>  	return 0;
>  }
>  
> +static int drm_mode_parse_cmdline_options(char *str, size_t len,
> +					  struct drm_connector *connector,
> +					  struct drm_cmdline_mode *mode)
> +{
> +	unsigned int rotation = 0;
> +	char *sep = str;
> +
> +	while ((sep = strchr(sep, ','))) {
> +		char *delim, *option;
> +
> +		option = sep + 1;
> +		delim = strchr(option, '=');
> +		if (!delim) {
> +			delim = strchr(option, ',');
> +
> +			if (!delim)
> +				delim = str + len;
> +		}
> +
> +		if (!strncmp(option, "rotate", delim - option)) {
> +			const char *value = delim + 1;
> +			unsigned int deg;
> +
> +			deg = simple_strtol(value, &sep, 10);
> +
> +			/* Make sure we have parsed something */
> +			if (sep == value)
> +				return -EINVAL;
> +
> +			switch (deg) {
> +			case 0:
> +				rotation |= DRM_MODE_ROTATE_0;
> +				break;
> +
> +			case 90:
> +				rotation |= DRM_MODE_ROTATE_90;
> +				break;
> +
> +			case 180:
> +				rotation |= DRM_MODE_ROTATE_180;
> +				break;
> +
> +			case 270:
> +				rotation |= DRM_MODE_ROTATE_270;
> +				break;
> +
> +			default:
> +				return -EINVAL;
> +			}
> +		} else if (!strncmp(option, "reflect_x", delim - option)) {
> +			rotation |= DRM_MODE_REFLECT_X;
> +			sep = delim;
> +		} else if (!strncmp(option, "reflect_y", delim - option)) {

I think you should drop reflect_x and _y for now since they're not
supported. People like me that only reads code and not documentation
(ahem..) will get the impression that this should work.

Documentation/fb/modedb.txt needs to be updated with this new video= option.

Noralf.

> +			rotation |= DRM_MODE_REFLECT_Y;
> +			sep = delim;
> +		} else {
> +			return -EINVAL;
> +		}
> +	}
> +
> +	mode->rotation = rotation;
> +
> +	return 0;
> +}
> +
>  /**
>   * drm_mode_parse_command_line_for_connector - parse command line modeline for connector
>   * @mode_option: optional per connector mode option
> @@ -1558,9 +1623,10 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>  {
>  	const char *name;
>  	bool named_mode = false, parse_extras = false;
> -	unsigned int bpp_off = 0, refresh_off = 0;
> +	unsigned int bpp_off = 0, refresh_off = 0, options_off = 0;
>  	unsigned int mode_end = 0;
>  	char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
> +	char *options_ptr = NULL;
>  	char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
>  	int ret;
>  
> @@ -1601,13 +1667,18 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>  		mode->refresh_specified = true;
>  	}
>  
> +	/* Locate the start of named options */
> +	options_ptr = strchr(name, ',');
> +	if (options_ptr)
> +		options_off = options_ptr - name;
> +
>  	/* 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) {
> +	if (bpp_ptr) {
>  		mode_end = bpp_off;
>  	} else if (refresh_ptr) {
>  		mode_end = refresh_off;
> +	} else if (options_ptr) {
> +		mode_end = options_off;
>  	} else {
>  		mode_end = strlen(name);
>  		parse_extras = true;
> @@ -1649,24 +1720,23 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>  	else if (refresh_ptr)
>  		extra_ptr = refresh_end_ptr;
>  
> -	if (extra_ptr) {
> -		if (!named_mode) {
> -			int len = strlen(name) - (extra_ptr - name);
> +	if (extra_ptr &&
> +	    extra_ptr != options_ptr) {
> +		int len = strlen(name) - (extra_ptr - name);
>  
> -			ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
> -							   connector, mode);
> -			if (ret)
> -				return false;
> -		} else {
> -			int remaining = strlen(name) - (extra_ptr - name);
> +		ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
> +						   connector, mode);
> +		if (ret)
> +			return false;
> +	}
>  
> -			/*
> -			 * We still have characters to process, while
> -			 * we shouldn't have any
> -			 */
> -			if (remaining > 0)
> -				return false;
> -		}
> +	if (options_ptr) {
> +		int len = strlen(name) - (options_ptr - name);
> +
> +		ret = drm_mode_parse_cmdline_options(options_ptr, len,
> +						     connector, mode);
> +		if (ret)
> +			return false;
>  	}
>  
>  	return true;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 06aa3b9cb920..6f57c1a3afff 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -908,6 +908,15 @@ struct drm_cmdline_mode {
>  	bool cvt;
>  	bool margins;
>  	enum drm_connector_force force;
> +
> +	/**
> +	 * @rotation:
> +	 *
> +	 * Initial rotation of the mode setup from the command line.
> +	 * See DRM_MODE_ROTATE_*. Only DRM_MODE_ROTATE_0 and
> +	 * DRM_MODE_ROTATE_180 are supported at the moment.
> +	 */
> +	unsigned int rotation;
>  };
>  
>  /**
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/6] drm/modes: Allow to specify rotation and reflection on the commandline

[Noralf Trønnes]

Den 18.04.2019 14.41, skrev Maxime Ripard:
> Rotations and reflections setup are needed in some scenarios to initialise
> properly the initial framebuffer. Some drivers already had a bunch of
> quirks to deal with this, such as either a private kernel command line
> parameter (omapdss) or on the device tree (various panels).
> 
> In order to accomodate this, let's create a video mode parameter to deal
> with the rotation and reflexion.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  drivers/gpu/drm/drm_client_modeset.c |  10 +++-
>  drivers/gpu/drm/drm_modes.c          | 110 ++++++++++++++++++++++------
>  include/drm/drm_connector.h          |   9 ++-
>  3 files changed, 109 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> index f2869c82510c..15145d2c86d5 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -823,6 +823,7 @@ EXPORT_SYMBOL(drm_client_modeset_probe);
>  bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotation)
>  {
>  	struct drm_connector *connector = modeset->connectors[0];
> +	struct drm_cmdline_mode *cmdline;
>  	struct drm_plane *plane = modeset->crtc->primary;
>  	u64 valid_mask = 0;
>  	unsigned int i;
> @@ -844,6 +845,15 @@ bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotat
>  		*rotation = DRM_MODE_ROTATE_0;
>  	}
>  
> +	/**
> +	 * We want the rotation on the command line to overwrite
> +	 * whatever comes from the panel.
> +	 */
> +	cmdline = &connector->cmdline_mode;
> +	if (cmdline->specified &&
> +	    cmdline->rotation != DRM_MODE_ROTATE_0)

I believe you need to drop that second check, otherwise rotate=0 will
not overwrite panel rotation.

> +		*rotation = cmdline->rotation;
> +
>  	/*
>  	 * TODO: support 90 / 270 degree hardware rotation,
>  	 * depending on the hardware this may require the framebuffer
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 9613c1a28487..ac8d70b92b62 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1531,6 +1531,71 @@ static int drm_mode_parse_cmdline_res_mode(const char *str, unsigned int length,
>  	return 0;
>  }
>  
> +static int drm_mode_parse_cmdline_options(char *str, size_t len,
> +					  struct drm_connector *connector,
> +					  struct drm_cmdline_mode *mode)
> +{
> +	unsigned int rotation = 0;
> +	char *sep = str;
> +
> +	while ((sep = strchr(sep, ','))) {
> +		char *delim, *option;
> +
> +		option = sep + 1;
> +		delim = strchr(option, '=');
> +		if (!delim) {
> +			delim = strchr(option, ',');
> +
> +			if (!delim)
> +				delim = str + len;
> +		}
> +
> +		if (!strncmp(option, "rotate", delim - option)) {
> +			const char *value = delim + 1;
> +			unsigned int deg;
> +
> +			deg = simple_strtol(value, &sep, 10);
> +
> +			/* Make sure we have parsed something */
> +			if (sep == value)
> +				return -EINVAL;
> +
> +			switch (deg) {
> +			case 0:
> +				rotation |= DRM_MODE_ROTATE_0;
> +				break;
> +
> +			case 90:
> +				rotation |= DRM_MODE_ROTATE_90;
> +				break;
> +
> +			case 180:
> +				rotation |= DRM_MODE_ROTATE_180;
> +				break;
> +
> +			case 270:
> +				rotation |= DRM_MODE_ROTATE_270;
> +				break;
> +
> +			default:
> +				return -EINVAL;
> +			}
> +		} else if (!strncmp(option, "reflect_x", delim - option)) {
> +			rotation |= DRM_MODE_REFLECT_X;
> +			sep = delim;
> +		} else if (!strncmp(option, "reflect_y", delim - option)) {

I think you should drop reflect_x and _y for now since they're not
supported. People like me that only reads code and not documentation
(ahem..) will get the impression that this should work.

Documentation/fb/modedb.txt needs to be updated with this new video= option.

Noralf.

> +			rotation |= DRM_MODE_REFLECT_Y;
> +			sep = delim;
> +		} else {
> +			return -EINVAL;
> +		}
> +	}
> +
> +	mode->rotation = rotation;
> +
> +	return 0;
> +}
> +
>  /**
>   * drm_mode_parse_command_line_for_connector - parse command line modeline for connector
>   * @mode_option: optional per connector mode option
> @@ -1558,9 +1623,10 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>  {
>  	const char *name;
>  	bool named_mode = false, parse_extras = false;
> -	unsigned int bpp_off = 0, refresh_off = 0;
> +	unsigned int bpp_off = 0, refresh_off = 0, options_off = 0;
>  	unsigned int mode_end = 0;
>  	char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
> +	char *options_ptr = NULL;
>  	char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
>  	int ret;
>  
> @@ -1601,13 +1667,18 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>  		mode->refresh_specified = true;
>  	}
>  
> +	/* Locate the start of named options */
> +	options_ptr = strchr(name, ',');
> +	if (options_ptr)
> +		options_off = options_ptr - name;
> +
>  	/* 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) {
> +	if (bpp_ptr) {
>  		mode_end = bpp_off;
>  	} else if (refresh_ptr) {
>  		mode_end = refresh_off;
> +	} else if (options_ptr) {
> +		mode_end = options_off;
>  	} else {
>  		mode_end = strlen(name);
>  		parse_extras = true;
> @@ -1649,24 +1720,23 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>  	else if (refresh_ptr)
>  		extra_ptr = refresh_end_ptr;
>  
> -	if (extra_ptr) {
> -		if (!named_mode) {
> -			int len = strlen(name) - (extra_ptr - name);
> +	if (extra_ptr &&
> +	    extra_ptr != options_ptr) {
> +		int len = strlen(name) - (extra_ptr - name);
>  
> -			ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
> -							   connector, mode);
> -			if (ret)
> -				return false;
> -		} else {
> -			int remaining = strlen(name) - (extra_ptr - name);
> +		ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
> +						   connector, mode);
> +		if (ret)
> +			return false;
> +	}
>  
> -			/*
> -			 * We still have characters to process, while
> -			 * we shouldn't have any
> -			 */
> -			if (remaining > 0)
> -				return false;
> -		}
> +	if (options_ptr) {
> +		int len = strlen(name) - (options_ptr - name);
> +
> +		ret = drm_mode_parse_cmdline_options(options_ptr, len,
> +						     connector, mode);
> +		if (ret)
> +			return false;
>  	}
>  
>  	return true;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 06aa3b9cb920..6f57c1a3afff 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -908,6 +908,15 @@ struct drm_cmdline_mode {
>  	bool cvt;
>  	bool margins;
>  	enum drm_connector_force force;
> +
> +	/**
> +	 * @rotation:
> +	 *
> +	 * Initial rotation of the mode setup from the command line.
> +	 * See DRM_MODE_ROTATE_*. Only DRM_MODE_ROTATE_0 and
> +	 * DRM_MODE_ROTATE_180 are supported at the moment.
> +	 */
> +	unsigned int rotation;
>  };
>  
>  /**
> 

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

Re: [PATCH v3 4/6] drm/modes: Parse overscan properties

[Noralf Trønnes]

Den 18.04.2019 14.41, skrev Maxime Ripard:
> Properly configuring the overscan properties might be needed for the
> initial setup of the framebuffer for display that still have overscan.
> Let's allow for more properties on the kernel command line to setup each
> margin.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  drivers/gpu/drm/drm_modes.c | 44 ++++++++++++++++++++++++++++++++++++++-
>  include/drm/drm_connector.h | 14 ++++++++++++-
>  2 files changed, 58 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index ac8d70b92b62..d93c44a97ce9 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1586,6 +1586,50 @@ static int drm_mode_parse_cmdline_options(char *str, size_t len,
>  		} else if (!strncmp(option, "reflect_y", delim - option)) {
>  			rotation |= DRM_MODE_REFLECT_Y;
>  			sep = delim;
> +		} else if (!strncmp(option, "margin_right", delim - option)) {
> +			const char *value = delim + 1;
> +			unsigned int margin;
> +
> +			margin = simple_strtol(value, &sep, 10);
> +
> +			/* Make sure we have parsed something */
> +			if (sep == value)
> +				return -EINVAL;
> +
> +			mode->tv_margins.right = margin;
> +		} else if (!strncmp(option, "margin_left", delim - option)) {
> +			const char *value = delim + 1;
> +			unsigned int margin;
> +
> +			margin = simple_strtol(value, &sep, 10);
> +
> +			/* Make sure we have parsed something */
> +			if (sep == value)
> +				return -EINVAL;
> +
> +			mode->tv_margins.left = margin;
> +		} else if (!strncmp(option, "margin_top", delim - option)) {
> +			const char *value = delim + 1;
> +			unsigned int margin;
> +
> +			margin = simple_strtol(value, &sep, 10);
> +
> +			/* Make sure we have parsed something */
> +			if (sep == value)
> +				return -EINVAL;
> +
> +			mode->tv_margins.top = margin;
> +		} else if (!strncmp(option, "margin_bottom", delim - option)) {
> +			const char *value = delim + 1;
> +			unsigned int margin;
> +
> +			margin = simple_strtol(value, &sep, 10);
> +
> +			/* Make sure we have parsed something */
> +			if (sep == value)
> +				return -EINVAL;
> +
> +			mode->tv_margins.bottom = margin;
>  		} else {
>  			return -EINVAL;
>  		}
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 6f57c1a3afff..89bc6ac38043 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -917,6 +917,20 @@ struct drm_cmdline_mode {
>  	 * DRM_MODE_ROTATE_180 are supported at the moment.
>  	 */
>  	unsigned int rotation;
> +
> +	/**
> +	 * @tv_margins: TV margins (in pixels)
> +	 * @tv_margins.left: left margin
> +	 * @tv_margins.right: right margin
> +	 * @tv_margins.top: top margin
> +	 * @tv_margins.bottom: bottom margin
> +	 */

I haven't seen kernel docs like this before so I assume you have tested
with make htmldocs.

This one also needs mention in Documentation/fb/modedb.txt. With that:

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

> +	struct {
> +		unsigned int left;
> +		unsigned int right;
> +		unsigned int top;
> +		unsigned int bottom;
> +	} tv_margins;
>  };
>  
>  /**
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 4/6] drm/modes: Parse overscan properties

[Noralf Trønnes]

Den 18.04.2019 14.41, skrev Maxime Ripard:
> Properly configuring the overscan properties might be needed for the
> initial setup of the framebuffer for display that still have overscan.
> Let's allow for more properties on the kernel command line to setup each
> margin.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  drivers/gpu/drm/drm_modes.c | 44 ++++++++++++++++++++++++++++++++++++++-
>  include/drm/drm_connector.h | 14 ++++++++++++-
>  2 files changed, 58 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index ac8d70b92b62..d93c44a97ce9 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1586,6 +1586,50 @@ static int drm_mode_parse_cmdline_options(char *str, size_t len,
>  		} else if (!strncmp(option, "reflect_y", delim - option)) {
>  			rotation |= DRM_MODE_REFLECT_Y;
>  			sep = delim;
> +		} else if (!strncmp(option, "margin_right", delim - option)) {
> +			const char *value = delim + 1;
> +			unsigned int margin;
> +
> +			margin = simple_strtol(value, &sep, 10);
> +
> +			/* Make sure we have parsed something */
> +			if (sep == value)
> +				return -EINVAL;
> +
> +			mode->tv_margins.right = margin;
> +		} else if (!strncmp(option, "margin_left", delim - option)) {
> +			const char *value = delim + 1;
> +			unsigned int margin;
> +
> +			margin = simple_strtol(value, &sep, 10);
> +
> +			/* Make sure we have parsed something */
> +			if (sep == value)
> +				return -EINVAL;
> +
> +			mode->tv_margins.left = margin;
> +		} else if (!strncmp(option, "margin_top", delim - option)) {
> +			const char *value = delim + 1;
> +			unsigned int margin;
> +
> +			margin = simple_strtol(value, &sep, 10);
> +
> +			/* Make sure we have parsed something */
> +			if (sep == value)
> +				return -EINVAL;
> +
> +			mode->tv_margins.top = margin;
> +		} else if (!strncmp(option, "margin_bottom", delim - option)) {
> +			const char *value = delim + 1;
> +			unsigned int margin;
> +
> +			margin = simple_strtol(value, &sep, 10);
> +
> +			/* Make sure we have parsed something */
> +			if (sep == value)
> +				return -EINVAL;
> +
> +			mode->tv_margins.bottom = margin;
>  		} else {
>  			return -EINVAL;
>  		}
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 6f57c1a3afff..89bc6ac38043 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -917,6 +917,20 @@ struct drm_cmdline_mode {
>  	 * DRM_MODE_ROTATE_180 are supported at the moment.
>  	 */
>  	unsigned int rotation;
> +
> +	/**
> +	 * @tv_margins: TV margins (in pixels)
> +	 * @tv_margins.left: left margin
> +	 * @tv_margins.right: right margin
> +	 * @tv_margins.top: top margin
> +	 * @tv_margins.bottom: bottom margin
> +	 */

I haven't seen kernel docs like this before so I assume you have tested
with make htmldocs.

This one also needs mention in Documentation/fb/modedb.txt. With that:

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

> +	struct {
> +		unsigned int left;
> +		unsigned int right;
> +		unsigned int top;
> +		unsigned int bottom;
> +	} tv_margins;
>  };
>  
>  /**
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 5/6] drm/selftests: Add command line parser selftests

[Noralf Trønnes]

Den 18.04.2019 14.41, skrev Maxime Ripard:
> The command line parser is pretty tough to get right and very error prone,
> so let's add a selftest to try to catch any regression.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---

Yay, unit tests \o/

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 5/6] drm/selftests: Add command line parser selftests

[Noralf Trønnes]

Den 18.04.2019 14.41, skrev Maxime Ripard:
> The command line parser is pretty tough to get right and very error prone,
> so let's add a selftest to try to catch any regression.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---

Yay, unit tests \o/

Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 6/6] drm/vc4: hdmi: Set default state margin at reset

[Noralf Trønnes]

Den 18.04.2019 14.41, skrev Maxime Ripard:
> Now that the TV margins are properly parsed and filled into
> drm_cmdline_mode, we just need to initialise the first state at reset to
> get those values and start using them.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 99fc8569e0f5..d86f154138f5 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -255,11 +255,25 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
>  	return ret;
>  }
>  
> +static void vc4_hdmi_connector_reset(struct drm_connector *connector)
> +{
> +	struct drm_cmdline_mode *cmdline = &connector->cmdline_mode;
> +	struct drm_connector_state *state;
> +
> +	drm_atomic_helper_connector_reset(connector);

Initially I wondered why not do this in the helper, but ofc that would
apply the values to all the connectors. I know too little about this
subject to argue for having it in the helper, and it can be changed
later if deemed convinient, so:

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

> +
> +	state = connector->state;
> +	state->tv.margins.left = cmdline->tv_margins.left;
> +	state->tv.margins.right = cmdline->tv_margins.right;
> +	state->tv.margins.top = cmdline->tv_margins.top;
> +	state->tv.margins.bottom = cmdline->tv_margins.bottom;
> +}
> +
>  static const struct drm_connector_funcs vc4_hdmi_connector_funcs = {
>  	.detect = vc4_hdmi_connector_detect,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.destroy = vc4_hdmi_connector_destroy,
> -	.reset = drm_atomic_helper_connector_reset,
> +	.reset = vc4_hdmi_connector_reset,
>  	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>  };
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/6] drm/modes: Allow to specify rotation and reflection on the commandline

[Noralf Trønnes]

Den 18.04.2019 18.40, skrev Noralf Trønnes:
> 
> 
> Den 18.04.2019 14.41, skrev Maxime Ripard:
>> Rotations and reflections setup are needed in some scenarios to initialise
>> properly the initial framebuffer. Some drivers already had a bunch of
>> quirks to deal with this, such as either a private kernel command line
>> parameter (omapdss) or on the device tree (various panels).
>>
>> In order to accomodate this, let's create a video mode parameter to deal
>> with the rotation and reflexion.
>>
>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>> ---
>>  drivers/gpu/drm/drm_client_modeset.c |  10 +++-
>>  drivers/gpu/drm/drm_modes.c          | 110 ++++++++++++++++++++++------
>>  include/drm/drm_connector.h          |   9 ++-
>>  3 files changed, 109 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
>> index f2869c82510c..15145d2c86d5 100644
>> --- a/drivers/gpu/drm/drm_client_modeset.c
>> +++ b/drivers/gpu/drm/drm_client_modeset.c
>> @@ -823,6 +823,7 @@ EXPORT_SYMBOL(drm_client_modeset_probe);
>>  bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotation)
>>  {
>>  	struct drm_connector *connector = modeset->connectors[0];
>> +	struct drm_cmdline_mode *cmdline;
>>  	struct drm_plane *plane = modeset->crtc->primary;
>>  	u64 valid_mask = 0;
>>  	unsigned int i;
>> @@ -844,6 +845,15 @@ bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotat
>>  		*rotation = DRM_MODE_ROTATE_0;
>>  	}
>>  
>> +	/**
>> +	 * We want the rotation on the command line to overwrite
>> +	 * whatever comes from the panel.
>> +	 */
>> +	cmdline = &connector->cmdline_mode;
>> +	if (cmdline->specified &&
>> +	    cmdline->rotation != DRM_MODE_ROTATE_0)
> 
> I believe you need to drop that second check, otherwise rotate=0 will
> not overwrite panel rotation.
> 
>> +		*rotation = cmdline->rotation;

I remembered that you wanted this to propagate to DRM userspace. That's
not happening here. The only way I see for that to happen, is to set
->panel_orientation. And to repeat myself, imo that makes 'orientation'
a better name for this video= option.

Noralf.

>> +
>>  	/*
>>  	 * TODO: support 90 / 270 degree hardware rotation,
>>  	 * depending on the hardware this may require the framebuffer
>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>> index 9613c1a28487..ac8d70b92b62 100644
>> --- a/drivers/gpu/drm/drm_modes.c
>> +++ b/drivers/gpu/drm/drm_modes.c
>> @@ -1531,6 +1531,71 @@ static int drm_mode_parse_cmdline_res_mode(const char *str, unsigned int length,
>>  	return 0;
>>  }
>>  
>> +static int drm_mode_parse_cmdline_options(char *str, size_t len,
>> +					  struct drm_connector *connector,
>> +					  struct drm_cmdline_mode *mode)
>> +{
>> +	unsigned int rotation = 0;
>> +	char *sep = str;
>> +
>> +	while ((sep = strchr(sep, ','))) {
>> +		char *delim, *option;
>> +
>> +		option = sep + 1;
>> +		delim = strchr(option, '=');
>> +		if (!delim) {
>> +			delim = strchr(option, ',');
>> +
>> +			if (!delim)
>> +				delim = str + len;
>> +		}
>> +
>> +		if (!strncmp(option, "rotate", delim - option)) {
>> +			const char *value = delim + 1;
>> +			unsigned int deg;
>> +
>> +			deg = simple_strtol(value, &sep, 10);
>> +
>> +			/* Make sure we have parsed something */
>> +			if (sep == value)
>> +				return -EINVAL;
>> +
>> +			switch (deg) {
>> +			case 0:
>> +				rotation |= DRM_MODE_ROTATE_0;
>> +				break;
>> +
>> +			case 90:
>> +				rotation |= DRM_MODE_ROTATE_90;
>> +				break;
>> +
>> +			case 180:
>> +				rotation |= DRM_MODE_ROTATE_180;
>> +				break;
>> +
>> +			case 270:
>> +				rotation |= DRM_MODE_ROTATE_270;
>> +				break;
>> +
>> +			default:
>> +				return -EINVAL;
>> +			}
>> +		} else if (!strncmp(option, "reflect_x", delim - option)) {
>> +			rotation |= DRM_MODE_REFLECT_X;
>> +			sep = delim;
>> +		} else if (!strncmp(option, "reflect_y", delim - option)) {
> 
> I think you should drop reflect_x and _y for now since they're not
> supported. People like me that only reads code and not documentation
> (ahem..) will get the impression that this should work.
> 
> Documentation/fb/modedb.txt needs to be updated with this new video= option.
> 
> Noralf.
> 
>> +			rotation |= DRM_MODE_REFLECT_Y;
>> +			sep = delim;
>> +		} else {
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	mode->rotation = rotation;
>> +
>> +	return 0;
>> +}
>> +
>>  /**
>>   * drm_mode_parse_command_line_for_connector - parse command line modeline for connector
>>   * @mode_option: optional per connector mode option
>> @@ -1558,9 +1623,10 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>>  {
>>  	const char *name;
>>  	bool named_mode = false, parse_extras = false;
>> -	unsigned int bpp_off = 0, refresh_off = 0;
>> +	unsigned int bpp_off = 0, refresh_off = 0, options_off = 0;
>>  	unsigned int mode_end = 0;
>>  	char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
>> +	char *options_ptr = NULL;
>>  	char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
>>  	int ret;
>>  
>> @@ -1601,13 +1667,18 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>>  		mode->refresh_specified = true;
>>  	}
>>  
>> +	/* Locate the start of named options */
>> +	options_ptr = strchr(name, ',');
>> +	if (options_ptr)
>> +		options_off = options_ptr - name;
>> +
>>  	/* 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) {
>> +	if (bpp_ptr) {
>>  		mode_end = bpp_off;
>>  	} else if (refresh_ptr) {
>>  		mode_end = refresh_off;
>> +	} else if (options_ptr) {
>> +		mode_end = options_off;
>>  	} else {
>>  		mode_end = strlen(name);
>>  		parse_extras = true;
>> @@ -1649,24 +1720,23 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>>  	else if (refresh_ptr)
>>  		extra_ptr = refresh_end_ptr;
>>  
>> -	if (extra_ptr) {
>> -		if (!named_mode) {
>> -			int len = strlen(name) - (extra_ptr - name);
>> +	if (extra_ptr &&
>> +	    extra_ptr != options_ptr) {
>> +		int len = strlen(name) - (extra_ptr - name);
>>  
>> -			ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
>> -							   connector, mode);
>> -			if (ret)
>> -				return false;
>> -		} else {
>> -			int remaining = strlen(name) - (extra_ptr - name);
>> +		ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
>> +						   connector, mode);
>> +		if (ret)
>> +			return false;
>> +	}
>>  
>> -			/*
>> -			 * We still have characters to process, while
>> -			 * we shouldn't have any
>> -			 */
>> -			if (remaining > 0)
>> -				return false;
>> -		}
>> +	if (options_ptr) {
>> +		int len = strlen(name) - (options_ptr - name);
>> +
>> +		ret = drm_mode_parse_cmdline_options(options_ptr, len,
>> +						     connector, mode);
>> +		if (ret)
>> +			return false;
>>  	}
>>  
>>  	return true;
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 06aa3b9cb920..6f57c1a3afff 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -908,6 +908,15 @@ struct drm_cmdline_mode {
>>  	bool cvt;
>>  	bool margins;
>>  	enum drm_connector_force force;
>> +
>> +	/**
>> +	 * @rotation:
>> +	 *
>> +	 * Initial rotation of the mode setup from the command line.
>> +	 * See DRM_MODE_ROTATE_*. Only DRM_MODE_ROTATE_0 and
>> +	 * DRM_MODE_ROTATE_180 are supported at the moment.
>> +	 */
>> +	unsigned int rotation;
>>  };
>>  
>>  /**
>>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/6] drm/modes: Allow to specify rotation and reflection on the commandline

[Noralf Trønnes]

Den 18.04.2019 18.40, skrev Noralf Trønnes:
> 
> 
> Den 18.04.2019 14.41, skrev Maxime Ripard:
>> Rotations and reflections setup are needed in some scenarios to initialise
>> properly the initial framebuffer. Some drivers already had a bunch of
>> quirks to deal with this, such as either a private kernel command line
>> parameter (omapdss) or on the device tree (various panels).
>>
>> In order to accomodate this, let's create a video mode parameter to deal
>> with the rotation and reflexion.
>>
>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>> ---
>>  drivers/gpu/drm/drm_client_modeset.c |  10 +++-
>>  drivers/gpu/drm/drm_modes.c          | 110 ++++++++++++++++++++++------
>>  include/drm/drm_connector.h          |   9 ++-
>>  3 files changed, 109 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
>> index f2869c82510c..15145d2c86d5 100644
>> --- a/drivers/gpu/drm/drm_client_modeset.c
>> +++ b/drivers/gpu/drm/drm_client_modeset.c
>> @@ -823,6 +823,7 @@ EXPORT_SYMBOL(drm_client_modeset_probe);
>>  bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotation)
>>  {
>>  	struct drm_connector *connector = modeset->connectors[0];
>> +	struct drm_cmdline_mode *cmdline;
>>  	struct drm_plane *plane = modeset->crtc->primary;
>>  	u64 valid_mask = 0;
>>  	unsigned int i;
>> @@ -844,6 +845,15 @@ bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotat
>>  		*rotation = DRM_MODE_ROTATE_0;
>>  	}
>>  
>> +	/**
>> +	 * We want the rotation on the command line to overwrite
>> +	 * whatever comes from the panel.
>> +	 */
>> +	cmdline = &connector->cmdline_mode;
>> +	if (cmdline->specified &&
>> +	    cmdline->rotation != DRM_MODE_ROTATE_0)
> 
> I believe you need to drop that second check, otherwise rotate=0 will
> not overwrite panel rotation.
> 
>> +		*rotation = cmdline->rotation;

I remembered that you wanted this to propagate to DRM userspace. That's
not happening here. The only way I see for that to happen, is to set
->panel_orientation. And to repeat myself, imo that makes 'orientation'
a better name for this video= option.

Noralf.

>> +
>>  	/*
>>  	 * TODO: support 90 / 270 degree hardware rotation,
>>  	 * depending on the hardware this may require the framebuffer
>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>> index 9613c1a28487..ac8d70b92b62 100644
>> --- a/drivers/gpu/drm/drm_modes.c
>> +++ b/drivers/gpu/drm/drm_modes.c
>> @@ -1531,6 +1531,71 @@ static int drm_mode_parse_cmdline_res_mode(const char *str, unsigned int length,
>>  	return 0;
>>  }
>>  
>> +static int drm_mode_parse_cmdline_options(char *str, size_t len,
>> +					  struct drm_connector *connector,
>> +					  struct drm_cmdline_mode *mode)
>> +{
>> +	unsigned int rotation = 0;
>> +	char *sep = str;
>> +
>> +	while ((sep = strchr(sep, ','))) {
>> +		char *delim, *option;
>> +
>> +		option = sep + 1;
>> +		delim = strchr(option, '=');
>> +		if (!delim) {
>> +			delim = strchr(option, ',');
>> +
>> +			if (!delim)
>> +				delim = str + len;
>> +		}
>> +
>> +		if (!strncmp(option, "rotate", delim - option)) {
>> +			const char *value = delim + 1;
>> +			unsigned int deg;
>> +
>> +			deg = simple_strtol(value, &sep, 10);
>> +
>> +			/* Make sure we have parsed something */
>> +			if (sep == value)
>> +				return -EINVAL;
>> +
>> +			switch (deg) {
>> +			case 0:
>> +				rotation |= DRM_MODE_ROTATE_0;
>> +				break;
>> +
>> +			case 90:
>> +				rotation |= DRM_MODE_ROTATE_90;
>> +				break;
>> +
>> +			case 180:
>> +				rotation |= DRM_MODE_ROTATE_180;
>> +				break;
>> +
>> +			case 270:
>> +				rotation |= DRM_MODE_ROTATE_270;
>> +				break;
>> +
>> +			default:
>> +				return -EINVAL;
>> +			}
>> +		} else if (!strncmp(option, "reflect_x", delim - option)) {
>> +			rotation |= DRM_MODE_REFLECT_X;
>> +			sep = delim;
>> +		} else if (!strncmp(option, "reflect_y", delim - option)) {
> 
> I think you should drop reflect_x and _y for now since they're not
> supported. People like me that only reads code and not documentation
> (ahem..) will get the impression that this should work.
> 
> Documentation/fb/modedb.txt needs to be updated with this new video= option.
> 
> Noralf.
> 
>> +			rotation |= DRM_MODE_REFLECT_Y;
>> +			sep = delim;
>> +		} else {
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	mode->rotation = rotation;
>> +
>> +	return 0;
>> +}
>> +
>>  /**
>>   * drm_mode_parse_command_line_for_connector - parse command line modeline for connector
>>   * @mode_option: optional per connector mode option
>> @@ -1558,9 +1623,10 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>>  {
>>  	const char *name;
>>  	bool named_mode = false, parse_extras = false;
>> -	unsigned int bpp_off = 0, refresh_off = 0;
>> +	unsigned int bpp_off = 0, refresh_off = 0, options_off = 0;
>>  	unsigned int mode_end = 0;
>>  	char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
>> +	char *options_ptr = NULL;
>>  	char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
>>  	int ret;
>>  
>> @@ -1601,13 +1667,18 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>>  		mode->refresh_specified = true;
>>  	}
>>  
>> +	/* Locate the start of named options */
>> +	options_ptr = strchr(name, ',');
>> +	if (options_ptr)
>> +		options_off = options_ptr - name;
>> +
>>  	/* 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) {
>> +	if (bpp_ptr) {
>>  		mode_end = bpp_off;
>>  	} else if (refresh_ptr) {
>>  		mode_end = refresh_off;
>> +	} else if (options_ptr) {
>> +		mode_end = options_off;
>>  	} else {
>>  		mode_end = strlen(name);
>>  		parse_extras = true;
>> @@ -1649,24 +1720,23 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>>  	else if (refresh_ptr)
>>  		extra_ptr = refresh_end_ptr;
>>  
>> -	if (extra_ptr) {
>> -		if (!named_mode) {
>> -			int len = strlen(name) - (extra_ptr - name);
>> +	if (extra_ptr &&
>> +	    extra_ptr != options_ptr) {
>> +		int len = strlen(name) - (extra_ptr - name);
>>  
>> -			ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
>> -							   connector, mode);
>> -			if (ret)
>> -				return false;
>> -		} else {
>> -			int remaining = strlen(name) - (extra_ptr - name);
>> +		ret = drm_mode_parse_cmdline_extra(extra_ptr, len,
>> +						   connector, mode);
>> +		if (ret)
>> +			return false;
>> +	}
>>  
>> -			/*
>> -			 * We still have characters to process, while
>> -			 * we shouldn't have any
>> -			 */
>> -			if (remaining > 0)
>> -				return false;
>> -		}
>> +	if (options_ptr) {
>> +		int len = strlen(name) - (options_ptr - name);
>> +
>> +		ret = drm_mode_parse_cmdline_options(options_ptr, len,
>> +						     connector, mode);
>> +		if (ret)
>> +			return false;
>>  	}
>>  
>>  	return true;
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 06aa3b9cb920..6f57c1a3afff 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -908,6 +908,15 @@ struct drm_cmdline_mode {
>>  	bool cvt;
>>  	bool margins;
>>  	enum drm_connector_force force;
>> +
>> +	/**
>> +	 * @rotation:
>> +	 *
>> +	 * Initial rotation of the mode setup from the command line.
>> +	 * See DRM_MODE_ROTATE_*. Only DRM_MODE_ROTATE_0 and
>> +	 * DRM_MODE_ROTATE_180 are supported at the moment.
>> +	 */
>> +	unsigned int rotation;
>>  };
>>  
>>  /**
>>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 4/6] drm/modes: Parse overscan properties

[Noralf Trønnes]

Den 18.04.2019 18.50, skrev Noralf Trønnes:
> 
> 
> Den 18.04.2019 14.41, skrev Maxime Ripard:
>> Properly configuring the overscan properties might be needed for the
>> initial setup of the framebuffer for display that still have overscan.
>> Let's allow for more properties on the kernel command line to setup each
>> margin.
>>
>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>> ---
>>  drivers/gpu/drm/drm_modes.c | 44 ++++++++++++++++++++++++++++++++++++++-
>>  include/drm/drm_connector.h | 14 ++++++++++++-
>>  2 files changed, 58 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>> index ac8d70b92b62..d93c44a97ce9 100644
>> --- a/drivers/gpu/drm/drm_modes.c
>> +++ b/drivers/gpu/drm/drm_modes.c
>> @@ -1586,6 +1586,50 @@ static int drm_mode_parse_cmdline_options(char *str, size_t len,
>>  		} else if (!strncmp(option, "reflect_y", delim - option)) {
>>  			rotation |= DRM_MODE_REFLECT_Y;
>>  			sep = delim;
>> +		} else if (!strncmp(option, "margin_right", delim - option)) {
>> +			const char *value = delim + 1;
>> +			unsigned int margin;
>> +
>> +			margin = simple_strtol(value, &sep, 10);
>> +
>> +			/* Make sure we have parsed something */
>> +			if (sep == value)
>> +				return -EINVAL;
>> +
>> +			mode->tv_margins.right = margin;
>> +		} else if (!strncmp(option, "margin_left", delim - option)) {
>> +			const char *value = delim + 1;
>> +			unsigned int margin;
>> +
>> +			margin = simple_strtol(value, &sep, 10);
>> +
>> +			/* Make sure we have parsed something */
>> +			if (sep == value)
>> +				return -EINVAL;
>> +
>> +			mode->tv_margins.left = margin;
>> +		} else if (!strncmp(option, "margin_top", delim - option)) {
>> +			const char *value = delim + 1;
>> +			unsigned int margin;
>> +
>> +			margin = simple_strtol(value, &sep, 10);
>> +
>> +			/* Make sure we have parsed something */
>> +			if (sep == value)
>> +				return -EINVAL;
>> +
>> +			mode->tv_margins.top = margin;
>> +		} else if (!strncmp(option, "margin_bottom", delim - option)) {
>> +			const char *value = delim + 1;
>> +			unsigned int margin;
>> +
>> +			margin = simple_strtol(value, &sep, 10);
>> +
>> +			/* Make sure we have parsed something */
>> +			if (sep == value)
>> +				return -EINVAL;
>> +
>> +			mode->tv_margins.bottom = margin;
>>  		} else {
>>  			return -EINVAL;
>>  		}
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 6f57c1a3afff..89bc6ac38043 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -917,6 +917,20 @@ struct drm_cmdline_mode {
>>  	 * DRM_MODE_ROTATE_180 are supported at the moment.
>>  	 */
>>  	unsigned int rotation;
>> +
>> +	/**
>> +	 * @tv_margins: TV margins (in pixels)
>> +	 * @tv_margins.left: left margin
>> +	 * @tv_margins.right: right margin
>> +	 * @tv_margins.top: top margin
>> +	 * @tv_margins.bottom: bottom margin
>> +	 */
> 
> I haven't seen kernel docs like this before so I assume you have tested
> with make htmldocs.
> 
> This one also needs mention in Documentation/fb/modedb.txt. With that:
> 

I discovered that the drm_mode_parse_command_line_for_connector() kernel
doc has the parameters mentioned, so it also needs to be updated.

> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> 
>> +	struct {
>> +		unsigned int left;
>> +		unsigned int right;
>> +		unsigned int top;
>> +		unsigned int bottom;
>> +	} tv_margins;
>>  };
>>  
>>  /**
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 4/6] drm/modes: Parse overscan properties

[Noralf Trønnes]

Den 18.04.2019 18.50, skrev Noralf Trønnes:
> 
> 
> Den 18.04.2019 14.41, skrev Maxime Ripard:
>> Properly configuring the overscan properties might be needed for the
>> initial setup of the framebuffer for display that still have overscan.
>> Let's allow for more properties on the kernel command line to setup each
>> margin.
>>
>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>> ---
>>  drivers/gpu/drm/drm_modes.c | 44 ++++++++++++++++++++++++++++++++++++++-
>>  include/drm/drm_connector.h | 14 ++++++++++++-
>>  2 files changed, 58 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>> index ac8d70b92b62..d93c44a97ce9 100644
>> --- a/drivers/gpu/drm/drm_modes.c
>> +++ b/drivers/gpu/drm/drm_modes.c
>> @@ -1586,6 +1586,50 @@ static int drm_mode_parse_cmdline_options(char *str, size_t len,
>>  		} else if (!strncmp(option, "reflect_y", delim - option)) {
>>  			rotation |= DRM_MODE_REFLECT_Y;
>>  			sep = delim;
>> +		} else if (!strncmp(option, "margin_right", delim - option)) {
>> +			const char *value = delim + 1;
>> +			unsigned int margin;
>> +
>> +			margin = simple_strtol(value, &sep, 10);
>> +
>> +			/* Make sure we have parsed something */
>> +			if (sep == value)
>> +				return -EINVAL;
>> +
>> +			mode->tv_margins.right = margin;
>> +		} else if (!strncmp(option, "margin_left", delim - option)) {
>> +			const char *value = delim + 1;
>> +			unsigned int margin;
>> +
>> +			margin = simple_strtol(value, &sep, 10);
>> +
>> +			/* Make sure we have parsed something */
>> +			if (sep == value)
>> +				return -EINVAL;
>> +
>> +			mode->tv_margins.left = margin;
>> +		} else if (!strncmp(option, "margin_top", delim - option)) {
>> +			const char *value = delim + 1;
>> +			unsigned int margin;
>> +
>> +			margin = simple_strtol(value, &sep, 10);
>> +
>> +			/* Make sure we have parsed something */
>> +			if (sep == value)
>> +				return -EINVAL;
>> +
>> +			mode->tv_margins.top = margin;
>> +		} else if (!strncmp(option, "margin_bottom", delim - option)) {
>> +			const char *value = delim + 1;
>> +			unsigned int margin;
>> +
>> +			margin = simple_strtol(value, &sep, 10);
>> +
>> +			/* Make sure we have parsed something */
>> +			if (sep == value)
>> +				return -EINVAL;
>> +
>> +			mode->tv_margins.bottom = margin;
>>  		} else {
>>  			return -EINVAL;
>>  		}
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 6f57c1a3afff..89bc6ac38043 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -917,6 +917,20 @@ struct drm_cmdline_mode {
>>  	 * DRM_MODE_ROTATE_180 are supported at the moment.
>>  	 */
>>  	unsigned int rotation;
>> +
>> +	/**
>> +	 * @tv_margins: TV margins (in pixels)
>> +	 * @tv_margins.left: left margin
>> +	 * @tv_margins.right: right margin
>> +	 * @tv_margins.top: top margin
>> +	 * @tv_margins.bottom: bottom margin
>> +	 */
> 
> I haven't seen kernel docs like this before so I assume you have tested
> with make htmldocs.
> 
> This one also needs mention in Documentation/fb/modedb.txt. With that:
> 

I discovered that the drm_mode_parse_command_line_for_connector() kernel
doc has the parameters mentioned, so it also needs to be updated.

> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> 
>> +	struct {
>> +		unsigned int left;
>> +		unsigned int right;
>> +		unsigned int top;
>> +		unsigned int bottom;
>> +	} tv_margins;
>>  };
>>  
>>  /**
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 6/6] drm/vc4: hdmi: Set default state margin at reset

[Noralf Trønnes]

Den 18.04.2019 18.59, skrev Noralf Trønnes:
> 
> 
> Den 18.04.2019 14.41, skrev Maxime Ripard:
>> Now that the TV margins are properly parsed and filled into
>> drm_cmdline_mode, we just need to initialise the first state at reset to
>> get those values and start using them.
>>
>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>> ---
>>  drivers/gpu/drm/vc4/vc4_hdmi.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
>> index 99fc8569e0f5..d86f154138f5 100644
>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
>> @@ -255,11 +255,25 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
>>  	return ret;
>>  }
>>  
>> +static void vc4_hdmi_connector_reset(struct drm_connector *connector)

You could turn this function into a helper
drm_atomic_helper_connector_tv_reset() or something.

>> +{
>> +	struct drm_cmdline_mode *cmdline = &connector->cmdline_mode;
>> +	struct drm_connector_state *state;
>> +
>> +	drm_atomic_helper_connector_reset(connector);
> 
> Initially I wondered why not do this in the helper, but ofc that would
> apply the values to all the connectors. I know too little about this
> subject to argue for having it in the helper, and it can be changed
> later if deemed convinient, so:
> 
> Acked-by: Noralf Trønnes <noralf@tronnes.org>
> 
>> +
>> +	state = connector->state;
>> +	state->tv.margins.left = cmdline->tv_margins.left;
>> +	state->tv.margins.right = cmdline->tv_margins.right;
>> +	state->tv.margins.top = cmdline->tv_margins.top;
>> +	state->tv.margins.bottom = cmdline->tv_margins.bottom;
>> +}
>> +
>>  static const struct drm_connector_funcs vc4_hdmi_connector_funcs = {
>>  	.detect = vc4_hdmi_connector_detect,
>>  	.fill_modes = drm_helper_probe_single_connector_modes,
>>  	.destroy = vc4_hdmi_connector_destroy,
>> -	.reset = drm_atomic_helper_connector_reset,
>> +	.reset = vc4_hdmi_connector_reset,
>>  	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>>  };
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 6/6] drm/vc4: hdmi: Set default state margin at reset

[Noralf Trønnes]

Den 18.04.2019 18.59, skrev Noralf Trønnes:
> 
> 
> Den 18.04.2019 14.41, skrev Maxime Ripard:
>> Now that the TV margins are properly parsed and filled into
>> drm_cmdline_mode, we just need to initialise the first state at reset to
>> get those values and start using them.
>>
>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>> ---
>>  drivers/gpu/drm/vc4/vc4_hdmi.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
>> index 99fc8569e0f5..d86f154138f5 100644
>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
>> @@ -255,11 +255,25 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
>>  	return ret;
>>  }
>>  
>> +static void vc4_hdmi_connector_reset(struct drm_connector *connector)

You could turn this function into a helper
drm_atomic_helper_connector_tv_reset() or something.

>> +{
>> +	struct drm_cmdline_mode *cmdline = &connector->cmdline_mode;
>> +	struct drm_connector_state *state;
>> +
>> +	drm_atomic_helper_connector_reset(connector);
> 
> Initially I wondered why not do this in the helper, but ofc that would
> apply the values to all the connectors. I know too little about this
> subject to argue for having it in the helper, and it can be changed
> later if deemed convinient, so:
> 
> Acked-by: Noralf Trønnes <noralf@tronnes.org>
> 
>> +
>> +	state = connector->state;
>> +	state->tv.margins.left = cmdline->tv_margins.left;
>> +	state->tv.margins.right = cmdline->tv_margins.right;
>> +	state->tv.margins.top = cmdline->tv_margins.top;
>> +	state->tv.margins.bottom = cmdline->tv_margins.bottom;
>> +}
>> +
>>  static const struct drm_connector_funcs vc4_hdmi_connector_funcs = {
>>  	.detect = vc4_hdmi_connector_detect,
>>  	.fill_modes = drm_helper_probe_single_connector_modes,
>>  	.destroy = vc4_hdmi_connector_destroy,
>> -	.reset = drm_atomic_helper_connector_reset,
>> +	.reset = vc4_hdmi_connector_reset,
>>  	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>>  };
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/6] drm/modes: Allow to specify rotation and reflection on the commandline

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1228 bytes --]

Hi Noralf,

On Thu, Apr 18, 2019 at 06:40:42PM +0200, Noralf Trønnes wrote:
> Den 18.04.2019 14.41, skrev Maxime Ripard:
> > +	/**
> > +	 * We want the rotation on the command line to overwrite
> > +	 * whatever comes from the panel.
> > +	 */
> > +	cmdline = &connector->cmdline_mode;
> > +	if (cmdline->specified &&
> > +	    cmdline->rotation != DRM_MODE_ROTATE_0)
>
> I believe you need to drop that second check, otherwise rotate=0 will
> not overwrite panel rotation.

Good catch :)

> > +		} else if (!strncmp(option, "reflect_x", delim - option)) {
> > +			rotation |= DRM_MODE_REFLECT_X;
> > +			sep = delim;
> > +		} else if (!strncmp(option, "reflect_y", delim - option)) {
>
> I think you should drop reflect_x and _y for now since they're not
> supported. People like me that only reads code and not documentation
> (ahem..) will get the impression that this should work.

I'm not sure what you mean here, this is definitely supposed to
work. Is there a limitation you're thinking of?

> Documentation/fb/modedb.txt needs to be updated with this new video= option.

Will do, thanks!

maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/6] drm/modes: Allow to specify rotation and reflection on the commandline

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1228 bytes --]

Hi Noralf,

On Thu, Apr 18, 2019 at 06:40:42PM +0200, Noralf Trønnes wrote:
> Den 18.04.2019 14.41, skrev Maxime Ripard:
> > +	/**
> > +	 * We want the rotation on the command line to overwrite
> > +	 * whatever comes from the panel.
> > +	 */
> > +	cmdline = &connector->cmdline_mode;
> > +	if (cmdline->specified &&
> > +	    cmdline->rotation != DRM_MODE_ROTATE_0)
>
> I believe you need to drop that second check, otherwise rotate=0 will
> not overwrite panel rotation.

Good catch :)

> > +		} else if (!strncmp(option, "reflect_x", delim - option)) {
> > +			rotation |= DRM_MODE_REFLECT_X;
> > +			sep = delim;
> > +		} else if (!strncmp(option, "reflect_y", delim - option)) {
>
> I think you should drop reflect_x and _y for now since they're not
> supported. People like me that only reads code and not documentation
> (ahem..) will get the impression that this should work.

I'm not sure what you mean here, this is definitely supposed to
work. Is there a limitation you're thinking of?

> Documentation/fb/modedb.txt needs to be updated with this new video= option.

Will do, thanks!

maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH v3 3/6] drm/modes: Allow to specify rotation and reflection on the commandline

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 3355 bytes --]

Hi Noralf,

On Fri, Apr 19, 2019 at 10:53:28AM +0200, Noralf Trønnes wrote:
> Den 18.04.2019 18.40, skrev Noralf Trønnes:
> >
> >
> > Den 18.04.2019 14.41, skrev Maxime Ripard:
> >> Rotations and reflections setup are needed in some scenarios to initialise
> >> properly the initial framebuffer. Some drivers already had a bunch of
> >> quirks to deal with this, such as either a private kernel command line
> >> parameter (omapdss) or on the device tree (various panels).
> >>
> >> In order to accomodate this, let's create a video mode parameter to deal
> >> with the rotation and reflexion.
> >>
> >> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> >> ---
> >>  drivers/gpu/drm/drm_client_modeset.c |  10 +++-
> >>  drivers/gpu/drm/drm_modes.c          | 110 ++++++++++++++++++++++------
> >>  include/drm/drm_connector.h          |   9 ++-
> >>  3 files changed, 109 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> >> index f2869c82510c..15145d2c86d5 100644
> >> --- a/drivers/gpu/drm/drm_client_modeset.c
> >> +++ b/drivers/gpu/drm/drm_client_modeset.c
> >> @@ -823,6 +823,7 @@ EXPORT_SYMBOL(drm_client_modeset_probe);
> >>  bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotation)
> >>  {
> >>  	struct drm_connector *connector = modeset->connectors[0];
> >> +	struct drm_cmdline_mode *cmdline;
> >>  	struct drm_plane *plane = modeset->crtc->primary;
> >>  	u64 valid_mask = 0;
> >>  	unsigned int i;
> >> @@ -844,6 +845,15 @@ bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotat
> >>  		*rotation = DRM_MODE_ROTATE_0;
> >>  	}
> >>
> >> +	/**
> >> +	 * We want the rotation on the command line to overwrite
> >> +	 * whatever comes from the panel.
> >> +	 */
> >> +	cmdline = &connector->cmdline_mode;
> >> +	if (cmdline->specified &&
> >> +	    cmdline->rotation != DRM_MODE_ROTATE_0)
> >
> > I believe you need to drop that second check, otherwise rotate=0 will
> > not overwrite panel rotation.
> >
> >> +		*rotation = cmdline->rotation;
>
> I remembered that you wanted this to propagate to DRM userspace. That's
> not happening here.

It's propated to the userspace through the plane's rotation property,
I just checked.

> The only way I see for that to happen, is to set
> ->panel_orientation. And to repeat myself, imo that makes
> 'orientation' a better name for this video= option.

orientation and rotation are two different things to me. The
orientation of a panel for example is absolute, while the rotation is
a transformation. In this particular case, I think that both the
orientation and the rotation should be taken into account, with the
orientation being the default state, and the hardware / panel will
tell us that, while the rotation would be a transformation from that
default to whatever the user wants.

More importantly, the orientation is a property of the hardware (ie,
how the display has been assembled), while the rotation is a software
construct.

And if the property being used to expose that is the rotation, I guess
it would make sense to just use the same name and remain consistent.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/6] drm/modes: Allow to specify rotation and reflection on the commandline

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 3355 bytes --]

Hi Noralf,

On Fri, Apr 19, 2019 at 10:53:28AM +0200, Noralf Trønnes wrote:
> Den 18.04.2019 18.40, skrev Noralf Trønnes:
> >
> >
> > Den 18.04.2019 14.41, skrev Maxime Ripard:
> >> Rotations and reflections setup are needed in some scenarios to initialise
> >> properly the initial framebuffer. Some drivers already had a bunch of
> >> quirks to deal with this, such as either a private kernel command line
> >> parameter (omapdss) or on the device tree (various panels).
> >>
> >> In order to accomodate this, let's create a video mode parameter to deal
> >> with the rotation and reflexion.
> >>
> >> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> >> ---
> >>  drivers/gpu/drm/drm_client_modeset.c |  10 +++-
> >>  drivers/gpu/drm/drm_modes.c          | 110 ++++++++++++++++++++++------
> >>  include/drm/drm_connector.h          |   9 ++-
> >>  3 files changed, 109 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> >> index f2869c82510c..15145d2c86d5 100644
> >> --- a/drivers/gpu/drm/drm_client_modeset.c
> >> +++ b/drivers/gpu/drm/drm_client_modeset.c
> >> @@ -823,6 +823,7 @@ EXPORT_SYMBOL(drm_client_modeset_probe);
> >>  bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotation)
> >>  {
> >>  	struct drm_connector *connector = modeset->connectors[0];
> >> +	struct drm_cmdline_mode *cmdline;
> >>  	struct drm_plane *plane = modeset->crtc->primary;
> >>  	u64 valid_mask = 0;
> >>  	unsigned int i;
> >> @@ -844,6 +845,15 @@ bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotat
> >>  		*rotation = DRM_MODE_ROTATE_0;
> >>  	}
> >>
> >> +	/**
> >> +	 * We want the rotation on the command line to overwrite
> >> +	 * whatever comes from the panel.
> >> +	 */
> >> +	cmdline = &connector->cmdline_mode;
> >> +	if (cmdline->specified &&
> >> +	    cmdline->rotation != DRM_MODE_ROTATE_0)
> >
> > I believe you need to drop that second check, otherwise rotate=0 will
> > not overwrite panel rotation.
> >
> >> +		*rotation = cmdline->rotation;
>
> I remembered that you wanted this to propagate to DRM userspace. That's
> not happening here.

It's propated to the userspace through the plane's rotation property,
I just checked.

> The only way I see for that to happen, is to set
> ->panel_orientation. And to repeat myself, imo that makes
> 'orientation' a better name for this video= option.

orientation and rotation are two different things to me. The
orientation of a panel for example is absolute, while the rotation is
a transformation. In this particular case, I think that both the
orientation and the rotation should be taken into account, with the
orientation being the default state, and the hardware / panel will
tell us that, while the rotation would be a transformation from that
default to whatever the user wants.

More importantly, the orientation is a property of the hardware (ie,
how the display has been assembled), while the rotation is a software
construct.

And if the property being used to expose that is the rotation, I guess
it would make sense to just use the same name and remain consistent.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH v3 3/6] drm/modes: Allow to specify rotation and reflection on the commandline

[Jani Nikula]

On Tue, 11 Jun 2019, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> Hi Noralf,
>
> On Fri, Apr 19, 2019 at 10:53:28AM +0200, Noralf Trønnes wrote:
>> Den 18.04.2019 18.40, skrev Noralf Trønnes:
>> >
>> >
>> > Den 18.04.2019 14.41, skrev Maxime Ripard:
>> >> Rotations and reflections setup are needed in some scenarios to initialise
>> >> properly the initial framebuffer. Some drivers already had a bunch of
>> >> quirks to deal with this, such as either a private kernel command line
>> >> parameter (omapdss) or on the device tree (various panels).
>> >>
>> >> In order to accomodate this, let's create a video mode parameter to deal
>> >> with the rotation and reflexion.
>> >>
>> >> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>> >> ---
>> >>  drivers/gpu/drm/drm_client_modeset.c |  10 +++-
>> >>  drivers/gpu/drm/drm_modes.c          | 110 ++++++++++++++++++++++------
>> >>  include/drm/drm_connector.h          |   9 ++-
>> >>  3 files changed, 109 insertions(+), 20 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
>> >> index f2869c82510c..15145d2c86d5 100644
>> >> --- a/drivers/gpu/drm/drm_client_modeset.c
>> >> +++ b/drivers/gpu/drm/drm_client_modeset.c
>> >> @@ -823,6 +823,7 @@ EXPORT_SYMBOL(drm_client_modeset_probe);
>> >>  bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotation)
>> >>  {
>> >>  	struct drm_connector *connector = modeset->connectors[0];
>> >> +	struct drm_cmdline_mode *cmdline;
>> >>  	struct drm_plane *plane = modeset->crtc->primary;
>> >>  	u64 valid_mask = 0;
>> >>  	unsigned int i;
>> >> @@ -844,6 +845,15 @@ bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotat
>> >>  		*rotation = DRM_MODE_ROTATE_0;
>> >>  	}
>> >>
>> >> +	/**
>> >> +	 * We want the rotation on the command line to overwrite
>> >> +	 * whatever comes from the panel.
>> >> +	 */
>> >> +	cmdline = &connector->cmdline_mode;
>> >> +	if (cmdline->specified &&
>> >> +	    cmdline->rotation != DRM_MODE_ROTATE_0)
>> >
>> > I believe you need to drop that second check, otherwise rotate=0 will
>> > not overwrite panel rotation.
>> >
>> >> +		*rotation = cmdline->rotation;
>>
>> I remembered that you wanted this to propagate to DRM userspace. That's
>> not happening here.
>
> It's propated to the userspace through the plane's rotation property,
> I just checked.
>
>> The only way I see for that to happen, is to set
>> ->panel_orientation. And to repeat myself, imo that makes
>> 'orientation' a better name for this video= option.
>
> orientation and rotation are two different things to me. The
> orientation of a panel for example is absolute, while the rotation is
> a transformation. In this particular case, I think that both the
> orientation and the rotation should be taken into account, with the
> orientation being the default state, and the hardware / panel will
> tell us that, while the rotation would be a transformation from that
> default to whatever the user wants.
>
> More importantly, the orientation is a property of the hardware (ie,
> how the display has been assembled), while the rotation is a software
> construct.

FWIW, agreed. The immutable orientation property is exposed using the
drm_connector_init_panel_orientation_property() call. You then rotate to
take the orientation into account, to not display stuff sideways or
upside down wrt the natural orientation of the device.

BR,
Jani.

>
> And if the property being used to expose that is the rotation, I guess
> it would make sense to just use the same name and remain consistent.
>
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Graphics Center

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/6] drm/modes: Allow to specify rotation and reflection on the commandline

[Jani Nikula]

On Tue, 11 Jun 2019, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> Hi Noralf,
>
> On Fri, Apr 19, 2019 at 10:53:28AM +0200, Noralf Trønnes wrote:
>> Den 18.04.2019 18.40, skrev Noralf Trønnes:
>> >
>> >
>> > Den 18.04.2019 14.41, skrev Maxime Ripard:
>> >> Rotations and reflections setup are needed in some scenarios to initialise
>> >> properly the initial framebuffer. Some drivers already had a bunch of
>> >> quirks to deal with this, such as either a private kernel command line
>> >> parameter (omapdss) or on the device tree (various panels).
>> >>
>> >> In order to accomodate this, let's create a video mode parameter to deal
>> >> with the rotation and reflexion.
>> >>
>> >> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>> >> ---
>> >>  drivers/gpu/drm/drm_client_modeset.c |  10 +++-
>> >>  drivers/gpu/drm/drm_modes.c          | 110 ++++++++++++++++++++++------
>> >>  include/drm/drm_connector.h          |   9 ++-
>> >>  3 files changed, 109 insertions(+), 20 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
>> >> index f2869c82510c..15145d2c86d5 100644
>> >> --- a/drivers/gpu/drm/drm_client_modeset.c
>> >> +++ b/drivers/gpu/drm/drm_client_modeset.c
>> >> @@ -823,6 +823,7 @@ EXPORT_SYMBOL(drm_client_modeset_probe);
>> >>  bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotation)
>> >>  {
>> >>  	struct drm_connector *connector = modeset->connectors[0];
>> >> +	struct drm_cmdline_mode *cmdline;
>> >>  	struct drm_plane *plane = modeset->crtc->primary;
>> >>  	u64 valid_mask = 0;
>> >>  	unsigned int i;
>> >> @@ -844,6 +845,15 @@ bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotat
>> >>  		*rotation = DRM_MODE_ROTATE_0;
>> >>  	}
>> >>
>> >> +	/**
>> >> +	 * We want the rotation on the command line to overwrite
>> >> +	 * whatever comes from the panel.
>> >> +	 */
>> >> +	cmdline = &connector->cmdline_mode;
>> >> +	if (cmdline->specified &&
>> >> +	    cmdline->rotation != DRM_MODE_ROTATE_0)
>> >
>> > I believe you need to drop that second check, otherwise rotate=0 will
>> > not overwrite panel rotation.
>> >
>> >> +		*rotation = cmdline->rotation;
>>
>> I remembered that you wanted this to propagate to DRM userspace. That's
>> not happening here.
>
> It's propated to the userspace through the plane's rotation property,
> I just checked.
>
>> The only way I see for that to happen, is to set
>> ->panel_orientation. And to repeat myself, imo that makes
>> 'orientation' a better name for this video= option.
>
> orientation and rotation are two different things to me. The
> orientation of a panel for example is absolute, while the rotation is
> a transformation. In this particular case, I think that both the
> orientation and the rotation should be taken into account, with the
> orientation being the default state, and the hardware / panel will
> tell us that, while the rotation would be a transformation from that
> default to whatever the user wants.
>
> More importantly, the orientation is a property of the hardware (ie,
> how the display has been assembled), while the rotation is a software
> construct.

FWIW, agreed. The immutable orientation property is exposed using the
drm_connector_init_panel_orientation_property() call. You then rotate to
take the orientation into account, to not display stuff sideways or
upside down wrt the natural orientation of the device.

BR,
Jani.

>
> And if the property being used to expose that is the rotation, I guess
> it would make sense to just use the same name and remain consistent.
>
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> _______________________________________________
> 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

Re: [PATCH v3 3/6] drm/modes: Allow to specify rotation and reflection on the commandline

[Noralf Trønnes]

Den 11.06.2019 14.49, skrev Maxime Ripard:
> Hi Noralf,
> 
> On Thu, Apr 18, 2019 at 06:40:42PM +0200, Noralf Trønnes wrote:
>> Den 18.04.2019 14.41, skrev Maxime Ripard:
>>> +	/**
>>> +	 * We want the rotation on the command line to overwrite
>>> +	 * whatever comes from the panel.
>>> +	 */
>>> +	cmdline = &connector->cmdline_mode;
>>> +	if (cmdline->specified &&
>>> +	    cmdline->rotation != DRM_MODE_ROTATE_0)
>>
>> I believe you need to drop that second check, otherwise rotate=0 will
>> not overwrite panel rotation.
> 
> Good catch :)
> 
>>> +		} else if (!strncmp(option, "reflect_x", delim - option)) {
>>> +			rotation |= DRM_MODE_REFLECT_X;
>>> +			sep = delim;
>>> +		} else if (!strncmp(option, "reflect_y", delim - option)) {
>>
>> I think you should drop reflect_x and _y for now since they're not
>> supported. People like me that only reads code and not documentation
>> (ahem..) will get the impression that this should work.
> 
> I'm not sure what you mean here, this is definitely supposed to
> work. Is there a limitation you're thinking of?
> 

It's this one in drm_client_panel_rotation() which limits rotation to
DRM_MODE_ROTATE_180:

	/*
	 * TODO: support 90 / 270 degree hardware rotation,
	 * depending on the hardware this may require the framebuffer
	 * to be in a specific tiling format.
	 */
	if (*rotation != DRM_MODE_ROTATE_180 || !plane->rotation_property)
		return false;

Noralf.

>> Documentation/fb/modedb.txt needs to be updated with this new video= option.
> 
> Will do, thanks!
> 
> maxime
> 
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/6] drm/modes: Allow to specify rotation and reflection on the commandline

[Noralf Trønnes]

Den 11.06.2019 14.49, skrev Maxime Ripard:
> Hi Noralf,
> 
> On Thu, Apr 18, 2019 at 06:40:42PM +0200, Noralf Trønnes wrote:
>> Den 18.04.2019 14.41, skrev Maxime Ripard:
>>> +	/**
>>> +	 * We want the rotation on the command line to overwrite
>>> +	 * whatever comes from the panel.
>>> +	 */
>>> +	cmdline = &connector->cmdline_mode;
>>> +	if (cmdline->specified &&
>>> +	    cmdline->rotation != DRM_MODE_ROTATE_0)
>>
>> I believe you need to drop that second check, otherwise rotate=0 will
>> not overwrite panel rotation.
> 
> Good catch :)
> 
>>> +		} else if (!strncmp(option, "reflect_x", delim - option)) {
>>> +			rotation |= DRM_MODE_REFLECT_X;
>>> +			sep = delim;
>>> +		} else if (!strncmp(option, "reflect_y", delim - option)) {
>>
>> I think you should drop reflect_x and _y for now since they're not
>> supported. People like me that only reads code and not documentation
>> (ahem..) will get the impression that this should work.
> 
> I'm not sure what you mean here, this is definitely supposed to
> work. Is there a limitation you're thinking of?
> 

It's this one in drm_client_panel_rotation() which limits rotation to
DRM_MODE_ROTATE_180:

	/*
	 * TODO: support 90 / 270 degree hardware rotation,
	 * depending on the hardware this may require the framebuffer
	 * to be in a specific tiling format.
	 */
	if (*rotation != DRM_MODE_ROTATE_180 || !plane->rotation_property)
		return false;

Noralf.

>> Documentation/fb/modedb.txt needs to be updated with this new video= option.
> 
> Will do, thanks!
> 
> maxime
> 
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/6] drm/modes: Allow to specify rotation and reflection on the commandline

[Noralf Trønnes]

Den 11.06.2019 15.20, skrev Maxime Ripard:
> Hi Noralf,
> 
> On Fri, Apr 19, 2019 at 10:53:28AM +0200, Noralf Trønnes wrote:
>> Den 18.04.2019 18.40, skrev Noralf Trønnes:
>>>
>>>
>>> Den 18.04.2019 14.41, skrev Maxime Ripard:
>>>> Rotations and reflections setup are needed in some scenarios to initialise
>>>> properly the initial framebuffer. Some drivers already had a bunch of
>>>> quirks to deal with this, such as either a private kernel command line
>>>> parameter (omapdss) or on the device tree (various panels).
>>>>
>>>> In order to accomodate this, let's create a video mode parameter to deal
>>>> with the rotation and reflexion.
>>>>
>>>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_client_modeset.c |  10 +++-
>>>>  drivers/gpu/drm/drm_modes.c          | 110 ++++++++++++++++++++++------
>>>>  include/drm/drm_connector.h          |   9 ++-
>>>>  3 files changed, 109 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
>>>> index f2869c82510c..15145d2c86d5 100644
>>>> --- a/drivers/gpu/drm/drm_client_modeset.c
>>>> +++ b/drivers/gpu/drm/drm_client_modeset.c
>>>> @@ -823,6 +823,7 @@ EXPORT_SYMBOL(drm_client_modeset_probe);
>>>>  bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotation)
>>>>  {
>>>>  	struct drm_connector *connector = modeset->connectors[0];
>>>> +	struct drm_cmdline_mode *cmdline;
>>>>  	struct drm_plane *plane = modeset->crtc->primary;
>>>>  	u64 valid_mask = 0;
>>>>  	unsigned int i;
>>>> @@ -844,6 +845,15 @@ bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotat
>>>>  		*rotation = DRM_MODE_ROTATE_0;
>>>>  	}
>>>>
>>>> +	/**
>>>> +	 * We want the rotation on the command line to overwrite
>>>> +	 * whatever comes from the panel.
>>>> +	 */
>>>> +	cmdline = &connector->cmdline_mode;
>>>> +	if (cmdline->specified &&
>>>> +	    cmdline->rotation != DRM_MODE_ROTATE_0)
>>>
>>> I believe you need to drop that second check, otherwise rotate=0 will
>>> not overwrite panel rotation.
>>>
>>>> +		*rotation = cmdline->rotation;
>>
>> I remembered that you wanted this to propagate to DRM userspace. That's
>> not happening here.
> 
> It's propated to the userspace through the plane's rotation property,
> I just checked.
> 

Oh, so the rotation property stores its value in plane_state->rotation.
And this is set in: drm_client_modeset_commit_atomic() ->
drm_client_panel_rotation(): plane_state->rotation = rotation;

>> The only way I see for that to happen, is to set
>> ->panel_orientation. And to repeat myself, imo that makes
>> 'orientation' a better name for this video= option.
> 
> orientation and rotation are two different things to me. The
> orientation of a panel for example is absolute, while the rotation is
> a transformation. In this particular case, I think that both the
> orientation and the rotation should be taken into account, with the
> orientation being the default state, and the hardware / panel will
> tell us that, while the rotation would be a transformation from that
> default to whatever the user wants.
> 
> More importantly, the orientation is a property of the hardware (ie,
> how the display has been assembled), while the rotation is a software
> construct.
> 
> And if the property being used to expose that is the rotation, I guess
> it would make sense to just use the same name and remain consistent.
> 

Ok, I see. Based on this, I would assume that rotation would be relative
to the orientation, but I see that in this patch rotation doesn't take
orintation into account, it just overwrites it. I don't how userspace
deals with rotation on top of orientation.

Noralf.

> Maxime
> 
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/6] drm/modes: Allow to specify rotation and reflection on the commandline

[Noralf Trønnes]

Den 11.06.2019 15.20, skrev Maxime Ripard:
> Hi Noralf,
> 
> On Fri, Apr 19, 2019 at 10:53:28AM +0200, Noralf Trønnes wrote:
>> Den 18.04.2019 18.40, skrev Noralf Trønnes:
>>>
>>>
>>> Den 18.04.2019 14.41, skrev Maxime Ripard:
>>>> Rotations and reflections setup are needed in some scenarios to initialise
>>>> properly the initial framebuffer. Some drivers already had a bunch of
>>>> quirks to deal with this, such as either a private kernel command line
>>>> parameter (omapdss) or on the device tree (various panels).
>>>>
>>>> In order to accomodate this, let's create a video mode parameter to deal
>>>> with the rotation and reflexion.
>>>>
>>>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_client_modeset.c |  10 +++-
>>>>  drivers/gpu/drm/drm_modes.c          | 110 ++++++++++++++++++++++------
>>>>  include/drm/drm_connector.h          |   9 ++-
>>>>  3 files changed, 109 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
>>>> index f2869c82510c..15145d2c86d5 100644
>>>> --- a/drivers/gpu/drm/drm_client_modeset.c
>>>> +++ b/drivers/gpu/drm/drm_client_modeset.c
>>>> @@ -823,6 +823,7 @@ EXPORT_SYMBOL(drm_client_modeset_probe);
>>>>  bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotation)
>>>>  {
>>>>  	struct drm_connector *connector = modeset->connectors[0];
>>>> +	struct drm_cmdline_mode *cmdline;
>>>>  	struct drm_plane *plane = modeset->crtc->primary;
>>>>  	u64 valid_mask = 0;
>>>>  	unsigned int i;
>>>> @@ -844,6 +845,15 @@ bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotat
>>>>  		*rotation = DRM_MODE_ROTATE_0;
>>>>  	}
>>>>
>>>> +	/**
>>>> +	 * We want the rotation on the command line to overwrite
>>>> +	 * whatever comes from the panel.
>>>> +	 */
>>>> +	cmdline = &connector->cmdline_mode;
>>>> +	if (cmdline->specified &&
>>>> +	    cmdline->rotation != DRM_MODE_ROTATE_0)
>>>
>>> I believe you need to drop that second check, otherwise rotate=0 will
>>> not overwrite panel rotation.
>>>
>>>> +		*rotation = cmdline->rotation;
>>
>> I remembered that you wanted this to propagate to DRM userspace. That's
>> not happening here.
> 
> It's propated to the userspace through the plane's rotation property,
> I just checked.
> 

Oh, so the rotation property stores its value in plane_state->rotation.
And this is set in: drm_client_modeset_commit_atomic() ->
drm_client_panel_rotation(): plane_state->rotation = rotation;

>> The only way I see for that to happen, is to set
>> ->panel_orientation. And to repeat myself, imo that makes
>> 'orientation' a better name for this video= option.
> 
> orientation and rotation are two different things to me. The
> orientation of a panel for example is absolute, while the rotation is
> a transformation. In this particular case, I think that both the
> orientation and the rotation should be taken into account, with the
> orientation being the default state, and the hardware / panel will
> tell us that, while the rotation would be a transformation from that
> default to whatever the user wants.
> 
> More importantly, the orientation is a property of the hardware (ie,
> how the display has been assembled), while the rotation is a software
> construct.
> 
> And if the property being used to expose that is the rotation, I guess
> it would make sense to just use the same name and remain consistent.
> 

Ok, I see. Based on this, I would assume that rotation would be relative
to the orientation, but I see that in this patch rotation doesn't take
orintation into account, it just overwrites it. I don't how userspace
deals with rotation on top of orientation.

Noralf.

> Maxime
> 
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/6] drm/modes: Allow to specify rotation and reflection on the commandline

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1234 bytes --]

Hi,

On Wed, Jun 12, 2019 at 02:43:44PM +0200, Noralf Trønnes wrote:
> Den 11.06.2019 14.49, skrev Maxime Ripard:
> >>> +		} else if (!strncmp(option, "reflect_x", delim - option)) {
> >>> +			rotation |= DRM_MODE_REFLECT_X;
> >>> +			sep = delim;
> >>> +		} else if (!strncmp(option, "reflect_y", delim - option)) {
> >>
> >> I think you should drop reflect_x and _y for now since they're not
> >> supported. People like me that only reads code and not documentation
> >> (ahem..) will get the impression that this should work.
> >
> > I'm not sure what you mean here, this is definitely supposed to
> > work. Is there a limitation you're thinking of?
> >
>
> It's this one in drm_client_panel_rotation() which limits rotation to
> DRM_MODE_ROTATE_180:
>
> 	/*
> 	 * TODO: support 90 / 270 degree hardware rotation,
> 	 * depending on the hardware this may require the framebuffer
> 	 * to be in a specific tiling format.
> 	 */
> 	if (*rotation != DRM_MODE_ROTATE_180 || !plane->rotation_property)
> 		return false;

Indeed, it doesn't work anymore since I rebased on your work. I'll fix
this, thanks!

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/6] drm/modes: Allow to specify rotation and reflection on the commandline

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1234 bytes --]

Hi,

On Wed, Jun 12, 2019 at 02:43:44PM +0200, Noralf Trønnes wrote:
> Den 11.06.2019 14.49, skrev Maxime Ripard:
> >>> +		} else if (!strncmp(option, "reflect_x", delim - option)) {
> >>> +			rotation |= DRM_MODE_REFLECT_X;
> >>> +			sep = delim;
> >>> +		} else if (!strncmp(option, "reflect_y", delim - option)) {
> >>
> >> I think you should drop reflect_x and _y for now since they're not
> >> supported. People like me that only reads code and not documentation
> >> (ahem..) will get the impression that this should work.
> >
> > I'm not sure what you mean here, this is definitely supposed to
> > work. Is there a limitation you're thinking of?
> >
>
> It's this one in drm_client_panel_rotation() which limits rotation to
> DRM_MODE_ROTATE_180:
>
> 	/*
> 	 * TODO: support 90 / 270 degree hardware rotation,
> 	 * depending on the hardware this may require the framebuffer
> 	 * to be in a specific tiling format.
> 	 */
> 	if (*rotation != DRM_MODE_ROTATE_180 || !plane->rotation_property)
> 		return false;

Indeed, it doesn't work anymore since I rebased on your work. I'll fix
this, thanks!

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH v3 3/6] drm/modes: Allow to specify rotation and reflection on the commandline

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1685 bytes --]

Hi,

On Wed, Jun 12, 2019 at 03:21:19PM +0200, Noralf Trønnes wrote:
> >> The only way I see for that to happen, is to set
> >> ->panel_orientation. And to repeat myself, imo that makes
> >> 'orientation' a better name for this video= option.
> >
> > orientation and rotation are two different things to me. The
> > orientation of a panel for example is absolute, while the rotation is
> > a transformation. In this particular case, I think that both the
> > orientation and the rotation should be taken into account, with the
> > orientation being the default state, and the hardware / panel will
> > tell us that, while the rotation would be a transformation from that
> > default to whatever the user wants.
> >
> > More importantly, the orientation is a property of the hardware (ie,
> > how the display has been assembled), while the rotation is a software
> > construct.
> >
> > And if the property being used to expose that is the rotation, I guess
> > it would make sense to just use the same name and remain consistent.
>
> Ok, I see. Based on this, I would assume that rotation would be relative
> to the orientation, but I see that in this patch rotation doesn't take
> orintation into account, it just overwrites it.

Yeah, that's a good point. I've updated the next version to add the
rotation on the command line to the orientation.

> I don't how userspace deals with rotation on top of orientation.

The orientation is exposed through the property, and the result is
available through the plane's rotation, so I guess that it's enough?

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/6] drm/modes: Allow to specify rotation and reflection on the commandline

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1685 bytes --]

Hi,

On Wed, Jun 12, 2019 at 03:21:19PM +0200, Noralf Trønnes wrote:
> >> The only way I see for that to happen, is to set
> >> ->panel_orientation. And to repeat myself, imo that makes
> >> 'orientation' a better name for this video= option.
> >
> > orientation and rotation are two different things to me. The
> > orientation of a panel for example is absolute, while the rotation is
> > a transformation. In this particular case, I think that both the
> > orientation and the rotation should be taken into account, with the
> > orientation being the default state, and the hardware / panel will
> > tell us that, while the rotation would be a transformation from that
> > default to whatever the user wants.
> >
> > More importantly, the orientation is a property of the hardware (ie,
> > how the display has been assembled), while the rotation is a software
> > construct.
> >
> > And if the property being used to expose that is the rotation, I guess
> > it would make sense to just use the same name and remain consistent.
>
> Ok, I see. Based on this, I would assume that rotation would be relative
> to the orientation, but I see that in this patch rotation doesn't take
> orintation into account, it just overwrites it.

Yeah, that's a good point. I've updated the next version to add the
rotation on the command line to the orientation.

> I don't how userspace deals with rotation on top of orientation.

The orientation is exposed through the property, and the result is
available through the plane's rotation, so I guess that it's enough?

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH v3 3/6] drm/modes: Allow to specify rotation and reflection on the commandline

[Noralf Trønnes]

Den 13.06.2019 14.50, skrev Maxime Ripard:
> Hi,
> 
> On Wed, Jun 12, 2019 at 03:21:19PM +0200, Noralf Trønnes wrote:
>>>> The only way I see for that to happen, is to set
>>>> ->panel_orientation. And to repeat myself, imo that makes
>>>> 'orientation' a better name for this video= option.
>>>
>>> orientation and rotation are two different things to me. The
>>> orientation of a panel for example is absolute, while the rotation is
>>> a transformation. In this particular case, I think that both the
>>> orientation and the rotation should be taken into account, with the
>>> orientation being the default state, and the hardware / panel will
>>> tell us that, while the rotation would be a transformation from that
>>> default to whatever the user wants.
>>>
>>> More importantly, the orientation is a property of the hardware (ie,
>>> how the display has been assembled), while the rotation is a software
>>> construct.
>>>
>>> And if the property being used to expose that is the rotation, I guess
>>> it would make sense to just use the same name and remain consistent.
>>
>> Ok, I see. Based on this, I would assume that rotation would be relative
>> to the orientation, but I see that in this patch rotation doesn't take
>> orintation into account, it just overwrites it.
> 
> Yeah, that's a good point. I've updated the next version to add the
> rotation on the command line to the orientation.
> 
>> I don't how userspace deals with rotation on top of orientation.
> 
> The orientation is exposed through the property, and the result is
> available through the plane's rotation, so I guess that it's enough?
> 

I was just wondering if Xserver/wayland applied rotation on top of
orientation or not. Maybe it would make sense to treat internal drm
clients the same. I understand that the DRM rotation property doesn't
apply on top orientation, just wondering how mutter/wayland (whoever is
in charge) does this. Not important to me, so feel free to disregard,
I'm just wondering.

Noralf.

> Maxime
> 
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/6] drm/modes: Allow to specify rotation and reflection on the commandline

[Noralf Trønnes]

Den 13.06.2019 14.50, skrev Maxime Ripard:
> Hi,
> 
> On Wed, Jun 12, 2019 at 03:21:19PM +0200, Noralf Trønnes wrote:
>>>> The only way I see for that to happen, is to set
>>>> ->panel_orientation. And to repeat myself, imo that makes
>>>> 'orientation' a better name for this video= option.
>>>
>>> orientation and rotation are two different things to me. The
>>> orientation of a panel for example is absolute, while the rotation is
>>> a transformation. In this particular case, I think that both the
>>> orientation and the rotation should be taken into account, with the
>>> orientation being the default state, and the hardware / panel will
>>> tell us that, while the rotation would be a transformation from that
>>> default to whatever the user wants.
>>>
>>> More importantly, the orientation is a property of the hardware (ie,
>>> how the display has been assembled), while the rotation is a software
>>> construct.
>>>
>>> And if the property being used to expose that is the rotation, I guess
>>> it would make sense to just use the same name and remain consistent.
>>
>> Ok, I see. Based on this, I would assume that rotation would be relative
>> to the orientation, but I see that in this patch rotation doesn't take
>> orintation into account, it just overwrites it.
> 
> Yeah, that's a good point. I've updated the next version to add the
> rotation on the command line to the orientation.
> 
>> I don't how userspace deals with rotation on top of orientation.
> 
> The orientation is exposed through the property, and the result is
> available through the plane's rotation, so I guess that it's enough?
> 

I was just wondering if Xserver/wayland applied rotation on top of
orientation or not. Maybe it would make sense to treat internal drm
clients the same. I understand that the DRM rotation property doesn't
apply on top orientation, just wondering how mutter/wayland (whoever is
in charge) does this. Not important to me, so feel free to disregard,
I'm just wondering.

Noralf.

> Maxime
> 
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel