| Message ID | 1464773676-20152-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 01, 2016 at 10:34:36AM +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. > > 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 > v3: Mode pruning should now be sufficient to delete stale cmdline modes > > 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_probe_helper.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > index 0329080d7f7c..a705ed12c062 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -82,13 +82,28 @@ 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) I think we might not have .vrefresh populated for the probed modes. We update .vrefresh only for the modes left on the real mode list in the end. > + continue; > + > + return 0; > + } > + > mode = drm_mode_create_from_cmdline_mode(connector->dev, > - &connector->cmdline_mode); > + cmdline_mode); > if (mode == NULL) > return 0; > > -- > 2.8.1
On Wed, Jun 01, 2016 at 12:43:53PM +0300, Ville Syrjälä wrote: > On Wed, Jun 01, 2016 at 10:34:36AM +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. > > > > 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 > > v3: Mode pruning should now be sufficient to delete stale cmdline modes > > > > 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_probe_helper.c | 19 +++++++++++++++++-- > > 1 file changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > > index 0329080d7f7c..a705ed12c062 100644 > > --- a/drivers/gpu/drm/drm_probe_helper.c > > +++ b/drivers/gpu/drm/drm_probe_helper.c > > @@ -82,13 +82,28 @@ 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) > > I think we might not have .vrefresh populated for the probed modes. > We update .vrefresh only for the modes left on the real mode list in the > end. In that case, if cmdline_mode->refresh_specified keep and leave a comment suggesting we might be able to do better :) -Chris
On Wed, Jun 01, 2016 at 12:43:53PM +0300, Ville Syrjälä wrote: > On Wed, Jun 01, 2016 at 10:34:36AM +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. > > > > 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 > > v3: Mode pruning should now be sufficient to delete stale cmdline modes > > > > 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_probe_helper.c | 19 +++++++++++++++++-- > > 1 file changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > > index 0329080d7f7c..a705ed12c062 100644 > > --- a/drivers/gpu/drm/drm_probe_helper.c > > +++ b/drivers/gpu/drm/drm_probe_helper.c > > @@ -82,13 +82,28 @@ 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) > > I think we might not have .vrefresh populated for the probed modes. > We update .vrefresh only for the modes left on the real mode list in the > end. Or drm_mode_vrefresh() ? -Chris
On Wed, Jun 01, 2016 at 10:47:51AM +0100, Chris Wilson wrote: > On Wed, Jun 01, 2016 at 12:43:53PM +0300, Ville Syrjälä wrote: > > On Wed, Jun 01, 2016 at 10:34:36AM +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. > > > > > > 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 > > > v3: Mode pruning should now be sufficient to delete stale cmdline modes > > > > > > 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_probe_helper.c | 19 +++++++++++++++++-- > > > 1 file changed, 17 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > > > index 0329080d7f7c..a705ed12c062 100644 > > > --- a/drivers/gpu/drm/drm_probe_helper.c > > > +++ b/drivers/gpu/drm/drm_probe_helper.c > > > @@ -82,13 +82,28 @@ 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) > > > > I think we might not have .vrefresh populated for the probed modes. > > We update .vrefresh only for the modes left on the real mode list in the > > end. > > Or drm_mode_vrefresh() ? That should work.
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 0329080d7f7c..a705ed12c062 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -82,13 +82,28 @@ 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; + + return 0; + } + mode = drm_mode_create_from_cmdline_mode(connector->dev, - &connector->cmdline_mode); + cmdline_mode); if (mode == NULL) return 0;
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. 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 v3: Mode pruning should now be sufficient to delete stale cmdline modes 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_probe_helper.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)