This fixes the extra_mode walk to be much more conservative. I still think the whole idea is bogus and that guessing about clone mode sizes is a userspace policy decision, but apparently xrandr --newmode / --addmode is unreasonably burdensome.
This should fix a number of reported regressions, please test.
- ajax
Signed-off-by: Adam Jackson ajax@redhat.com --- drivers/gpu/drm/drm_edid.c | 11 +++++++++-- 1 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 5873e48..4a3e61a 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1007,8 +1007,15 @@ range_pixel_clock(struct edid *edid, u8 *t) if (edid->revision >= 4 && t[10] == 0x04) return (t[9] * 10000) - ((t[12] >> 2) * 250);
- /* 1.3 is pathetic, so fuzz up a bit */ - return t[9] * 10000 + 5001; + /* + * Otherwise we have only 10MHz granularity. If we were using this + * code to filter an EDID mode list we'd want to round up like + * xserver does, as it's common to see a max of 160MHz but a 162MHz + * 1600x1200 mode as the preferred timing. But since we're using + * this as a filter while _building_ the list from implied support, + * not rounding up is the safe thing. + */ + return t[9] * 10000; }
static bool
Essentially, only do this on digital displays. It's almost certainly not wise to add them on CRTs, but we can't tell CRT from LCD before EDID 1.4, so just use digital as a proxy for that.
Bugzilla: https://bugs.freedesktop.org/51146 Signed-off-by: Adam Jackson ajax@redhat.com --- drivers/gpu/drm/drm_edid.c | 66 ++++++++----------------------------------- 1 files changed, 13 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 4a3e61a..e198325 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1081,44 +1081,16 @@ static void fixup_mode_1366x768(struct drm_display_mode *mode) }
static int -drm_gtf_modes_for_range(struct drm_connector *connector, struct edid *edid, - struct detailed_timing *timing) -{ - int i, modes = 0; - struct drm_display_mode *newmode; - struct drm_device *dev = connector->dev; - - for (i = 0; i < num_extra_modes; i++) { - const struct minimode *m = &extra_modes[i]; - newmode = drm_gtf_mode(dev, m->w, m->h, m->r, 0, 0); - if (!newmode) - return modes; - - fixup_mode_1366x768(newmode); - if (!mode_in_range(newmode, edid, timing)) { - drm_mode_destroy(dev, newmode); - continue; - } - - drm_mode_probed_add(connector, newmode); - modes++; - } - - return modes; -} - -static int -drm_cvt_modes_for_range(struct drm_connector *connector, struct edid *edid, - struct detailed_timing *timing) +drm_extra_modes_for_range(struct drm_connector *connector, struct edid *edid, + struct detailed_timing *timing) { int i, modes = 0; struct drm_display_mode *newmode; struct drm_device *dev = connector->dev; - bool rb = drm_monitor_supports_rb(edid);
for (i = 0; i < num_extra_modes; i++) { const struct minimode *m = &extra_modes[i]; - newmode = drm_cvt_mode(dev, m->w, m->h, m->r, rb, 0, 0); + newmode = drm_cvt_mode(dev, m->w, m->h, m->r, true, 0, 0); if (!newmode) return modes;
@@ -1140,7 +1112,6 @@ do_inferred_modes(struct detailed_timing *timing, void *c) { struct detailed_mode_closure *closure = c; struct detailed_non_pixel *data = &timing->data.other_data; - struct detailed_data_monitor_range *range = &data->data.range;
if (data->type != EDID_DETAIL_MONITOR_RANGE) return; @@ -1149,28 +1120,17 @@ do_inferred_modes(struct detailed_timing *timing, void *c) closure->edid, timing); - if (!version_greater(closure->edid, 1, 1)) - return; /* GTF not defined yet */ - - switch (range->flags) { - case 0x02: /* secondary gtf, XXX could do more */ - case 0x00: /* default gtf */ - closure->modes += drm_gtf_modes_for_range(closure->connector, - closure->edid, - timing); - break; - case 0x04: /* cvt, only in 1.4+ */ - if (!version_greater(closure->edid, 1, 3)) - break; + /* Don't look at extra_modes for pre-CVT-era displays */ + if (!version_greater(closure->edid, 1, 2)) + return; /* CVT not defined yet */
- closure->modes += drm_cvt_modes_for_range(closure->connector, - closure->edid, - timing); - break; - case 0x01: /* just the ranges, no formula */ - default: - break; - } + /* Don't look at extra_modes for non-rb displays */ + if (!drm_monitor_supports_rb(closure->edid)) + return; + + closure->modes += drm_extra_modes_for_range(closure->connector, + closure->edid, + timing); }
static int
On 2012-06-25 17:25 +0200, Adam Jackson wrote:
This fixes the extra_mode walk to be much more conservative. I still think the whole idea is bogus and that guessing about clone mode sizes is a userspace policy decision, but apparently xrandr --newmode / --addmode is unreasonably burdensome.
This should fix a number of reported regressions, please test.
Does not help in my case, unfortunately: instead of a bogus 1680x945 resolution I get a bogus 1400x1050 rather than the correct 1280x1024.
Going to try Takashi's patch instead.
Cheers, Sven
On Mon, 2012-06-25 at 19:14 +0200, Sven Joachim wrote:
On 2012-06-25 17:25 +0200, Adam Jackson wrote:
This fixes the extra_mode walk to be much more conservative. I still think the whole idea is bogus and that guessing about clone mode sizes is a userspace policy decision, but apparently xrandr --newmode / --addmode is unreasonably burdensome.
This should fix a number of reported regressions, please test.
Does not help in my case, unfortunately: instead of a bogus 1680x945 resolution I get a bogus 1400x1050 rather than the correct 1280x1024.
Going to try Takashi's patch instead.
Takashi's patch will promite 1280x1024 to the default - which is correct - but you'll still see a 1400x1050 in the mode list, because your monitor claims a maximum pixel clock of 140MHz and maximum hsync of 81kHz, and 1400x1050@60 fits in that.
Fixing that would probably require additional quirk work to add "preferred mode is physical pixel size". EDID 1.4 redefines the "first detailed mode is preferred" bit to mean that anyway, but we're not currently using that to filter the mode list.
- ajax
On Tue, Jun 26, 2012 at 3:41 AM, Adam Jackson ajax@redhat.com wrote:
On Mon, 2012-06-25 at 19:14 +0200, Sven Joachim wrote:
On 2012-06-25 17:25 +0200, Adam Jackson wrote:
This fixes the extra_mode walk to be much more conservative. I still think the whole idea is bogus and that guessing about clone mode sizes is a userspace policy decision, but apparently xrandr --newmode / --addmode is unreasonably burdensome.
This should fix a number of reported regressions, please test.
Does not help in my case, unfortunately: instead of a bogus 1680x945 resolution I get a bogus 1400x1050 rather than the correct 1280x1024.
Going to try Takashi's patch instead.
Takashi's patch will promite 1280x1024 to the default - which is correct
- but you'll still see a 1400x1050 in the mode list, because your
monitor claims a maximum pixel clock of 140MHz and maximum hsync of 81kHz, and 1400x1050@60 fits in that.
Fixing that would probably require additional quirk work to add "preferred mode is physical pixel size". EDID 1.4 redefines the "first detailed mode is preferred" bit to mean that anyway, but we're not currently using that to filter the mode list.
Hey ajax
still want these two patches in -next?
I've got them on my unreviewed list.
Dave.
dri-devel@lists.freedesktop.org
-
Adam Jackson -
Dave Airlie -
Sven Joachim