Bug 65524 - [sdvo lvds regression] "Use &intel_sdvo->ddc instead of intel_sdvo->i2c for DDC"
Summary: [sdvo lvds regression] "Use &intel_sdvo->ddc instead of intel_sdvo->i2c for DDC"
Status: CLOSED FIXED
Alias: None
Product: DRI
Classification: Unclassified
Component: DRM/Intel (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Intel GFX Bugs mailing list
QA Contact: Intel GFX Bugs mailing list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-07 22:42 UTC by Chris Wilson
Modified: 2017-07-24 22:58 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
working dmesg (71.42 KB, text/plain)
2013-06-07 23:10 UTC, Chris Wilson
no flags Details
Broken dmesg (72.01 KB, text/plain)
2013-06-07 23:11 UTC, Chris Wilson
no flags Details
prefer vbt over edid for sdvo lvds (2.78 KB, patch)
2013-06-08 13:07 UTC, Daniel Vetter
no flags Details | Splinter Review

Description Chris Wilson 2013-06-07 22:42:58 UTC
Most amusing,

53d3b4d7778daf15900867336c85d3f8dd70600c is the first bad commit
commit 53d3b4d7778daf15900867336c85d3f8dd70600c
Author: Egbert Eich <eich@suse.de>
Date:   Tue Jun 4 17:13:21 2013 +0200

    drm/i915/sdvo: Use &intel_sdvo->ddc instead of intel_sdvo->i2c for DDC.

causes the loss of modesetting on an SVDO LVDS output (Libretto W105).
Comment 1 Chris Wilson 2013-06-07 23:10:19 UTC
Of course, that commit is just the icing on the case. After Egbert's patch, we successfully read the EDID for the SDVO panel and then things go wrong...

Before:
[    0.610486] [drm:drm_helper_probe_single_connector_modes], [CONNECTOR:15:LVDS-2] probed modes :
[    0.610490] [drm:drm_mode_debug_printmodeline], Modeline 17:"1024x600" 60 50500 1024 1034 1066 1348 600 612 616 624 0x48 0xa

After:
[    0.653730] [drm:drm_helper_probe_single_connector_modes], [CONNECTOR:15:LVDS-2] probed modes :
[    0.653733] [drm:drm_mode_debug_printmodeline], Modeline 18:"1024x600" 60 44700 1024 1034 1066 1194 600 612 616 624 0x48 0xa
[    0.653739] [drm:drm_mode_debug_printmodeline], Modeline 19:"1024x600" 40 29800 1024 1034 1066 1194 600 612 616 624 0x40 0xa

And then there is an even greater discrepancy in the adjusted modes for the first modeset:

Before:
[    1.110223] [drm:intel_dump_pipe_config], adjusted mode:
[    1.110228] [drm:drm_mode_debug_printmodeline], Modeline 0:"1024x600" 60 101000 1024 1034 1066 1348 600 612 616 624 0x48 0xa

After:
[    1.186459] [drm:intel_dump_pipe_config], adjusted mode:
[    1.186463] [drm:drm_mode_debug_printmodeline], Modeline 0:"1024x600" 60 178800 1024 1034 1066 1194 600 612 616 624 0x48 0xa


Before we have a pixel multiplier of about 2, and afterwards closer to 4.
Comment 2 Chris Wilson 2013-06-07 23:10:47 UTC
Created attachment 80497 [details]
working dmesg
Comment 3 Chris Wilson 2013-06-07 23:11:05 UTC
Created attachment 80498 [details]
Broken dmesg
Comment 4 Chris Wilson 2013-06-07 23:13:42 UTC
Note in broken "[    1.425654] [drm:intel_enable_sdvo], First SDVOB output reported failure to sync"
Comment 5 Chris Wilson 2013-06-07 23:18:06 UTC
Indeed tweaking the pixel multiplier so that it comes out at 2 makes the SDVO LVDS panel work again:

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 45aa32a..20a6c79 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -577,9 +577,9 @@ log_fail:
 
 static int intel_sdvo_get_pixel_multiplier(struct drm_display_mode *mode)
 {
-	if (mode->clock >= 100000)
+	if (mode->clock >= 80000)
 		return 1;
-	else if (mode->clock >= 50000)
+	else if (mode->clock >= 40000)
 		return 2;
 	else
 		return 4;
Comment 6 Daniel Vetter 2013-06-08 13:01:10 UTC
Since sdvo lvds apparently worked since forever ... should we just do the edid read _after_ the vbt reading?

Iirc there's been quite a few machines with sdvo lvds out there and the bug Egbert found is really old, so I'm fairly positive that it should work.
Comment 7 Daniel Vetter 2013-06-08 13:07:14 UTC
Created attachment 80525 [details] [review]
prefer vbt over edid for sdvo lvds
Comment 8 Chris Wilson 2013-06-08 22:19:38 UTC
The modelines in the EDID look sane, so I think we do have an issue in the way we compute the pixel multiplier raising it above the bandwidth limit.
Comment 9 Daniel Vetter 2013-06-09 09:13:04 UTC
Spec says clearly that the sdvo signal must be at least 100MHz, and at least the two sdvo encoders I have here (one DVI, other HDMI) have no issues with the low-res modes.

My pet theory here is simply that this board/sdvo encoder combination has a problem with 4x pixel multiplier mode and the firmware engineer "fixed" this by frobbing the mode in the vbt a bit to push it over 50MHz dotclock.

We could test this if you force the 4x mutliplier on the vbt mode, sdvo dotclock should be in the range 100-225MHz iirc, so it should still work.
Comment 10 Chris Wilson 2013-06-09 17:17:13 UTC
Using the EDID:

requested: Modeline 0:"1024x600" 60 44700 1024 1034 1066 1194 600 612 616 624 0x48 0xa
preferred input: Modeline 0:"1024x600" 60 44700 1024 1034 1066 1194 600 612 616 624 0x48 0xa
adjusted: Modeline 0:"1024x600" 60 178800 1024 1034 1066 1194 600 612 616 624 0x48 0xa

-> refclk=120000, clock=(.m1=14, .m2=7, n=4, .p1=1, .p2=10), computed dotclock = 178000 [broken]

with pixel_multiplier=2:

refclk=120000, clock=(.m1=14, .m2=7, n=2, .p1=3, .p2=10), computed dotclock = 89000 [works]

Using VBT:

request: Modeline 0:"1024x600" 60 50500 1024 1034 1066 1348 600 612 616 624 0x48 0xa
preferred input: Modeline 0:"1024x600" 60 50500 1024 1034 1066 1348 600 612 616 624 0x48 0xa
adjusted:  Modeline 0:"1024x600" 60 101000 1024 1034 1066 1348 600 612 616 624 0x48 0xa

->  refclk=120000, clock=(.m1=16, .m2=9, n=2, .p1=3, .p2=10), computed dotclock = 101000 [works]

with pixel_multiplier=4:
-> refclk=120000, clock=(.m1=13, .m2=7, n=3, .p1=1, .p2=10), computed dotclock = 201600 [broken]


The bspec is very clear in that the allowable dotclock range for sDVO is 100-225MHz on Ironlake. So I have no idea why either the forced pixel_multiplier=2 works for the EDID mode, nor why it fails with pixel_multiplier=4. I also distrust the VBT modes, on other machines they are garbage. It is a less than satisfactory situation.
Comment 11 Daniel Vetter 2013-06-09 18:41:46 UTC
Personally your experimental results reinforces my opinion that someone ordered the shittiest sdvo encoder and the tiniest panel they could find and then noticed that stuff doesn't work with a pixel mutliplier of 4.

To fix this they've frobbed the panel's mode so that it's just barely above 50MHz and this way tricked the Windows driver into picking a pixel mutliplier of 2 without overtly offending the panel.

I share your fear for the reliability of VBT modes, but in this specific case of SDVO-LVDS it looks like the vbt is actually more reliable. Especially if you combine this with the fact that we've ignored the edid modes for a few years completely. Maybe we need to double-check with egbert, but iirc the panel which fails for him didn't have any vbt mode at all.

Egbert, can you please check whether my patch doesn't break your system?
Comment 12 Egbert Eich 2013-06-09 20:02:12 UTC
I'm positive that your patch will not harm the TGCS (foermerly IBM) POS system on which I discovered that reading EDID was broken.
For entertainment let me share the story that led to this discovery:
The vendor's QA complained that the driver we ship in the next SP (SP3) of our enterprise product only allows to set an 800x600 mode on the built-in 1024x768 panel correctly when the VBIOS option "Windows 7" is selected but not when "All Others" is set. The additional claim that this used to work with SP1 let me to investigate the issue further: it turned out that the 'Windows 7' VBIOS contained a VBT table, while the other one didn't. A test with the SP1 kernel revealed that on this kernel the panel provided an EDID thus the fixed panel mode was set even in the absence of a VBT.
The rest was then easy to spot.
Comment 13 Daniel Vetter 2013-06-10 08:55:57 UTC
Patch merged to -fixes as:

commit c3456fb3e4712d0448592af3c5d644c9472cd3c1
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Mon Jun 10 09:47:58 2013 +0200

    drm/i915: prefer VBT modes for SVDO-LVDS over EDID
Comment 14 Trey Ramsay 2013-06-22 12:45:56 UTC
Amusing.. I was going to commit a fix to the upstream kernel when I came across this bug.   

The complaint was that they were not able read the /sys/class/drm/card0-LVDS-1/edid information from their lvds display.  Sound familiar?

Long story short it was the same &intel_sdvo->ddc change.  However I didn't know about the pixel multiplier discrepancy.  Good job fixing that!


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.