Bug 63031 - [Intel GM45][i915-kms]: VGA encoder on SDVOB remains blank when i915.ko is loaded while gfx is in text mode.
Summary: [Intel GM45][i915-kms]: VGA encoder on SDVOB remains blank when i915.ko is lo...
Status: CLOSED FIXED
Alias: None
Product: DRI
Classification: Unclassified
Component: DRM/Intel (show other bugs)
Version: XOrg git
Hardware: x86-64 (AMD64) All
: medium normal
Assignee: Daniel Vetter
QA Contact: Intel GFX Bugs mailing list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-02 14:25 UTC by Egbert Eich
Modified: 2017-07-24 22:58 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
dmesg output of non-working case. (134.34 KB, text/plain)
2013-04-02 15:01 UTC, Egbert Eich
no flags Details
dmesg output of working case. (105.06 KB, text/plain)
2013-04-02 15:01 UTC, Egbert Eich
no flags Details
check sdvo port in sdvo connector get_hw_state (1.64 KB, patch)
2013-04-02 15:43 UTC, Daniel Vetter
no flags Details | Splinter Review
log with patch from https://bugs.freedesktop.org/show_bug.cgi?id=60138 applied. (99.09 KB, text/plain)
2013-04-02 16:54 UTC, Egbert Eich
no flags Details
Fix (1.32 KB, patch)
2013-04-04 14:58 UTC, Egbert Eich
no flags Details | Splinter Review
Fix backing out commit derived from attachment #77328. (2.80 KB, patch)
2013-04-04 19:45 UTC, Egbert Eich
no flags Details | Splinter Review

Description Egbert Eich 2013-04-02 14:25:29 UTC
With any latest Intel DRM kernels the VGA output on SDVOB remains dark when a VGA text mode was set prior to loading the i915 driver. 
When a VBE gfx mode was loaded, the system comes up fine.

Here is what happens:
When the driver loads intel_modeset_setup_hw_state() is called at one point to collect the current state of the hardware setting. 
At one point, this function calls into intel_sdvo_get_hw_state() to check if bit 32 of register GEN3_SDVOB. If this bit is 0 the function returns 'false' indicating there is no active pipe connected to this encoder. intel_modeset_setup_hw_state() then calls intel_sanitize_encoder() which will call intel_connector_break_all_links() in this situation. 
At a later point intel_connector_check_state() will complain heavily.
As a result SDVOB will be treated as disabled and unconnected during the entire mode setting.

Setting bit 31 of GEN3_SDVOB at any point before intel_modeset_setup_hw_state() is called will cause the driver to correctly initialize the SDVOB.
Also setting a VBE gfx mode prior to loading the driver will set this bit and thus the DAC encoder on SDVOB will be initialized correctly.

Thus the kernel is misled by a bogus mode set by the BIOS,
Comment 1 Egbert Eich 2013-04-02 15:01:01 UTC
Created attachment 77323 [details]
dmesg output of non-working case.
Comment 2 Egbert Eich 2013-04-02 15:01:40 UTC
Created attachment 77324 [details]
dmesg output of working case.
Comment 3 Daniel Vetter 2013-04-02 15:43:56 UTC
Created attachment 77328 [details] [review]
check sdvo port in sdvo connector get_hw_state

I've thought I've written this particular patch a few times by now, but it seems to be lost :( Please test.
Comment 4 Daniel Vetter 2013-04-02 15:45:15 UTC
Ah, found the patch+bug again: https://bugs.freedesktop.org/show_bug.cgi?id=60138
Comment 5 Egbert Eich 2013-04-02 16:11:34 UTC
The patch made the settings created by intel_modeset_setup_hw_state() more consistent, such that the noise from sanitize_encoder() is now gone. 
Unfortunately, the display still does not light up. I believe the reason for this is:

[   67.744037] [drm:intel_sdvo_debug_write], SDVOB: W: 0B                         (SDVO_CMD_GET_ATTACHED_DISPLAYS)
[   67.745586] [drm:intel_sdvo_read_response], SDVOB: R: (Pending)... failed
[   67.928935] [drm:output_poll_execute], [CONNECTOR:63:VGA-2] status updated from 3 to 3

Those lines were in the attached dmesg output of the non-working case and they are still there.
Comment 6 Chris Wilson 2013-04-02 16:23:13 UTC
You can play around with the delays in intel_sdvo_read_response() to see how long it takes for the VGA device to respond.
Comment 7 Egbert Eich 2013-04-02 16:54:57 UTC
Created attachment 77330 [details]
log with patch from https://bugs.freedesktop.org/show_bug.cgi?id=60138 applied.

Patch from https://bugs.freedesktop.org/show_bug.cgi?id=60138 gets rid of the warnings.
Same is true for patch from attachment #77328 [details] [review].

Tested-By: Egbert Eich <eich_at_freedesktop.org>

for either one.
Comment 8 Egbert Eich 2013-04-02 18:39:02 UTC
(In reply to comment #6)
> You can play around with the delays in intel_sdvo_read_response() to see how
> long it takes for the VGA device to respond.

Chris, I did already. I was not able to find a value where the VGA device did respond.
Currently I'm comparing the code paths taken for the case where bit 1 in GEN3_SDVOB is set when the driver is loaded (ie where a an active pipe for this encoder is found) and where it isn't as in the former case we always get a valid response.
Comment 9 Egbert Eich 2013-04-03 12:49:38 UTC
Ok, got a bit further, 
looks like the state of the SDVOB is not consistent:
I do get valid output from SDVO_CMD_GET_ATTACHED_DISPLAYS when SDVO_CMD_SET_ACTIVE_OUTPUTS is set to 0 prior to reading SDVO_CMD_GET_ATTACHED_DISPLAYS.

To get valid output with SDVO_CMD_SET_ACTIVE_OUTPUTS set to anything but 0 some other conditions need to be met also. I have not yet been able to determine what these are.

In any case, calling encoder->disable() encoder somewhere in sanitize_encoder() for DAC-11 will get sdvo_detect() to detect the attached display correctly.
Comment 10 Daniel Vetter 2013-04-03 15:28:17 UTC
Ooops, there seems to be a bug in my santize_encoder function. Does the below snippet help?

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8e6f906..9a525e5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9074,7 +9074,7 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
                /* Connector is active, but has no active pipe. This is
                 * fallout from our resume register restoring. Disable
                 * the encoder manually again. */
-               if (encoder->base.crtc) {
+               if (!encoder->base.crtc) {
                        DRM_DEBUG_KMS("[ENCODER:%d:%s] manually disabled\n",
                                      encoder->base.base.id,
                                      drm_get_encoder_name(&encoder->base));
Comment 11 Daniel Vetter 2013-04-03 15:32:13 UTC
Actually this is wrong, if the encoder is active it should _always_ have a crtc. Can you please sprinkle printks and try to figure out why the encoder is active, but doesn't seem to have a crtc attached?
Comment 12 Egbert Eich 2013-04-03 16:52:44 UTC
(In reply to comment #11)
> Actually this is wrong, if the encoder is active it should _always_ have a
> crtc. Can you please sprinkle printks and try to figure out why the encoder
> is active, but doesn't seem to have a crtc attached?


I had added such printks already. In the logs I've posted you will find lines containing "DAC-11->base.crtc <=".
In fact the encoder is not active: intel_sdvo_get_hw_state() returns false. Thus it doesn't have a crtc:
[    3.703241] [drm:intel_modeset_setup_hw_state], 2: DAC-11->base.crtc <=           (null)
Still it has active outputs: intel_sdvo_get_active_outputs() returns 2.
What I seem to have found out now:
When an encoder is disabled (bit 31 of GEN3_SDVOx is 0) then it must not have active outputs or SDVO_CMD_GET_ATTACHED_DISPLAYS will not return a result (it will remain in pending state).
Comment 13 Daniel Vetter 2013-04-03 20:37:34 UTC
Hm, maybe we should adjust the sdvo hw state code then to return true when either the SDVO port is on or when the external sdvo encoder reports an active output? Then the encoder sanitize functions would call ->disable when we want it to do so.
Comment 14 Egbert Eich 2013-04-04 14:58:27 UTC
Created attachment 77425 [details] [review]
Fix

Daniel, indeed, your suggestion did the job.
The inconsistency in state (ie encoder active while connector inactive) gets fixed later on in intel_sanitize_crtc().
Comment 15 Daniel Vetter 2013-04-04 16:05:43 UTC
My quick patch review on irc, in case it gets lost:

danvet> egbert, inverted logic in your patch description
<danvet> it's only considered _in_active when both tests fail
<danvet> egbert, does that patch cause any issue wrt state checking?
<danvet> since right now the connector will say that it's not active, but the encoder says it is
<danvet> so I think we need to also revert my earlier patch again

Egbert, can you pls spin a v2 and then submit it to intel-gfx?
Comment 16 Egbert Eich 2013-04-04 19:45:46 UTC
Created attachment 77442 [details] [review]
Fix backing out commit derived from attachment #77328 [details] [review].
Comment 17 Daniel Vetter 2013-04-10 20:12:52 UTC
Patch merged

commit 1e303c6febfdc23258d6ecea79c9592389a28964
Author: Egbert Eich <eich@suse.de>
Date:   Thu Apr 4 16:04:02 2013 -0400

    drm/i915: Fix SDVO connector and encoder get_hw_state functions

Ebgert, thanks a lot for going through this ordeal!


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.