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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, May 22, 2015 at 10:54:12AM +0100, Chris Wilson wrote:
> 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@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;
> > 
> > Why do we need the DRIVER flag here?
> 
> So we can differentiate it from an equivalent mode added by the user
> later on.

Users can't actually add modes to the connector mode list.

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

Oh right drm_mode_connector_list_update() only removes the duplicates.

And the the mode->status handling is a bit strange. Not helped by
the fact that MODE_OK is zero so all kzalloced modes start out as
MODE_OK. While I was doing the mode santiy check stuff I did
consider that I should change new modes to be MODE_UNVERIFIED by
default, but I was feeling lazy and decided against it in the end.

So yeah getting the normal prune mechanism to kill the mode would
required some rework to the mode status->handling probably, so
seems like a somewhat bigger effort.

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

Yeah, I was just thinking it might be nice to have the other dmt modes
still be on the list. But I don't really mind at this time since I try
to avoid cables that don't connect the ddc pins.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel