[PATCH 0/2] drm/edid: Undo the damage from adding extra_modes

[PATCH 0/2] drm/edid: Undo the damage from adding extra_modes

[Adam Jackson]

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

[PATCH 1/2] drm/edid: Be more conservative about maximum pixel clock

[Adam Jackson]

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
-- 
1.7.7.6

[PATCH 2/2] drm/edid: Be much more cautious about walking extra_modes

[Adam Jackson]

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
-- 
1.7.7.6

Re: [PATCH 0/2] drm/edid: Undo the damage from adding extra_modes

[Sven Joachim]

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

Re: [PATCH 0/2] drm/edid: Undo the damage from adding extra_modes

[Adam Jackson]

[-- Attachment #1.1: Type: text/plain, Size: 1158 bytes --]

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

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/2] drm/edid: Undo the damage from adding extra_modes

[Dave Airlie]

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.