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.
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
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 | 76 +++++++++++++++++++++--
1 file changed, 71 insertions(+), 5 deletions(-)
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removing the top-level interrupt mute in the remove path leaves already-latched interrupts unmasked, causing an unhandled interrupt storm after the IRQ is freed.
--
commit 0025156a478e265d2ea625b34b904b8645d80bed
Author: Jonas Karlman <jonas@kwiboo.se>
drm: bridge: dw_hdmi: Use delayed_work to debounce hotplug event
This commit uses a delayed work queue to debounce HDMI hotplug detect events,
improving the handling of HPD low voltage level pulses. It also updates
the driver remove path to explicitly mask the PHY interrupt and free the
IRQ handler earlier during cleanup.
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
[ ... ]
> @@ -3531,6 +3592,14 @@ 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);
> +
> + /* Disable cable hot plug irq. */
> + hdmi_writeb(hdmi, ~0, HDMI_PHY_MASK0);
> + devm_free_irq(hdmi->dev, irq, hdmi);
> + cancel_delayed_work_sync(&hdmi->hpd_work);
> +
> drm_bridge_remove(&hdmi->bridge);
>
> if (hdmi->audio && !IS_ERR(hdmi->audio))
> @@ -3538,9 +3607,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);
> -
Can removing the top-level interrupt mute cause an unhandled interrupt storm?
While writing to HDMI_PHY_MASK0 stops the PHY from latching new interrupts,
it does not clear interrupts that are already latched in the
HDMI_IH_PHY_STAT0 register. Because the top-level HDMI_IH_MUTE_PHY_STAT0
mute is no longer applied, any previously latched interrupt might continue
to assert the physical IRQ line.
Immediately after writing the mask earlier in the function, devm_free_irq()
removes the driver's handler from the IRQF_SHARED line. With the handler
removed, the kernel will not invoke dw_hdmi_hardirq() to clear the device's
pending interrupt.
Since the interrupt controller might see the line continuously asserted,
other drivers sharing the IRQ would return IRQ_NONE. Could this lead to the
kernel logging an 'irq nobody cared' error and permanently disabling the
shared IRQ line, breaking other devices?
Would it be safer to restore the HDMI_IH_MUTE_PHY_STAT0 write to guarantee
the IRQ line drops before the handler is freed?
> if (hdmi->i2c)
> i2c_del_adapter(&hdmi->i2c->adap);
> else
@@ -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);
@@ -3531,6 +3592,14 @@ 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);
+
+ /* Disable cable hot plug irq. */
+ hdmi_writeb(hdmi, ~0, HDMI_PHY_MASK0);
+ devm_free_irq(hdmi->dev, irq, hdmi);
+ cancel_delayed_work_sync(&hdmi->hpd_work);
+
drm_bridge_remove(&hdmi->bridge);
if (hdmi->audio && !IS_ERR(hdmi->audio))
@@ -3538,9 +3607,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