Re: [PATCH] drm, drm/i915/lvds: Honour video= parameter to override LVDS fixed mode

[Dave Airlie <airlied@redhat.com>]

On Wed, 2011-02-09 at 15:01 +0000, Chris Wilson wrote:
> The LVDS code ignores any connector for which it cannot find a fixed
> mode (through an EDID, vBIOS tables or the current active mode). Some
> platforms may include an LVDS header on the board and this may then be
> partnered with a panel without an EDID. This results in us ignoring the
> connector and not lighting up the panel.

Yeah not like this.

you want to make the command line the *last* option we use, the final
fallback. LVDS panels have EDID and VBT hardcoded modes for a good
reason, they don't work with other modes that well. You always want to
use a scaler on the LVDS panel to do modes not the native mode. So if I
have a VBT or EDID and you set video= I should get a scaled mode, not
garbage.

So what I suspect you really want is to leave video= alone or enhance it
somehow, or maybe add i915.lvds_native_mode= parameter.

Dave.

> 
> Under UMS, it was possible to override this by specifying the mode
> through the Xorg.conf. For KMS, one specifies the modeline through the
> video= parameter. So we need to include this user modeline when checking
> for panel fixed modes.
> 
> The machinery to parse the video= modes and generate the appropriate
> drm_mode is already built into drm_fb_herlper and so we can just
> extract, move it to the core and also use it from intel_lvds.c
> 
> Reported-by: Oliver Seitz <info@vtnd.de>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c   |  207 +++++++------------------------------
>  drivers/gpu/drm/drm_modes.c       |  154 +++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_lvds.c |   17 +++
>  include/drm/drmP.h                |   25 +++++
>  include/drm/drm_fb_helper.h       |   16 +---
>  5 files changed, 233 insertions(+), 186 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 6977a1c..5a80412 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;
>  }
> @@ -883,7 +759,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;
>  
> @@ -1105,7 +981,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;
>  }
> @@ -1113,7 +989,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;
> @@ -1145,19 +1021,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 58e65f9..f9da47e 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/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index cd08960..af4ef17 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -874,6 +874,8 @@ bool intel_lvds_init(struct drm_device *dev)
>  	struct drm_encoder *encoder;
>  	struct drm_display_mode *scan; /* *modes, *bios_mode; */
>  	struct drm_crtc *crtc;
> +	char *cmdline_option = NULL;
> +	struct drm_cmdline_mode cmdline_mode;
>  	u32 lvds;
>  	int pipe;
>  	u8 pin;
> @@ -951,6 +953,7 @@ bool intel_lvds_init(struct drm_device *dev)
>  	intel_lvds->fitting_mode = DRM_MODE_SCALE_ASPECT;
>  	/*
>  	 * LVDS discovery:
> +	 * 0) user override
>  	 * 1) check for EDID on DDC
>  	 * 2) check for VBT data
>  	 * 3) check to see if LVDS is already on
> @@ -959,6 +962,20 @@ bool intel_lvds_init(struct drm_device *dev)
>  	 *    if closed, act like it's not there for now
>  	 */
>  
> +	if (fb_get_options(drm_get_connector_name(connector),
> +			   &cmdline_option) == 0 &&
> +	    drm_mode_parse_command_line_for_connector(cmdline_option,
> +						      connector,
> +						      &cmdline_mode)) {
> +		intel_lvds->fixed_mode =
> +			drm_mode_create_from_cmdline_mode(dev, &cmdline_mode);
> +		if (intel_lvds->fixed_mode) {
> +			intel_lvds->fixed_mode->type |=
> +				DRM_MODE_TYPE_PREFERRED;
> +			goto out;
> +		}
> +	}
> +
>  	/*
>  	 * Attempt to get the fixed panel mode from DDC.  Assume that the
>  	 * preferred mode is the right one.
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index fe29aad..bf01108 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -968,6 +968,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;
> @@ -1381,6 +1397,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 {