On Mon, 20 Apr 2015, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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; > 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@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Jani Nikula, Intel Open Source Technology Center -- 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