commit eaf99c749d43 ("drm: Perform cmdline mode parsing during connector initialisation") breaks HDMI output on BeagleBone Black with LG TV (model 19LS4R-ZA) when "video=HDMI-A-1:1280x720@60" is specified on the command line.
The reason is newly added mode '"1280x720" 60 74440 1280 1336 1472 1664 720 721 724 746 0x20 0x6' , which is added by function drm_helper_probe_add_cmdline_mode (introduced in above mentioned commit). This mode causes TV to go black and show "No signal" message.
When cmdline_mode is set, it is preferred to use matching mode obtained from EDID, than mode calculated by function drm_mode_create_from_cmdline_mode
Signed-off-by: Radek Dostal rd@radekdostal.com --- drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index cac4229..72d6ed0 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1284,6 +1284,7 @@ struct drm_display_mode *drm_pick_cmdline_mode(struct drm_fb_helper_connector *f { struct drm_cmdline_mode *cmdline_mode; struct drm_display_mode *mode; + bool prefer_non_userdef; bool prefer_non_interlace;
cmdline_mode = &fb_helper_conn->connector->cmdline_mode; @@ -1296,6 +1297,7 @@ struct drm_display_mode *drm_pick_cmdline_mode(struct drm_fb_helper_connector *f if (cmdline_mode->rb || cmdline_mode->margins) goto create_mode;
+ prefer_non_userdef = true; prefer_non_interlace = !cmdline_mode->interlace; again: list_for_each_entry(mode, &fb_helper_conn->connector->modes, head) { @@ -1312,6 +1314,9 @@ again: if (cmdline_mode->interlace) { if (!(mode->flags & DRM_MODE_FLAG_INTERLACE)) continue; + } else if (prefer_non_userdef) { + if (mode->type & DRM_MODE_TYPE_USERDEF) + continue; } else if (prefer_non_interlace) { if (mode->flags & DRM_MODE_FLAG_INTERLACE) continue; @@ -1319,6 +1324,11 @@ again: return mode; }
+ if (prefer_non_userdef) { + prefer_non_userdef = true; + goto again; + } + if (prefer_non_interlace) { prefer_non_interlace = false; goto again;
commit eaf99c749d43 ("drm: Perform cmdline mode parsing during connector initialisation") breaks HDMI output on BeagleBone Black with LG TV (model 19LS4R-ZA) when "video=HDMI-A-1:1280x720@60" is specified on the command line.
The reason is newly added mode '"1280x720" 60 74440 1280 1336 1472 1664 720 721 724 746 0x20 0x6' , which is added by function drm_helper_probe_add_cmdline_mode (introduced in above mentioned commit). This mode causes TV to go black and show "No signal" message.
When cmdline_mode is set, it is preferred to use matching mode obtained from EDID, than mode calculated by function drm_mode_create_from_cmdline_mode
Signed-off-by: Radek Dostal rd@radekdostal.com --- v2: fixed if (prefer_non_userdef) condition, which was causing infinite loop before.
drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index cac4229..fac425e 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1284,6 +1284,7 @@ struct drm_display_mode *drm_pick_cmdline_mode(struct drm_fb_helper_connector *f { struct drm_cmdline_mode *cmdline_mode; struct drm_display_mode *mode; + bool prefer_non_userdef; bool prefer_non_interlace;
cmdline_mode = &fb_helper_conn->connector->cmdline_mode; @@ -1296,6 +1297,7 @@ struct drm_display_mode *drm_pick_cmdline_mode(struct drm_fb_helper_connector *f if (cmdline_mode->rb || cmdline_mode->margins) goto create_mode;
+ prefer_non_userdef = true; prefer_non_interlace = !cmdline_mode->interlace; again: list_for_each_entry(mode, &fb_helper_conn->connector->modes, head) { @@ -1312,6 +1314,9 @@ again: if (cmdline_mode->interlace) { if (!(mode->flags & DRM_MODE_FLAG_INTERLACE)) continue; + } else if (prefer_non_userdef) { + if (mode->type & DRM_MODE_TYPE_USERDEF) + continue; } else if (prefer_non_interlace) { if (mode->flags & DRM_MODE_FLAG_INTERLACE) continue; @@ -1319,6 +1324,11 @@ again: return mode; }
+ if (prefer_non_userdef) { + prefer_non_userdef = false; + goto again; + } + if (prefer_non_interlace) { prefer_non_interlace = false; goto again;
On Mon, Apr 20, 2015 at 07:26:33AM +0200, Radek Dostal wrote:
commit eaf99c749d43 ("drm: Perform cmdline mode parsing during connector initialisation") breaks HDMI output on BeagleBone Black with LG TV (model 19LS4R-ZA) when "video=HDMI-A-1:1280x720@60" is specified on the command line.
The reason is newly added mode '"1280x720" 60 74440 1280 1336 1472 1664 720 721 724 746 0x20 0x6' , which is added by function drm_helper_probe_add_cmdline_mode (introduced in above mentioned commit). This mode causes TV to go black and show "No signal" message.
When cmdline_mode is set, it is preferred to use matching mode obtained from EDID, than mode calculated by function drm_mode_create_from_cmdline_mode
The EDID modes should be earlier in the list, and so higher priority than the cmdline mode. The only instance I see that breaking down is if the mode gets created by drm_pick_cmdline_mode, i.e.
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index cac422916c7a..d55c2de6a99f 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1327,7 +1327,7 @@ again: create_mode: mode = drm_mode_create_from_cmdline_mode(fb_helper_conn->connector->dev, cmdline_mode); - list_add(&mode->head, &fb_helper_conn->connector->modes); + list_add_tail(&mode->head, &fb_helper_conn->connector->modes); return mode; } EXPORT_SYMBOL(drm_pick_cmdline_mode);
Can you please print the matching modes and trace where the userdef mode gets added before the EDID modes? -Chris
Hi Chris,
On 04/20/2015 11:09 AM, Chris Wilson wrote:
The EDID modes should be earlier in the list, and so higher priority than the cmdline mode. The only instance I see that breaking down is if the mode gets created by drm_pick_cmdline_mode, i.e.
indeed at the beginning the command line mode is added to the end of the list, but later on it seems to me that all modes are reordered based on the resolution and clock and than mode generated by drm_helper_probe_add_cmdline_mode gets upper in the list as it has higher clock value. Please see attached output of dmesg.
Additionally I am attaching the output with commit eaf99c749d43 reverted, where you can see that the mode "1280x720@60 with pixel clock 74440" is not even added to the list.
diff --git a/drivers/gpu/drm/drm_fb_helper.c
b/drivers/gpu/drm/drm_fb_helper.c
index cac422916c7a..d55c2de6a99f 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1327,7 +1327,7 @@ again: create_mode: mode =
drm_mode_create_from_cmdline_mode(fb_helper_conn->connector->dev,
cmdline_mode);
list_add(&mode->head, &fb_helper_conn->connector->modes);
list_add_tail(&mode->head, &fb_helper_conn->connector->modes); return mode;} EXPORT_SYMBOL(drm_pick_cmdline_mode);
I am pretty sure, this will not help as in my case it never makes it to create_mode.
Thanks, Radek
On Mon, Apr 20, 2015 at 11:36:04AM +0200, Radek Dostál wrote:
Hi Chris,
On 04/20/2015 11:09 AM, Chris Wilson wrote:
The EDID modes should be earlier in the list, and so higher priority than the cmdline mode. The only instance I see that breaking down is if the mode gets created by drm_pick_cmdline_mode, i.e.
indeed at the beginning the command line mode is added to the end of the list, but later on it seems to me that all modes are reordered based on the resolution and clock and than mode generated by drm_helper_probe_add_cmdline_mode gets upper in the list as it has higher clock value. Please see attached output of dmesg.
Ah thanks, drm_mode_sort()!
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 213b11e..9c8357f 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -1127,6 +1127,7 @@ static int drm_mode_compare(void *priv, struct list_head *lh_a, struct list_head ((a->type & DRM_MODE_TYPE_PREFERRED) != 0); if (diff) return diff; + diff = b->hdisplay * b->vdisplay - a->hdisplay * a->vdisplay; if (diff) return diff; @@ -1136,7 +1137,16 @@ static int drm_mode_compare(void *priv, struct list_head *lh_a, struct list_head return diff;
diff = b->clock - a->clock; - return diff; + if (diff) + return diff; + + /* sort user-defined modes after reported modes of same resolution */ + diff = ((a->type & DRM_MODE_TYPE_USERDEF) != 0) - + ((b->type & DRM_MODE_TYPE_USERDEF) != 0); + if (diff) + return diff; + + return 0; }
Perhaps? -Chris
On Mon, Apr 20, 2015 at 10:46:08AM +0100, Chris Wilson wrote:
On Mon, Apr 20, 2015 at 11:36:04AM +0200, Radek Dostál wrote:
Hi Chris,
On 04/20/2015 11:09 AM, Chris Wilson wrote:
The EDID modes should be earlier in the list, and so higher priority than the cmdline mode. The only instance I see that breaking down is if the mode gets created by drm_pick_cmdline_mode, i.e.
indeed at the beginning the command line mode is added to the end of the list, but later on it seems to me that all modes are reordered based on the resolution and clock and than mode generated by drm_helper_probe_add_cmdline_mode gets upper in the list as it has higher clock value. Please see attached output of dmesg.
Ah thanks, drm_mode_sort()!
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 213b11e..9c8357f 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -1127,6 +1127,7 @@ static int drm_mode_compare(void *priv, struct list_head *lh_a, struct list_head ((a->type & DRM_MODE_TYPE_PREFERRED) != 0); if (diff) return diff;
diff = b->hdisplay * b->vdisplay - a->hdisplay * a->vdisplay; if (diff) return diff;@@ -1136,7 +1137,16 @@ static int drm_mode_compare(void *priv, struct list_head *lh_a, struct list_head return diff;
diff = b->clock - a->clock;
return diff;
if (diff)return diff;/* sort user-defined modes after reported modes of same resolution */diff = ((a->type & DRM_MODE_TYPE_USERDEF) != 0) -((b->type & DRM_MODE_TYPE_USERDEF) != 0);if (diff)return diff;
Hmm, so that should be before the clock comparison as well to fix your example. Not as neat.
The other idea I was considering was not adding the GTF cmdline mode if the probed modes already contain one of a matching resolution. The goal here is to also not present a mode to userspace that is invalid. -Chris
Hi Chris,
On 04/20/2015 11:58 AM, Chris Wilson wrote:
Hmm, so that should be before the clock comparison as well to fix your example. Not as neat.
indeed that is required.
The other idea I was considering was not adding the GTF cmdline mode if the probed modes already contain one of a matching resolution. The goal here is to also not present a mode to userspace that is invalid.
Unfortunately you can not do that. I already tried. At the time when drm_helper_probe_add_cmdline_mode is called EDID informations are not yet available.
Only option would be to remove the GTF cmdline mode, when matching mode is found in EDID.
Thanks, Radek
On Mon, Apr 20, 2015 at 12:38:25PM +0200, Radek Dostál wrote:
Hi Chris,
On 04/20/2015 11:58 AM, Chris Wilson wrote:
Hmm, so that should be before the clock comparison as well to fix your example. Not as neat.
indeed that is required.
The other idea I was considering was not adding the GTF cmdline mode if the probed modes already contain one of a matching resolution. The goal here is to also not present a mode to userspace that is invalid.
Unfortunately you can not do that. I already tried. At the time when drm_helper_probe_add_cmdline_mode is called EDID informations are not yet available.
My understanding is that it should be. fb_helper.initial_config does a probe first, and the intel_fb_initial_config() should only keep the active mode.
Only option would be to remove the GTF cmdline mode, when matching mode is found in EDID.
So basically
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 6350387..9212bec 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -79,13 +79,29 @@ 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->modes, head) { + if (mode->hdisplay != cmdline_mode->xres || + mode->vdisplay != cmdline_mode->yres) + continue; + + if (cmdline_mode->refresh_specified) { + if (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;
is not sufficient? -Chris
Hi Chris,
On 04/20/2015 12:48 PM, Chris Wilson wrote:
Unfortunately you can not do that. I already tried. At the time when
drm_helper_probe_add_cmdline_mode is called EDID informations are not yet available.
My understanding is that it should be. fb_helper.initial_config does a probe first, and the intel_fb_initial_config() should only keep the active mode.
uff, sorry I am not sure that I follow here - I am not that familiar with the whole subsystem.
Only option would be to remove the GTF cmdline mode, when matching mode is found in EDID.
So basically
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 6350387..9212bec 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -79,13 +79,29 @@ 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->modes, head) {if (mode->hdisplay != cmdline_mode->xres ||mode->vdisplay != cmdline_mode->yres)continue;if (cmdline_mode->refresh_specified) {if (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;is not sufficient?
indeed, this was my first idea how to fix this problem, but at the time drm_helper_probe_add_cmdline_mode is called connector->modes is empty => the GTF cmdline mode is still added.
Thanks, Radek
On Mon, Apr 20, 2015 at 12:57:33PM +0200, Radek Dostál wrote:
Hi Chris,
On 04/20/2015 12:48 PM, Chris Wilson wrote:
Unfortunately you can not do that. I already tried. At the time when
drm_helper_probe_add_cmdline_mode is called EDID informations are not yet available.
My understanding is that it should be. fb_helper.initial_config does a probe first, and the intel_fb_initial_config() should only keep the active mode.
uff, sorry I am not sure that I follow here - I am not that familiar with the whole subsystem.
Only option would be to remove the GTF cmdline mode, when matching mode is found in EDID.
So basically
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 6350387..9212bec 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -79,13 +79,29 @@ 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->modes, head) {if (mode->hdisplay != cmdline_mode->xres ||mode->vdisplay != cmdline_mode->yres)continue;if (cmdline_mode->refresh_specified) {if (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;is not sufficient?
indeed, this was my first idea how to fix this problem, but at the time drm_helper_probe_add_cmdline_mode is called connector->modes is empty => the GTF cmdline mode is still added.
Can you do a WARN_ON(list_empty(&connector->modes)) here to see at what point we set up the invalid GTF mode? -Chris
Hi Chris,
On 04/20/2015 01:00 PM, Chris Wilson wrote:
Can you do a WARN_ON(list_empty(&connector->modes)) here to see at what point we set up the invalid GTF mode?
sure please see attached patch adding WARN_ON and corresponding output of dmesg. As mentioned in the original commit, the mode is indeed added in drm_helper_probe_add_cmdline_mode.
Thanks, Radek
On Mon, Apr 20, 2015 at 01:20:05PM +0200, Radek Dostál wrote:
Hi Chris,
On 04/20/2015 01:00 PM, Chris Wilson wrote:
Can you do a WARN_ON(list_empty(&connector->modes)) here to see at what point we set up the invalid GTF mode?
sure please see attached patch adding WARN_ON and corresponding output of dmesg. As mentioned in the original commit, the mode is indeed added in drm_helper_probe_add_cmdline_mode.
Ah, maybe this on top of the previous try:
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 88f5a74..5d22ca0 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -87,7 +87,7 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector) return 0;
/* Only add a GTF mode if we find no matching probed modes */ - list_for_each_entry(mode, &connector->modes, head) { + list_for_each_entry(mode, &connector->probed_modes, head) { if (mode->hdisplay != cmdline_mode->xres || mode->vdisplay != cmdline_mode->yres) continue; @@ -211,7 +211,8 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect mode_flags |= DRM_MODE_FLAG_3D_MASK;
list_for_each_entry(mode, &connector->modes, head) { - mode->status = drm_mode_validate_basic(mode); + if (mode->status == MODE_OK) + mode->status = drm_mode_validate_basic(mode);
if (mode->status == MODE_OK) mode->status = drm_mode_validate_size(mode, maxX, maxY
Hi Chris,
On 04/20/2015 01:44 PM, Chris Wilson wrote:
Ah, maybe this on top of the previous try:
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 88f5a74..5d22ca0 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -87,7 +87,7 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector) return 0;
/* Only add a GTF mode if we find no matching probed modes */
list_for_each_entry(mode, &connector->modes, head) {
list_for_each_entry(mode, &connector->probed_modes, head) { if (mode->hdisplay != cmdline_mode->xres || mode->vdisplay != cmdline_mode->yres) continue;@@ -211,7 +211,8 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect mode_flags |= DRM_MODE_FLAG_3D_MASK;
list_for_each_entry(mode, &connector->modes, head) {
mode->status = drm_mode_validate_basic(mode);
if (mode->status == MODE_OK)mode->status = drm_mode_validate_basic(mode); if (mode->status == MODE_OK) mode->status = drm_mode_validate_size(mode, maxX, maxY
perfect - this seems to work :)
Radek
The intention of using video=<connector>:<mode: is primarily to select the user's preferred resolution at startup. Currently we always create a GTF 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 GTF mode rather the native mode during fb_helper->inital_config() and so on a non-GTF monitor 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...
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).
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 | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 63503879a676..b4b61cc4168e 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -79,16 +79,33 @@ 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;
mode = drm_mode_create_from_cmdline_mode(connector->dev, - &connector->cmdline_mode); + cmdline_mode); if (mode == NULL) 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) { + if (mode->vrefresh != cmdline_mode->refresh) + continue; + } + + mode->status = MODE_BAD; + break; + } + drm_mode_probed_add(connector, mode); return 1; } @@ -179,9 +196,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); if (count == 0) goto prune;
@@ -195,7 +212,8 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect mode_flags |= DRM_MODE_FLAG_3D_MASK;
list_for_each_entry(mode, &connector->modes, head) { - mode->status = drm_mode_validate_basic(mode); + if (mode->status != MODE_BAD) + mode->status = drm_mode_validate_basic(mode);
if (mode->status == MODE_OK) mode->status = drm_mode_validate_size(mode, maxX, maxY);
Hi Chris,
On 04/20/2015 02:26 PM, 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 GTF 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 GTF mode rather the native mode during fb_helper->inital_config() and so on a non-GTF monitor 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...
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 initialisationthat breaks HDMI output on BeagleBone Black with LG TV (model 19LS4R-ZA).
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
NAKed-by: Radek Dostál rd@radekdostal.com
sorry to let you know, but this patch does NOT work :( Attached is the original.patch which you provided previously and it did work, but one of the last minute changes had to break it :(
I hope you will find the issue soon, Radek
On Mon, Apr 20, 2015 at 03:06:30PM +0200, Radek Dostál wrote:
Hi Chris,
On 04/20/2015 02:26 PM, 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 GTF 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 GTF mode rather the native mode during fb_helper->inital_config() and so on a non-GTF monitor 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...
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 initialisationthat breaks HDMI output on BeagleBone Black with LG TV (model 19LS4R-ZA).
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
NAKed-by: Radek Dostál rd@radekdostal.com
sorry to let you know, but this patch does NOT work :( Attached is the original.patch which you provided previously and it did work, but one of the last minute changes had to break it :(
The original patch relied on the MODE_UNVERIFIED being retained by the unwanted cmdline mode. This patch relies on overwriting the existing unwanted mode->status with MODE_BAD by drm_mode_connector_list_update(). I made that change because the first version would have deleted all user modes and not just our fake cmdline mode.
Alternatively we could do a loop over the old modes, remove the fake then loops over probed modes and only add if it we don't match. Let's try that... -Chris
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; 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; + + /* 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); + 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); if (count == 0) goto prune;
On 04/20/2015 03:28 PM, 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 initialisationthat 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
works now :)
Tested-by: Radek Dostál rd@radekdostal.com
Thanks, Radek
On Mon, Apr 20, 2015 at 03:41:48PM +0200, Radek Dostál wrote:
On 04/20/2015 03:28 PM, 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 initialisationthat 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
works now :)
Tested-by: Radek Dostál rd@radekdostal.com
Daniel, Dave do either or you want to pick this up for your fixes tree? -Chris
On Mon, 20 Apr 2015, Chris Wilson chris@chris-wilson.co.uk 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 initialisationthat 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; 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);@@ -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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
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 initialisationthat 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?
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;/* 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?
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);break;}return 0;- }
- mode = drm_mode_create_from_cmdline_mode(connector->dev,
&connector->cmdline_mode);
cmdline_mode);@@ -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.
if (count == 0) goto prune;
-- 2.1.4
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 initialisationthat 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.
/* 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
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 initialisationthat 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.
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) + continue; + + return 0; + } + mode = drm_mode_create_from_cmdline_mode(connector->dev, - &connector->cmdline_mode); + cmdline_mode); if (mode == NULL) return 0;
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 initialisationthat 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);-- 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 initialisationthat 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 initialisationthat 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 initialisationthat 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.
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 v4: Compute the vrefresh for the probed 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_probe_helper.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 0329080d7f7c..a0df377d7d1c 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -82,13 +82,30 @@ 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) { + /* The probed mode's vrefresh is set until later */ + if (drm_mode_vrefresh(mode) != 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;
On Wed, Jun 1, 2016 at 5:50 AM, Chris Wilson chris@chris-wilson.co.uk 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 initialisationthat 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 v4: Compute the vrefresh for the probed mode
Reviewed-by: Alex Deucher alexander.deucher@amd.com
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 | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 0329080d7f7c..a0df377d7d1c 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -82,13 +82,30 @@ 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) {/* The probed mode's vrefresh is set until later */if (drm_mode_vrefresh(mode) != 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;-- 2.8.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 06/01/2016 11:50 AM, Chris Wilson wrote:
Fixes regression from
commit eaf99c749d43ae74ac7ffece5512f3c73f01dfd2 Author: Chris Wilsonchris@chris-wilson.co.uk Date: Wed Aug 6 10:08:32 2014 +0200
drm: Perform cmdline mode parsing during connector initialisationthat breaks HDMI output on BeagleBone Black with LG TV (model 19LS4R-ZA).
please remove this from the commit message. The original bug is no longer reproducible with 4.7-rc1
Thanks, Radek
On Thu, Jun 02, 2016 at 11:38:26AM +0200, Radek Dostál wrote:
On 06/01/2016 11:50 AM, Chris Wilson wrote:
Fixes regression from
commit eaf99c749d43ae74ac7ffece5512f3c73f01dfd2 Author: Chris Wilsonchris@chris-wilson.co.uk Date: Wed Aug 6 10:08:32 2014 +0200
drm: Perform cmdline mode parsing during connector initialisationthat breaks HDMI output on BeagleBone Black with LG TV (model 19LS4R-ZA).
please remove this from the commit message. The original bug is no longer reproducible with 4.7-rc1
If there's no motivation for the patch anymore, it can just wither away in one of my old trees.
Does anyone care about pruning the autogenerated video= mode if a probed one matches? Presumably, it is still visible to userspace and switching to it will cause the same issue as before? Or was it always a driver bug (failing to set the mode)? -Chris
On Thu, Jun 02, 2016 at 11:52:17AM +0100, Chris Wilson wrote:
On Thu, Jun 02, 2016 at 11:38:26AM +0200, Radek Dostál wrote:
On 06/01/2016 11:50 AM, Chris Wilson wrote:
Fixes regression from
commit eaf99c749d43ae74ac7ffece5512f3c73f01dfd2 Author: Chris Wilsonchris@chris-wilson.co.uk Date: Wed Aug 6 10:08:32 2014 +0200
drm: Perform cmdline mode parsing during connector initialisationthat breaks HDMI output on BeagleBone Black with LG TV (model 19LS4R-ZA).
please remove this from the commit message. The original bug is no longer reproducible with 4.7-rc1
If there's no motivation for the patch anymore, it can just wither away in one of my old trees.
Does anyone care about pruning the autogenerated video= mode if a probed one matches? Presumably, it is still visible to userspace and switching to it will cause the same issue as before? Or was it always a driver bug (failing to set the mode)?
IMO the patch makes total sense even if it's not needed for this particular bug. Feel free to add
Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
On 06/02/2016 01:30 PM, Ville Syrjälä wrote:
IMO the patch makes total sense even if it's not needed for this particular bug. Feel free to add
I agree and additionally can confirm, that with this patch BBB still works as expected with LG 19LS4R-ZA.
Thanks, Radek
On Thu, Jun 02, 2016 at 02:30:40PM +0300, Ville Syrjälä wrote:
On Thu, Jun 02, 2016 at 11:52:17AM +0100, Chris Wilson wrote:
On Thu, Jun 02, 2016 at 11:38:26AM +0200, Radek Dostál wrote:
On 06/01/2016 11:50 AM, Chris Wilson wrote:
Fixes regression from
commit eaf99c749d43ae74ac7ffece5512f3c73f01dfd2 Author: Chris Wilsonchris@chris-wilson.co.uk Date: Wed Aug 6 10:08:32 2014 +0200
drm: Perform cmdline mode parsing during connector initialisationthat breaks HDMI output on BeagleBone Black with LG TV (model 19LS4R-ZA).
please remove this from the commit message. The original bug is no longer reproducible with 4.7-rc1
If there's no motivation for the patch anymore, it can just wither away in one of my old trees.
Does anyone care about pruning the autogenerated video= mode if a probed one matches? Presumably, it is still visible to userspace and switching to it will cause the same issue as before? Or was it always a driver bug (failing to set the mode)?
IMO the patch makes total sense even if it's not needed for this particular bug. Feel free to add
Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
Agreed. I dropped the cc: stable and adjusted the commit message to explain the situation. Merged to drm-misc, thanks. -Daniel
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Chris Wilson -
Daniel Vetter -
Jani Nikula -
Radek Dostal -
Radek Dostál
-
Ville Syrjälä