Bug 35977 - [945GME] TV Mode incorrectly set in KMS on driver load
Summary: [945GME] TV Mode incorrectly set in KMS on driver load
Status: CLOSED FIXED
Alias: None
Product: DRI
Classification: Unclassified
Component: DRM/Intel (show other bugs)
Version: XOrg git
Hardware: x86 (IA32) Linux (All)
: medium normal
Assignee: Chris Wilson
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-04 22:49 UTC by Mathew McKernan
Modified: 2017-07-24 23:05 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
dmesg output (30.61 KB, text/plain)
2011-04-04 22:49 UTC, Mathew McKernan
no flags Details
lspci output (1.68 KB, text/plain)
2011-04-04 22:52 UTC, Mathew McKernan
no flags Details
Register Dump on Linux (127.05 KB, application/download)
2011-04-05 00:47 UTC, Mathew McKernan
no flags Details
PRoper Register Dump (12.98 KB, text/plain)
2011-04-05 01:12 UTC, Mathew McKernan
no flags Details
Remember TV type (2.40 KB, patch)
2011-04-05 23:19 UTC, Chris Wilson
no flags Details | Splinter Review
Only poll for TV connections. (1.25 KB, patch)
2011-04-05 23:43 UTC, Chris Wilson
no flags Details | Splinter Review
Remember TV type (2.40 KB, patch)
2011-04-06 00:14 UTC, Chris Wilson
no flags Details | Splinter Review

Description Mathew McKernan 2011-04-04 22:49:00 UTC
This issue occurs on a 945GME device that has only a component video device plugged into it (no LVDS and no VGA). Running kernel 2.6.38.2 or 2.6.37.5.

During the BIOS process the chipset correctly outputs 480i (component interlaced 480) and the TV displays the image.

When the i915 driver is loaded, the output reverts to NTSC and only provides composite video output. 

Even once Xorg is loaded, xrandr will not permit changing the output mode to 576p, 720p etc.
Comment 1 Mathew McKernan 2011-04-04 22:49:38 UTC
Created attachment 45261 [details]
dmesg output
Comment 2 Mathew McKernan 2011-04-04 22:52:31 UTC
Created attachment 45263 [details]
lspci output
Comment 3 Chris Wilson 2011-04-04 23:56:35 UTC
It looks like the code is sane enough in its handling of component-only formats. So it might just the register programming. I have yet to stumble across any Intel chipset with integrated TV, so you will be in for a long and bumpy ride...
Comment 4 Mathew McKernan 2011-04-04 23:59:55 UTC
(In reply to comment #3)
> It looks like the code is sane enough in its handling of component-only
> formats. So it might just the register programming. I have yet to stumble
> across any Intel chipset with integrated TV, so you will be in for a long and
> bumpy ride...

Thanks Chris.

What is the best step to take next, I can arrange an export of registers or to further investigate where the issue may be. I also have a working board on Windows that I can get information from if that helps.
Comment 5 Chris Wilson 2011-04-05 00:42:46 UTC
One step is that I want to liberally sprinkle the code with debug statements so that I can confirm that we are processing the component-only formats and modes correctly.

If you can grab a reg dump, that will help:

http://cgit.freedesktop.org/xorg/app/intel-gpu-tools tools/intel_reg_dumper

will work for linux. And if it is possible to grab an equivalent dump from a working windows box...

After that it is a grim task comparing code against spec looking for [the right] discrepancy.
Comment 6 Mathew McKernan 2011-04-05 00:47:49 UTC
Created attachment 45264 [details]
Register Dump on Linux
Comment 7 Mathew McKernan 2011-04-05 00:48:28 UTC
Thanks Chris for your help.

What is the best way to get a register dump from Windows?
Comment 8 Mathew McKernan 2011-04-05 00:55:04 UTC
Also, I'll have a crack tomorrow at smashing in some debug statements to help with finding the issue.

I presume debug statements in intel_tv_detect_type, intel_tv_get_modes would be the best place to start.
Comment 9 Chris Wilson 2011-04-05 01:00:25 UTC
(In reply to comment #6)
> Created an attachment (id=45264) [details]
> Register Dump on Linux

Ah, that's the output from intel_gpu_dump. We need intel_reg_dumper. (The program is not in the binary, but needs to be built from git).
Comment 10 Mathew McKernan 2011-04-05 01:12:04 UTC
Created attachment 45265 [details]
PRoper Register Dump

Apologies, clearly not reading today.
Comment 11 Mathew McKernan 2011-04-05 16:59:06 UTC
Did some more investigating, it appears the TV_CTL register is not playing nice.

The register is reporting an incorrect value or it is being set earlier by the i915 module.

It appears bits 28 and 29 are being reset to 00 rather than 10 in Component Mode. Using intel_reg_write I am able to re-enable the component mode output.

I have modified the driver on my host to troubleshoot and show what changes occur to the register before it is written back to the TV_CTL Register.

More information to follow shortly.
Comment 12 Mathew McKernan 2011-04-05 18:46:13 UTC
Within the function intel_tv_detect_type the driver detects the type of the outputs connected.

However, it appears intel_tv->type is always set to 0 as it is never set by any driver or DRM itself.

By adding:

intel_tv->type = type;

within the function after we detect the connector type this resolves the issue.

Further testing underway, as it also appears that there is some image issues on all resolutions. This appears related to bug #23899
Comment 13 Mathew McKernan 2011-04-05 21:16:09 UTC
Testing proved successful. Patch to be provided shortly.

Appears the screen tearing/sync loss occurs during the hotplug tests.

Disabling hotplug to see if this rectifies the issue. This appears to be the case due to something errornously thinking that a new device for video has been plugged in.
Comment 14 Mathew McKernan 2011-04-05 22:01:58 UTC
The temporary sync/image loss appears to be the fact that output_poll_execute was triggering tv_detect every time it fired.

Due to the requirements of checking the registers, you must turn off the TV Encoder before checking TV_DAC and TV_CTL for the presence of a screen.

Modified tv_init to disable polling on TV connectors.
Comment 15 Chris Wilson 2011-04-05 23:19:35 UTC
Created attachment 45336 [details] [review]
Remember TV type

Excellent work Matthew. In many ways I'm relieved to see such a superficial bug.

Can you please test the attached patch, and if you like it, reply with your signed-off-by. (i.e. Signed-off-by: Mathew McKernan <...>)

So after this patch we are reduced to bug #23899?
Comment 16 Mathew McKernan 2011-04-05 23:23:28 UTC
Not a problem Chris, took a bit of playing around but the ability to dump and write registers on the go made it all the easier to do.

The first issue of "probing" that appeared after fixing the tv_type appeared and has now been fixed too. I'll include it in the same patch, I just need to remove my tirade of debug statements which are definately not good for production code (e.g. abbreviations and silly statements to figure out what code was running when).

The issues from bug #23899 are still there, but I have managed to work around them by setting the resolution multiple times. It is a strange one, but it may be some sort of corruption in the registers with timing etc. I will look into that tomorrow.

I am glad it was a superficial bug too, I gathered it was as from looking at the code alone it looked as though it was doing everything it should.
Comment 17 Chris Wilson 2011-04-05 23:43:02 UTC
Created attachment 45337 [details] [review]
Only poll for TV connections.

Matthew, does this match what you did?
Comment 18 Mathew McKernan 2011-04-06 00:02:14 UTC
Hi Chris,

Looks good.

I simply set polled = false, I would suggest this would work in the same way.

The TV Type patch looks good.

I will test first thing tomorrow to make sure this all works.

Thanks very much for getting this sorted out for me, I am glad to see we have an end result that is working.
Comment 19 Chris Wilson 2011-04-06 00:14:14 UTC
Created attachment 45338 [details] [review]
Remember TV type

Back to basics...
Comment 20 Mathew McKernan 2011-04-10 15:54:09 UTC
Review of attachment 45337 [details] [review]:

Signed-off-by: Mathew McKernan <matmckernan@rauland.com.au>
Comment 21 Mathew McKernan 2011-04-10 15:54:26 UTC
Review of attachment 45338 [details] [review]:

Signed-off-by: Mathew McKernan <matmckernan@rauland.com.au>
Comment 22 Chris Wilson 2011-04-13 04:03:58 UTC
Your patches are on their way upstream, now residing in drm-intel-fixes, thanks!

commit d5627663f2088fa4be447fdcfd52bcb233448d85
Author: Mathew McKernan <matmckernan@rauland.com.au>
Date:   Tue Apr 12 06:51:37 2011 +0100

    drm/i915/tv: Remember the detected TV type
    
    During detect() we would probe the connection bits to determine if
    there was a TV attached, and what video input type (Component, S-Video,
    Composite, etc) to use. However, we promptly discarded this vital bit of
    information and never propagated it to where it was used to determine
    the correct modes and setup the control registers. Fix it!
    
    This fixes a regression from 7b334fcb45b757ffb093696ca3de1b0c8b4a33f1.
    
    Reported-and-tested-by: Mathew McKernan <matmckernan@rauland.com.au>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=35977
    Signed-off-by: Mathew McKernan <matmckernan@rauland.com.au>
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
    Cc: stable@kernel.org
    Acked-by: Paul Menzel <paulepanter@users.sourceforge.net>
    Signed-off-by: Keith Packard <keithp@keithp.com>

commit 89ea42d716e1ee94f643ecdc516d90a4111ec135
Author: Mathew McKernan <matmckernan@rauland.com.au>
Date:   Tue Apr 12 06:51:38 2011 +0100

    drm/i915/tv: Only poll for TV connections
    
    As a probe for a TV connection modifies the TV_CTL register, it causes a
    loss of sync and a regular glitch on the output. This is highly
    undesirable when using the TV, so only poll for TV connections and wait
    for an explicit query for detecting the disconnection event.
    
    Reported-by: Mathew McKernan <matmckernan@rauland.com.au>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=35977
    Signed-off-by: Mathew McKernan <matmckernan@rauland.com.au>
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
    Signed-off-by: Keith Packard <keithp@keithp.com>


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.