[PATCH v2] drm: Only create a cmdline mode if no probed modes match

[Ville Syrjälä]

On Mon, Apr 20, 2015 at 02:28:56PM +0100, Chris Wilson wrote:
> The intention of using video=<connector>:<mode> is primarily to select
> the user's preferred resolution at startup. Currently we always create a
> new mode irrespective of whether the monitor has a native mode at the
> desired resolution. This has the issue that we may then select the fake
> mode rather the native mode during fb_helper->inital_config() and so
> if the fake mode is invalid we then end up with a loss of signal. Oops.
> This invalid fake mode would also be exported to userspace, who
> potentially may make the same mistake.
> 
> To avoid this issue, we filter out the added command line mode if we
> detect the desired resolution (and clock if specified) amongst the
> probed modes. This fixes the immediate problem of adding a duplicate
> mode, but perhaps more generically we should avoid adding a GTF mode if
> the monitor has an EDID that is not GTF-compatible, or similarly for
> CVT.
> 
> A second issue sneaked into this patch is to add the cmdline mode mode
> ahead of the absolute fallback 1024x768 mode. That is if the user has
> specified a mode that we create as a fallback, we do not need to add a
> second unused fallback mode.
> 
> Fixes regression from
> 
> commit eaf99c749d43ae74ac7ffece5512f3c73f01dfd2
> Author: Chris Wilson <chris at chris-wilson.co.uk>
> Date:   Wed Aug 6 10:08:32 2014 +0200
> 
>     drm: Perform cmdline mode parsing during connector initialisation
> 
> that breaks HDMI output on BeagleBone Black with LG TV (model 19LS4R-ZA).
> 
> v2: Explicitly delete our earlier cmdline mode
> 
> Reported-by: Radek Dostál <rd at radekdostal.com>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Radek Dostál <rd at radekdostal.com>
> Cc: Jesse Barnes <jbarnes at virtuousgeek.org>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: dri-devel at lists.freedesktop.org
> Cc: Julia Lemire <jlemire at matrox.com>
> Cc: Dave Airlie <airlied at redhat.com>
> Cc: stable at vger.kernel.org
> ---
>  drivers/gpu/drm/drm_modes.c        |  2 +-
>  drivers/gpu/drm/drm_probe_helper.c | 39 +++++++++++++++++++++++++++++++++++---
>  2 files changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 213b11ea69b5..13293e009990 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1400,7 +1400,7 @@ drm_mode_create_from_cmdline_mode(struct drm_device *dev,
>  	if (!mode)
>  		return NULL;
>  
> -	mode->type |= DRM_MODE_TYPE_USERDEF;
> +	mode->type |= DRM_MODE_TYPE_USERDEF | DRM_MODE_TYPE_DRIVER;

Why do we need the DRIVER flag here?

>  	drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V);
>  	return mode;
>  }
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 63503879a676..2ad8aaf46318 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -79,13 +79,46 @@ drm_mode_validate_flag(const struct drm_display_mode *mode,
>  
>  static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
>  {
> +	struct drm_cmdline_mode *cmdline_mode;
>  	struct drm_display_mode *mode;
>  
> -	if (!connector->cmdline_mode.specified)
> +	cmdline_mode = &connector->cmdline_mode;
> +	if (!cmdline_mode->specified)
>  		return 0;
>  
> +	/* Only add a GTF mode if we find no matching probed modes */
> +	list_for_each_entry(mode, &connector->probed_modes, head) {
> +		if (mode->hdisplay != cmdline_mode->xres ||
> +		    mode->vdisplay != cmdline_mode->yres)
> +			continue;
> +
> +		if (cmdline_mode->refresh_specified &&
> +		    mode->vrefresh != cmdline_mode->refresh)
> +			continue;
> +
> +		/* Remove the existing fake mode */
> +		list_for_each_entry(mode, &connector->modes, head) {
> +			if ((mode->type & (DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_USERDEF)) != (DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_USERDEF))
> +				continue;

Doesn't drm_mode_connector_list_update() kill it from the list
eventually if there's no matching mode present on the
probed_modes list?

> +
> +			if (mode->hdisplay != cmdline_mode->xres ||
> +			    mode->vdisplay != cmdline_mode->yres)
> +				continue;
> +
> +			if (cmdline_mode->refresh_specified &&
> +			    mode->vrefresh != cmdline_mode->refresh)
> +				continue;
> +
> +			list_del(&mode->head);
> +			drm_mode_destroy(connector->dev, mode);
> +			break;
> +		}
> +
> +		return 0;
> +	}
> +
>  	mode = drm_mode_create_from_cmdline_mode(connector->dev,
> -						 &connector->cmdline_mode);
> +						 cmdline_mode);
>  	if (mode == NULL)
>  		return 0;
>  
> @@ -179,9 +212,9 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
>  			count = (*connector_funcs->get_modes)(connector);
>  	}
>  
> +	count += drm_helper_probe_add_cmdline_mode(connector);
>  	if (count == 0 && connector->status == connector_status_connected)
>  		count = drm_add_modes_noedid(connector, 1024, 768);
> -	count += drm_helper_probe_add_cmdline_mode(connector);

Hmm. This means drm_add_modes_noedid() will never be called if the
cmdline mode is present, and hence the mode list will only ever have
that single mode user specified mode. Not sure if that can be considered
a real problem or not.

>  	if (count == 0)
>  		goto prune;
>  
> -- 
> 2.1.4

-- 
Ville Syrjälä
Intel OTC