[PATCH 1/2] drm: Export the command-line mode parser

[PATCH 1/2] drm: Export the command-line mode parser

[Chris Wilson]

In the absence of configuration data for providing the fixed mode for
a panel, I would like to be able to pass such modes along a separate
module paramenter. To do so, I then need to parse a modeline from a
string, which drm is already capable of. Export that capability to the
drivers.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Dave Airlie <airlied@linux.ie>
---
 drivers/gpu/drm/drm_fb_helper.c |  207 +++++++--------------------------------
 drivers/gpu/drm/drm_modes.c     |  154 +++++++++++++++++++++++++++++
 include/drm/drmP.h              |   25 +++++
 include/drm/drm_fb_helper.h     |   16 +---
 4 files changed, 216 insertions(+), 186 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 9507204..972a523 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -70,174 +70,50 @@ fail:
 }
 EXPORT_SYMBOL(drm_fb_helper_single_add_all_connectors);
 
-/**
- * drm_fb_helper_connector_parse_command_line - parse command line for connector
- * @connector - connector to parse line for
- * @mode_option - per connector mode option
- *
- * This parses the connector specific then generic command lines for
- * modes and options to configure the connector.
- *
- * This uses the same parameters as the fb modedb.c, except for extra
- *	<xres>x<yres>[M][R][-<bpp>][@<refresh>][i][m][eDd]
- *
- * enable/enable Digital/disable bit at the end
- */
-static bool drm_fb_helper_connector_parse_command_line(struct drm_fb_helper_connector *fb_helper_conn,
-						       const char *mode_option)
-{
-	const char *name;
-	unsigned int namelen;
-	int res_specified = 0, bpp_specified = 0, refresh_specified = 0;
-	unsigned int xres = 0, yres = 0, bpp = 32, refresh = 0;
-	int yres_specified = 0, cvt = 0, rb = 0, interlace = 0, margins = 0;
-	int i;
-	enum drm_connector_force force = DRM_FORCE_UNSPECIFIED;
-	struct drm_fb_helper_cmdline_mode *cmdline_mode;
-	struct drm_connector *connector;
-
-	if (!fb_helper_conn)
-		return false;
-	connector = fb_helper_conn->connector;
-
-	cmdline_mode = &fb_helper_conn->cmdline_mode;
-	if (!mode_option)
-		mode_option = fb_mode_option;
-
-	if (!mode_option) {
-		cmdline_mode->specified = false;
-		return false;
-	}
-
-	name = mode_option;
-	namelen = strlen(name);
-	for (i = namelen-1; i >= 0; i--) {
-		switch (name[i]) {
-		case '@':
-			namelen = i;
-			if (!refresh_specified && !bpp_specified &&
-			    !yres_specified) {
-				refresh = simple_strtol(&name[i+1], NULL, 10);
-				refresh_specified = 1;
-				if (cvt || rb)
-					cvt = 0;
-			} else
-				goto done;
-			break;
-		case '-':
-			namelen = i;
-			if (!bpp_specified && !yres_specified) {
-				bpp = simple_strtol(&name[i+1], NULL, 10);
-				bpp_specified = 1;
-				if (cvt || rb)
-					cvt = 0;
-			} else
-				goto done;
-			break;
-		case 'x':
-			if (!yres_specified) {
-				yres = simple_strtol(&name[i+1], NULL, 10);
-				yres_specified = 1;
-			} else
-				goto done;
-		case '0' ... '9':
-			break;
-		case 'M':
-			if (!yres_specified)
-				cvt = 1;
-			break;
-		case 'R':
-			if (cvt)
-				rb = 1;
-			break;
-		case 'm':
-			if (!cvt)
-				margins = 1;
-			break;
-		case 'i':
-			if (!cvt)
-				interlace = 1;
-			break;
-		case 'e':
-			force = DRM_FORCE_ON;
-			break;
-		case 'D':
-			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':
-			force = DRM_FORCE_OFF;
-			break;
-		default:
-			goto done;
-		}
-	}
-	if (i < 0 && yres_specified) {
-		xres = simple_strtol(name, NULL, 10);
-		res_specified = 1;
-	}
-done:
-
-	DRM_DEBUG_KMS("cmdline mode for connector %s %dx%d@%dHz%s%s%s\n",
-		drm_get_connector_name(connector), xres, yres,
-		(refresh) ? refresh : 60, (rb) ? " reduced blanking" :
-		"", (margins) ? " with margins" : "", (interlace) ?
-		" interlaced" : "");
-
-	if (force) {
-		const char *s;
-		switch (force) {
-		case DRM_FORCE_OFF: s = "OFF"; break;
-		case DRM_FORCE_ON_DIGITAL: s = "ON - dig"; break;
-		default:
-		case DRM_FORCE_ON: s = "ON"; break;
-		}
-
-		DRM_INFO("forcing %s connector %s\n",
-			 drm_get_connector_name(connector), s);
-		connector->force = force;
-	}
-
-	if (res_specified) {
-		cmdline_mode->specified = true;
-		cmdline_mode->xres = xres;
-		cmdline_mode->yres = yres;
-	}
-
-	if (refresh_specified) {
-		cmdline_mode->refresh_specified = true;
-		cmdline_mode->refresh = refresh;
-	}
-
-	if (bpp_specified) {
-		cmdline_mode->bpp_specified = true;
-		cmdline_mode->bpp = bpp;
-	}
-	cmdline_mode->rb = rb ? true : false;
-	cmdline_mode->cvt = cvt  ? true : false;
-	cmdline_mode->interlace = interlace ? true : false;
-
-	return true;
-}
-
 static int drm_fb_helper_parse_command_line(struct drm_fb_helper *fb_helper)
 {
 	struct drm_fb_helper_connector *fb_helper_conn;
 	int i;
 
 	for (i = 0; i < fb_helper->connector_count; i++) {
+		struct drm_cmdline_mode *mode;
+		struct drm_connector *connector;
 		char *option = NULL;
 
 		fb_helper_conn = fb_helper->connector_info[i];
+		connector = fb_helper_conn->connector;
+		mode = &fb_helper_conn->cmdline_mode;
 
 		/* do something on return - turn off connector maybe */
-		if (fb_get_options(drm_get_connector_name(fb_helper_conn->connector), &option))
+		if (fb_get_options(drm_get_connector_name(connector), &option))
 			continue;
 
-		drm_fb_helper_connector_parse_command_line(fb_helper_conn, option);
+		if (drm_mode_parse_command_line_for_connector(option,
+							      connector,
+							      mode)) {
+			if (mode->force) {
+				const char *s;
+				switch (mode->force) {
+				case DRM_FORCE_OFF: s = "OFF"; break;
+				case DRM_FORCE_ON_DIGITAL: s = "ON - dig"; break;
+				default:
+				case DRM_FORCE_ON: s = "ON"; break;
+				}
+
+				DRM_INFO("forcing %s connector %s\n",
+					 drm_get_connector_name(connector), s);
+				connector->force = mode->force;
+			}
+
+			DRM_DEBUG_KMS("cmdline mode for connector %s %dx%d@%dHz%s%s%s\n",
+				      drm_get_connector_name(connector),
+				      mode->xres, mode->yres,
+				      mode->refresh_specified ? mode->refresh : 60,
+				      mode->rb ? " reduced blanking" : "",
+				      mode->margins ? " with margins" : "",
+				      mode->interlace ?  " interlaced" : "");
+		}
+
 	}
 	return 0;
 }
@@ -888,7 +764,7 @@ int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	/* first up get a count of crtcs now in use and new min/maxes width/heights */
 	for (i = 0; i < fb_helper->connector_count; i++) {
 		struct drm_fb_helper_connector *fb_helper_conn = fb_helper->connector_info[i];
-		struct drm_fb_helper_cmdline_mode *cmdline_mode;
+		struct drm_cmdline_mode *cmdline_mode;
 
 		cmdline_mode = &fb_helper_conn->cmdline_mode;
 
@@ -1110,7 +986,7 @@ static struct drm_display_mode *drm_has_preferred_mode(struct drm_fb_helper_conn
 
 static bool drm_has_cmdline_mode(struct drm_fb_helper_connector *fb_connector)
 {
-	struct drm_fb_helper_cmdline_mode *cmdline_mode;
+	struct drm_cmdline_mode *cmdline_mode;
 	cmdline_mode = &fb_connector->cmdline_mode;
 	return cmdline_mode->specified;
 }
@@ -1118,7 +994,7 @@ static bool drm_has_cmdline_mode(struct drm_fb_helper_connector *fb_connector)
 static struct drm_display_mode *drm_pick_cmdline_mode(struct drm_fb_helper_connector *fb_helper_conn,
 						      int width, int height)
 {
-	struct drm_fb_helper_cmdline_mode *cmdline_mode;
+	struct drm_cmdline_mode *cmdline_mode;
 	struct drm_display_mode *mode = NULL;
 
 	cmdline_mode = &fb_helper_conn->cmdline_mode;
@@ -1150,19 +1026,8 @@ static struct drm_display_mode *drm_pick_cmdline_mode(struct drm_fb_helper_conne
 	}
 
 create_mode:
-	if (cmdline_mode->cvt)
-		mode = drm_cvt_mode(fb_helper_conn->connector->dev,
-				    cmdline_mode->xres, cmdline_mode->yres,
-				    cmdline_mode->refresh_specified ? cmdline_mode->refresh : 60,
-				    cmdline_mode->rb, cmdline_mode->interlace,
-				    cmdline_mode->margins);
-	else
-		mode = drm_gtf_mode(fb_helper_conn->connector->dev,
-				    cmdline_mode->xres, cmdline_mode->yres,
-				    cmdline_mode->refresh_specified ? cmdline_mode->refresh : 60,
-				    cmdline_mode->interlace,
-				    cmdline_mode->margins);
-	drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V);
+	mode = drm_mode_create_from_cmdline_mode(fb_helper_conn->connector->dev,
+						 cmdline_mode);
 	list_add(&mode->head, &fb_helper_conn->connector->modes);
 	return mode;
 }
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 25bf873..207b7eb 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -974,3 +974,157 @@ void drm_mode_connector_list_update(struct drm_connector *connector)
 	}
 }
 EXPORT_SYMBOL(drm_mode_connector_list_update);
+
+/**
+ * drm_mode_parse_command_line_for_connector - parse command line for connector
+ * @mode_option - per connector mode option
+ * @connector - connector to parse line for
+ *
+ * This parses the connector specific then generic command lines for
+ * modes and options to configure the connector.
+ *
+ * This uses the same parameters as the fb modedb.c, except for extra
+ *	<xres>x<yres>[M][R][-<bpp>][@<refresh>][i][m][eDd]
+ *
+ * enable/enable Digital/disable bit at the end
+ */
+bool drm_mode_parse_command_line_for_connector(const char *mode_option,
+					       struct drm_connector *connector,
+					       struct drm_cmdline_mode *mode)
+{
+	const char *name;
+	unsigned int namelen;
+	int res_specified = 0, bpp_specified = 0, refresh_specified = 0;
+	unsigned int xres = 0, yres = 0, bpp = 32, refresh = 0;
+	int yres_specified = 0, cvt = 0, rb = 0, interlace = 0, margins = 0;
+	int i;
+	enum drm_connector_force force = DRM_FORCE_UNSPECIFIED;
+
+	if (!mode_option)
+		mode_option = fb_mode_option;
+
+	if (!mode_option) {
+		mode->specified = false;
+		return false;
+	}
+
+	name = mode_option;
+	namelen = strlen(name);
+	for (i = namelen-1; i >= 0; i--) {
+		switch (name[i]) {
+		case '@':
+			namelen = i;
+			if (!refresh_specified && !bpp_specified &&
+			    !yres_specified) {
+				refresh = simple_strtol(&name[i+1], NULL, 10);
+				refresh_specified = 1;
+				if (cvt || rb)
+					cvt = 0;
+			} else
+				goto done;
+			break;
+		case '-':
+			namelen = i;
+			if (!bpp_specified && !yres_specified) {
+				bpp = simple_strtol(&name[i+1], NULL, 10);
+				bpp_specified = 1;
+				if (cvt || rb)
+					cvt = 0;
+			} else
+				goto done;
+			break;
+		case 'x':
+			if (!yres_specified) {
+				yres = simple_strtol(&name[i+1], NULL, 10);
+				yres_specified = 1;
+			} else
+				goto done;
+		case '0' ... '9':
+			break;
+		case 'M':
+			if (!yres_specified)
+				cvt = 1;
+			break;
+		case 'R':
+			if (cvt)
+				rb = 1;
+			break;
+		case 'm':
+			if (!cvt)
+				margins = 1;
+			break;
+		case 'i':
+			if (!cvt)
+				interlace = 1;
+			break;
+		case 'e':
+			force = DRM_FORCE_ON;
+			break;
+		case 'D':
+			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':
+			force = DRM_FORCE_OFF;
+			break;
+		default:
+			goto done;
+		}
+	}
+	if (i < 0 && yres_specified) {
+		xres = simple_strtol(name, NULL, 10);
+		res_specified = 1;
+	}
+done:
+	if (res_specified) {
+		mode->specified = true;
+		mode->xres = xres;
+		mode->yres = yres;
+	}
+
+	if (refresh_specified) {
+		mode->refresh_specified = true;
+		mode->refresh = refresh;
+	}
+
+	if (bpp_specified) {
+		mode->bpp_specified = true;
+		mode->bpp = bpp;
+	}
+	mode->rb = rb ? true : false;
+	mode->cvt = cvt  ? true : false;
+	mode->interlace = interlace ? true : false;
+	mode->force = force;
+
+	return true;
+}
+EXPORT_SYMBOL(drm_mode_parse_command_line_for_connector);
+
+struct drm_display_mode *
+drm_mode_create_from_cmdline_mode(struct drm_device *dev,
+				  struct drm_cmdline_mode *cmd)
+{
+	struct drm_display_mode *mode;
+
+	if (cmd->cvt)
+		mode = drm_cvt_mode(dev,
+				    cmd->xres, cmd->yres,
+				    cmd->refresh_specified ? cmd->refresh : 60,
+				    cmd->rb, cmd->interlace,
+				    cmd->margins);
+	else
+		mode = drm_gtf_mode(dev,
+				    cmd->xres, cmd->yres,
+				    cmd->refresh_specified ? cmd->refresh : 60,
+				    cmd->interlace,
+				    cmd->margins);
+	if (!mode)
+		return NULL;
+
+	drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V);
+	return mode;
+}
+EXPORT_SYMBOL(drm_mode_create_from_cmdline_mode);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index ad5770f..232a313 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1000,6 +1000,22 @@ struct drm_minor {
 	struct drm_mode_group mode_group;
 };
 
+/* mode specified on the command line */
+struct drm_cmdline_mode {
+	bool specified;
+	bool refresh_specified;
+	bool bpp_specified;
+	int xres, yres;
+	int bpp;
+	int refresh;
+	bool rb;
+	bool interlace;
+	bool cvt;
+	bool margins;
+	enum drm_connector_force force;
+};
+
+
 struct drm_pending_vblank_event {
 	struct drm_pending_event base;
 	int pipe;
@@ -1395,6 +1411,15 @@ extern int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
 						 struct drm_crtc *refcrtc);
 extern void drm_calc_timestamping_constants(struct drm_crtc *crtc);
 
+extern bool
+drm_mode_parse_command_line_for_connector(const char *mode_option,
+					  struct drm_connector *connector,
+					  struct drm_cmdline_mode *mode);
+
+extern struct drm_display_mode *
+drm_mode_create_from_cmdline_mode(struct drm_device *dev,
+				  struct drm_cmdline_mode *cmd);
+
 /* Modesetting support */
 extern void drm_vblank_pre_modeset(struct drm_device *dev, int crtc);
 extern void drm_vblank_post_modeset(struct drm_device *dev, int crtc);
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index f22e7fe..4e66488 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -40,20 +40,6 @@ struct drm_fb_helper_crtc {
 	struct drm_display_mode *desired_mode;
 };
 
-/* mode specified on the command line */
-struct drm_fb_helper_cmdline_mode {
-	bool specified;
-	bool refresh_specified;
-	bool bpp_specified;
-	int xres, yres;
-	int bpp;
-	int refresh;
-	bool rb;
-	bool interlace;
-	bool cvt;
-	bool margins;
-};
-
 struct drm_fb_helper_surface_size {
 	u32 fb_width;
 	u32 fb_height;
@@ -74,8 +60,8 @@ struct drm_fb_helper_funcs {
 };
 
 struct drm_fb_helper_connector {
-	struct drm_fb_helper_cmdline_mode cmdline_mode;
 	struct drm_connector *connector;
+	struct drm_cmdline_mode cmdline_mode;
 };
 
 struct drm_fb_helper {
-- 
1.7.4.1

[PATCH 2/2] drm/i915/lvds: Use i915.lvds_fixed_mode= as a last resort

[Chris Wilson]

If we can find no other reliable source of panel configuration data,
turn to the user and see if they have a passed a mode line (ala video=)
through the i915.lvds_fixed_mode= string.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Oliver Seitz <info@vtnd.de>
Cc: Mike Isely <isely@isely.net>
Cc: Dave Airlied <airlied@linux.ie>
---
 drivers/gpu/drm/i915/i915_drv.c   |    4 +++
 drivers/gpu/drm/i915/i915_drv.h   |    3 +-
 drivers/gpu/drm/i915/intel_lvds.c |   46 ++++++++++++++++++++++++++++---------
 3 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 16a2532..4a618f6 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -67,6 +67,10 @@ module_param_named(vbt_sdvo_panel_type, i915_vbt_sdvo_panel_type, int, 0600);
 static bool i915_try_reset = true;
 module_param_named(reset, i915_try_reset, bool, 0600);
 
+char *i915_lvds_fixed_mode;
+module_param_named(lvds_fixed_mode, i915_lvds_fixed_mode, charp, 0600);
+MODULE_PARM_DESC(lvds_fixed_mode, "specify the mode line for the LVDS panel to be used in the absence of any configuration data, using the video= format");
+
 unsigned int i915_lvds_24bit = 0;
 module_param_named(lvds_24bit, i915_lvds_24bit, int, 0600);
 MODULE_PARM_DESC(lvds_24bit, "LVDS 24 bit pixel format: 0=leave untouched (default), 1=24 bit '2.0' format, 2=24 bit '2.1' format, 3=force older 18 bit format");
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2112af3..c6cc4e2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -977,12 +977,13 @@ extern unsigned int i915_fbpercrtc;
 extern int i915_panel_ignore_lid;
 extern unsigned int i915_powersave;
 extern unsigned int i915_semaphores;
+extern char *i915_lvds_fixed_mode;
 extern unsigned int i915_lvds_channels;
+extern unsigned int i915_lvds_24bit;
 extern unsigned int i915_lvds_downclock;
 extern unsigned int i915_panel_use_ssc;
 extern int i915_vbt_sdvo_panel_type;
 extern unsigned int i915_enable_rc6;
-extern unsigned int i915_lvds_24bit;
 
 extern int i915_suspend(struct drm_device *dev, pm_message_t state);
 extern int i915_resume(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index a562bd2..32b86ea 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -924,6 +924,11 @@ bool intel_lvds_init(struct drm_device *dev)
 	 *    if none of the above, no panel
 	 * 4) make sure lid is open
 	 *    if closed, act like it's not there for now
+	 *
+	 * Finally, we allow the user to specify his own mode. We do this
+	 * last because we want to prevent the user from damaging their
+	 * hardware with a dangerous modeline. Though we may eventually
+	 * be forced to add an override for truly broken machines.
 	 */
 
 	/*
@@ -982,19 +987,38 @@ bool intel_lvds_init(struct drm_device *dev)
 	 */
 
 	/* Ironlake: FIXME if still fail, not try pipe mode now */
-	if (HAS_PCH_SPLIT(dev))
-		goto failed;
+	if (!HAS_PCH_SPLIT(dev)) {
+		lvds = I915_READ(LVDS);
+		pipe = (lvds & LVDS_PIPEB_SELECT) ? 1 : 0;
+		crtc = intel_get_crtc_for_pipe(dev, pipe);
+
+		if (crtc && (lvds & LVDS_PORT_EN)) {
+			intel_lvds->fixed_mode = intel_crtc_mode_get(dev, crtc);
+			if (intel_lvds->fixed_mode) {
+				intel_lvds->fixed_mode->type |=
+					DRM_MODE_TYPE_PREFERRED;
+				goto out;
+			}
+		}
+	}
 
-	lvds = I915_READ(LVDS);
-	pipe = (lvds & LVDS_PIPEB_SELECT) ? 1 : 0;
-	crtc = intel_get_crtc_for_pipe(dev, pipe);
+	/* No panel cnnfiguration data, and nothing already driving the panel
+	 * at its preferred mode. Check to see if the user provided this vital
+	 * bit of information...
+	 */
+	if (i915_lvds_fixed_mode) {
+		struct drm_cmdline_mode mode;
 
-	if (crtc && (lvds & LVDS_PORT_EN)) {
-		intel_lvds->fixed_mode = intel_crtc_mode_get(dev, crtc);
-		if (intel_lvds->fixed_mode) {
-			intel_lvds->fixed_mode->type |=
-				DRM_MODE_TYPE_PREFERRED;
-			goto out;
+		if (drm_mode_parse_command_line_for_connector(i915_lvds_fixed_mode,
+							      connector,
+							      &mode)) {
+			intel_lvds->fixed_mode =
+				drm_mode_create_from_cmdline_mode(dev, &mode);
+			if (intel_lvds->fixed_mode) {
+				intel_lvds->fixed_mode->type |=
+					DRM_MODE_TYPE_PREFERRED;
+				goto out;
+			}
 		}
 	}
 
-- 
1.7.4.1

Re: [PATCH 2/2] drm/i915/lvds: Use i915.lvds_fixed_mode= as a last resort

[Mike Isely]

Chris:

Can you specify the commit this patch was built on top of?  I'm still 
not totally fluent in git.  I did get it to apply cleanly but my build 
attempt failed due to errors which suggest stuff is needed from other 
commits missing from my build branch :-( For example, 
"drm_mode_parse_command_line_for_connector()" is missing.

  -Mike


On Sun, 17 Apr 2011, Chris Wilson wrote:

> If we can find no other reliable source of panel configuration data,
> turn to the user and see if they have a passed a mode line (ala video=)
> through the i915.lvds_fixed_mode= string.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Oliver Seitz <info@vtnd.de>
> Cc: Mike Isely <isely@isely.net>
> Cc: Dave Airlied <airlied@linux.ie>
> ---
>  drivers/gpu/drm/i915/i915_drv.c   |    4 +++
>  drivers/gpu/drm/i915/i915_drv.h   |    3 +-
>  drivers/gpu/drm/i915/intel_lvds.c |   46 ++++++++++++++++++++++++++++---------
>  3 files changed, 41 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 16a2532..4a618f6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -67,6 +67,10 @@ module_param_named(vbt_sdvo_panel_type, i915_vbt_sdvo_panel_type, int, 0600);
>  static bool i915_try_reset = true;
>  module_param_named(reset, i915_try_reset, bool, 0600);
>  
> +char *i915_lvds_fixed_mode;
> +module_param_named(lvds_fixed_mode, i915_lvds_fixed_mode, charp, 0600);
> +MODULE_PARM_DESC(lvds_fixed_mode, "specify the mode line for the LVDS panel to be used in the absence of any configuration data, using the video= format");
> +
>  unsigned int i915_lvds_24bit = 0;
>  module_param_named(lvds_24bit, i915_lvds_24bit, int, 0600);
>  MODULE_PARM_DESC(lvds_24bit, "LVDS 24 bit pixel format: 0=leave untouched (default), 1=24 bit '2.0' format, 2=24 bit '2.1' format, 3=force older 18 bit format");
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2112af3..c6cc4e2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -977,12 +977,13 @@ extern unsigned int i915_fbpercrtc;
>  extern int i915_panel_ignore_lid;
>  extern unsigned int i915_powersave;
>  extern unsigned int i915_semaphores;
> +extern char *i915_lvds_fixed_mode;
>  extern unsigned int i915_lvds_channels;
> +extern unsigned int i915_lvds_24bit;
>  extern unsigned int i915_lvds_downclock;
>  extern unsigned int i915_panel_use_ssc;
>  extern int i915_vbt_sdvo_panel_type;
>  extern unsigned int i915_enable_rc6;
> -extern unsigned int i915_lvds_24bit;
>  
>  extern int i915_suspend(struct drm_device *dev, pm_message_t state);
>  extern int i915_resume(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index a562bd2..32b86ea 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -924,6 +924,11 @@ bool intel_lvds_init(struct drm_device *dev)
>  	 *    if none of the above, no panel
>  	 * 4) make sure lid is open
>  	 *    if closed, act like it's not there for now
> +	 *
> +	 * Finally, we allow the user to specify his own mode. We do this
> +	 * last because we want to prevent the user from damaging their
> +	 * hardware with a dangerous modeline. Though we may eventually
> +	 * be forced to add an override for truly broken machines.
>  	 */
>  
>  	/*
> @@ -982,19 +987,38 @@ bool intel_lvds_init(struct drm_device *dev)
>  	 */
>  
>  	/* Ironlake: FIXME if still fail, not try pipe mode now */
> -	if (HAS_PCH_SPLIT(dev))
> -		goto failed;
> +	if (!HAS_PCH_SPLIT(dev)) {
> +		lvds = I915_READ(LVDS);
> +		pipe = (lvds & LVDS_PIPEB_SELECT) ? 1 : 0;
> +		crtc = intel_get_crtc_for_pipe(dev, pipe);
> +
> +		if (crtc && (lvds & LVDS_PORT_EN)) {
> +			intel_lvds->fixed_mode = intel_crtc_mode_get(dev, crtc);
> +			if (intel_lvds->fixed_mode) {
> +				intel_lvds->fixed_mode->type |=
> +					DRM_MODE_TYPE_PREFERRED;
> +				goto out;
> +			}
> +		}
> +	}
>  
> -	lvds = I915_READ(LVDS);
> -	pipe = (lvds & LVDS_PIPEB_SELECT) ? 1 : 0;
> -	crtc = intel_get_crtc_for_pipe(dev, pipe);
> +	/* No panel cnnfiguration data, and nothing already driving the panel
> +	 * at its preferred mode. Check to see if the user provided this vital
> +	 * bit of information...
> +	 */
> +	if (i915_lvds_fixed_mode) {
> +		struct drm_cmdline_mode mode;
>  
> -	if (crtc && (lvds & LVDS_PORT_EN)) {
> -		intel_lvds->fixed_mode = intel_crtc_mode_get(dev, crtc);
> -		if (intel_lvds->fixed_mode) {
> -			intel_lvds->fixed_mode->type |=
> -				DRM_MODE_TYPE_PREFERRED;
> -			goto out;
> +		if (drm_mode_parse_command_line_for_connector(i915_lvds_fixed_mode,
> +							      connector,
> +							      &mode)) {
> +			intel_lvds->fixed_mode =
> +				drm_mode_create_from_cmdline_mode(dev, &mode);
> +			if (intel_lvds->fixed_mode) {
> +				intel_lvds->fixed_mode->type |=
> +					DRM_MODE_TYPE_PREFERRED;
> +				goto out;
> +			}
>  		}
>  	}
>  
> 

-- 

Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

Re: [PATCH 2/2] drm/i915/lvds: Use i915.lvds_fixed_mode= as a last resort

[Mike Isely]

Never mind.  As usual the answer is right in front of my nose - the 
other patch in this series.  I was only focusing on the second patch not 
the first one.  D'Oh!

  -Mike


On Mon, 18 Apr 2011, Mike Isely wrote:

> 
> Chris:
> 
> Can you specify the commit this patch was built on top of?  I'm still 
> not totally fluent in git.  I did get it to apply cleanly but my build 
> attempt failed due to errors which suggest stuff is needed from other 
> commits missing from my build branch :-( For example, 
> "drm_mode_parse_command_line_for_connector()" is missing.
> 
>   -Mike
> 
> 
> On Sun, 17 Apr 2011, Chris Wilson wrote:
> 
> > If we can find no other reliable source of panel configuration data,
> > turn to the user and see if they have a passed a mode line (ala video=)
> > through the i915.lvds_fixed_mode= string.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Oliver Seitz <info@vtnd.de>
> > Cc: Mike Isely <isely@isely.net>
> > Cc: Dave Airlied <airlied@linux.ie>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c   |    4 +++
> >  drivers/gpu/drm/i915/i915_drv.h   |    3 +-
> >  drivers/gpu/drm/i915/intel_lvds.c |   46 ++++++++++++++++++++++++++++---------
> >  3 files changed, 41 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 16a2532..4a618f6 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -67,6 +67,10 @@ module_param_named(vbt_sdvo_panel_type, i915_vbt_sdvo_panel_type, int, 0600);
> >  static bool i915_try_reset = true;
> >  module_param_named(reset, i915_try_reset, bool, 0600);
> >  
> > +char *i915_lvds_fixed_mode;
> > +module_param_named(lvds_fixed_mode, i915_lvds_fixed_mode, charp, 0600);
> > +MODULE_PARM_DESC(lvds_fixed_mode, "specify the mode line for the LVDS panel to be used in the absence of any configuration data, using the video= format");
> > +
> >  unsigned int i915_lvds_24bit = 0;
> >  module_param_named(lvds_24bit, i915_lvds_24bit, int, 0600);
> >  MODULE_PARM_DESC(lvds_24bit, "LVDS 24 bit pixel format: 0=leave untouched (default), 1=24 bit '2.0' format, 2=24 bit '2.1' format, 3=force older 18 bit format");
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 2112af3..c6cc4e2 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -977,12 +977,13 @@ extern unsigned int i915_fbpercrtc;
> >  extern int i915_panel_ignore_lid;
> >  extern unsigned int i915_powersave;
> >  extern unsigned int i915_semaphores;
> > +extern char *i915_lvds_fixed_mode;
> >  extern unsigned int i915_lvds_channels;
> > +extern unsigned int i915_lvds_24bit;
> >  extern unsigned int i915_lvds_downclock;
> >  extern unsigned int i915_panel_use_ssc;
> >  extern int i915_vbt_sdvo_panel_type;
> >  extern unsigned int i915_enable_rc6;
> > -extern unsigned int i915_lvds_24bit;
> >  
> >  extern int i915_suspend(struct drm_device *dev, pm_message_t state);
> >  extern int i915_resume(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index a562bd2..32b86ea 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -924,6 +924,11 @@ bool intel_lvds_init(struct drm_device *dev)
> >  	 *    if none of the above, no panel
> >  	 * 4) make sure lid is open
> >  	 *    if closed, act like it's not there for now
> > +	 *
> > +	 * Finally, we allow the user to specify his own mode. We do this
> > +	 * last because we want to prevent the user from damaging their
> > +	 * hardware with a dangerous modeline. Though we may eventually
> > +	 * be forced to add an override for truly broken machines.
> >  	 */
> >  
> >  	/*
> > @@ -982,19 +987,38 @@ bool intel_lvds_init(struct drm_device *dev)
> >  	 */
> >  
> >  	/* Ironlake: FIXME if still fail, not try pipe mode now */
> > -	if (HAS_PCH_SPLIT(dev))
> > -		goto failed;
> > +	if (!HAS_PCH_SPLIT(dev)) {
> > +		lvds = I915_READ(LVDS);
> > +		pipe = (lvds & LVDS_PIPEB_SELECT) ? 1 : 0;
> > +		crtc = intel_get_crtc_for_pipe(dev, pipe);
> > +
> > +		if (crtc && (lvds & LVDS_PORT_EN)) {
> > +			intel_lvds->fixed_mode = intel_crtc_mode_get(dev, crtc);
> > +			if (intel_lvds->fixed_mode) {
> > +				intel_lvds->fixed_mode->type |=
> > +					DRM_MODE_TYPE_PREFERRED;
> > +				goto out;
> > +			}
> > +		}
> > +	}
> >  
> > -	lvds = I915_READ(LVDS);
> > -	pipe = (lvds & LVDS_PIPEB_SELECT) ? 1 : 0;
> > -	crtc = intel_get_crtc_for_pipe(dev, pipe);
> > +	/* No panel cnnfiguration data, and nothing already driving the panel
> > +	 * at its preferred mode. Check to see if the user provided this vital
> > +	 * bit of information...
> > +	 */
> > +	if (i915_lvds_fixed_mode) {
> > +		struct drm_cmdline_mode mode;
> >  
> > -	if (crtc && (lvds & LVDS_PORT_EN)) {
> > -		intel_lvds->fixed_mode = intel_crtc_mode_get(dev, crtc);
> > -		if (intel_lvds->fixed_mode) {
> > -			intel_lvds->fixed_mode->type |=
> > -				DRM_MODE_TYPE_PREFERRED;
> > -			goto out;
> > +		if (drm_mode_parse_command_line_for_connector(i915_lvds_fixed_mode,
> > +							      connector,
> > +							      &mode)) {
> > +			intel_lvds->fixed_mode =
> > +				drm_mode_create_from_cmdline_mode(dev, &mode);
> > +			if (intel_lvds->fixed_mode) {
> > +				intel_lvds->fixed_mode->type |=
> > +					DRM_MODE_TYPE_PREFERRED;
> > +				goto out;
> > +			}
> >  		}
> >  	}
> >  
> > 
> 
> 

-- 

Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

Re: [PATCH 2/2] drm/i915/lvds: Use i915.lvds_fixed_mode= as a last resort

[Mike Isely]

Chris:

I've tested this patch and it doesn't help us here.  Even if I add 
lvds_fixed_mode=<whatever>, the display still comes up with the messed 
up configuration from the motherboard's completely ignorant BIOS.  It 
appears that lvds_fixed_mode is being ignored by the driver.

I think there's still a misunderstanding of the situation.  This can't 
be treated as a "last resort"; it really should be treated as "oh look 
the user is telling us what to do, gee maybe it's because the detected 
settings are actually wrong".  From the patch comment:

> If we can find no other reliable source of panel configuration data,
                          ^^^^^^^^

> turn to the user and see if they have a passed a mode line (ala video=)
> through the i915.lvds_fixed_mode= string.
> 

Note the word "reliable".  Is this patch assuming that if there's 
configuration data in the BIOS that it is considered to be "reliable"?  

See that's the entire point - if the LVDS display device is discrete, 
i.e. a separate component from the motherboard, then the BIOS is 
unrelated to the display device so how in the world can it possibly be 
relied upon to provide good configuration data?  In a laptop with an 
integral display, sure no problem.  But here in the embedded world with 
COTS parts, we're combining an SBC with a GMA-4500 from one vendor, with 
its canned BIOS, with a special-purpose LVDS-driven display device from 
another vendor.  The display device has non-standard unusual timing 
requirements that the BIOS will never understand.  We however understand 
exactly what those timing requirements are but there's no way to tell 
this to hardware if the driving software insists on preferring the crap 
from the BIOS from the SBC vendor who simply has no clue about what 
we're connecting to the SBC's LVDS port.

The patch I had posted that implemented the lvds_fixed switch to disable 
scaling solves the problem for us because the switch in combination with 
the specified mode allows us to force the driver to ignore the BIOS.  
This is also what the UMS patch (xorg intel driver) did that I 
implemented back in 2008.  The lvds_fixed patch I had submitted in this 
sense really only restored functionality that had already been 
previously available with UMS.

But if this lvds_fixed_mode patch is still allowing the driver to ignore 
the user-specified actual lvds_fixed_mode setting in favor of the wrong 
BIOS set-up timings, then we're no better off than before.

Another comment from the patch:

+	 *
+	 * Finally, we allow the user to specify his own mode. We do this
+	 * last because we want to prevent the user from damaging their
+	 * hardware with a dangerous modeline. Though we may eventually
+	 * be forced to add an override for truly broken machines.
 
As I said, the problem here is that the display device is not integral 
to the system, and the BIOS simply cannot have any foreknowledge of what 
the display device wants.  This is not "broken" behavior, it's a fact of 
life when the COTS SBC vendor and the display vendor are different 
entities.

Do you really think it's wise to ignore the user when he went out of his 
way to specify these timings?  Sure, print a warning, make it obscure, 
add lots of caveats.  But the fact is really if the user went to the 
trouble to figure out how to specify video timings to a kernel module, 
then there's a pretty good chance that he knows what he's doing.  
Trying to be "smarter" than the user here I really think is wrong.

Until this patch can actually override the BIOS, it isn't a functional 
replacement for the lvds_fixed patch I had posted earlier.  Sorry :-(

Maybe there should also be an "ignore preset LVDS configuration" switch 
added with a comment visible from modinfo to the effect of "use at your 
own risk since this might damage hardware"?

  -Mike




On Sun, 17 Apr 2011, Chris Wilson wrote:

> If we can find no other reliable source of panel configuration data,
> turn to the user and see if they have a passed a mode line (ala video=)
> through the i915.lvds_fixed_mode= string.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Oliver Seitz <info@vtnd.de>
> Cc: Mike Isely <isely@isely.net>
> Cc: Dave Airlied <airlied@linux.ie>
> ---
>  drivers/gpu/drm/i915/i915_drv.c   |    4 +++
>  drivers/gpu/drm/i915/i915_drv.h   |    3 +-
>  drivers/gpu/drm/i915/intel_lvds.c |   46 ++++++++++++++++++++++++++++---------
>  3 files changed, 41 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 16a2532..4a618f6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -67,6 +67,10 @@ module_param_named(vbt_sdvo_panel_type, i915_vbt_sdvo_panel_type, int, 0600);
>  static bool i915_try_reset = true;
>  module_param_named(reset, i915_try_reset, bool, 0600);
>  
> +char *i915_lvds_fixed_mode;
> +module_param_named(lvds_fixed_mode, i915_lvds_fixed_mode, charp, 0600);
> +MODULE_PARM_DESC(lvds_fixed_mode, "specify the mode line for the LVDS panel to be used in the absence of any configuration data, using the video= format");
> +
>  unsigned int i915_lvds_24bit = 0;
>  module_param_named(lvds_24bit, i915_lvds_24bit, int, 0600);
>  MODULE_PARM_DESC(lvds_24bit, "LVDS 24 bit pixel format: 0=leave untouched (default), 1=24 bit '2.0' format, 2=24 bit '2.1' format, 3=force older 18 bit format");
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2112af3..c6cc4e2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -977,12 +977,13 @@ extern unsigned int i915_fbpercrtc;
>  extern int i915_panel_ignore_lid;
>  extern unsigned int i915_powersave;
>  extern unsigned int i915_semaphores;
> +extern char *i915_lvds_fixed_mode;
>  extern unsigned int i915_lvds_channels;
> +extern unsigned int i915_lvds_24bit;
>  extern unsigned int i915_lvds_downclock;
>  extern unsigned int i915_panel_use_ssc;
>  extern int i915_vbt_sdvo_panel_type;
>  extern unsigned int i915_enable_rc6;
> -extern unsigned int i915_lvds_24bit;
>  
>  extern int i915_suspend(struct drm_device *dev, pm_message_t state);
>  extern int i915_resume(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index a562bd2..32b86ea 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -924,6 +924,11 @@ bool intel_lvds_init(struct drm_device *dev)
>  	 *    if none of the above, no panel
>  	 * 4) make sure lid is open
>  	 *    if closed, act like it's not there for now
> +	 *
> +	 * Finally, we allow the user to specify his own mode. We do this
> +	 * last because we want to prevent the user from damaging their
> +	 * hardware with a dangerous modeline. Though we may eventually
> +	 * be forced to add an override for truly broken machines.
>  	 */
>  
>  	/*
> @@ -982,19 +987,38 @@ bool intel_lvds_init(struct drm_device *dev)
>  	 */
>  
>  	/* Ironlake: FIXME if still fail, not try pipe mode now */
> -	if (HAS_PCH_SPLIT(dev))
> -		goto failed;
> +	if (!HAS_PCH_SPLIT(dev)) {
> +		lvds = I915_READ(LVDS);
> +		pipe = (lvds & LVDS_PIPEB_SELECT) ? 1 : 0;
> +		crtc = intel_get_crtc_for_pipe(dev, pipe);
> +
> +		if (crtc && (lvds & LVDS_PORT_EN)) {
> +			intel_lvds->fixed_mode = intel_crtc_mode_get(dev, crtc);
> +			if (intel_lvds->fixed_mode) {
> +				intel_lvds->fixed_mode->type |=
> +					DRM_MODE_TYPE_PREFERRED;
> +				goto out;
> +			}
> +		}
> +	}
>  
> -	lvds = I915_READ(LVDS);
> -	pipe = (lvds & LVDS_PIPEB_SELECT) ? 1 : 0;
> -	crtc = intel_get_crtc_for_pipe(dev, pipe);
> +	/* No panel cnnfiguration data, and nothing already driving the panel
> +	 * at its preferred mode. Check to see if the user provided this vital
> +	 * bit of information...
> +	 */
> +	if (i915_lvds_fixed_mode) {
> +		struct drm_cmdline_mode mode;
>  
> -	if (crtc && (lvds & LVDS_PORT_EN)) {
> -		intel_lvds->fixed_mode = intel_crtc_mode_get(dev, crtc);
> -		if (intel_lvds->fixed_mode) {
> -			intel_lvds->fixed_mode->type |=
> -				DRM_MODE_TYPE_PREFERRED;
> -			goto out;
> +		if (drm_mode_parse_command_line_for_connector(i915_lvds_fixed_mode,
> +							      connector,
> +							      &mode)) {
> +			intel_lvds->fixed_mode =
> +				drm_mode_create_from_cmdline_mode(dev, &mode);
> +			if (intel_lvds->fixed_mode) {
> +				intel_lvds->fixed_mode->type |=
> +					DRM_MODE_TYPE_PREFERRED;
> +				goto out;
> +			}
>  		}
>  	}
>  
> 

-- 

Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

Re: [PATCH 2/2] drm/i915/lvds: Use i915.lvds_fixed_mode= as a last resort

[Chris Wilson]

On Wed, 20 Apr 2011 14:48:36 -0500 (CDT), Mike Isely <isely@isely.net> wrote:
> 
> Chris:
> 
> I've tested this patch and it doesn't help us here.  Even if I add 
> lvds_fixed_mode=<whatever>, the display still comes up with the messed 
> up configuration from the motherboard's completely ignorant BIOS.  It 
> appears that lvds_fixed_mode is being ignored by the driver.

You can test the functionality of the patch by parsing
i915.lvds_fixed_mode first rather than last.

Then I just have to weigh up the wishes of Dave who has hordes of users
desperate to fry their hardware, versus the tiny number who truly need
an override and know what they are doing...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915/lvds: Use i915.lvds_fixed_mode= as a last resort

[Mike Isely]

On Wed, 20 Apr 2011, Chris Wilson wrote:

> On Wed, 20 Apr 2011 14:48:36 -0500 (CDT), Mike Isely <isely@isely.net> wrote:
> > 
> > Chris:
> > 
> > I've tested this patch and it doesn't help us here.  Even if I add 
> > lvds_fixed_mode=<whatever>, the display still comes up with the messed 
> > up configuration from the motherboard's completely ignorant BIOS.  It 
> > appears that lvds_fixed_mode is being ignored by the driver.
> 
> You can test the functionality of the patch by parsing
> i915.lvds_fixed_mode first rather than last.

I will test for that - it was the next thing I was going to dig into.


> 
> Then I just have to weigh up the wishes of Dave who has hordes of users
> desperate to fry their hardware, versus the tiny number who truly need
> an override and know what they are doing...

I understand your sentiment here.  But please also consider that this 
same feature existed on the UMS side for 3 years and I don't remember 
hearing about any flood of fried hardware because of it...

And really, this should be all about making legitimate capabilities 
available, not deliberately blocking them.

A good compromise, if this is really viewed as a legitimate problem 
(which I don't think it should be), would be to add an "I know what I'm 
doing, darnit" switch to the driver which would enable potentially 
hazardous tweaks.  Then one can hang all the "here there be dragons", 
"do not enter", "don't blame me for frying your hardware", etc caveats 
and warnings onto that switch...

  -Mike


-- 

Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

Re: [PATCH 2/2] drm/i915/lvds: Use i915.lvds_fixed_mode= as a last resort

[Mike Isely]

On Wed, 20 Apr 2011, Mike Isely wrote:

> On Wed, 20 Apr 2011, Chris Wilson wrote:
> 
> > On Wed, 20 Apr 2011 14:48:36 -0500 (CDT), Mike Isely <isely@isely.net> wrote:
> > > 
> > > Chris:
> > > 
> > > I've tested this patch and it doesn't help us here.  Even if I add 
> > > lvds_fixed_mode=<whatever>, the display still comes up with the messed 
> > > up configuration from the motherboard's completely ignorant BIOS.  It 
> > > appears that lvds_fixed_mode is being ignored by the driver.
> > 
> > You can test the functionality of the patch by parsing
> > i915.lvds_fixed_mode first rather than last.
> 
> I will test for that - it was the next thing I was going to dig into.

Just tested this.  However the results were not conclusive.

It looks like the driver did notice the specified mode and used it - 
because the display's refresh rate did get adjusted and the overall 
resolution is correct.  However the display timings are slightly "off", 
the image is stretched a few pixels off the right edge and a few lines 
off the bottom edge.  Yet the same modeline given to "video=" in 
combination with my controversial disable-fixed-mode patch however 
produces a perfectly aligned image.

There might some other affect here.  I'm not testing in the same git 
repo in the two cases and there are other differences.  So this problem 
might be unrelated to the patch.  I will sort that out.

  -Mike

-- 

Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

Re: [PATCH 2/2] drm/i915/lvds: Use i915.lvds_fixed_mode= as a last resort

[Chris Wilson]

On Thu, 21 Apr 2011 17:30:21 -0500 (CDT), Mike Isely <isely@isely.net> wrote:
> It looks like the driver did notice the specified mode and used it - 
> because the display's refresh rate did get adjusted and the overall 
> resolution is correct.  However the display timings are slightly "off", 
> the image is stretched a few pixels off the right edge and a few lines 
> off the bottom edge.  Yet the same modeline given to "video=" in 
> combination with my controversial disable-fixed-mode patch however 
> produces a perfectly aligned image.

Adding a few printk will help. And perhaps comparing the resultant register
settings with intel_reg_dumper.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915/lvds: Use i915.lvds_fixed_mode= as a last resort

[Mike Isely]

On Thu, 21 Apr 2011, Chris Wilson wrote:

> On Thu, 21 Apr 2011 17:30:21 -0500 (CDT), Mike Isely <isely@isely.net> wrote:
> > It looks like the driver did notice the specified mode and used it - 
> > because the display's refresh rate did get adjusted and the overall 
> > resolution is correct.  However the display timings are slightly "off", 
> > the image is stretched a few pixels off the right edge and a few lines 
> > off the bottom edge.  Yet the same modeline given to "video=" in 
> > combination with my controversial disable-fixed-mode patch however 
> > produces a perfectly aligned image.
> 
> Adding a few printk will help. And perhaps comparing the resultant register
> settings with intel_reg_dumper.

Oh absolutely.  But right now I'm not doing a very fair apples-to-apples 
comparison.  I need to address that first and make sure nothing else is 
influencing this.

  -Mike


-- 

Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

Re: [PATCH 2/2] drm/i915/lvds: Use i915.lvds_fixed_mode= as a last resort

[Mike Isely]

On Thu, 21 Apr 2011, Mike Isely wrote:

> On Thu, 21 Apr 2011, Chris Wilson wrote:
> 
> > On Thu, 21 Apr 2011 17:30:21 -0500 (CDT), Mike Isely <isely@isely.net> wrote:
> > > It looks like the driver did notice the specified mode and used it - 
> > > because the display's refresh rate did get adjusted and the overall 
> > > resolution is correct.  However the display timings are slightly "off", 
> > > the image is stretched a few pixels off the right edge and a few lines 
> > > off the bottom edge.  Yet the same modeline given to "video=" in 
> > > combination with my controversial disable-fixed-mode patch however 
> > > produces a perfectly aligned image.
> > 
> > Adding a few printk will help. And perhaps comparing the resultant register
> > settings with intel_reg_dumper.
> 
> Oh absolutely.  But right now I'm not doing a very fair apples-to-apples 
> comparison.  I need to address that first and make sure nothing else is 
> influencing this.
> 
>   -Mike
> 

Chris:

Sorry this simple thing has taken 2 weeks, but I'm having to time-slice 
a lot of different tasks at the moment.  People keep pulling me away 
from this :-( However I've finally dug down and found the problem.  
There's a bug in the function drm_mode_parse_command_line_for_connector 
(the subject of this thread) that is leaving the margins field 
uninitialized.  This has been the cause all along for the differing 
results I was getting.

This bug didn't start with the underlying patch ("Use 
i915.lvds_fixed_mode= as a last resort").  I also looked at 
drm_fb_helper_connector_parse_command_line from before this patch and it 
looks like the same bug exists there.  Amazing that nobody has noticed 
this.

Patch to follow.

  -Mike


-- 

Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

[PATCH] drm/i915: Fix unset margins flag in drm_mode_parse_command_line_for_connector

[Mike Isely]

The drm function which parses a mode into a standard mode line is
computing the "margins" flag but then failing to copy that parameter
into the parsed structure output.  This seems to leave the field in a
random state, which unfortunately influences the results when the
parsed results are then subsequently used to calculate a mode line.
The fix here is obvious: ensure the the margins flag is properly
initialized in the result structure at the end of the parse operation.

Signed-off-by: Mike Isely <isely@pobox.com>
---
 drivers/gpu/drm/drm_modes.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 207b7eb..3697c56 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1094,6 +1094,7 @@ done:
 		mode->bpp_specified = true;
 		mode->bpp = bpp;
 	}
+	mode->margins = margins ? true : false;
 	mode->rb = rb ? true : false;
 	mode->cvt = cvt  ? true : false;
 	mode->interlace = interlace ? true : false;
-- 
1.7.2.5


-- 

Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

Re: [PATCH] drm/i915: Fix unset margins flag in drm_mode_parse_command_line_for_connector

[Robert Lowery]

>
> The drm function which parses a mode into a standard mode line is
> computing the "margins" flag but then failing to copy that parameter
> into the parsed structure output.  This seems to leave the field in a
> random state, which unfortunately influences the results when the
> parsed results are then subsequently used to calculate a mode line.
> The fix here is obvious: ensure the the margins flag is properly
> initialized in the result structure at the end of the parse operation.

Is this to fix Bug 25430 - [GM965 KMS] TV margins ignored in xorg.conf
(https://bugs.freedesktop.org/show_bug.cgi?id=25430) or am I completely
off base here?


> Signed-off-by: Mike Isely <isely@pobox.com>
> ---
>  drivers/gpu/drm/drm_modes.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 207b7eb..3697c56 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1094,6 +1094,7 @@ done:
>  		mode->bpp_specified = true;
>  		mode->bpp = bpp;
>  	}
> +	mode->margins = margins ? true : false;
>  	mode->rb = rb ? true : false;
>  	mode->cvt = cvt  ? true : false;
>  	mode->interlace = interlace ? true : false;
> --
> 1.7.2.5
>
>
> --
>
> Mike Isely
> isely @ isely (dot) net
> PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>