Re: [PATCH v7 19/23] drm: bridge: dw_hdmi: Use delayed_work to debounce hotplug event

[Neil Armstrong <neil.armstrong@linaro.org>]

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/HE1PR06MB40115700084D1D875673D60EAC9D0@HE1PR06MB4011.eurprd06.prod.outlook.com/
> 
> Regards,
> Jonas
> 
>>
>> Neil
>>
>>>
>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>> ---
>>> 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
>>
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic