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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 20 Apr 2015, Chris Wilson <chris@chris-wilson.co.uk> 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@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@radekdostal.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Radek Dostál <rd@radekdostal.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: dri-devel@lists.freedesktop.org
> Cc: Julia Lemire <jlemire@matrox.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: stable@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;
>  	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;

Would a drm_cmdline_mode_equal() or somesuch be helpful? There's two
copies here, and another variant in drm_pick_cmdline_mode - which also
checks interlace while this one doesn't - why?

> +
> +		/* 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;
> +
> +			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);

drm_mode_remove()

> +			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);

Shouldn't this hunk be a separate patch?

BR,
Jani.

>  	if (count == 0)
>  		goto prune;
>  
> -- 
> 2.1.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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