With the current implementation of collecting edid modes, in case rb mode exists for a non rb mode of same resolution and vrefresh, the non-rb mode is never fed to display controller to be probed, as a result we lose on using the non-rb mode, if the display controller does not support rb mode but supports non-rb.
For instance in case of 1680x1050@60Hz: with monitor whose EDID supports RB, if connected to display controller does not support 1680x1050@60Hz RB but it supports 1680x1050@60Hz, with the current implementation the non-rb mode 1680x1050@60Hz is not listed in connectors probed modes and hence on running xrandr is not listed.
Shirish S (1): drm: edid: enable probing and listing of non rb modes
drivers/gpu/drm/drm_edid.c | 74 +++++++++++++++++++++----------------------- 1 file changed, 36 insertions(+), 38 deletions(-)
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(-)
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))
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.
- 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.
@@ -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++;}
- 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.
- ajax
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?
- 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
dri-devel@lists.freedesktop.org
-
Adam Jackson -
Shirish S -
Shirish S