Export dw_hdmi_qp_hpd_notify() for platform drivers to report hot-plug
detection events. Unlike drm_helper_hpd_irq_event() which polls all
connectors, this helper targets only the affected connector and ensures
.detect_ctx() is invoked on reconnection events to trigger SCDC state
recovery.
Note that this helper may sleep, so it must only be called from a
threaded HPD interrupt handler or a work function.
Tested-by: Maud Spierings <maud_spierings@hotmail.com>
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 36 ++++++++++++++++++++++++++++
include/drm/bridge/dw_hdmi_qp.h | 1 +
2 files changed, 37 insertions(+)
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Unprotected lockless access to `hdmi->curr_conn` between atomic commit threads and the HPD notification thread, leading to a data race.
--
--- Patch [23]: [PATCH v9 23/61] drm/bridge: dw-hdmi-qp: Provide dw_hdmi_qp_hpd_notify() helper ---
commit f35dc78b72396dddf94643e1b74b8f00fbaf1275
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
drm/bridge: dw-hdmi-qp: Provide dw_hdmi_qp_hpd_notify() helper
This commit exports a helper function for platform drivers to report hot-plug
detection events. It targets the affected connector and ensures context
detection is invoked on reconnection events for SCDC state recovery.
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> index ee74266b7aa9c..be1bd765cb033 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
[ ... ]
> @@ -785,6 +787,8 @@ static void dw_hdmi_qp_bridge_atomic_enable(struct drm_bridge *bridge,
> dw_hdmi_qp_mod(hdmi, op_mode, OPMODE_DVI, LINK_CONFIG0);
>
> drm_atomic_helper_connector_hdmi_update_infoframes(connector, state);
> +
> + hdmi->curr_conn = connector;
[Severity: Medium]
Is a WRITE_ONCE() needed here? hdmi->curr_conn appears to be read locklessly
by the HPD threaded IRQ or work queue context. Without WRITE_ONCE(), could
the compiler reorder the store before hardware initialization is complete?
> }
>
> static void dw_hdmi_qp_bridge_atomic_disable(struct drm_bridge *bridge,
> @@ -793,6 +797,7 @@ static void dw_hdmi_qp_bridge_atomic_disable(struct drm_bridge *bridge,
> struct dw_hdmi_qp *hdmi = bridge->driver_private;
> struct drm_connector *connector;
>
> + hdmi->curr_conn = NULL;
[Severity: Medium]
Does this assignment to hdmi->curr_conn also need a WRITE_ONCE() to ensure
safe concurrent access during concurrent hotplug events and modesets?
[ ... ]
> @@ -1409,6 +1414,37 @@ struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
> }
> EXPORT_SYMBOL_GPL(dw_hdmi_qp_bind);
>
> +/**
> + * dw_hdmi_qp_hpd_notify() - Notify a hot-plug detection event
> + * @hdmi: pointer to the DW HDMI QP controller
> + *
> + * Platform drivers should call this from their threaded IRQ handler or work
> + * function to notify the bridge of a connection status change.
> + * The bridge's .read_hpd() phy_ops callback is used to read the current
> + * connection status.
> + */
> +void dw_hdmi_qp_hpd_notify(struct dw_hdmi_qp *hdmi)
> +{
> + struct drm_connector *conn = hdmi->curr_conn;
[Severity: Medium]
Since this is called from a threaded HPD interrupt handler or work function,
can this lockless read of hdmi->curr_conn race with the KMS modeset thread?
If the writes in the atomic commit paths use WRITE_ONCE(), does this need a
corresponding READ_ONCE() to prevent compiler optimizations like register
caching from causing stale reads?
> + enum drm_connector_status status;
@@ -28,6 +28,7 @@
#include <drm/drm_edid.h>
#include <drm/drm_modes.h>
#include <drm/drm_print.h>
+#include <drm/drm_probe_helper.h>
#include <media/cec.h>
@@ -163,6 +164,7 @@ struct dw_hdmi_qp {
struct regmap *regm;
int main_irq;
+ struct drm_connector *curr_conn;
unsigned long tmds_char_rate;
bool no_hpd;
};
@@ -785,6 +787,8 @@ static void dw_hdmi_qp_bridge_atomic_enable(struct drm_bridge *bridge,
dw_hdmi_qp_mod(hdmi, op_mode, OPMODE_DVI, LINK_CONFIG0);
drm_atomic_helper_connector_hdmi_update_infoframes(connector, state);
+
+ hdmi->curr_conn = connector;
}
static void dw_hdmi_qp_bridge_atomic_disable(struct drm_bridge *bridge,
@@ -793,6 +797,7 @@ static void dw_hdmi_qp_bridge_atomic_disable(struct drm_bridge *bridge,
struct dw_hdmi_qp *hdmi = bridge->driver_private;
struct drm_connector *connector;
+ hdmi->curr_conn = NULL;
hdmi->tmds_char_rate = 0;
connector = drm_atomic_get_old_connector_for_encoder(state, bridge->encoder);
@@ -1409,6 +1414,37 @@ struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
}
EXPORT_SYMBOL_GPL(dw_hdmi_qp_bind);
+/**
+ * dw_hdmi_qp_hpd_notify() - Notify a hot-plug detection event
+ * @hdmi: pointer to the DW HDMI QP controller
+ *
+ * Platform drivers should call this from their threaded IRQ handler or work
+ * function to notify the bridge of a connection status change.
+ * The bridge's .read_hpd() phy_ops callback is used to read the current
+ * connection status.
+ */
+void dw_hdmi_qp_hpd_notify(struct dw_hdmi_qp *hdmi)
+{
+ struct drm_connector *conn = hdmi->curr_conn;
+ enum drm_connector_status status;
+
+ status = hdmi->phy.ops->read_hpd(hdmi, hdmi->phy.data);
+ dev_dbg(hdmi->dev, "%s status=%d\n", __func__, status);
+
+ /*
+ * When the display pipeline has been already active, switch to
+ * drm_connector_helper_hpd_irq_event() to ensure .detect_ctx()
+ * gets invoked, i.e. via drm_helper_probe_detect(), because
+ * drm_bridge_hpd_notify() defers to a delayed hotplug path in
+ * this case.
+ */
+ if (conn && status == connector_status_connected)
+ drm_connector_helper_hpd_irq_event(conn);
+ else
+ drm_bridge_hpd_notify(&hdmi->bridge, status);
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_qp_hpd_notify);
+
void dw_hdmi_qp_suspend(struct device *dev, struct dw_hdmi_qp *hdmi)
{
disable_irq(hdmi->main_irq);
@@ -36,6 +36,7 @@ struct dw_hdmi_qp_plat_data {
struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
struct drm_encoder *encoder,
const struct dw_hdmi_qp_plat_data *plat_data);
+void dw_hdmi_qp_hpd_notify(struct dw_hdmi_qp *hdmi);
void dw_hdmi_qp_suspend(struct device *dev, struct dw_hdmi_qp *hdmi);
void dw_hdmi_qp_resume(struct device *dev, struct dw_hdmi_qp *hdmi);
#endif /* __DW_HDMI_QP__ */