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

[Dmitry Baryshkov]

On Tue, 28 Apr 2026 at 22:34, Jonas Karlman <jonas at kwiboo.se> wrote:
>
> Hi Dmitry,
>
> On 4/28/2026 2:38 PM, Dmitry Baryshkov wrote:
> > On Fri, Apr 03, 2026 at 06:52:55PM +0000, 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 at 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);
> >
> > Nit: please consider this for the followup cleanup. The
> > drm_helper_hpd_irq_event() is not very effective: we already know which
> > bridge (and thus connector) caused the HPD event.
> >
> > Moreover, the call should be redundant as drm_bridge_hpd_notify will
> > call the registered callback, which normally would be
> > drm_bridge_connector_hpd_cb(). Internally, this function calls
> > drm_kms_helper_connector_hotplug_event().
>
> I disagree in part, based on my testing, without a call to
> drm_helper_hpd_irq_event() or drm_connector_helper_hpd_irq_event() the
> internal check_connector_changed() never seem to be reached to detect
> any connector->status and/or connector->epoch_counter changes.
>
> In my ongoing work to convert dw-hdmi to become a hdmi bridge, I have
> actually done the opposite to what you suggest here and instead fully
> dropped the call to drm_bridge_hpd_notify() made here.

That doesnt' sound correct, but maybe it's because dw_hdmi isn't using
drm_bridge_conenctor. The latter one registers a hook in the HPD
bridge, which then receives the event and performs all the
notifications. If the dw_hdmi doesn't do it (because you don't need
it), then it's fine for now. When you work on conveting dw_hdmi to
DBC, I'd recommend using the bridge notification instead (as it would
then play nice with the rest of the bridge and HDMI framework).

>
> drm_bridge_connector_detect() calls drm_bridge_connector_hpd_notify()
> so any bridges should already be notified as part of any internal
> check_connector_changed() handling.
>
> Maybe something like following could be an option:
>
>         changed = drm_connector_helper_hpd_irq_event(connector);
>         if (changed)
>                 drm_bridge_hpd_notify(bridge, connector->status);
>
> I will likely keep this as-is to keep existing behavior in a v4, as this
> patch was just about de-bouncing the existing calls being made at HPD.

Yep.

>
> Regards,
> Jonas
>
> >
> >> +            drm_bridge_hpd_notify(&hdmi->bridge, hdmi->last_connector_result);
> >> +    }
> >> +}
> >> +
> >>  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))
> >> --
> >> 2.53.0
> >>
> >
>


-- 
With best wishes
Dmitry