Interrupt-based HDMI Hotplug Detection #4313
Conversation
|
I'll test in a moment, but looks good based on the patches. |
|
checkpatch complains on the last patch |
|
Certainly works nicely for 2711. |
|
Are we convinced that the non-2711 version doesn't cause both the VPU and the ARM to use GPIO interrupts for the same bank? |
Hmm, slight can of worms. Most cases are GPIO46. 3B+ is GPIO28. Pi1 has SD_CARD_DETECT on GPIO47, but I don't immediately see any firmware code looking at the pin_define for that. Power low is a BANK1 input on some boards. I thought it was polled and it is, but it is using the event status flags from the GPIO block to capture brief pulses (not using the interrupt itself though). I'm just noting that CM3 and Pi3B (not +) have HPD on the GPIO expander, so no interrupt support there. (Edited to correct pulse time. 240ms is APX803S. APX803 is now not recommended for new designs) |
I think it's more that the ARM would see unexpected interrupts, but it would then acknowledge them - the driver doesn't keep a record of those which are expected and ignore them otherwise. |
I think it's going to clear those flags from the GPEDS register out from under the firmware (via bcm2835_gpio_irq_ack), so the firmware wouldn't see any low voltage events. |
|
Something not quite so happy on Pi3B+. Booted OK, disconnected HDMI and splat: It seems repeatable too. I do need to find a new Pi3 to test with generally as this one is INCREDIBLY sensitive to power supply glitches/earthing changes. Boot without HDMI connected, and touching the HDMI cable from the monitor to the HDMI socket of the Pi is enough to reboot it :-( |
|
Sorry, it looks like something went wrong during my rebase. I'll have a look tomorrow at that crash. I have the same issue with my Pi3, I've been using my tc358743 to trigger HPD pulses without crashing it |
|
I looked into it and for some reason the 5.10 kernel differs a bit from mainline there. I'm not sure why exactly yet, but it seems that during the atomic commit that enables back the CRTC when the display is found to be connected again the pointers we usually follow aren't set anymore, and thus we end up dereferencing null pointers. I just pushed some quick fixes that should allow you to test this. |
|
Thanks, I'll have a look in the morning. |
|
OK, so Pi3B is an issue (so CM3 will be as well) due to HPD being on the GPIO expander. so that we drop back to polled if we can't acquire the IRQ. Some of my woes are probably self inflicted as I'm testing on top of the YCbCr output patches, and with 2711 gamma support thrown in. It's still blowing up for me on Pi3 with the above diff: I'll clean up my tree and retest with just these patches. |
|
The fallback on the polling looks great. If it ever causes an issue on those boards, I guess we can always try to figure out if the internal hotplug logic actually works. The crash you have should be fixed by using the commits I pushed yesterday night |
|
The hotplug logic has worked so far, it's just a little slow to notice changes, and will certainly miss your 200ms pulse on changing the EDID of a TC358743. I have your patches from last night, but still get encoder->crtc = NULL in vc4_hdmi_encoder_pre_crtc_enable when trying to get to the mode. This is actually DISABLING HDMI, and ENABLING composite - they share a PV on Pi0-3. Getting the call into vc4_hdmi_encoder_pre_crtc_enable therefore seems a little odd as we are not enabling the HDMI encoder, and the HDMI encoder not being linked to a crtc would be correct (the VEC encoder will be linked to it). I suspect that this path has been wrong for a while, but we're only just noticing it. |
You mean with boards that would rely on polling, or on the 3B+ or 4?
That's a good lead, I'll look into it, thanks! |
|
The issue is with 3cf3d39 that has been merged in the 5.10 branch but not in mainline. The issue it was trying to work around is that at boot, the entities don't have a state yet so one cannot rely on the connector->state to get back the encoder, and then disable it to prevent the stuck pixel. However the code relies on the 1:1 mapping of the pixelvalve and encoders that we have on the RPi4, and here that assumption is broken. I'll try to see if testing for connector->state and then using the old logic if it's !NULL would work to fix this |
Having to poll on 3B and CM3 will mean we miss short pulses. All other platforms can be interrupt driven, so should be responsive.
I'm assuming you meant the polling logic within the core. My issues on 3B+ may be a partial red herring. This board is seemingly unhappy with hotplugging HDMI at all, and will easily reboot on me. Can I suggest that we split this PR to only deal with 2711, and we'll sort the GPIO path out later? |
No, sorry, I meant the HOTPLUG_INT registers in the HDMI controller you weren't sure if they were usable.
Sure. The encoder retrieval logic is completely broken though on the Pi3, so we'll want to fix that too. I'll try to fix it next week and will let you know |
The current core while setting the min and max rate properly in the clk_request structure will not make sure that the requested rate is within these boundaries, leaving it to each and every driver to make sure it is. Add a clamp call to make sure it's always done. Signed-off-by: Maxime Ripard <[email protected]>
This reverts commit 3cf3d39. This commit was making the assumption that we had a 1:1 mapping between the encoders and their CRTC. While this is true for the HDMI controllers on the BCM2711, this isn't true for the other encoders (DSI0 and DPI share the PixelValve 0, and DSI1 and SMI share the PixelValve1), and this isn't true at all on the older SoCs, effectively breaking the encoder retrieval logic. Signed-off-by: Maxime Ripard <[email protected]>
The vc4_crtc_config_pv will need to access the drm_atomic_state structure and its only parent function, vc4_crtc_atomic_enable already has access to it. Let's pass it as a parameter. Fixes: 792c313 ("drm/vc4: encoder: Add finer-grained encoder callbacks") Signed-off-by: Maxime Ripard <[email protected]>
The vc4_get_crtc_encoder function currently only works when the connector->state->crtc pointer is set, which is only true when the connector is currently enabled. However, we use it as part of the disable path as well, and our lookup will fail in that case, resulting in it returning a null pointer we can't act on. We can access the connector that used to be connected to that crtc though using the old connector state in the disable path. Since we want to support both the enable and disable path, we can support it by passing the state accessor variant as a function pointer, together with the atomic state. Fixes: 792c313 ("drm/vc4: encoder: Add finer-grained encoder callbacks") Signed-off-by: Maxime Ripard <[email protected]>
At boot, we can't rely on the vc4_get_crtc_encoder since we don't have a state yet and thus will not be able to figure out which connector is attached to our CRTC. However, we have a muxing bit in the CRTC register we can use to get the encoder currently connected to the pixelvalve. We can thus read that register, lookup the associated register through the vc4_pv_data structure, and then pass it to vc4_crtc_disable so that we can perform the proper operations. Fixes: 875a4d5 ("drm/vc4: drv: Disable the CRTC at boot time") Signed-off-by: Maxime Ripard <[email protected]>
mripard
force-pushed
the
rpi/5.10-hotplug-irq
branch
from
a0b2234 to
42e984a
Compare
|
I just pushed a new version that addresses the encoder retrieval logic, fixes the CPU hang we were seeing during a hotplug on the Pi3B+ (and was there but hidden away on the Pi4), and adds the interrupt-based hotplug on the Pi4. |
pm_runtime_get_sync increases the PM usage counter even if it fails, and forgetting to do so will result in a reference leak. We can't really do anything in atomic_enable in case of a failure though, and we probably can't recover either, but at least switching to pm_runtime_resume_and_get makes us play nice with the PM subsystem. Fixes: 4f6e3d6 ("drm/vc4: Add runtime PM support to the HDMI encoder driver") Signed-off-by: Maxime Ripard <[email protected]>
If the HPD GPIO is not available and drm_probe_ddc fails, we end up reading the HDMI_HOTPLUG register, but the controller might be powered off resulting in a CPU hang. Make sure we have the power domain and the HSM clock powered during the detect cycle to prevent the hang from happening. Fixes: 4f6e3d6 ("drm/vc4: Add runtime PM support to the HDMI encoder driver") Signed-off-by: Maxime Ripard <[email protected]>
When we have the entire DRM state, retrieving the connector state only requires the drm_connector pointer. Fortunately for us, we have allocated it as a part of the vc4_hdmi structure, so we can retrieve get a pointer by simply accessing our field in that structure. Signed-off-by: Maxime Ripard <[email protected]>
DRM currently polls for the HDMI connector status every 10s, which can be an issue when we connect/disconnect a display quickly or the device on the other end only issues a hotplug pulse (for example on EDID change). Switch the driver to rely on the internal controller logic for the BCM2711/RPi4. Signed-off-by: Maxime Ripard <[email protected]>
mripard
force-pushed
the
rpi/5.10-hotplug-irq
branch
from
42e984a to
671a806
Compare
| if (ret) | ||
| return ret; | ||
|
|
||
| connector->polled = DRM_CONNECTOR_POLL_HPD; |
There was a problem hiding this comment.
This doesn't compile (error: ‘connector’ undeclared). @mripard was there something lost in a rebase?
There was a problem hiding this comment.
I think it needs:
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index f84c8257bd7e..f1930549de1f 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -2177,6 +2177,7 @@ static irqreturn_t vc4_hdmi_hpd_irq_thread(int irq, void *priv)
static int vc4_hdmi_hotplug_init(struct vc4_hdmi *vc4_hdmi)
{
struct platform_device *pdev = vc4_hdmi->pdev;
+ struct drm_connector *connector = &vc4_hdmi->connector;
struct device *dev = &pdev->dev;
int ret;
See: raspberrypi/linux#4313 kernel: drm/vc4: hdmi: Add a workqueue to set scrambling See: raspberrypi/linux#4329
See: raspberrypi/linux#4313 kernel: drm/vc4: hdmi: Add a workqueue to set scrambling See: raspberrypi/linux#4329
Hi,
So far the HDMI driver was relying on polling every 10s for the HDMI connector status which could lead to issues when the cable was quickly unplugged and then plugged back, or when a device like an HDMI bridge would just create a short hotplug pulse.
Here's a PR that converts that logic to rely on either the controller interrupts (for the BCM2711) or the HPD gpio interrupt (for earlier SoCs) to remove that limitation.