On 5/21/26 22:13, Jonas Karlman wrote:
Hi Neil,On 5/20/2026 11:58 AM, Neil Armstrong wrote:Hi, On 5/18/26 20:01, Jonas Karlman wrote:HDMI Specification Version 1.4b chapter 8.5 mentions: An HDMI Sink shall not assert high voltage level on its Hot Plug Detect pin when the E-EDID is not available for reading. A Source may use a high voltage level Hot Plug Detect signal to initiate the reading of E-EDID data. An HDMI Sink shall indicate any change to the contents of the E-EDID by driving a low voltage level pulse on the Hot Plug Detect pin. This pulse shall be at least 100 msec. Use a delayed work to debounce reacting on HPD events to improve handling of a HPD low voltage level pulse when a sink changes the EDID. The delayed work is only enabled between enable_hpd()/hpd_enable() and disable_hpd()/hpd_disable() calls from core, i.e. enabled after attach/bind/resume and disabled before detach/unbind/suspend. The 1100 msec hotplug debounce timeout was arbitrarily picked to match other drivers using same const, and testing using a Raspberry Pi Monitor seem to use a 200-300 msec pulse when going from standby to power on state.The logic looks ok, but I'm puzzled by the 1.1 sec debounce, which after plugging in a monitor will only send an irq event after 1.1s which is very long. Since the spec says 100ms and the real worls values are more like 200-300ms, I would first reduce this to 500ms.You are correct, this value was picked based on existing values used by other drivers: exynos/exynos_hdmi.c:#define HOTPLUG_DEBOUNCE_MS 1100 bridge/ti-tfp410.c:#define HOTPLUG_DEBOUNCE_MS 1100 amd/display/amdgpu_dm/amdgpu_dm.h:#define AMDGPU_DM_MAX_HDMI_HPD_DEBOUNCE_MS 5000 rockchip/dw_hdmi_qp-rockchip.c:#define HOTPLUG_DEBOUNCE_MS 150 150 ms was too short for my test monitor (Raspberry Pi Monitor), it does a HPD low voltage level pulse when powering on of around 200+ ms. [82.641903] dw_hdmi_hardirq: EVENT=plugout [82.841939] dw_hdmi_hardirq: EVENT=plugin And on my LG OLED 4K TV there was typically a pulse of around 700-900 ms. So the 1.1 seconds used by other drivers seemed like a good candidate :-)But as I understand the code right now, on the first HPD front the irq work is programmed to run after the debounce time, but if it's a pulse the irq would also trigger on the second HPD front and then delay again the work after the debounce time.Your analysis is correct, any HPD event would keep debouncing adding 1.1 timout from each HPD irq, both plugout and plugin. The intention was to allow for up to 1.1 seconds to pass between a plugout and a plugin, before we treated a plugout as a full disconnect event, and not to delay a possible connected event by 1.1 seconds.My understanding of a debounce was that we "ignore" the pulse by only generating a single irq event when the pulse is finished.Correct, as long as the pulse was less than 1.1 seconds.The current code does that, we will only have a single irq event and the HPD will return as connected state, good. But this delays the irq event 1.1s _after_ the end of the pulse, which I would expect the event to be send at tht debounce time after the start of the pulse.Good catch and I agree, we should delay/timeout the disconnected state longer than plugin and react on plugin almost immediately.Like, program the work at the beginning of the pulse, if somehow the pulse ends before the debounce time, send the irq event immediately, otherwise let the debounce work run after the debounce time which will trigger a disconnect event. But the delay is too high, 1.1s could be a manual unplug/plug or bad connector with false contact on the hpd pin. I would rather reduce this to something more realistic like 500ms or less and try to better handle the pulse somehow. But I don't have any idea if the scheme I described is doable.We can configure different delays for plugout and plugin, I am currently testing something like following: #define HOTPLUG_CONNECTED_DEBOUNCE_MS 150 #define HOTPLUG_DISCONNECTED_DEBOUNCE_MS 500 delay = status == connector_status_connected ? HOTPLUG_CONNECTED_DEBOUNCE_MS : HOTPLUG_DISCONNECTED_DEBOUNCE_MS; mod_delayed_work(system_percpu_wq, &hdmi->hpd_work, msecs_to_jiffies(delay)); That would mean: - at plugout we (re-)start the timer to trigger in 500ms - at plugin we (re-)start the timer to trigger in 150ms - whenever the timer triggers the hpd_work is started - the hpw_work trigger normal detect() handling to determin if connector status (or EDID) has changed Example during power on of a Raspberry Pi Monitor: [82.641903] dw_hdmi_hardirq: EVENT=plugout [82.648276] dw_hdmi_schedule_hpd_work(status=2) [82.841939] dw_hdmi_hardirq: EVENT=plugin [82.848211] dw_hdmi_schedule_hpd_work(status=1) [83.008573] dw_hdmi_hpd_work(): START [83.020358] dw_hdmi_bridge_detect() [83.034958] dw_hdmi_bridge_edid_read() [83.187102] [drm:update_display_info.part.0] [CONNECTOR:55:HDMI-A-1] CEA VCDB 0x4a [83.194471] [drm:update_display_info.part.0] [CONNECTOR:55:HDMI-A-1] HDMI: DVI dual 0, max TMDS clock 0 kHz [83.201755] [drm:update_display_info.part.0] [CONNECTOR:55:HDMI-A-1] ELD monitor RPI MON156 [83.208968] [drm:update_display_info.part.0] [CONNECTOR:55:HDMI-A-1] HDMI: latency present 0 0, video latency 0 0, audio latency 0 0 [83.216588] [drm:update_display_info.part.0] [CONNECTOR:55:HDMI-A-1] ELD size 36, SAD count 1 [83.224445] [drm:check_connector_changed] [CONNECTOR:55:HDMI-A-1] Same epoch counter 2 [83.231553] dw_hdmi_hpd_work(): END Around 205ms between HPD=0 and HPD=1 and around 160ms from plugin until delayed hpd_work starts. And at full plugout the debounce time is around 497 ms: [95.125105] dw_hdmi_hardirq: EVENT=plugout [95.147586] dw_hdmi_schedule_hpd_work(status=2) [95.645277] dw_hdmi_hpd_work(): START [95.667741] dw_hdmi_bridge_detect() [95.691523] [drm:drm_edid_connector_update] [CONNECTOR:55:HDMI-A-1] EDID changed, epoch counter 3 [95.709861] [drm:check_connector_changed] [CONNECTOR:55:HDMI-A-1] status updated from connected to disconnected [95.722674] [drm:check_connector_changed] [CONNECTOR:55:HDMI-A-1] Changed epoch counter 2 => 4 [95.735173] [drm:drm_sysfs_connector_hotplug_event] [CONNECTOR:55:HDMI-A-1] generating connector hotplug event [95.746326] [drm:drm_fb_helper_hotplug_event] [95.754871] [drm:drm_client_modeset_probe] [95.762429] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:55:HDMI-A-1] [95.769758] dw_hdmi_bridge_detect() [95.776597] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:55:HDMI-A-1] disconnected [95.784389] [drm:drm_client_modeset_probe] No connectors reported connected with modes [95.791865] [drm:drm_client_modeset_probe] [CONNECTOR:55:HDMI-A-1] enabled? no [95.799426] [drm:drm_client_firmware_config.isra.0] Not using firmware configuration [95.807140] [drm:drm_client_modeset_probe] picking CRTCs for 1920x1080 config [95.818422] dw_hdmi_bridge_atomic_disable() [95.826412] [drm:vop2_plane_atomic_disable] Smart0-win0 disable [95.868341] [drm:drm_client_hotplug] fbdev: ret=0 [95.878206] dw_hdmi_hpd_work(): END With a much quicker reaction on plugin event maybe the 1.1 seconds could stay to avoid having to do a full teardown, disable() + enable() cycle, at slightly longer HPD pulses.
Yes those traces a good
Or maybe it is time to re-spin an old cec-adapter debounce series [1] that can allow for some additional time until the CEC phys addr is fully invalidated at a longer HPD low voltage level pulse.
Either one are good, the cec-adapt one looks simpler but still effective. Thanks, Neil
[1] https://lore.kernel.org/linux-media/he1pr06mb40115700084d1d875673d60eac...@he1pr06mb4011.eurprd06.prod.outlook.com/ Regards, JonasNeilSigned-off-by: Jonas Karlman <[email protected]> --- v7: Change to free irq before mute and clear using IH regs, also include clear of STAT0_RX_SENSE v6: Change back to disable_delayed_work_sync() in hpd disable ops, Ensure HPD interrupt is masked and IRQ handler is disabled early in dw_hdmi_remove() to prevent any irq re-arming of delayed work, Drop use of suspend helper v5: Change to none-sync disable_delayed_work() in hpd disable ops, Change to cancel_delayed_work_sync() in remove, Add cancel_delayed_work_sync() to new suspend helper v4: Disable/mask delayed_work until enable_hpd()/hpd_enable(), Read connector status directly from HW regs in hpd_work v3: New patch --- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 80 +++++++++++++++++++++-- 1 file changed, 75 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 8afc9d240121..270db58a0e7c 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -50,6 +50,8 @@#define HDMI14_MAX_TMDSCLK 340000000 +#define HOTPLUG_DEBOUNCE_MS 1100+ static const u16 csc_coeff_default[3][4] = { { 0x2000, 0x0000, 0x0000, 0x0000 }, { 0x0000, 0x2000, 0x0000, 0x0000 }, @@ -185,6 +187,7 @@ struct dw_hdmi { hdmi_codec_plugged_cb plugged_cb; struct device *codec_dev; enum drm_connector_status last_connector_result; + struct delayed_work hpd_work; };const struct dw_hdmi_plat_data *dw_hdmi_to_plat_data(struct dw_hdmi *hdmi)@@ -2517,6 +2520,20 @@ static void dw_hdmi_connector_force(struct drm_connector *connector) dw_hdmi_connector_status_update(hdmi, connector, connector->status); }+static void dw_hdmi_connector_enable_hpd(struct drm_connector *connector)+{ + struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi, connector); + + enable_delayed_work(&hdmi->hpd_work); +} + +static void dw_hdmi_connector_disable_hpd(struct drm_connector *connector) +{ + struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi, connector); + + disable_delayed_work_sync(&hdmi->hpd_work); +} + static void dw_hdmi_connector_destroy(struct drm_connector *connector) { struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi, connector); @@ -2538,6 +2555,8 @@ static const struct drm_connector_funcs dw_hdmi_connector_funcs = { static const struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = { .get_modes = dw_hdmi_connector_get_modes, .atomic_check = dw_hdmi_connector_atomic_check, + .enable_hpd = dw_hdmi_connector_enable_hpd, + .disable_hpd = dw_hdmi_connector_disable_hpd, };static int dw_hdmi_connector_create(struct dw_hdmi *hdmi)@@ -2968,6 +2987,20 @@ static const struct drm_edid *dw_hdmi_bridge_edid_read(struct drm_bridge *bridge return dw_hdmi_edid_read(hdmi, connector); }+static void dw_hdmi_bridge_hpd_enable(struct drm_bridge *bridge)+{ + struct dw_hdmi *hdmi = bridge->driver_private; + + enable_delayed_work(&hdmi->hpd_work); +} + +static void dw_hdmi_bridge_hpd_disable(struct drm_bridge *bridge) +{ + struct dw_hdmi *hdmi = bridge->driver_private; + + disable_delayed_work_sync(&hdmi->hpd_work); +} + static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = { .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, @@ -2981,6 +3014,8 @@ static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = { .mode_valid = dw_hdmi_bridge_mode_valid, .detect = dw_hdmi_bridge_detect, .edid_read = dw_hdmi_bridge_edid_read, + .hpd_enable = dw_hdmi_bridge_hpd_enable, + .hpd_disable = dw_hdmi_bridge_hpd_disable, };/* -----------------------------------------------------------------------------@@ -3101,8 +3136,8 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id) status == connector_status_connected ? "plugin" : "plugout");- if (hdmi->bridge.dev)- drm_helper_hpd_irq_event(hdmi->bridge.dev); + mod_delayed_work(system_percpu_wq, &hdmi->hpd_work, + msecs_to_jiffies(HOTPLUG_DEBOUNCE_MS)); }hdmi_writeb(hdmi, intr_stat, HDMI_IH_PHY_STAT0);@@ -3112,6 +3147,29 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id) return IRQ_HANDLED; }+static void dw_hdmi_hpd_work(struct work_struct *work)+{ + struct dw_hdmi *hdmi = container_of(work, struct dw_hdmi, hpd_work.work); + struct drm_device *dev = hdmi->bridge.dev; + + if (WARN_ON(!dev)) + return; + + /* + * Notify the DRM core of the HPD event using drm_helper_hpd_irq_event() + * instead of drm_bridge_hpd_notify(). This will cause the DRM function + * check_connector_changed() to be called, which in turn calls the + * connector detect()/force() funcs to detect any connection status or + * epoch changes. The bridge connector detect() func also ensures that + * any hpd_notify() funcs are called for all bridges in the chain. + * + * drm_bridge_hpd_notify() shares a mutex with drm_bridge_hpd_disable(), + * and can result in a deadlock due to the disable_delayed_work_sync() + * call to wait on work to complete in dw_hdmi_bridge_hpd_disable(). + */ + drm_helper_hpd_irq_event(dev); +} + static const struct dw_hdmi_phy_data dw_hdmi_phys[] = { { .type = DW_HDMI_PHY_DWC_HDMI_TX_PHY, @@ -3396,6 +3454,9 @@ struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev, goto err_res; }+ INIT_DELAYED_WORK(&hdmi->hpd_work, dw_hdmi_hpd_work);+ disable_delayed_work(&hdmi->hpd_work); + ret = devm_request_threaded_irq(dev, irq, dw_hdmi_hardirq, dw_hdmi_irq, IRQF_SHARED, dev_name(dev), hdmi); @@ -3532,6 +3593,18 @@ EXPORT_SYMBOL_GPL(dw_hdmi_probe);void dw_hdmi_remove(struct dw_hdmi *hdmi){ + struct platform_device *pdev = to_platform_device(hdmi->dev); + int irq = platform_get_irq(pdev, 0); + + /* Free, mute and clear phy interrupts */ + devm_free_irq(hdmi->dev, irq, hdmi); + hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0); + hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, + HDMI_IH_PHY_STAT0); + + /* Cancel any pending hot plug work */ + cancel_delayed_work_sync(&hdmi->hpd_work); + drm_bridge_remove(&hdmi->bridge);if (hdmi->audio && !IS_ERR(hdmi->audio))@@ -3539,9 +3612,6 @@ void dw_hdmi_remove(struct dw_hdmi *hdmi) if (!IS_ERR(hdmi->cec)) platform_device_unregister(hdmi->cec);- /* Disable all interrupts */- hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0); - if (hdmi->i2c) i2c_del_adapter(&hdmi->i2c->adap); else
