[v7,23/23] drm: bridge: dw_hdmi: Merge top and bottom half IRQ handlers
Commit Message
The bottom half IRQ handler only modify delay of or queue a delayed work
used for HPD handling. The mod_delayed_work() called is documented as
being safe to call from any context including IRQ handler.
Merge top and bottom half IRQ handlers to simplify IRQ handling now that
HPD event is handled using a delayed work.
Tested-by: Diederik de Haas <diederik@cknow-tech.com> # Rock64, RockPro64, Quartz64-B
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v7: No change
v6: Collect r-b tag
v5: No change
v4: New patch
---
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 34 ++++++++---------------
1 file changed, 11 insertions(+), 23 deletions(-)
Comments
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Clearing the HPD interrupt status after modifying the interrupt polarity creates a race window where an edge transition is missed, breaking hotplug detection.
--
commit b4c743935eb9892c0e2a6a962a52440a37e985de
Author: Jonas Karlman <jonas@kwiboo.se>
drm: bridge: dw_hdmi: Merge top and bottom half IRQ handlers
This commit simplifies IRQ handling by merging the top and bottom half IRQ
handlers into a single handler. It transitions HPD event handling to use a
delayed work queue instead of an IRQ thread.
> 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
> @@ -2993,30 +2993,18 @@ static irqreturn_t dw_hdmi_hardirq(int irq, void *dev_id)
[ ... ]
> - intr_stat = hdmi_readb(hdmi, HDMI_IH_PHY_STAT0);
> - if (intr_stat & HDMI_IH_PHY_STAT0_HPD) {
> + intr_stat = hdmi_readb(hdmi, HDMI_IH_PHY_STAT0) & HDMI_IH_PHY_STAT0_HPD;
> + if (intr_stat) {
> enum drm_connector_status status;
>
> + hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0);
> +
> /* Set HPD interrupt polarity based on current HPD status. */
> status = dw_hdmi_phy_read_hpd(hdmi, hdmi->phy.data);
> hdmi_modb(hdmi, status == connector_status_connected ?
> @@ -3028,12 +3016,13 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
>
> mod_delayed_work(system_percpu_wq, &hdmi->hpd_work,
> msecs_to_jiffies(HOTPLUG_DEBOUNCE_MS));
> - }
>
> - hdmi_writeb(hdmi, intr_stat, HDMI_IH_PHY_STAT0);
> - hdmi_writeb(hdmi, ~HDMI_IH_PHY_STAT0_HPD, HDMI_IH_MUTE_PHY_STAT0);
> + hdmi_writeb(hdmi, intr_stat, HDMI_IH_PHY_STAT0);
Does clearing the HPD interrupt status after modifying the interrupt polarity
create a race window where an edge transition is missed?
While this isn't a new regression introduced by this patch, it persists in
the new merged handler:
1. The ISR reads HDMI_IH_PHY_STAT0 and finds the HPD bit set.
2. It reads the current PHY state (e.g. CONNECTED) and updates the polarity
HDMI_PHY_POL0 to look for the opposite edge (e.g. PLUGOUT).
3. If a fast bounce occurs right here (the cable is unplugged), the hardware
detects the edge and sets the HPD sticky bit to 1 again.
4. The ISR then writes the original intr_stat (with the HPD bit set) back to
HDMI_IH_PHY_STAT0 (a write-1-to-clear register). This clears the newly latched
unplug event.
This leaves the hardware waiting for a PLUGOUT event, but the cable is already
disconnected. When the cable is plugged back in, the hardware ignores the
PLUGIN edge because it is still looking for a PLUGOUT.
Could this lead to a regression where hotplug interrupts break until the next
reboot? Should the W1C register be cleared before reading the PHY status and
updating the polarity?
> + hdmi_writeb(hdmi, ~HDMI_IH_PHY_STAT0_HPD, HDMI_IH_MUTE_PHY_STAT0);
> + ret = IRQ_HANDLED;
> + }
>
> - return IRQ_HANDLED;
> + return ret;
> }
@@ -2993,30 +2993,18 @@ static irqreturn_t dw_hdmi_hardirq(int irq, void *dev_id)
if (hdmi->i2c)
ret = dw_hdmi_i2c_irq(hdmi);
- intr_stat = hdmi_readb(hdmi, HDMI_IH_PHY_STAT0);
- if (intr_stat) {
- hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0);
- return IRQ_WAKE_THREAD;
- }
-
- return ret;
-}
-
-static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
-{
- struct dw_hdmi *hdmi = dev_id;
- u8 intr_stat;
-
/*
* Interrupt generation is accomplished in the following way:
* interrupt = (mask == 0) && (polarity == status)
* All interrupts are forwarded to the Interrupt Handler sticky bit
* register ih_phy_stat0 and muted using the register ih_mute_phy_stat0.
*/
- intr_stat = hdmi_readb(hdmi, HDMI_IH_PHY_STAT0);
- if (intr_stat & HDMI_IH_PHY_STAT0_HPD) {
+ intr_stat = hdmi_readb(hdmi, HDMI_IH_PHY_STAT0) & HDMI_IH_PHY_STAT0_HPD;
+ if (intr_stat) {
enum drm_connector_status status;
+ hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0);
+
/* Set HPD interrupt polarity based on current HPD status. */
status = dw_hdmi_phy_read_hpd(hdmi, hdmi->phy.data);
hdmi_modb(hdmi, status == connector_status_connected ?
@@ -3028,12 +3016,13 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
mod_delayed_work(system_percpu_wq, &hdmi->hpd_work,
msecs_to_jiffies(HOTPLUG_DEBOUNCE_MS));
+
+ hdmi_writeb(hdmi, intr_stat, HDMI_IH_PHY_STAT0);
+ hdmi_writeb(hdmi, ~HDMI_IH_PHY_STAT0_HPD, HDMI_IH_MUTE_PHY_STAT0);
+ ret = IRQ_HANDLED;
}
- hdmi_writeb(hdmi, intr_stat, HDMI_IH_PHY_STAT0);
- hdmi_writeb(hdmi, ~HDMI_IH_PHY_STAT0_HPD, HDMI_IH_MUTE_PHY_STAT0);
-
- return IRQ_HANDLED;
+ return ret;
}
static void dw_hdmi_hpd_work(struct work_struct *work)
@@ -3343,9 +3332,8 @@ struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
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);
+ ret = devm_request_irq(dev, irq, dw_hdmi_hardirq, IRQF_SHARED,
+ dev_name(dev), hdmi);
if (ret)
goto err_res;