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@xxxxxxxxxxxxxxxxxx>
> > > 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@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > Cc: Radek Dostál <rd@xxxxxxxxxxxxxxx>
> > > Cc: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>
> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > Cc: Julia Lemire <jlemire@xxxxxxxxxx>
> > > Cc: Dave Airlie <airlied@xxxxxxxxxx>
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > ---
> > > 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
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html