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

[Chris Wilson]

On Fri, May 22, 2015 at 12:03:27PM +0300, Ville Syrjälä wrote:
> 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?

So we can differentiate it from an equivalent mode added by the user
later on.

> > +		/* 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?

Hmm, that's what I thought I tried at first. If I remember correctly we
had to set mode->status in order to prune it since
drm_mode_connector_list_update() itself doesn't do the deletion. Using
the mode->status was problematic, and the simplest way to do delete the
original cmdline mode was by explicitly removing it ourselves.

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

I consider it to a real problem as it goes against my expectations as a
user, that is if I specify a mode to use, I expect that mode to be used.
Doesn't need to be in this patch though.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre