Bug 35977

Created attachment 45261 [details]
dmesg output

Created attachment 45263 [details]
lspci output

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...

(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.

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.

Created attachment 45264 [details]
Register Dump on Linux

Thanks Chris for your help.

What is the best way to get a register dump from Windows?

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.

(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).

Created attachment 45265 [details]
PRoper Register Dump

Apologies, clearly not reading today.

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.

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

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.

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.

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?

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.

Created attachment 45337 [details] [review]
Only poll for TV connections.

Matthew, does this match what you did?

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.

Created attachment 45338 [details] [review]
Remember TV type

Back to basics...

Review of attachment 45337 [details] [review]:

Signed-off-by: Mathew McKernan <matmckernan@rauland.com.au>

Review of attachment 45338 [details] [review]:

Signed-off-by: Mathew McKernan <matmckernan@rauland.com.au>

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>