Bug 47007

Summary: HDMI monitor polling causing 100ms rendering stalls
Product: DRI Reporter: Tvrtko Ursulin <tvrtko.ursulin>
Component: DRM/RadeonAssignee: Default DRI bug account <dri-devel>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium CC: tcwardrobe
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments:
Description Flags
DRM related kernel messages
none
Xorg.log, as the name says.
none
Patch which makes the stalls go away
none
Different approach on fixing the stalls
none
Do not re-fetch full EDID while a HPD capable output remains connected
none
Use shared HPD for VGA on R600+
none
Video BIOS from the board with DP + DVI-I (shared DDC setup)
none
Correctly identify connector on Fujitsu D3003-S2 board
none
Donot probe extensively if HPD hasn't changed on R600+ ASICs
none
add quirk for fujitsu board
none
Do not probe extensively if HPD hasn't changed on R600+ ASICs
none
fix dvi-i load detection none

Description Tvrtko Ursulin 2012-03-06 07:51:09 UTC
I am experiencing rendering stalls every ten seconds (drm_kms_helper output connector poll interval) on a AMD G-T56N (Radeon HD 6310) box connected to the monitor via HDMI connector.

Kernel is kernel.org's 3.3.0-rc6 kernel.

I've delved into the code a bit, sprayed some printks around, and found that the culprit is the drm_get_edid call from radeon_dvi_detect where the former takes more than 100ms. I suspect I2C transfers there are disabling interrupts because I don't see any locks other than drm_device mode mutex being help at that at least sounds harmless.

This function (radeon_dvi_detect) is a bit advanced for a newcomer in this area, but, looking at the accompanying comment and the code, I am not sure it could not be improved?

For example if monitor is connected, and we know it is a digital monitor, do we need to discard and re-fetch complete EDID every time? Couldn't we just do a quick probe to check for disconnection instead?

This is if I understand correctly that EDID probing must be done for disconnection check?
Comment 1 Alex Deucher 2012-03-06 07:56:58 UTC
Please attach your xorg log and dmesg output.  Unless the OEM forgot to wire up the hotplug pin, the driver shouldn't be polling.  Check to make sure your desktop environment is not manually polling (gnome power manager or the kde equivalent, etc.).
Comment 2 Tvrtko Ursulin 2012-03-06 08:11:28 UTC
Created attachment 58063 [details] [review]
DRM related kernel messages

With my debug I have a problem with the kernel log buffer filling to quickly. Is this DRM related subset of dmesg sufficient or you want me to collect something more?
Comment 3 Tvrtko Ursulin 2012-03-06 08:12:04 UTC
Created attachment 58064 [details] [review]
Xorg.log, as the name says.
Comment 4 Tvrtko Ursulin 2012-03-06 08:13:20 UTC
(In reply to comment #1)
> Please attach your xorg log and dmesg output.  Unless the OEM forgot to wire up

Attached.

> the hotplug pin, the driver shouldn't be polling.  Check to make sure your
> desktop environment is not manually polling (gnome power manager or the kde
> equivalent, etc.).

There is no desktop environment running. What would this polling be in terms of X calls?
Comment 5 Tvrtko Ursulin 2012-03-06 08:15:32 UTC
(In reply to comment #1)
> Please attach your xorg log and dmesg output.  Unless the OEM forgot to wire up
> the hotplug pin, the driver shouldn't be polling.  Check to make sure your
> desktop environment is not manually polling (gnome power manager or the kde
> equivalent, etc.).

Also, there are no stalls when the VGA output is used (HDMI disconnected).
Comment 6 Tvrtko Ursulin 2012-03-07 02:10:26 UTC
Further investigation along the lines of your comment.

For the HDMI connector ATOM_HPD_INT_RECORD_TYPE contains a gpio->mask of 0x100 which translates to RADEON_HPD_2. hpd.plugged_state is zero.

When hpd.hpd is not RADEON_HPD_NONE radeon_add_atom_connector sets connector->polled to DRM_CONNECTOR_POLL_HPD. So if poll helper is running it will result in the observed behaviour -> discard and re-fetch full EDID on every poll even when the connector hasn't been re-connected.

But you are saying poll helper should not be running right? I'll investigate that area next.
Comment 7 Tvrtko Ursulin 2012-03-07 02:42:18 UTC
Poll helper is running every ten seconds because VGA connector "asks it to", given how it has DRM_CONNECTOR_POLL_CONNECT set.

Since the poll helper runs, and HDMI connector has DRM_CONNECTOR_POLL_HPD set due to hpd.hpd being not RADEON_HPD_NONE, it calls it's detect method (radeon_dvi_detect).

This in turns discards an re-fetches the full EDID every time it runs.

What is the bug here?

1. That HDMI connector has DRM_CONNECTOR_POLL_HPD set when hpd.hpd is not RADEON_HPD_NONE?
2. That discard and full fetch of EDID is done when is not needed?

Or both? Or something else? Hard to say for me. HPD sounds like Hotplug detect? So setting that DRM_CONNECTOR_POLL_HPD when the connector supports something other than RADEON_HPD_NONE sounds fishy to me.

But re-fetching full EDID is also silly. Might not be relevant if the detect "method" should not run in the first place...
Comment 8 Alex Deucher 2012-03-07 12:03:07 UTC
(In reply to comment #7)
> Poll helper is running every ten seconds because VGA connector "asks it to",
> given how it has DRM_CONNECTOR_POLL_CONNECT set.

Right.  The VGA connector has to poll because analog VGA does not support hotplug detect.

> 
> Since the poll helper runs, and HDMI connector has DRM_CONNECTOR_POLL_HPD set
> due to hpd.hpd being not RADEON_HPD_NONE, it calls it's detect method
> (radeon_dvi_detect).
> 
> This in turns discards an re-fetches the full EDID every time it runs.
> 
> What is the bug here?
> 
> 1. That HDMI connector has DRM_CONNECTOR_POLL_HPD set when hpd.hpd is not
> RADEON_HPD_NONE?

That's correct.  If the connector supports an hotplug pin (hpd.hpd != RADEON_HPD_NONE), there is no need for the driver to poll every 10 seconds because the hw will generate an interrupt on a plug or unplug event.  When an interrupt arrives, the the poll handler runs to see if the monitor is still there and to send an event to userspace (same poll handler that is run by the driver for non-HPD capable connectors).

> 2. That discard and full fetch of EDID is done when is not needed?
> 
> Or both? Or something else? Hard to say for me. HPD sounds like Hotplug detect?

Yes, HPD stands for hotplug detect.

Does your monitor have multiple inputs?  Some monitors have an option to poll their inputs at regular intervals.  When they do that, it often causes a disconnect/connect signal on the HPD pin which generates an interrupt and the driver's detect logic runs.  Try and disable the monitor's polling logic if you can and see if that helps.
Comment 9 Tvrtko Ursulin 2012-03-08 01:56:12 UTC
Created attachment 58160 [details] [review]
Patch which makes the stalls go away

Here is the patch which makes the stalls go away. However, I am not that confident we got to the bottom of this.

I was looking at how is detection supposed to work on HPD interrupt. What happens is that poll helper gets scheduled, but, since the connector is now not in polled mode, that won't do anything to update it's status.

In our use case things work because when no connectors are in connected status we keep probing them manually. I _suspect_ this translates into drm_helper_probe_single_connector_modes call, right?

But without this userspace behaviour, and with this patch, connector status will not get updated on it's own. At least this is my understanding and why I said that I don't think we got to the bottom of it.

Side question - are connector detect methods supposed to be re-entrant? I am not sure they are looking at radeon_dvi_detect. And as far as I can see they can be running simultaneously from drm_helper_probe_single_connector_modes and output_poll_execute, no?
Comment 10 Tvrtko Ursulin 2012-03-12 06:44:33 UTC
Created attachment 58318 [details] [review]
Different approach on fixing the stalls

After talking with a colleague who is more at home in this code we think the previous patch is wrong.

This new patch uses a different approach which also works for us. In short, it doesn't do the full EDID re-fetch if HPD sense says we are still connected. Plus some other conditions only of which the shared_ddc one I am not completely sure. I've put it in to minimise the change in behaviour.

Comments?
Comment 11 Alex Deucher 2012-03-12 07:56:06 UTC
The code is correct as is.  I think what's happening is that your monitor is polling and causing hotplug unplug/plug events what cause the detect code to run.  Can you try disabling the input polling on your monitor?
Comment 12 Simon Farnsworth 2012-03-12 08:13:05 UTC
Alex,

The monitor is not polling - the hotplug detect code is being called because the VGA is polled as DRM_CONNECTOR_POLL_CONNECT, and the output_poll_execute function in drm_crtc_helper.c checks every output when a poll happens.

Because the VGA needs polling once every ten seconds, output_poll_execute is called on each HPD interrupt, and once every ten seconds beyond that. radeon_dvi_detect is heavy-duty, and does a full EDID fetch every time it's called, stalling us for 100ms once every ten seconds for as long as the VGA is disconnected.

Tvrtko's fix changes radeon_dvi_detect to be lightweight in the case that affects us, where a HPD pin is wired up correctly.

The alternative is to do major surgery on the core DRM, and teach output_poll_execute and drm_helper_hpd_irq_event, to only check outputs that have either had a HPD interrupt since the last time they were checked, or that are marked for polling.
Comment 13 Alex Deucher 2012-03-12 11:34:39 UTC
Sorry for the confusion, I mixed up the patches.  I was referring to the previous patch (attachment 58160 [details] [review]).  The patch in attachment 58318 [details] [review] looks good.  The only thing I would add is a check to make sure rdev->family >= CHIP_R600 since the HPD mapping was not always set up reliably by OEMs on prior asics.  With that, you can add:

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Comment 14 Tvrtko Ursulin 2012-03-13 01:46:42 UTC
Created attachment 58360 [details] [review]
Do not re-fetch full EDID while a HPD capable output remains connected

So something like this:
---
On multi output boards where one output is not connected it can cause KMS poll helper to run periodically. This makes the connected DVI/HDMI output re-fetch full EDID on every poll causing 100ms rendering stalls.

Fix is to skip re-fetching full EDID while a HPD capable output remains connected on R600 and newer.

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Comment 15 Tvrtko Ursulin 2012-03-13 04:44:04 UTC
Bad news from a different motherboard. This one has DVI-D and DP connectors. With a DVI monitor connected stalls are still there due to the shared_ddc being true (DDC shared between DVI and VGA). Connectors look like this here:

[drm] Radeon Display Connectors
[drm] Connector 0:
[drm]   DisplayPort
[drm]   HPD1
[drm]   DDC: 0x6430 0x6430 0x6434 0x6434 0x6438 0x6438 0x643c 0x643c
[drm]   Encoders:
[drm]     DFP1: INTERNAL_UNIPHY
[drm] Connector 1:
[drm]   DVI-D
[drm]   HPD2
[drm]   DDC: 0x6440 0x6440 0x6444 0x6444 0x6448 0x6448 0x644c 0x644c
[drm]   Encoders:
[drm]     DFP2: INTERNAL_UNIPHY
[drm] Connector 2:
[drm]   VGA
[drm]   DDC: 0x6440 0x6440 0x6444 0x6444 0x6448 0x6448 0x644c 0x644c
[drm]   Encoders:
[drm]     CRT1: INTERNAL_KLDSCP_DAC1

If I remove the shared_ddc check from the patch that removes the 100ms stall in radeon_dvi_detect. However in this configuration there is another 60ms stall from drm_get_edid in radeon_vga_detect now.

Also, radeon_dp_getsinktype from radeon_dp_detect adds another 3ms for the disconnected DP port.

Recap of latency contributors on this hardware:

1. radeon_dvi_detect

 * 100ms when connected from drm_get_edid

Can be fixed with the patch like the above, but with shared_ddc check criteria removed if that is OK? That would then work for both motherboards that I tested on. HDMI on the first one, and DVI on this one.

2. radeon_vga_detect

 * not connected
    * 5ms from radeon_ddc_probe
    * 60ms from drm_get_edid

Not sure how to improve this without making the code a lot smarter. It would have to consider who is sharing DDC with who, and if another connector is connected it would imply this one can not be and then skip the EDID fetch?

Also perhaps 5ms from radeon_ddc_probe could be saved by somehow passing the header to drm_get_edid, if I gather correctly that the latter re-fetches the same header radeon_ddc_probe retrieved.

3. radeon_dp_detect

 * 8ms when not connected from radeon_dp_getsinktype
 * haven't tested the connected case - that is unreliable anyway, bug 46711

Not sure if this would be passable if it remained the only latency source. It's not ideal that's for sure. Does it need to do the radeon_dp_getsinktype call in the absence of a HPD interrupt though? If not could we short-circuit it in some way similar to radeon_dvi_detect? Call it after radeon_hpd_sense only if something is connected? However looking at the code it doesn't suggest all latency is avoidable since it doesn't seem to trust HPD sense. Same story with OEM wiring reliability?
Comment 16 Tvrtko Ursulin 2012-03-15 02:50:51 UTC
Created attachment 58481 [details] [review]
Use shared HPD for VGA on R600+

I've come up with a new approach which seems to work well on hardware I have here. However it makes one assumption which I cannot be certain is true and therefore needs AMD opinion and review. Assumption is that if DDC lines are shared between the VGA and DVI connectors, that also means HPD line is shared (it is on our hardware).

Under that assumption I have reworked connector adding a bit so shared DDC is not a boolean any more but a pointer to the shared connector instance. That allows me to enable HPD for the VGA connector which makes the poll worker not run every ten seconds (fixing a major source of pain for us). 

It also enables me to avoid fetching the VGA EDID if the shared connector is connected - rationale being that only one of the pair can be connected at a time. This logic is dependant on R600+ ASICs where we can rely on HPD lines being present.

Furthermore, I've left in the bit which trusts HPD sense on R600+ ASICs, meaning as long as HPD sense remained unchanged, connector detect functions will bail out early.

Last two are only considered if detect is not triggered from userspace. Otherwise full probe will be done as before.
Comment 17 Tvrtko Ursulin 2012-03-15 03:52:33 UTC
(In reply to comment #16)
> Furthermore, I've left in the bit which trusts HPD sense on R600+ ASICs,
> meaning as long as HPD sense remained unchanged, connector detect functions
> will bail out early.

Wrong wording here, I did not leave it in, but it is also required to avoid VGA connector triggering polls on for example HDMI, where HPD is not shared.
Comment 18 Simon Farnsworth 2012-03-15 04:20:56 UTC
I've spoken with Tvrtko - we think the patch is not quite right.

Specifically, it assumes that we will get appropriate HPD sense when a VGA monitor is connected, but we've come up with two cases where that's not true:

1) DVI-A to VGA adapter plugged in first, monitor plugged in a few minute later.
2) Cheap DVI-A to VGA adapter that doesn't wire up the HPD pin.

Tvrtko's planning to look into this in a bit more depth.
Comment 19 Alex Deucher 2012-03-15 06:10:34 UTC
Only digital connectors (HDMI, DVI (digital portion only), DP) have HDP pins.  There is no hotplug spec for analog connectors.  They almost always have to be polled if you want connect/disconnect.  DVI-I connectors have an HPD pin, but it's only applicable to the digital portion.  It may or may not work for the analog part.  Also, different connectors with the same ddc line may not have same HPD pin (analog connectors won't have and HPD pin at all).
Comment 20 Tvrtko Ursulin 2012-03-15 06:29:20 UTC
Right, thanks for your comments. In this case how about this approach:

1. Where there is shared DDC, one end being connected will imply the other is disconnected.

This will ensure DVI or VGA DDC is not needlessly probed (EDID fetched) for the unconnected end of the pair.

2. On HPD irq force detect will run on shared DDC connector pairs.

This will ensure correct state after VGA is disconnected and DVI connected. Otherwise VGA remains in its "sticky connect" (can't poll for disconnected).

I can add something to pass a HPD irq flag in radeon_connector which detect functions would consume.

3. I keep the code which skips expensive probing unless HPD sense has changed. On R600+ and only for unshared connectors.

How does this sound?
Comment 21 Alex Deucher 2012-03-15 06:59:49 UTC
(In reply to comment #15)
> Bad news from a different motherboard. This one has DVI-D and DP connectors.
> With a DVI monitor connected stalls are still there due to the shared_ddc being
> true (DDC shared between DVI and VGA). Connectors look like this here:
> 
> [drm] Radeon Display Connectors
> [drm] Connector 0:
> [drm]   DisplayPort
> [drm]   HPD1
> [drm]   DDC: 0x6430 0x6430 0x6434 0x6434 0x6438 0x6438 0x643c 0x643c
> [drm]   Encoders:
> [drm]     DFP1: INTERNAL_UNIPHY
> [drm] Connector 1:
> [drm]   DVI-D
> [drm]   HPD2
> [drm]   DDC: 0x6440 0x6440 0x6444 0x6444 0x6448 0x6448 0x644c 0x644c
> [drm]   Encoders:
> [drm]     DFP2: INTERNAL_UNIPHY
> [drm] Connector 2:
> [drm]   VGA
> [drm]   DDC: 0x6440 0x6440 0x6444 0x6444 0x6448 0x6448 0x644c 0x644c
> [drm]   Encoders:
> [drm]     CRT1: INTERNAL_KLDSCP_DAC1
> 

What system is this?  Most new boards don't have shared ddc setups.  Does DDC work on both the VGA and DVI ports?  Can you send me a copy of the vbios?

(as root)
(use lspci to get the bus id)
cd /sys/bus/pci/devices/<pci bus id>
echo 1 > rom
cat rom > /tmp/vbios.rom
echo 0 > rom
Comment 22 Alex Deucher 2012-03-15 07:08:45 UTC
(In reply to comment #20)
> Right, thanks for your comments. In this case how about this approach:
> 
> 1. Where there is shared DDC, one end being connected will imply the other is
> disconnected.
> 

I'm not sure we necessarily want to do that.  Even though they may have a shared ddc line, it would be nice to report the proper connected status.

> This will ensure DVI or VGA DDC is not needlessly probed (EDID fetched) for the
> unconnected end of the pair.
> 
> 2. On HPD irq force detect will run on shared DDC connector pairs.
> 
> This will ensure correct state after VGA is disconnected and DVI connected.
> Otherwise VGA remains in its "sticky connect" (can't poll for disconnected).
> 
> I can add something to pass a HPD irq flag in radeon_connector which detect
> functions would consume.
> 
> 3. I keep the code which skips expensive probing unless HPD sense has changed.
> On R600+ and only for unshared connectors.
> 
> How does this sound?

What about the following:

DVI + VGA with shared ddc line.
VGA connected and in use.  User connects DVI port, gets hpd irq, detect called.  hpd sense returns true, DVI is updated as connected, VGA is marked as disconnected.  User then potentially loses the monitor they are currently using.
Comment 23 Tvrtko Ursulin 2012-03-15 07:56:20 UTC
(In reply to comment #21)
> (In reply to comment #15)
> > Bad news from a different motherboard. This one has DVI-D and DP connectors.
> > With a DVI monitor connected stalls are still there due to the shared_ddc being
> > true (DDC shared between DVI and VGA). Connectors look like this here:
> > 
> > [drm] Radeon Display Connectors
> > [drm] Connector 0:
> > [drm]   DisplayPort
> > [drm]   HPD1
> > [drm]   DDC: 0x6430 0x6430 0x6434 0x6434 0x6438 0x6438 0x643c 0x643c
> > [drm]   Encoders:
> > [drm]     DFP1: INTERNAL_UNIPHY
> > [drm] Connector 1:
> > [drm]   DVI-D
> > [drm]   HPD2
> > [drm]   DDC: 0x6440 0x6440 0x6444 0x6444 0x6448 0x6448 0x644c 0x644c
> > [drm]   Encoders:
> > [drm]     DFP2: INTERNAL_UNIPHY
> > [drm] Connector 2:
> > [drm]   VGA
> > [drm]   DDC: 0x6440 0x6440 0x6444 0x6444 0x6448 0x6448 0x644c 0x644c
> > [drm]   Encoders:
> > [drm]     CRT1: INTERNAL_KLDSCP_DAC1
> > 
> 
> What system is this?  Most new boards don't have shared ddc setups.

FUJITSU D3003-S2

> Does DDC
> work on both the VGA and DVI ports?

Yes as far as I can tell, both connectors successfully fetch EDID.

>  Can you send me a copy of the vbios?
> 
> (as root)
> (use lspci to get the bus id)
> cd /sys/bus/pci/devices/<pci bus id>
> echo 1 > rom
> cat rom > /tmp/vbios.rom
> echo 0 > rom

Will attach shortly.
Comment 24 Tvrtko Ursulin 2012-03-15 07:57:11 UTC
Created attachment 58515 [details]
Video BIOS from the board with DP + DVI-I (shared DDC setup)
Comment 25 Alex Deucher 2012-03-15 08:02:57 UTC
(In reply to comment #23)
> > What system is this?  Most new boards don't have shared ddc setups.
> 
> FUJITSU D3003-S2
> 
> > Does DDC
> > work on both the VGA and DVI ports?
> 
> Yes as far as I can tell, both connectors successfully fetch EDID.

Ok, then it probably is a shared ddc system and not just a bug in the connector tables.
Comment 26 Tvrtko Ursulin 2012-03-15 08:03:32 UTC
(In reply to comment #22)
> (In reply to comment #20)
> > Right, thanks for your comments. In this case how about this approach:
> > 
> > 1. Where there is shared DDC, one end being connected will imply the other is
> > disconnected.
> > 
> 
> I'm not sure we necessarily want to do that.  Even though they may have a
> shared ddc line, it would be nice to report the proper connected status.

What do you consider the proper connected status? With DVI-I made out of VGA-1 and DVI-1 in software, only one of them should be connected at a time, no?

> > This will ensure DVI or VGA DDC is not needlessly probed (EDID fetched) for the
> > unconnected end of the pair.
> > 
> > 2. On HPD irq force detect will run on shared DDC connector pairs.
> > 
> > This will ensure correct state after VGA is disconnected and DVI connected.
> > Otherwise VGA remains in its "sticky connect" (can't poll for disconnected).
> > 
> > I can add something to pass a HPD irq flag in radeon_connector which detect
> > functions would consume.
> > 
> > 3. I keep the code which skips expensive probing unless HPD sense has changed.
> > On R600+ and only for unshared connectors.
> > 
> > How does this sound?
> 
> What about the following:
> 
> DVI + VGA with shared ddc line.
> VGA connected and in use.  User connects DVI port, gets hpd irq, detect called.
>  hpd sense returns true, DVI is updated as connected, VGA is marked as
> disconnected.  User then potentially loses the monitor they are currently
> using.

Are you implying DVI + VGA with shared DDC lines come as either one shared (DVI-I) connector, or two physical ones (DVI-? + VGA)? This is a very deep hole indeed...
Comment 27 Tvrtko Ursulin 2012-03-15 08:14:08 UTC
(In reply to comment #26)
> > What about the following:
> > 
> > DVI + VGA with shared ddc line.
> > VGA connected and in use.  User connects DVI port, gets hpd irq, detect called.
> >  hpd sense returns true, DVI is updated as connected, VGA is marked as
> > disconnected.  User then potentially loses the monitor they are currently
> > using.
> 
> Are you implying DVI + VGA with shared DDC lines come as either one shared
> (DVI-I) connector, or two physical ones (DVI-? + VGA)? This is a very deep hole
> indeed...

And in your scenario, do you get EDID from the DVI monitor or the VGA one, when both are connected, and detect method fetch it from the shared I2C bus? I don't get it...
Comment 28 Alex Deucher 2012-03-15 08:16:33 UTC
(In reply to comment #26)
> > 
> > I'm not sure we necessarily want to do that.  Even though they may have a
> > shared ddc line, it would be nice to report the proper connected status.
> 
> What do you consider the proper connected status? With DVI-I made out of VGA-1
> and DVI-1 in software, only one of them should be connected at a time, no?
> 

I'm not sure what the right answer is.

> > What about the following:
> > 
> > DVI + VGA with shared ddc line.
> > VGA connected and in use.  User connects DVI port, gets hpd irq, detect called.
> >  hpd sense returns true, DVI is updated as connected, VGA is marked as
> > disconnected.  User then potentially loses the monitor they are currently
> > using.
> 
> Are you implying DVI + VGA with shared DDC lines come as either one shared
> (DVI-I) connector, or two physical ones (DVI-? + VGA)? This is a very deep hole
> indeed...

Yes, the shared_ddc flag is only set of there are two different physical connectors that are both wired to the same ddc line (e.g., a separate VGA plug and HDMI plug).  With DVI-I it's a single physical connector.  DVI-I ports always have a single ddc line.
Comment 29 Tvrtko Ursulin 2012-03-15 08:34:48 UTC
(In reply to comment #28)
> (In reply to comment #26)
> > > 
> > > I'm not sure we necessarily want to do that.  Even though they may have a
> > > shared ddc line, it would be nice to report the proper connected status.
> > 
> > What do you consider the proper connected status? With DVI-I made out of VGA-1
> > and DVI-1 in software, only one of them should be connected at a time, no?
> > 
> 
> I'm not sure what the right answer is.

Is it possible to have DVI-D + VGA breakout cable from a single DVI-I which would give two independent simultaneous outputs? If not then it looks to me only one KMS connector can be connected at a time.

> > > What about the following:
> > > 
> > > DVI + VGA with shared ddc line.
> > > VGA connected and in use.  User connects DVI port, gets hpd irq, detect called.
> > >  hpd sense returns true, DVI is updated as connected, VGA is marked as
> > > disconnected.  User then potentially loses the monitor they are currently
> > > using.
> > 
> > Are you implying DVI + VGA with shared DDC lines come as either one shared
> > (DVI-I) connector, or two physical ones (DVI-? + VGA)? This is a very deep hole
> > indeed...
> 
> Yes, the shared_ddc flag is only set of there are two different physical
> connectors that are both wired to the same ddc line (e.g., a separate VGA plug
> and HDMI plug).  With DVI-I it's a single physical connector.  DVI-I ports
> always have a single ddc line.

Hm, shared_ddc is also set with single physical DVI-I connector, at least on this motherboard. 

Could you clarify what EDID do you get with two physical connectors which share DDC?
Comment 30 Alex Deucher 2012-03-15 08:45:10 UTC
(In reply to comment #29)
t sure what the right answer is.
> 
> Is it possible to have DVI-D + VGA breakout cable from a single DVI-I which
> would give two independent simultaneous outputs? If not then it looks to me
> only one KMS connector can be connected at a time.
> 

It's possible.  there are two separate encoders (dac and tmds) that can be driven independently connected to the single port. However, it's not something we support since you wouldn't be able to fetch the EDIDs from each monitor directly and it's confusing to users since there is only one physical port on the board.

> 
> Hm, shared_ddc is also set with single physical DVI-I connector, at least on
> this motherboard. 

According to your vbios tables there are actually 3 physical connectors on this board, VGA, DVI-D, and DP.  If there are only two physical connectors DP and DVI-I port, then the oem set up the connector tables wrong and we need a quirk to properly expose it as single DVI-I connector.

> 
> Could you clarify what EDID do you get with two physical connectors which share
> DDC?

Garbage? whichever one happens to go first?  It's not really an ideal scenario, but sometimes oems do broken things.
Comment 31 Tvrtko Ursulin 2012-03-15 09:13:58 UTC
(In reply to comment #30)
> > Hm, shared_ddc is also set with single physical DVI-I connector, at least on
> > this motherboard. 
> 
> According to your vbios tables there are actually 3 physical connectors on this
> board, VGA, DVI-D, and DP.  If there are only two physical connectors DP and
> DVI-I port, then the oem set up the connector tables wrong and we need a quirk
> to properly expose it as single DVI-I connector.

It is a single DVI-I physically.

But even identifying it would not change anything with regards to polling induced rendering stalls, right? 

Thinking again about my proposed solution from #20, then your failure case from #22. I can see it is a real concern and presumably this setup is not that uncommon since you worry about it? 

I just don't at the moment see how to fit all these requirements into a generic solution. :(
Comment 32 Alex Deucher 2012-03-15 10:41:15 UTC
(In reply to comment #31)
> It is a single DVI-I physically.
> 
> But even identifying it would not change anything with regards to polling
> induced rendering stalls, right? 
> 

Well, shared_ddc wouldn't apply in that case since there is only one connector, and for better or worse we wouldn't be polling since the connector does have an HPD pin (even if it doesn't necessarily work reliably with the analog part of a DVI-I connector).  So fixing that would avoid the polling stalls on your particular board.
Comment 33 Tvrtko Ursulin 2012-03-16 01:47:31 UTC
(In reply to comment #32)
> (In reply to comment #31)
> > It is a single DVI-I physically.
> > 
> > But even identifying it would not change anything with regards to polling
> > induced rendering stalls, right? 
> > 
> 
> Well, shared_ddc wouldn't apply in that case since there is only one connector,
> and for better or worse we wouldn't be polling since the connector does have an
> HPD pin (even if it doesn't necessarily work reliably with the analog part of a
> DVI-I connector).  So fixing that would avoid the polling stalls on your
> particular board.

Ok, I misunderstood would would happen after applying this quirk. I though we would still have three connectors, just that DVI-D would become DVI-I. In fact what should happen is that DVI-D becomes DVI-I and VGA vbios entry is ignored for this board?

Is this something which can be done in radeon_atom_apply_quirks? I'll give it a go.
Comment 34 Tvrtko Ursulin 2012-03-16 03:54:47 UTC
(In reply to comment #32)
> (In reply to comment #31)
> > It is a single DVI-I physically.
> > 
> > But even identifying it would not change anything with regards to polling
> > induced rendering stalls, right? 
> > 
> 
> Well, shared_ddc wouldn't apply in that case since there is only one connector,
> and for better or worse we wouldn't be polling since the connector does have an
> HPD pin (even if it doesn't necessarily work reliably with the analog part of a
> DVI-I connector).  So fixing that would avoid the polling stalls on your
> particular board.

Now I know how to do the quirk to make only DVI-I appear instead of DVI-D and VGA, however obvious limitation here is that:
 a) use cases from #18 are broken
 b) code seems to be buggy in bringing up even HPD enabled connectors anyway

Consider this for the latter:

* DVI monitor connected to DVI-I
* Unplug the cable
* HPD interrupt happens
* radeon_dvi_detect called
* DDC probe fails
* connector marked as disconnected
* output_poll_changed called,
* drm_helper_probe_single_connector_modes
* radeon_dvi_detect called again with force=true
* DAC load detect puts the connector in unknown status
* poll scheduled for ten seconds later
* ten seconds later radeon_dvi_detect called
* ddc probe fails
* connector still in unknown state
* Now plug the cable back in
* HPD irq fires
* radeon_dvi_detect called
* DDC is not ready yet for some reason and probe fails (I get this a lot)
* monitor still in unknown status
* end of story, no display
Comment 35 Tvrtko Ursulin 2012-03-16 06:18:42 UTC
Created attachment 58554 [details] [review]
Correctly identify connector on Fujitsu D3003-S2 board
Comment 36 Tvrtko Ursulin 2012-03-16 06:57:03 UTC
Created attachment 58555 [details] [review]
Donot probe extensively if HPD hasn't changed on R600+ ASICs

Now, with these last two patches things should look better for us you think? 

Problem I have now is that with VGA connected to this DVI-I as soon as KMS kicks in (radeon.ko loads) monitor goes to power save. Even after X is running and xrandr shows that a mode is active, monitor is still off. This is caused purely by the Fujitsu quirk patch.

Same symptoms if I boot on DVI and then plug VGA later. Although in this case it may be a different cause. When I unplug the DVI connector goes into the "unknown" state. This is because ext_encoder test radeon_atom_dig_detect fails. And Xorg sees that as "unknown" status but show an active mode while monitor is firmly in power save.
Comment 37 Alex Deucher 2012-03-16 07:04:36 UTC
Created attachment 58556 [details] [review]
add quirk for fujitsu board

(In reply to comment #35)
> Created attachment 58554 [details] [review] [review]
> Correctly identify connector on Fujitsu D3003-S2 board

The patch is not quite correct.  You dropped the VGA connect, but did not associate the dac with the new DVI-I connector.  I think this patch should fix that.  This may be the cause of the problems you were seeing.
Comment 38 Tvrtko Ursulin 2012-03-16 07:16:44 UTC
(In reply to comment #37)
> Created attachment 58556 [details] [review] [review]
> add quirk for fujitsu board
> 
> (In reply to comment #35)
> > Created attachment 58554 [details] [review] [review] [review]
> > Correctly identify connector on Fujitsu D3003-S2 board
> 
> The patch is not quite correct.  You dropped the VGA connect, but did not
> associate the dac with the new DVI-I connector.  I think this patch should fix
> that.  This may be the cause of the problems you were seeing.

Magic. :) Thanks, seems to work now.

What are the chances of upstreaming the two?
Comment 39 Alex Deucher 2012-03-16 07:46:10 UTC
(In reply to comment #38)
> 
> Magic. :) Thanks, seems to work now.
> 
> What are the chances of upstreaming the two?

Please provide a git patch for your hpd unchanged patch with your signed-off-by, and I'll send that and the quirk patch upstream.
Comment 40 Tvrtko Ursulin 2012-03-16 08:09:09 UTC
Created attachment 58566 [details] [review]
Do not probe extensively if HPD hasn't changed on R600+ ASICs

Made by git format-patch and signed off.
Comment 41 Tvrtko Ursulin 2012-03-16 08:16:13 UTC
Btw is it correct that it is not possible to load detect with this hardware on DVI-I when nothing is connected? (Comes up as connector status unknown).
Comment 42 Alex Deucher 2012-03-16 08:30:04 UTC
(In reply to comment #41)
> Btw is it correct that it is not possible to load detect with this hardware on
> DVI-I when nothing is connected? (Comes up as connector status unknown).

It should work.  Sounds like you were testing without the proper quirk patch in place or it was being called on the wrong encoder.  See atombios_dac_load_detect() and radeon_atom_dac_detect() in atombios_encoders.c.
Comment 43 Tvrtko Ursulin 2012-03-16 08:35:35 UTC
(In reply to comment #42)
> (In reply to comment #41)
> > Btw is it correct that it is not possible to load detect with this hardware on
> > DVI-I when nothing is connected? (Comes up as connector status unknown).
> 
> It should work.  Sounds like you were testing without the proper quirk patch in
> place or it was being called on the wrong encoder.  See
> atombios_dac_load_detect() and radeon_atom_dac_detect() in atombios_encoders.c.

Hmmm.. radeon_atom_dig_detect is actually called (hpd unplug, ddc probe fails) where the ext_encoder test fails setting it into unknown state. I definitely have your version of the quirk.
Comment 44 Alex Deucher 2012-03-16 08:59:36 UTC
Created attachment 58568 [details] [review]
fix dvi-i load detection

It's being called on the wrong encoder.  This should fix the load detection issue.
Comment 45 Tvrtko Ursulin 2012-03-16 09:09:56 UTC
(In reply to comment #44)
> Created attachment 58568 [details] [review] [review]
> fix dvi-i load detection
> 
> It's being called on the wrong encoder.  This should fix the load detection
> issue.

Works perfectly now, thank you!
Comment 47 Alex Deucher 2012-04-01 05:57:46 UTC
*** Bug 48137 has been marked as a duplicate of this bug. ***

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.