diff mbox

drm: edid: enable probing and listing of non rb modes

Message ID 1384492138-12471-2-git-send-email-s.shirish@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Shirish S Nov. 15, 2013, 5:08 a.m. UTC 
The current solution checks for the existing RB mode,
if available in the edid block returns by adding it,
but does not populate the connector with the modes
of same resolution but which are non-rb modes.

As a result the probing and listing of non-rb modes can't
be made, in case the rb mode's pixel clock is not
supported but non-rb mode is supported.

This patch does 2 things:
1. adds "rb" suffix to rb modes.
2. changes the drm_mode_std mode selection to collect
   all the supported modes and not just one mode.

Signed-off-by: Shirish S <s.shirish@samsung.com>
---
 drivers/gpu/drm/drm_edid.c |   74 +++++++++++++++++++++-----------------------
 1 file changed, 36 insertions(+), 38 deletions(-)

Comments

Shirish S Nov. 19, 2013, 3:51 a.m. UTC | #1 
Hi,

On Sat, Nov 16, 2013 at 12:29 AM, Adam Jackson <ajax@redhat.com> wrote:
> On Fri, 2013-11-15 at 10:38 +0530, Shirish S wrote:
>> The current solution checks for the existing RB mode,
>> if available in the edid block returns by adding it,
>> but does not populate the connector with the modes
>> of same resolution but which are non-rb modes.
>>
>> As a result the probing and listing of non-rb modes can't
>> be made, in case the rb mode's pixel clock is not
>> supported but non-rb mode is supported.
>
> This is... almost okay.
>
> So the EDID 1.4 spec says that for modes in standard descriptors:
>
> a) if there's a match in DMT, use DMT
> b) if there's a range descriptor and it says GTF, use GTF
> c) if there's a range descriptor and it says CVT, use CVT
> d) if there's no range descriptor, use CVT
>
> But case d) is clearly insane if the sink is EDID 1.3, CVT wasn't a
> standard when 1.3 was defined, so the sink may not in fact support CVT
> timings.  Hence the logic in standard_timing_level().
>
> The other thing the spec says is that if you're using CVT for standard
> descriptors that you should use normal blanking instead of reduced.
> This is... problematic.  If we see 1920x1200@60 in a standard descriptor
> we almost certainly _don't_ mean normal blanking, because that won't fit
> on single-link DVI.  And if both the source and the sink support reduced
> blanking we should probably prefer it, it's marginally less power
> consumption on digital links and marginally better picture quality on
> analog links.
>
> So probably what we should do instead is:
>
> - if drm_monitor_supports_rb(), add both normal and reduced blanking
> timings for either the DMT or CVT path
> - extend drm_connector to also have rb-allowed to match interlace and
> doublescan
> - fix up all the drivers to indicate rb-allowed (except whatever broken
> thing it is you're trying to work around here)
> - fix drm_mode_validate_flag and callers to filter out rb modes as
> appropriate
>
> Also: CVT dates to 2003.  EDID 1.4 was 2006.  Any hardware being made in
> 2013 that can't generate RB timings is going out of its way to be
> broken.  You should really demand better from your hardware.
>
True, but we have to deal with such hardware also, so with this patch
can we add up to the mode collection logic, to accomodate such hardwares also?
>> 1. adds "rb" suffix to rb modes.
>
> No.  The userspace convention is:
>
> dmt:~% cvt -r 1920 1080
> # 1920x1080 59.93 Hz (CVT 2.07M9-R) hsync: 66.59 kHz; pclk: 138.50 MHz
> Modeline "1920x1080R"  138.50  1920 1968 2000 2080  1080 1083 1088 1111 +hsync -vsync
>
> drm_mode_parse_command_line_for_connector() expects the same thing.
> Let's be consistent.
>
I found that there is a suffix for interlaced modes, which got added
recently, hence thought of adding this,
with the addition of 'rb" suffix my system shows up more modes that is
supports and there is no need to
actually set the particular mode to know if its RB or non-RB.
Without this change it never showed 1920x1080R kind of mode, am i
missing anything?
>> @@ -1621,19 +1621,20 @@ drm_mode_std(struct drm_connector *connector, struct edid *edid,
>>               mode->hdisplay = 1366;
>>               mode->hsync_start = mode->hsync_start - 1;
>>               mode->hsync_end = mode->hsync_end - 1;
>> -             return mode;
>> +             goto done;
>>       }
>>
>>       /* check whether it can be found in default mode table */
>>       if (drm_monitor_supports_rb(edid)) {
>>               mode = drm_mode_find_dmt(dev, hsize, vsize, vrefresh_rate,
>>                                        true);
>> -             if (mode)
>> -                     return mode;
>> +             if (mode) {
>> +                     drm_mode_probed_add(connector, mode);
>> +                     modes++;
>> +             }
>>       }
>>       mode = drm_mode_find_dmt(dev, hsize, vsize, vrefresh_rate, false);
>> -     if (mode)
>> -             return mode;
>> +     goto done;
>>
>>       /* okay, generate it */
>>       switch (timing_level) {
>
> Unconditional "goto done" that skips all the logic for generating modes
> not in the dmt list?  This breaks everything _but_ case a) above, so eg.
> 1600x900@60 would no longer be generated.
>
My bad ,missed it, will correct it in the next patch set, if you are
ok with this logic of mine.
Kindly confirm if you want me to send next patch set with updated
changes or what/way i am working on
is not required.
> - ajax
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 830f750..3276761 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -184,7 +184,7 @@  static const struct drm_display_mode drm_dmt_modes[] = {
 		   896, 1048, 0, 600, 601, 604, 631, 0,
 		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC) },
 	/* 800x600@120Hz RB */
-	{ DRM_MODE("800x600", DRM_MODE_TYPE_DRIVER, 73250, 800, 848,
+	{ DRM_MODE("800x600rb", DRM_MODE_TYPE_DRIVER, 73250, 800, 848,
 		   880, 960, 0, 600, 603, 607, 636, 0,
 		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 	/* 848x480@60Hz */
@@ -213,7 +213,7 @@  static const struct drm_display_mode drm_dmt_modes[] = {
 		   1168, 1376, 0, 768, 769, 772, 808, 0,
 		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC) },
 	/* 1024x768@120Hz RB */
-	{ DRM_MODE("1024x768", DRM_MODE_TYPE_DRIVER, 115500, 1024, 1072,
+	{ DRM_MODE("1024x768rb", DRM_MODE_TYPE_DRIVER, 115500, 1024, 1072,
 		   1104, 1184, 0, 768, 771, 775, 813, 0,
 		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 	/* 1152x864@75Hz */
@@ -221,7 +221,7 @@  static const struct drm_display_mode drm_dmt_modes[] = {
 		   1344, 1600, 0, 864, 865, 868, 900, 0,
 		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC) },
 	/* 1280x768@60Hz RB */
-	{ DRM_MODE("1280x768", DRM_MODE_TYPE_DRIVER, 68250, 1280, 1328,
+	{ DRM_MODE("1280x768rb", DRM_MODE_TYPE_DRIVER, 68250, 1280, 1328,
 		   1360, 1440, 0, 768, 771, 778, 790, 0,
 		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 	/* 1280x768@60Hz */
@@ -237,11 +237,11 @@  static const struct drm_display_mode drm_dmt_modes[] = {
 		   1496, 1712, 0, 768, 771, 778, 809, 0,
 		   DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_PVSYNC) },
 	/* 1280x768@120Hz RB */
-	{ DRM_MODE("1280x768", DRM_MODE_TYPE_DRIVER, 140250, 1280, 1328,
+	{ DRM_MODE("1280x768rb", DRM_MODE_TYPE_DRIVER, 140250, 1280, 1328,
 		   1360, 1440, 0, 768, 771, 778, 813, 0,
 		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 	/* 1280x800@60Hz RB */
-	{ DRM_MODE("1280x800", DRM_MODE_TYPE_DRIVER, 71000, 1280, 1328,
+	{ DRM_MODE("1280x800rb", DRM_MODE_TYPE_DRIVER, 71000, 1280, 1328,
 		   1360, 1440, 0, 800, 803, 809, 823, 0,
 		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 	/* 1280x800@60Hz */
@@ -257,7 +257,7 @@  static const struct drm_display_mode drm_dmt_modes[] = {
 		   1496, 1712, 0, 800, 803, 809, 843, 0,
 		   DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_PVSYNC) },
 	/* 1280x800@120Hz RB */
-	{ DRM_MODE("1280x800", DRM_MODE_TYPE_DRIVER, 146250, 1280, 1328,
+	{ DRM_MODE("1280x800rb", DRM_MODE_TYPE_DRIVER, 146250, 1280, 1328,
 		   1360, 1440, 0, 800, 803, 809, 847, 0,
 		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 	/* 1280x960@60Hz */
@@ -269,7 +269,7 @@  static const struct drm_display_mode drm_dmt_modes[] = {
 		   1504, 1728, 0, 960, 961, 964, 1011, 0,
 		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC) },
 	/* 1280x960@120Hz RB */
-	{ DRM_MODE("1280x960", DRM_MODE_TYPE_DRIVER, 175500, 1280, 1328,
+	{ DRM_MODE("1280x960rb", DRM_MODE_TYPE_DRIVER, 175500, 1280, 1328,
 		   1360, 1440, 0, 960, 963, 967, 1017, 0,
 		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 	/* 1280x1024@60Hz */
@@ -285,7 +285,7 @@  static const struct drm_display_mode drm_dmt_modes[] = {
 		   1504, 1728, 0, 1024, 1025, 1028, 1072, 0,
 		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC) },
 	/* 1280x1024@120Hz RB */
-	{ DRM_MODE("1280x1024", DRM_MODE_TYPE_DRIVER, 187250, 1280, 1328,
+	{ DRM_MODE("1280x1024rb", DRM_MODE_TYPE_DRIVER, 187250, 1280, 1328,
 		   1360, 1440, 0, 1024, 1027, 1034, 1084, 0,
 		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 	/* 1360x768@60Hz */
@@ -293,11 +293,11 @@  static const struct drm_display_mode drm_dmt_modes[] = {
 		   1536, 1792, 0, 768, 771, 777, 795, 0,
 		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC) },
 	/* 1360x768@120Hz RB */
-	{ DRM_MODE("1360x768", DRM_MODE_TYPE_DRIVER, 148250, 1360, 1408,
+	{ DRM_MODE("1360x768rb", DRM_MODE_TYPE_DRIVER, 148250, 1360, 1408,
 		   1440, 1520, 0, 768, 771, 776, 813, 0,
 		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 	/* 1400x1050@60Hz RB */
-	{ DRM_MODE("1400x1050", DRM_MODE_TYPE_DRIVER, 101000, 1400, 1448,
+	{ DRM_MODE("1400x1050rb", DRM_MODE_TYPE_DRIVER, 101000, 1400, 1448,
 		   1480, 1560, 0, 1050, 1053, 1057, 1080, 0,
 		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 	/* 1400x1050@60Hz */
@@ -313,11 +313,11 @@  static const struct drm_display_mode drm_dmt_modes[] = {
 		   1656, 1912, 0, 1050, 1053, 1057, 1105, 0,
 		   DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_PVSYNC) },
 	/* 1400x1050@120Hz RB */
-	{ DRM_MODE("1400x1050", DRM_MODE_TYPE_DRIVER, 208000, 1400, 1448,
+	{ DRM_MODE("1400x1050rb", DRM_MODE_TYPE_DRIVER, 208000, 1400, 1448,
 		   1480, 1560, 0, 1050, 1053, 1057, 1112, 0,
 		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 	/* 1440x900@60Hz RB */
-	{ DRM_MODE("1440x900", DRM_MODE_TYPE_DRIVER, 88750, 1440, 1488,
+	{ DRM_MODE("1440x900rb", DRM_MODE_TYPE_DRIVER, 88750, 1440, 1488,
 		   1520, 1600, 0, 900, 903, 909, 926, 0,
 		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 	/* 1440x900@60Hz */
@@ -333,7 +333,7 @@  static const struct drm_display_mode drm_dmt_modes[] = {
 		   1696, 1952, 0, 900, 903, 909, 948, 0,
 		   DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_PVSYNC) },
 	/* 1440x900@120Hz RB */
-	{ DRM_MODE("1440x900", DRM_MODE_TYPE_DRIVER, 182750, 1440, 1488,
+	{ DRM_MODE("1440x900rb", DRM_MODE_TYPE_DRIVER, 182750, 1440, 1488,
 		   1520, 1600, 0, 900, 903, 909, 953, 0,
 		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 	/* 1600x1200@60Hz */
@@ -357,11 +357,11 @@  static const struct drm_display_mode drm_dmt_modes[] = {
 		   1856, 2160, 0, 1200, 1201, 1204, 1250, 0,
 		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC) },
 	/* 1600x1200@120Hz RB */
-	{ DRM_MODE("1600x1200", DRM_MODE_TYPE_DRIVER, 268250, 1600, 1648,
+	{ DRM_MODE("1600x1200rb", DRM_MODE_TYPE_DRIVER, 268250, 1600, 1648,
 		   1680, 1760, 0, 1200, 1203, 1207, 1271, 0,
 		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 	/* 1680x1050@60Hz RB */
-	{ DRM_MODE("1680x1050", DRM_MODE_TYPE_DRIVER, 119000, 1680, 1728,
+	{ DRM_MODE("1680x1050rb", DRM_MODE_TYPE_DRIVER, 119000, 1680, 1728,
 		   1760, 1840, 0, 1050, 1053, 1059, 1080, 0,
 		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
 	/* 1680x1050@60Hz */
@@ -1561,12 +1561,12 @@  bad_std_timing(u8 a, u8 b)
  * Take the standard timing params (in this case width, aspect, and refresh)
  * and convert them into a real mode using CVT/GTF/DMT.
  */
-static struct drm_display_mode *
-drm_mode_std(struct drm_connector *connector, struct edid *edid,
+unsigned int drm_mode_std(struct drm_connector *connector, struct edid *edid,
 	     struct std_timing *t, int revision)
 {
 	struct drm_device *dev = connector->dev;
 	struct drm_display_mode *m, *mode = NULL;
+	unsigned int modes = 0;
 	int hsize, vsize;
 	int vrefresh_rate;
 	unsigned aspect_ratio = (t->vfreq_aspect & EDID_TIMING_ASPECT_MASK)
@@ -1576,7 +1576,7 @@  drm_mode_std(struct drm_connector *connector, struct edid *edid,
 	int timing_level = standard_timing_level(edid);
 
 	if (bad_std_timing(t->hsize, t->vfreq_aspect))
-		return NULL;
+		return modes;
 
 	/* According to the EDID spec, the hdisplay = hsize * 8 + 248 */
 	hsize = t->hsize * 8 + 248;
@@ -1612,7 +1612,7 @@  drm_mode_std(struct drm_connector *connector, struct edid *edid,
 	list_for_each_entry(m, &connector->probed_modes, head)
 		if (m->hdisplay == hsize && m->vdisplay == vsize &&
 		    drm_mode_vrefresh(m) == vrefresh_rate)
-			return NULL;
+			return modes;
 
 	/* HDTV hack, part 2 */
 	if (hsize == 1366 && vsize == 768 && vrefresh_rate == 60) {
@@ -1621,19 +1621,20 @@  drm_mode_std(struct drm_connector *connector, struct edid *edid,
 		mode->hdisplay = 1366;
 		mode->hsync_start = mode->hsync_start - 1;
 		mode->hsync_end = mode->hsync_end - 1;
-		return mode;
+		goto done;
 	}
 
 	/* check whether it can be found in default mode table */
 	if (drm_monitor_supports_rb(edid)) {
 		mode = drm_mode_find_dmt(dev, hsize, vsize, vrefresh_rate,
 					 true);
-		if (mode)
-			return mode;
+		if (mode) {
+			drm_mode_probed_add(connector, mode);
+			modes++;
+		}
 	}
 	mode = drm_mode_find_dmt(dev, hsize, vsize, vrefresh_rate, false);
-	if (mode)
-		return mode;
+	goto done;
 
 	/* okay, generate it */
 	switch (timing_level) {
@@ -1650,7 +1651,7 @@  drm_mode_std(struct drm_connector *connector, struct edid *edid,
 		 */
 		mode = drm_gtf_mode(dev, hsize, vsize, vrefresh_rate, 0, 0);
 		if (!mode)
-			return NULL;
+			return modes;
 		if (drm_mode_hsync(mode) > drm_gtf2_hbreak(edid)) {
 			drm_mode_destroy(dev, mode);
 			mode = drm_gtf_mode_complex(dev, hsize, vsize,
@@ -1666,7 +1667,14 @@  drm_mode_std(struct drm_connector *connector, struct edid *edid,
 				    false);
 		break;
 	}
-	return mode;
+
+done:
+	if (mode) {
+		drm_mode_probed_add(connector, mode);
+		modes++;
+	}
+
+	return modes;
 }
 
 /*
@@ -2148,15 +2156,10 @@  do_standard_modes(struct detailed_timing *timing, void *c)
 		int i;
 		for (i = 0; i < 6; i++) {
 			struct std_timing *std;
-			struct drm_display_mode *newmode;
 
 			std = &data->data.timings[i];
-			newmode = drm_mode_std(connector, edid, std,
+			closure->modes += drm_mode_std(connector, edid, std,
 					       edid->revision);
-			if (newmode) {
-				drm_mode_probed_add(connector, newmode);
-				closure->modes++;
-			}
 		}
 	}
 }
@@ -2177,15 +2180,10 @@  add_standard_modes(struct drm_connector *connector, struct edid *edid)
 	};
 
 	for (i = 0; i < EDID_STD_TIMINGS; i++) {
-		struct drm_display_mode *newmode;
 
-		newmode = drm_mode_std(connector, edid,
+		modes += drm_mode_std(connector, edid,
 				       &edid->standard_timings[i],
 				       edid->revision);
-		if (newmode) {
-			drm_mode_probed_add(connector, newmode);
-			modes++;
-		}
 	}
 
 	if (version_greater(edid, 1, 0))