[PATCH v7 19/23] drm: bridge: dw_hdmi: Use delayed_work to debounce hotplug event
Neil Armstrong
neil.armstrong at linaro.org
Wed May 20 02:58:26 PDT 2026
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.
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.
My understanding of a debounce was that we "ignore" the pulse by only generating
a single irq event when the pulse is finished.
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.
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.
Neil
>
> Signed-off-by: Jonas Karlman <jonas at 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
More information about the Linux-rockchip
mailing list