drm/vc4: hdmi: Remove the DDC probing for status detection

[mripard]

Commit 9d44abb ("drm/vc4: Fall back to using an EDID probe in the
absence of a GPIO.") added some code to read the EDID through DDC in the
HDMI driver detect hook since the Pi3 had no HPD GPIO back then.
However, commit b1b8f45 ("ARM: dts: bcm2837: Add missing GPIOs of
Expander") changed that a couple of years later.

This causes an issue though since some TV (like the LG 55C8) when it
comes out of standy will deassert the HPD line, but the EDID will
remain readable.

It causes an issues nn platforms without an HPD GPIO, like the Pi4,
where the DDC probing will be our primary mean to detect a display, and
thus we will never detect the HPD pulse. This was fine before since the
pulse was small enough that we would never detect it, and we also didn't
have anything (like the scrambler) that needed to be set up in the
display.

However, now that we have both, the display during the HPD pulse will
clear its scrambler status, and since we won't detect the
disconnect/reconnect cycle we will never enable the scrambler back.

As our main reason for that DDC probing is gone, let's just remove it.

Signed-off-by: Maxime Ripard [email protected]

[popcornmix]

There is a slight issue that a tv (or cable) with a broken hotplug signal will no longer work after this change.
It's hard to say how common this is (we so the same thing of treating an edid response as a hotplug alternative on firmware side).

But I'm okay merging this, as it fixes an issue with 4kp60 on a display that isn't broken.
It can be revisited if we get multiple reports of regression.

For info the firmware does something slightly cleverer:

On initial boot if there is no hotplug asserted it tries reading edid. If this fails we assume hotplug is broken and never use it again, just assume it is permanently asserted. We obviously won't handle real hotplug events in this state (which may cause an issue with 4kp60).

In all other cases, we trust the hotplug signal (and so will never read the edid when it is not asserted.

[HiassofT]

I tested this PR with my LG 55C8 but the issue is not fully resolved.

When using just the linux framebuffer console (running at 3840x2160-60) putting the TV into standby and back on works fine.

But if I run kodi or even just modetest I get "no signal" on my TV.

eg:

• run modetest -M vc4 -s 32:3840x2160-50
• switch TV into standby
• switch TV back on -> no signal
• press enter to exit modetest (and have it switch back to 3840x2160-60) -> signal is back

Here's dmesg with drm.debug=0x04, I first switched TV to standby and back on with the framebuffer console, then did the modetest check as outlined above.
http://ix.io/3oXU

[mripard]

I'm not really sure it's supposed to work in the first place.

I guess it can be perceived as a regression, since before the interrupt-based hotplug handling, the hotplug pulse from the TV wouldn't have been detected and thus the display mode would have been kept as is.

However, from a kernel stand-point, it could have been a different display entirely being connected now so the current mode should be invalid, and the application should just read the EDID again, and pick the new mode it wants.

Now, modetest doesn't handle hotplug at all, so it won't do that. I'm not entirely sure about Kodi though.

I'll ask around to see how we're supposed to handle this.

[mripard]

I just pushed the PR #4412 that should address what you were seeing

[mripard]

This just fixed another issue reported by @timg236 where a device that would report null EDID (so valid transfer but invalid EDID) would be reported as connected even though the HPD line is low. Can we merge this, or did you consider the detection at boot of whether hotplug actually works to be a blocker?

[popcornmix]

I'm okay with merging. @pelwell ?

[popcornmix]

This has caused an annoying issue where TV/AVR turns back on right after turning them off:
https://forum.libreelec.tv/thread/24783-tv-avr-turns-back-on-right-after-turning-them-off/

I've bisected and it starts with this commit. Not completely obvious why this causes the issue.

[6by9]

This has caused an annoying issue where TV/AVR turns back on right after turning them off:
https://forum.libreelec.tv/thread/24783-tv-avr-turns-back-on-right-after-turning-them-off/

You're partly copying the "functionality" of an Amazon 4k Fire Stick then. Hit the power button on that, both TV and AVR go off, and then the AVR gets powered back on about 2 seconds later. CEC just seems to be flakey that way.

[popcornmix]

But if I revert this commit the behaviour is consistently better (for numerous users).

[pelwell]

@6by9 @mripard Do we understand the CEC regression that @popcornmix is reporting? And can anything be done about it?

[6by9]

At a guess the device was responding to the EDID probe and therefore still reporting connected when the HPD line says it isn't. That triggers a CEC invalidate.

The original logic is incorrect though. The order of checks should be

int 2711_hpd_detect()
{
   return HDMI_READ(HDMI_HOTPLUG) & VC4_HDMI_HOTPLUG_CONNECTED);
}
...
	if (vc4_hdmi->hpd_gpio) {
		if (gpio_get_value_cansleep(vc4_hdmi->hpd_gpio) ^
		    vc4_hdmi->hpd_active_low)
			connected = true;
	} else {
		if (variant->hpd_detect && variant->hpd_detect(vc4_hdmi)))
			connected = true;
	}

	if (!connected && drm_probe_ddc(vc4_hdmi->ddc))
		connected = true;
...
bcm2711_hdmi0_variant {
	.hpd_detect = 2711_hpd_detect,
...
bcm2711_hdmi1_variant {
	.hpd_detect = 2711_hpd_detect,

ie check the defined HPD signal first. If not connected, then try a drm_probe_ddc. It was backwards for Pi4 in the previous version, and it would also be possible to define a CM1/3 DT with no HPD GPIO that would then drop into the Pi4 register read.

I'll convert that into a PR for @popcornmix to test.

[mripard]

I'm not sure it's the right thing to do. That patch was done in the first place because some displays have been found to have the EDID accessible while the HPD signal is deasserted.

With your suggestion, it would still be reported as connected (variant->hpd_detect would be true, hpd_detect false, thus connected is still false, and we go through drm_probe_ddc)

This is especially troublesome when scrambling is enabled, since we wouldn't be able to detect that the scrambling status has been reset.

[6by9]

I guess what we really need is logs for what happens on these displays/AVRs when powered off. drm.debug=0x14 is probably sufficient, although knowing what the CEC side gets up to would be nice too.

If they change the HPD line in weird ways, then that is probably the way it is.
The disconnect will trigger a cec_phys_addr_invalidate, so I assume CEC renegotiates at some point. Is that controlled by something in userspace, or just the kernel? I'm suspecting the former, so are we sure that isn't sending a power on?
The DDC probe was probably masking any HPD pulse, and therefore it didn't trigger anything.

@mripard you are right that if scrambling is enabled, then deasserting HPD would expect a renegotiate of scrambling, regardless of the DDC probe state.

[6by9]

#4371 for the PR.