diff mbox series

[v10,37/69] drm/rockchip: dw_hdmi_qp: Control the HPD IRQ line via the bridge HPD ops

Message ID 20260731-dw-hdmi-qp-scramb-v10-37-294364b2cf15@collabora.com (mailing list archive)
State New
Headers
Series Add HDMI 2.0 support to DW HDMI QP TX |

Commit Message

Cristian Ciocaltea July 31, 2026, 4:19 p.m. UTC 
The HPD interrupt must not be delivered before the DRM connector is fully
set up, and it should be quiesced while the display pipeline is suspended.
On top of that, the threaded handler re-arms HPD unconditionally after the
hotplug debounce, which must not override an explicit disable requested by
the DRM core.

Request the HPD IRQ in bind() with IRQF_NO_AUTOEN so it stays disabled
until the core arms it, and drive enable_irq()/disable_irq() from the
phy ops invoked by the bridge through its hpd_{enable,disable}()
callbacks.  This ensures the line is only armed after the connector is
ready.  disable_irq() additionally waits for the threaded handler to
finish, so its GRF re-arm can no longer race with and override a
core-requested disable.

Note that IRQF_NO_AUTOEN is incompatible with IRQF_SHARED; the latter is
dropped as this IRQ has no other users.

Tested-by: Diederik de Haas <diederik@cknow-tech.com>  # NanoPC-T6 LTS, Rock 5B
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

Comments

sashiko-bot@kernel.org July 31, 2026, 9:47 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hardware is suspended before canceling asynchronous HPD work, creating a race condition that leads to I2C timeouts during system suspend.
- [Medium] The disable_hpd callbacks do not cancel the pending delayed HPD work, allowing spurious HPD events.
--

--- Patch [37]: [PATCH v10 37/69] drm/rockchip: dw_hdmi_qp: Control the HPD IRQ line via the bridge HPD ops ---
commit ad6fbe00662ae75dcead6bf784de9de47ed9dd6c
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

drm/rockchip: dw_hdmi_qp: Control the HPD IRQ line via the bridge HPD ops

The HPD interrupt must not be delivered before the DRM connector is fully
set up, and it should be quiesced while the display pipeline is suspended.
On top of that, the threaded handler re-arms HPD unconditionally after the
hotplug debounce, which must not override an explicit disable requested by
the DRM core.

> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> index 46a61112c14f4..253da2162ce6f 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
[ ... ]
> @@ -308,12 +309,14 @@ static void dw_hdmi_qp_rk3588_enable_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
>  	struct rockchip_hdmi_qp *hdmi = (struct rockchip_hdmi_qp *)data;
>  
>  	dw_hdmi_qp_rk3588_unmask_hpd(hdmi);
> +	enable_irq(hdmi->hpd_irq);
>  }
>  
>  static void dw_hdmi_qp_rk3588_disable_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
>  {
>  	struct rockchip_hdmi_qp *hdmi = (struct rockchip_hdmi_qp *)data;
>  
> +	disable_irq(hdmi->hpd_irq);
>  	dw_hdmi_qp_rk3588_mask_hpd(hdmi);
>  }

[Severity: Medium]
The DRM core disables HPD (e.g., during driver unbind or DRM pipeline disable),
which invokes dw_hdmi_qp_rk3588_disable_hpd() and dw_hdmi_qp_rk3576_disable_hpd().
This disables the HPD IRQ but does not cancel the already queued hdmi->hpd_work.

Could this delayed work execute up to 150ms later, probing the connector and
running EDID reads when the driver expects HPD to be fully quiesced, resulting
in spurious HPD events?

[ ... ]
> @@ -688,14 +693,14 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>  	if (plat_data.cec_irq < 0)
>  		return plat_data.cec_irq;
>  
> -	irq = platform_get_irq_byname(pdev, "hpd");
> -	if (irq < 0)
> -		return irq;
> +	hdmi->hpd_irq = platform_get_irq_byname(pdev, "hpd");
> +	if (hdmi->hpd_irq < 0)
> +		return hdmi->hpd_irq;
>  
> -	ret = devm_request_threaded_irq(dev, irq,
> +	ret = devm_request_threaded_irq(dev, hdmi->hpd_irq,
>  					cfg->ctrl_ops->hardirq_callback,
>  					cfg->ctrl_ops->irq_callback,
> -					IRQF_SHARED, "dw-hdmi-qp-hpd",
> +					IRQF_NO_AUTOEN, "dw-hdmi-qp-hpd",
>  					hdmi);
>  	if (ret)
>  		return ret;

[Severity: High]
When the system suspends, dw_hdmi_qp_rockchip_suspend() is called. It first
calls dw_hdmi_qp_suspend(), which disables the main_irq used for I2C
completions. It then calls cancel_delayed_work_sync(&hdmi->hpd_work):

dw_hdmi_qp_rockchip_suspend() {
    ...
    dw_hdmi_qp_suspend(dev, hdmi->hdmi);
    cancel_delayed_work_sync(&hdmi->hpd_work);
    ...
}

If the HPD work is executing concurrently, it will attempt to read the EDID via
I2C, but since main_irq is disabled, the I2C read will time out.

Can this race condition lead to I2C timeouts and bus hangs during system
suspend?
diff mbox series

Patch

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
index 46a61112c14f..253da2162ce6 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
@@ -99,6 +99,7 @@  struct rockchip_hdmi_qp {
 	struct phy *phy;
 	struct gpio_desc *frl_enable_gpio;
 	struct delayed_work hpd_work;
+	int hpd_irq;
 	int port_id;
 	const struct rockchip_hdmi_qp_ctrl_ops *ctrl_ops;
 	unsigned long long tmds_char_rate;
@@ -308,12 +309,14 @@  static void dw_hdmi_qp_rk3588_enable_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
 	struct rockchip_hdmi_qp *hdmi = (struct rockchip_hdmi_qp *)data;
 
 	dw_hdmi_qp_rk3588_unmask_hpd(hdmi);
+	enable_irq(hdmi->hpd_irq);
 }
 
 static void dw_hdmi_qp_rk3588_disable_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
 {
 	struct rockchip_hdmi_qp *hdmi = (struct rockchip_hdmi_qp *)data;
 
+	disable_irq(hdmi->hpd_irq);
 	dw_hdmi_qp_rk3588_mask_hpd(hdmi);
 }
 
@@ -355,12 +358,14 @@  static void dw_hdmi_qp_rk3576_enable_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
 	struct rockchip_hdmi_qp *hdmi = (struct rockchip_hdmi_qp *)data;
 
 	dw_hdmi_qp_rk3576_unmask_hpd(hdmi);
+	enable_irq(hdmi->hpd_irq);
 }
 
 static void dw_hdmi_qp_rk3576_disable_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
 {
 	struct rockchip_hdmi_qp *hdmi = (struct rockchip_hdmi_qp *)data;
 
+	disable_irq(hdmi->hpd_irq);
 	dw_hdmi_qp_rk3576_mask_hpd(hdmi);
 }
 
@@ -584,7 +589,7 @@  static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	struct resource *res;
 	struct clk_bulk_data *clks;
 	struct clk *ref_clk;
-	int ret, irq, i;
+	int ret, i;
 
 	if (!dev->of_node)
 		return -ENODEV;
@@ -688,14 +693,14 @@  static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	if (plat_data.cec_irq < 0)
 		return plat_data.cec_irq;
 
-	irq = platform_get_irq_byname(pdev, "hpd");
-	if (irq < 0)
-		return irq;
+	hdmi->hpd_irq = platform_get_irq_byname(pdev, "hpd");
+	if (hdmi->hpd_irq < 0)
+		return hdmi->hpd_irq;
 
-	ret = devm_request_threaded_irq(dev, irq,
+	ret = devm_request_threaded_irq(dev, hdmi->hpd_irq,
 					cfg->ctrl_ops->hardirq_callback,
 					cfg->ctrl_ops->irq_callback,
-					IRQF_SHARED, "dw-hdmi-qp-hpd",
+					IRQF_NO_AUTOEN, "dw-hdmi-qp-hpd",
 					hdmi);
 	if (ret)
 		return ret;