[v6,16/22] drm: bridge: dw_hdmi: Use display_info is_hdmi and has_audio
Commit Message
drm_edid_connector_update() is being called from bridge connector funcs
and from detect and force funcs for dw-hdmi connector.
Change to use is_hdmi and has_audio from display_info directly instead
of keeping our own state in sink_is_hdmi and sink_has_audio.
Also remove the old and unused edid struct member and related define.
Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
Reviewed-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
Tested-by: Diederik de Haas <diederik@cknow-tech.com> # Rock64, RockPro64, Quartz64-B
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v6: Collect t-b tag
v5: No change
v4: Collect r-b tag
v3: No change
v2: Collect r-b tag
---
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 32 ++++-------------------
1 file changed, 5 insertions(+), 27 deletions(-)
Comments
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Direct, lockless access to `connector->display_info` from the `atomic_enable` path creates a race condition with concurrent EDID probes, which can result in incorrectly initializing the display in DVI mode with wrong audio or SCDC settings.
--
commit 339e5d6c4cc3c0ee74a9425b8a94dabb6a044d69
Author: Jonas Karlman <jonas@kwiboo.se>
drm: bridge: dw_hdmi: Use display_info is_hdmi and has_audio
This commit removes the private sink_is_hdmi and sink_has_audio tracking in
dw_hdmi, and switches to using display->is_hdmi and display->has_audio
from connector->display_info directly instead.
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 1402b3963ae12..e9c4e24c090ca 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
[ ... ]
> @@ -2053,7 +2047,7 @@ static void hdmi_av_composer(struct dw_hdmi *hdmi,
> HDMI_FC_INVIDCONF_IN_I_P_INTERLACED :
> HDMI_FC_INVIDCONF_IN_I_P_PROGRESSIVE;
>
> - inv_val |= hdmi->sink_is_hdmi ?
> + inv_val |= display->is_hdmi ?
Is it safe to read connector->display_info directly in the atomic_enable path?
Since dw_hdmi_poweron() and hdmi_av_composer() are called from the bridge's
atomic_enable callback, they can run asynchronously for non-blocking commits
after the DRM modeset locks have been released.
If a concurrent EDID probe triggered by userspace (e.g., via
DRM_IOCTL_MODE_GETCONNECTOR) calls drm_edid_connector_update() on another CPU,
it starts by calling drm_reset_display_info() which clears display_info.is_hdmi
and has_audio to false until the EDID is fully re-parsed.
If this code reads the display_info fields during that window, won't it observe
transient zeroes and incorrectly program the hardware for DVI mode, as well as
disabling SCDC scrambling and audio?
Would it be better to capture these properties into an atomic state structure
during atomic_check() and read them safely from the state object during
atomic_enable()?
On Sat, 16 May 2026, Jonas Karlman <jonas@kwiboo.se> wrote:
> - /*
> - * FIXME: This should use connector->display_info.is_hdmi and
> - * connector->display_info.has_audio from a path that has read the EDID
> - * and called drm_edid_connector_update().
> - */
> - edid = drm_edid_raw(drm_edid);
> -
> - dev_dbg(hdmi->dev, "got edid: width[%d] x height[%d]\n",
> - edid->width_cm, edid->height_cm);
> -
> - hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);
> - hdmi->sink_has_audio = drm_detect_monitor_audio(edid);
Yay! \o/
>
> return drm_edid;
> }
@@ -45,8 +45,6 @@
#define DDC_CI_ADDR 0x37
#define DDC_SEGMENT_ADDR 0x30
-#define HDMI_EDID_LEN 512
-
/* DW-HDMI Controller >= 0x200a are at least compliant with SCDC version 1 */
#define SCDC_MIN_SOURCE_VERSION 0x1
@@ -146,8 +144,6 @@ struct dw_hdmi {
int vic;
- u8 edid[HDMI_EDID_LEN];
-
struct {
const struct dw_hdmi_phy_ops *ops;
const char *name;
@@ -157,8 +153,6 @@ struct dw_hdmi {
struct i2c_adapter *ddc;
void __iomem *regs;
- bool sink_is_hdmi;
- bool sink_has_audio;
struct pinctrl *pinctrl;
struct pinctrl_state *default_state;
@@ -2053,7 +2047,7 @@ static void hdmi_av_composer(struct dw_hdmi *hdmi,
HDMI_FC_INVIDCONF_IN_I_P_INTERLACED :
HDMI_FC_INVIDCONF_IN_I_P_PROGRESSIVE;
- inv_val |= hdmi->sink_is_hdmi ?
+ inv_val |= display->is_hdmi ?
HDMI_FC_INVIDCONF_DVI_MODEZ_HDMI_MODE :
HDMI_FC_INVIDCONF_DVI_MODEZ_DVI_MODE;
@@ -2289,7 +2283,7 @@ static int dw_hdmi_poweron(struct dw_hdmi *hdmi,
if (hdmi->hdmi_data.enc_out_bus_format == MEDIA_BUS_FMT_FIXED)
hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
- hdmi->hdmi_data.rgb_limited_range = hdmi->sink_is_hdmi &&
+ hdmi->hdmi_data.rgb_limited_range = display->is_hdmi &&
drm_default_rgb_quant_range(mode) ==
HDMI_QUANTIZATION_RANGE_LIMITED;
@@ -2309,7 +2303,7 @@ static int dw_hdmi_poweron(struct dw_hdmi *hdmi,
/* HDMI Initialization Step B.3 */
dw_hdmi_enable_video_path(hdmi);
- if (hdmi->sink_has_audio) {
+ if (display->has_audio) {
dev_dbg(hdmi->dev, "sink has audio support\n");
/* HDMI Initialization Step E - Configure audio */
@@ -2318,7 +2312,7 @@ static int dw_hdmi_poweron(struct dw_hdmi *hdmi,
}
/* not for DVI mode */
- if (hdmi->sink_is_hdmi) {
+ if (display->is_hdmi) {
dev_dbg(hdmi->dev, "%s HDMI mode\n", __func__);
/* HDMI Initialization Step F - Configure AVI InfoFrame */
@@ -2432,29 +2426,13 @@ static const struct drm_edid *dw_hdmi_edid_read(struct dw_hdmi *hdmi,
struct drm_connector *connector)
{
const struct drm_edid *drm_edid;
- const struct edid *edid;
if (!hdmi->ddc)
return NULL;
drm_edid = drm_edid_read_ddc(connector, hdmi->ddc);
- if (!drm_edid) {
+ if (!drm_edid)
dev_dbg(hdmi->dev, "failed to get edid\n");
- return NULL;
- }
-
- /*
- * FIXME: This should use connector->display_info.is_hdmi and
- * connector->display_info.has_audio from a path that has read the EDID
- * and called drm_edid_connector_update().
- */
- edid = drm_edid_raw(drm_edid);
-
- dev_dbg(hdmi->dev, "got edid: width[%d] x height[%d]\n",
- edid->width_cm, edid->height_cm);
-
- hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);
- hdmi->sink_has_audio = drm_detect_monitor_audio(edid);
return drm_edid;
}