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
