Re: [PATCH v3 09/13] drm: bridge: dw_hdmi: Use delayed_work to debounce hotplug event

Jonas Karlman
2026-04-28T22:12:17Z

Hi Nicolas,

On 4/27/2026 8:27 PM, Nicolas Frattaroli wrote:
> On Friday, 3 April 2026 20:52:55 Central European Summer Time 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 work queue to debounce reacting on HPD events to better handle a
>> HPD low voltage level pulse when a sink changes the EDID.
>>
>> 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.
>>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>> v3: New patch
>> ---
>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 24 +++++++++++++++++++----
>> 1 file changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> index f3f8144ae98e..4d079b689b3b 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -51,6 +51,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 },
>> @@ -192,6 +194,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)
>> @@ -2528,6 +2531,7 @@ static void dw_hdmi_connector_force(struct drm_connector *connector)
>>
>> mutex_lock(&hdmi->mutex);
>> hdmi->force = connector->force;
>> + hdmi->last_connector_result = connector->status;
>> dw_hdmi_update_phy_mask(hdmi);
>> mutex_unlock(&hdmi->mutex);
>> }
>> @@ -3046,6 +3050,16 @@ void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense)
>> }
>> EXPORT_SYMBOL_GPL(dw_hdmi_setup_rx_sense);
>>
>> +static void dw_hdmi_hpd_work(struct work_struct *work)
>> +{
>> + struct dw_hdmi *hdmi = container_of(work, struct dw_hdmi, hpd_work.work);
>> +
>> + if (hdmi->bridge.dev) {
>> + drm_helper_hpd_irq_event(hdmi->bridge.dev);
>> + drm_bridge_hpd_notify(&hdmi->bridge, hdmi->last_connector_result);
>
> I think you'll want to `mutex_lock(&hdmi->mutex);` (or better yet,
> `guard(mutex)(&hdmi->mutex)` or `scoped_guard(mutex, &hdmi->mutex) {`)
> here before reading last_connector_result, otherwise you'll have a
> data race with any writers.

Thanks, I will adjust to something like that in a v4.

With the first call to drm_helper_hpd_irq_event() the dw_hdmi_detect()
that updates the last_connector_result value should have been called.
So by the time drm_bridge_hpd_notify() is called we should have the
latest connector->status value in last_connector_result.

Ideally I would like to fully drop the drm_bridge_hpd_notify() call here
and instead only call drm_connector_helper_hpd_irq_event() to detect any
connector changes due to HPD and/or EDID changes. Something to look
closer at in a future series :-)

Regards,
Jonas

>
>> + }
>> +}
>> +
>> static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
>> {
>> struct dw_hdmi *hdmi = dev_id;
>> @@ -3097,10 +3111,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);
>> - drm_bridge_hpd_notify(&hdmi->bridge, status);
>> - }
>> + mod_delayed_work(system_percpu_wq, &hdmi->hpd_work,
>> + msecs_to_jiffies(HOTPLUG_DEBOUNCE_MS));
>> }
>>
>> hdmi_writeb(hdmi, intr_stat, HDMI_IH_PHY_STAT0);
>> @@ -3420,6 +3432,8 @@ struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
>> goto err_res;
>> }
>>
>> + INIT_DELAYED_WORK(&hdmi->hpd_work, dw_hdmi_hpd_work);
>> +
>> ret = devm_request_threaded_irq(dev, irq, dw_hdmi_hardirq,
>> dw_hdmi_irq, IRQF_SHARED,
>> dev_name(dev), hdmi);
>> @@ -3552,6 +3566,8 @@ EXPORT_SYMBOL_GPL(dw_hdmi_probe);
>>
>> void dw_hdmi_remove(struct dw_hdmi *hdmi)
>> {
>> + disable_delayed_work_sync(&hdmi->hpd_work);
>> +
>> drm_bridge_remove(&hdmi->bridge);
>>
>> if (hdmi->audio && !IS_ERR(hdmi->audio))
>>
>
> Kind regards,
> Nicolas Frattaroli
>
>