Initial support Cadence MHDP8501(HDMI/DP) for i.MX8MQ

Allow HDMI PHYs to be configured through the generic functions through a custom structure added to the generic union.

The parameters added here are based on HDMI PHY implementation practices. The current set of parameters should cover the potential users.

Signed-off-by Sandor Yu <[email protected]>

---
v15->v16:
- Remove pixel_clk_rate, bpc and color_space fields from struct phy_configure_opts_hdmi, they were replaced by unsigned long long tmds_char_rate.
- Remove r-b and a-c tags because this patch have important change.

v9->v15:
*No change.

 include/linux/phy/phy-hdmi.h | 20 ++++++++++++++++++++
 include/linux/phy/phy.h      |  7 ++++++-
 2 files changed, 26 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/phy/phy-hdmi.h

include/linux/phy/phy-hdmi.h new file +0 -0

new file mode 100644
index 000000000000..dd923e19d17c

include/linux/phy/phy-hdmi.h new file +11 -0

+/**
+ * struct phy_configure_opts_hdmi - HDMI configuration set
+ * @tmds_char_rate: HDMI TMDS Character Rate in Hertz.
+ *
+ * This structure is used to represent the configuration state of a HDMI phy.
+ */
+struct phy_configure_opts_hdmi {
+	unsigned long long tmds_char_rate;
+};
+
+#endif /* __PHY_HDMI_H_ */

include/linux/phy/phy.h +6 -1

index 03cd5bae92d3..4ac486b101fe 100644
@@ -17,6 +17,7 @@
 #include <linux/regulator/consumer.h>
 
 #include <linux/phy/phy-dp.h>
+#include <linux/phy/phy-hdmi.h>
 #include <linux/phy/phy-lvds.h>
 #include <linux/phy/phy-mipi-dphy.h>
 

@@ -42,7 +43,8 @@ enum phy_mode {
 	PHY_MODE_MIPI_DPHY,
 	PHY_MODE_SATA,
 	PHY_MODE_LVDS,
-	PHY_MODE_DP
+	PHY_MODE_DP,
+	PHY_MODE_HDMI,
 };
 
 enum phy_media {

@@ -60,11 +62,14 @@ enum phy_media {
  *		the DisplayPort protocol.
  * @lvds:	Configuration set applicable for phys supporting
  *		the LVDS phy mode.
+ * @hdmi:	Configuration set applicable for phys supporting
+ *		the HDMI phy mode.
  */
 union phy_configure_opts {
 	struct phy_configure_opts_mipi_dphy	mipi_dphy;
 	struct phy_configure_opts_dp		dp;
 	struct phy_configure_opts_lvds		lvds;
+	struct phy_configure_opts_hdmi		hdmi;
 };
 
 /**

Signature · 1 line

2.34.1

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +23 -0

+enum drm_connector_status cdns_mhdp8501_detect(struct cdns_mhdp8501_device *mhdp)
+{
+	u8 hpd = 0xf;
+
+	hpd = cdns_mhdp8501_read_hpd(mhdp);
+	if (hpd == 1)
+		return connector_status_connected;
+	else if (hpd == 0)
+		return connector_status_disconnected;
+
+	dev_warn(mhdp->dev, "Unknown cable status, hdp=%u\n", hpd);
+	return connector_status_unknown;
+}
+
+static void hotplug_work_func(struct work_struct *work)
+{
+	struct cdns_mhdp8501_device *mhdp = container_of(work,
+						     struct cdns_mhdp8501_device,
+						     hotplug_work.work);
+	enum drm_connector_status status = cdns_mhdp8501_detect(mhdp);
+
+	drm_bridge_hpd_notify(&mhdp->bridge, status);
+

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +7 -0

+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);
+	ret = phy_init(mhdp->phy);
+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret) {
+		dev_err(dev, "Failed to initialize PHY: %d\n", ret);
+		goto clk_disable;

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +26 -0

+static enum drm_mode_status
+cdns_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
+			       const struct drm_display_mode *mode,
+			       unsigned long long tmds_rate)
+{
+	struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
+	union phy_configure_opts phy_cfg;
+	int ret;
+
+	phy_cfg.hdmi.tmds_char_rate = tmds_rate;
+
+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);
+	ret = phy_validate(mhdp->phy, PHY_MODE_HDMI, 0, &phy_cfg);
+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret < 0)
+		return MODE_CLOCK_RANGE;
+
+	return MODE_OK;
+}
+
+static enum drm_mode_status
+cdns_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
+			    const struct drm_display_info *info,
+			    const struct drm_display_mode *mode)
+{

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +22 -0

+static void cdns_hdmi_bridge_atomic_enable(struct drm_bridge *bridge,
+					   struct drm_bridge_state *old_state)
+{
+	struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
+	struct drm_atomic_state *state = old_state->base.state;
+	struct drm_connector *connector;
+	struct video_info *video = &mhdp->video_info;
+	struct drm_crtc_state *crtc_state;
+	struct drm_connector_state *conn_state;
+	struct drm_display_mode *mode = &mhdp->mode;
+	union phy_configure_opts phy_cfg;
+	int ret;
+
+	connector = drm_atomic_get_new_connector_for_encoder(state,
+							     bridge->encoder);
+	if (WARN_ON(!connector))
+		return;
+
+	mhdp->curr_conn = connector;
+
+	conn_state = drm_atomic_get_new_connector_state(state, connector);
+	if (WARN_ON(!conn_state))

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +26 -0

+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);
+	ret = phy_configure(mhdp->phy, &phy_cfg);
+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret) {
+		dev_err(mhdp->dev, "%s: phy_configure() failed: %d\n",
+			__func__, ret);
+		return;
+	}
+
+	cdns_hdmi_sink_config(mhdp);
+
+	ret = cdns_hdmi_ctrl_init(mhdp);
+	if (ret < 0) {
+		dev_err(mhdp->dev, "%s, ret = %d\n", __func__, ret);
+		return;
+	}
+
+	/* Config GCP */
+	if (mhdp->video_info.bpc == 8)
+		cdns_hdmi_disable_gcp(mhdp);
+	else
+		cdns_hdmi_enable_gcp(mhdp);
+
+	ret = cdns_hdmi_mode_config(mhdp, mode, &mhdp->video_info);
+	if (ret < 0) {

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +20 -0

+static int cdns_hdmi_bridge_write_infoframe(struct drm_bridge *bridge,
+					    enum hdmi_infoframe_type type,
+					    const u8 *buffer, size_t len)
+{
+	struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
+
+	switch (type) {
+	case HDMI_INFOFRAME_TYPE_AVI:
+		cdns_hdmi_config_infoframe(mhdp, 0, len, buffer, HDMI_INFOFRAME_TYPE_AVI);
+		break;
+	case HDMI_INFOFRAME_TYPE_SPD:
+		cdns_hdmi_config_infoframe(mhdp, 1, len, buffer, HDMI_INFOFRAME_TYPE_SPD);
+		break;
+	case HDMI_INFOFRAME_TYPE_VENDOR:
+		cdns_hdmi_config_infoframe(mhdp, 2, len, buffer, HDMI_INFOFRAME_TYPE_VENDOR);
+		break;
+	default:
+		dev_dbg(mhdp->dev, "Unsupported infoframe type %x\n", type);
+	}
+

-----Original Message-----
From: dri-devel <[email protected]> On Behalf Of Maxime Ripard
Sent: 2024年7月2日 21:25
To: Sandor Yu <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; Laurent Pinchart <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; dl-linux-imx <[email protected]>; Oliver Brown <[email protected]>; [email protected]; [email protected]
Subject: [EXT] Re: [PATCH v16 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

Hi,

There's still the scrambler issue we discussed on v15, but I have some more comments.

Quoted 22 lines · Sandor Yu

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +22 -0

+enum drm_connector_status cdns_mhdp8501_detect(struct
+cdns_mhdp8501_device *mhdp) {
+	u8 hpd = 0xf;
+
+	hpd = cdns_mhdp8501_read_hpd(mhdp);
+	if (hpd == 1)
+		return connector_status_connected;
+	else if (hpd == 0)
+		return connector_status_disconnected;
+
+	dev_warn(mhdp->dev, "Unknown cable status, hdp=%u\n", hpd);
+	return connector_status_unknown;
+}
+
+static void hotplug_work_func(struct work_struct *work) {
+	struct cdns_mhdp8501_device *mhdp = container_of(work,
+						     struct cdns_mhdp8501_device,
+						     hotplug_work.work);
+	enum drm_connector_status status = cdns_mhdp8501_detect(mhdp);
+
+	drm_bridge_hpd_notify(&mhdp->bridge, status);
+

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c line 84

Quoted 10 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +10 -0

+	if (status == connector_status_connected) {
+		/* Cable connected  */
+		DRM_INFO("HDMI/DP Cable Plug In\n");
+		enable_irq(mhdp->irq[IRQ_OUT]);
+	} else if (status == connector_status_disconnected) {
+		/* Cable Disconnected  */
+		DRM_INFO("HDMI/DP Cable Plug Out\n");
+		enable_irq(mhdp->irq[IRQ_IN]);
+	}
+}

You shouldn't play with the interrupt being enabled here: hotplug interrupts should always enabled.

If you can't for some reason, the reason should be documented in your driver.

Quoted 12 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +11 -0

+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);
+	ret = phy_init(mhdp->phy);
+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret) {
+		dev_err(dev, "Failed to initialize PHY: %d\n", ret);
+		goto clk_disable;
+	}
+
+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c line 275

Quoted 6 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +6 -0

+	ret = phy_set_mode(mhdp->phy, phy_mode);
+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret) {
+		dev_err(dev, "Failed to configure PHY: %d\n", ret);
+		goto clk_disable;
+	}

Why do you need a shared mutex between the phy and HDMI controller?

Quoted 29 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +28 -0

+static enum drm_mode_status
+cdns_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
+			       const struct drm_display_mode *mode,
+			       unsigned long long tmds_rate) {
+	struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
+	union phy_configure_opts phy_cfg;
+	int ret;
+
+	phy_cfg.hdmi.tmds_char_rate = tmds_rate;
+
+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);
+	ret = phy_validate(mhdp->phy, PHY_MODE_HDMI, 0, &phy_cfg);
+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret < 0)
+		return MODE_CLOCK_RANGE;
+
+	return MODE_OK;
+}
+
+static enum drm_mode_status
+cdns_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
+			    const struct drm_display_info *info,
+			    const struct drm_display_mode *mode) {
+	unsigned long long tmds_rate;
+
+	/* We don't support double-clocked and Interlaced modes */
+	if (mode->flags & DRM_MODE_FLAG_DBLCLK ||

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c line 584

Quoted 6 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c new file +6 -0

+	    mode->flags & DRM_MODE_FLAG_INTERLACE)
+		return MODE_BAD;
+
+	/* MAX support pixel clock rate 594MHz */
+	if (mode->clock > 594000)
+		return MODE_CLOCK_HIGH;

This needs to be in the tmds_char_rate_valid function

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c line 430

Quoted 9 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +8 -0

+	if (mode->hdisplay > 3840)
+		return MODE_BAD_HVALUE;
+
+	if (mode->vdisplay > 2160)
+		return MODE_BAD_VVALUE;
+
+	tmds_rate = mode->clock * 1000ULL;
+	return cdns_hdmi_tmds_char_rate_valid(bridge, mode, tmds_rate);

It will already be called by the core so this is redundant.

Quoted 27 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +26 -0

+static void cdns_hdmi_bridge_atomic_enable(struct drm_bridge *bridge,
+					   struct drm_bridge_state *old_state) {
+	struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
+	struct drm_atomic_state *state = old_state->base.state;
+	struct drm_connector *connector;
+	struct video_info *video = &mhdp->video_info;
+	struct drm_crtc_state *crtc_state;
+	struct drm_connector_state *conn_state;
+	struct drm_display_mode *mode = &mhdp->mode;
+	union phy_configure_opts phy_cfg;
+	int ret;
+
+	connector = drm_atomic_get_new_connector_for_encoder(state,
+							     bridge->encoder);
+	if (WARN_ON(!connector))
+		return;
+
+	mhdp->curr_conn = connector;
+
+	conn_state = drm_atomic_get_new_connector_state(state, connector);
+	if (WARN_ON(!conn_state))
+		return;
+
+	crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
+	if (WARN_ON(!crtc_state))
+		return;

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c line 494

Quoted 5 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +5 -0

+
+	video->color_fmt = conn_state->hdmi.output_format;
+	video->bpc = conn_state->hdmi.output_bpc;
+
+	drm_mode_copy(&mhdp->mode, &crtc_state->adjusted_mode);

Why do you need a copy of all these fields? You should pass the connector / bridge state around and not copy these fields.

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c line 498

Quoted 3 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +3 -0

+	/* video mode check */
+	if (mode->clock == 0 || mode->hdisplay == 0 || mode->vdisplay == 0)
+		return;

This should be checked in atomic_check, but I'm pretty sure it's redundant.

Quoted 4 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +3 -0

+	dev_dbg(mhdp->dev, "Mode: %dx%dp%d\n",
+		mode->hdisplay, mode->vdisplay, mode->clock);
+

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c line 510

Quoted 10 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +10 -0

+	drm_atomic_helper_connector_hdmi_update_infoframes(connector,
+state);
+
+	/* Line swapping */
+	cdns_mhdp_reg_write(&mhdp->base, LANES_CONFIG, 0x00400000 |
+mhdp->lane_mapping);
+
+	mhdp->hdmi.char_rate = drm_hdmi_compute_mode_clock(mode,
+							   mhdp->video_info.bpc,
+							   mhdp->video_info.color_fmt);

The TMDS char rate is already available in the connector_state so there no need to recompute it.

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c line 511

Quoted 1 line

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +1 -0

+	phy_cfg.hdmi.tmds_char_rate = mhdp->hdmi.char_rate;

And you shouldn't store a copy either.

Quoted 18 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +17 -0

+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);
+	ret = phy_configure(mhdp->phy, &phy_cfg);
+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret) {
+		dev_err(mhdp->dev, "%s: phy_configure() failed: %d\n",
+			__func__, ret);
+		return;
+	}
+
+	cdns_hdmi_sink_config(mhdp);
+
+	ret = cdns_hdmi_ctrl_init(mhdp);
+	if (ret < 0) {
+		dev_err(mhdp->dev, "%s, ret = %d\n", __func__, ret);
+		return;
+	}

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c

Quoted 10 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +10 -0

+
+	/* Config GCP */
+	if (mhdp->video_info.bpc == 8)
+		cdns_hdmi_disable_gcp(mhdp);
+	else
+		cdns_hdmi_enable_gcp(mhdp);
+
+	ret = cdns_hdmi_mode_config(mhdp, mode, &mhdp->video_info);
+	if (ret < 0) {
+		dev_err(mhdp->dev, "CDN_API_HDMITX_SetVic_blocking ret

= %d\n", ret);

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c line 548

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +8 -0

+		return;
+	}
+}
+
+static int cdns_hdmi_bridge_clear_infoframe(struct drm_bridge *bridge,
+					    enum hdmi_infoframe_type type) {
+	return 0;
+}

Either implement it or don't, but an empty function is dead code.

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c

Quoted 9 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +8 -0

+static int cdns_hdmi_bridge_write_infoframe(struct drm_bridge *bridge,
+					    enum hdmi_infoframe_type type,
+					    const u8 *buffer, size_t len) {
+	struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
+
+	switch (type) {
+	case HDMI_INFOFRAME_TYPE_AVI:
+		cdns_hdmi_config_infoframe(mhdp, 0, len, buffer,

HDMI_INFOFRAME_TYPE_AVI);

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c

Quoted 3 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +3 -0

+		break;
+	case HDMI_INFOFRAME_TYPE_SPD:
+		cdns_hdmi_config_infoframe(mhdp, 1, len, buffer,

HDMI_INFOFRAME_TYPE_SPD);

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c

Quoted 3 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +3 -0

+		break;
+	case HDMI_INFOFRAME_TYPE_VENDOR:
+		cdns_hdmi_config_infoframe(mhdp, 2, len, buffer,

HDMI_INFOFRAME_TYPE_VENDOR);

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c

Quoted 13 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +13 -0

+		break;
+	default:
+		dev_dbg(mhdp->dev, "Unsupported infoframe type %x\n", type);
+	}
+
+	return 0;
+}
+
+static int cdns_hdmi_bridge_atomic_check(struct drm_bridge *bridge,
+					 struct drm_bridge_state *bridge_state,
+					 struct drm_crtc_state *crtc_state,
+					 struct drm_connector_state *conn_state) {
+	return

drm_atomic_helper_connector_hdmi_check(conn_state->connector,

Quoted 1 line

+conn_state->state); }

You should also call your mode_valid function here.

Maxime

Hi Maxime,

Thanks for your comments,

Quoted 62 lines

-----Original Message-----
From: dri-devel <[email protected]> On Behalf Of Maxime Ripard
Sent: 2024年7月2日 21:25
To: Sandor Yu <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; Laurent Pinchart <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; dl-linux-imx <[email protected]>; Oliver Brown <[email protected]>; [email protected]; [email protected]
Subject: [EXT] Re: [PATCH v16 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

Hi,

There's still the scrambler issue we discussed on v15, but I have some more comments.

Quoted 22 lines · Sandor Yu

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +22 -0

+enum drm_connector_status cdns_mhdp8501_detect(struct
+cdns_mhdp8501_device *mhdp) {
+	u8 hpd = 0xf;
+
+	hpd = cdns_mhdp8501_read_hpd(mhdp);
+	if (hpd == 1)
+		return connector_status_connected;
+	else if (hpd == 0)
+		return connector_status_disconnected;
+
+	dev_warn(mhdp->dev, "Unknown cable status, hdp=%u\n", hpd);
+	return connector_status_unknown;
+}
+
+static void hotplug_work_func(struct work_struct *work) {
+	struct cdns_mhdp8501_device *mhdp = container_of(work,
+						     struct cdns_mhdp8501_device,
+						     hotplug_work.work);
+	enum drm_connector_status status = cdns_mhdp8501_detect(mhdp);
+
+	drm_bridge_hpd_notify(&mhdp->bridge, status);
+

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c line 84

Quoted 10 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +10 -0

+	if (status == connector_status_connected) {
+		/* Cable connected  */
+		DRM_INFO("HDMI/DP Cable Plug In\n");
+		enable_irq(mhdp->irq[IRQ_OUT]);
+	} else if (status == connector_status_disconnected) {
+		/* Cable Disconnected  */
+		DRM_INFO("HDMI/DP Cable Plug Out\n");
+		enable_irq(mhdp->irq[IRQ_IN]);
+	}
+}

You shouldn't play with the interrupt being enabled here: hotplug interrupts should always enabled.

Quoted 1 line

If you can't for some reason, the reason should be documented in your driver.

iMX8MQ have two HPD interrupters, one for plugout and the other for plugin, because they could not be masked, so we have to enable one and disable the other. I will add more comments here.

Quoted 12 lines

Quoted 12 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +11 -0

+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);
+	ret = phy_init(mhdp->phy);
+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret) {
+		dev_err(dev, "Failed to initialize PHY: %d\n", ret);
+		goto clk_disable;
+	}
+
+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c line 275

Quoted 6 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +6 -0

+	ret = phy_set_mode(mhdp->phy, phy_mode);
+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret) {
+		dev_err(dev, "Failed to configure PHY: %d\n", ret);
+		goto clk_disable;
+	}

Why do you need a shared mutex between the phy and HDMI controller?

Both PHY and HDMI controller could access to the HDMI firmware by mailbox, So add mutex to avoid race condition.

Quoted 29 lines

Quoted 29 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +28 -0

+static enum drm_mode_status
+cdns_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
+			       const struct drm_display_mode *mode,
+			       unsigned long long tmds_rate) {
+	struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
+	union phy_configure_opts phy_cfg;
+	int ret;
+
+	phy_cfg.hdmi.tmds_char_rate = tmds_rate;
+
+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);
+	ret = phy_validate(mhdp->phy, PHY_MODE_HDMI, 0, &phy_cfg);
+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret < 0)
+		return MODE_CLOCK_RANGE;
+
+	return MODE_OK;
+}
+
+static enum drm_mode_status
+cdns_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
+			    const struct drm_display_info *info,
+			    const struct drm_display_mode *mode) {
+	unsigned long long tmds_rate;
+
+	/* We don't support double-clocked and Interlaced modes */
+	if (mode->flags & DRM_MODE_FLAG_DBLCLK ||

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c line 584

Quoted 6 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c new file +6 -0

+	    mode->flags & DRM_MODE_FLAG_INTERLACE)
+		return MODE_BAD;
+
+	/* MAX support pixel clock rate 594MHz */
+	if (mode->clock > 594000)
+		return MODE_CLOCK_HIGH;

This needs to be in the tmds_char_rate_valid function

This clock rate check is covered by function tmds_char_rate_valid()
It could be removed if keep function tmds_char_rate_valid() be called by mode_valid.

Quoted 11 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c line 430

Quoted 9 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +8 -0

+	if (mode->hdisplay > 3840)
+		return MODE_BAD_HVALUE;
+
+	if (mode->vdisplay > 2160)
+		return MODE_BAD_VVALUE;
+
+	tmds_rate = mode->clock * 1000ULL;
+	return cdns_hdmi_tmds_char_rate_valid(bridge, mode, tmds_rate);

It will already be called by the core so this is redundant.

mode_valid function is use to filter the mode list in drm_helper_probe_single_connector_modes(), if function cdns_hdmi_tmds_char_rate_valid() is not called, unsupported modes will in mode list.

Quoted 27 lines

Quoted 27 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +26 -0

+static void cdns_hdmi_bridge_atomic_enable(struct drm_bridge *bridge,
+					   struct drm_bridge_state *old_state) {
+	struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
+	struct drm_atomic_state *state = old_state->base.state;
+	struct drm_connector *connector;
+	struct video_info *video = &mhdp->video_info;
+	struct drm_crtc_state *crtc_state;
+	struct drm_connector_state *conn_state;
+	struct drm_display_mode *mode = &mhdp->mode;
+	union phy_configure_opts phy_cfg;
+	int ret;
+
+	connector = drm_atomic_get_new_connector_for_encoder(state,
+							     bridge->encoder);
+	if (WARN_ON(!connector))
+		return;
+
+	mhdp->curr_conn = connector;
+
+	conn_state = drm_atomic_get_new_connector_state(state, connector);
+	if (WARN_ON(!conn_state))
+		return;
+
+	crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
+	if (WARN_ON(!crtc_state))
+		return;

Quoted 9 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c line 494

Quoted 5 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +5 -0

+
+	video->color_fmt = conn_state->hdmi.output_format;
+	video->bpc = conn_state->hdmi.output_bpc;
+
+	drm_mode_copy(&mhdp->mode, &crtc_state->adjusted_mode);

Why do you need a copy of all these fields? You should pass the connector / bridge state around and not copy these fields.

OK, mode will be removed, and it will replace by drm_connector_state.

Quoted 5 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c line 498

Quoted 3 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +3 -0

+	/* video mode check */
+	if (mode->clock == 0 || mode->hdisplay == 0 || mode->vdisplay == 0)
+		return;

This should be checked in atomic_check, but I'm pretty sure it's redundant.

OK, It will be removed.

Quoted 18 lines

Quoted 4 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +3 -0

+	dev_dbg(mhdp->dev, "Mode: %dx%dp%d\n",
+		mode->hdisplay, mode->vdisplay, mode->clock);
+

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c line 510

Quoted 10 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +10 -0

+	drm_atomic_helper_connector_hdmi_update_infoframes(connector,
+state);
+
+	/* Line swapping */
+	cdns_mhdp_reg_write(&mhdp->base, LANES_CONFIG, 0x00400000 |
+mhdp->lane_mapping);
+
+	mhdp->hdmi.char_rate = drm_hdmi_compute_mode_clock(mode,
+							   mhdp->video_info.bpc,
+							   mhdp->video_info.color_fmt);

The TMDS char rate is already available in the connector_state so there no need to recompute it.

Quoted 3 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c line 511

Quoted 1 line

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +1 -0

+	phy_cfg.hdmi.tmds_char_rate = mhdp->hdmi.char_rate;

And you shouldn't store a copy either.

OK, variable char_rate will be removed and will use the drm_connector_state-> hdmi.tmds_char_rate

Quoted 29 lines

Quoted 18 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +17 -0

+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);
+	ret = phy_configure(mhdp->phy, &phy_cfg);
+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret) {
+		dev_err(mhdp->dev, "%s: phy_configure() failed: %d\n",
+			__func__, ret);
+		return;
+	}
+
+	cdns_hdmi_sink_config(mhdp);
+
+	ret = cdns_hdmi_ctrl_init(mhdp);
+	if (ret < 0) {
+		dev_err(mhdp->dev, "%s, ret = %d\n", __func__, ret);
+		return;
+	}

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c

Quoted 10 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +10 -0

+
+	/* Config GCP */
+	if (mhdp->video_info.bpc == 8)
+		cdns_hdmi_disable_gcp(mhdp);
+	else
+		cdns_hdmi_enable_gcp(mhdp);
+
+	ret = cdns_hdmi_mode_config(mhdp, mode, &mhdp->video_info);
+	if (ret < 0) {
+		dev_err(mhdp->dev, "CDN_API_HDMITX_SetVic_blocking ret

= %d\n", ret);

Quoted 10 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c line 548

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +8 -0

+		return;
+	}
+}
+
+static int cdns_hdmi_bridge_clear_infoframe(struct drm_bridge *bridge,
+					    enum hdmi_infoframe_type type) {
+	return 0;
+}

Either implement it or don't, but an empty function is dead code.

Must have function hdmi_clear_infoframe when set DRM_BRIDGE_OP_HDMI flag in &drm_bridge->ops, otherwise the drm_bridge_connector_init() will fail.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/drm_bridge_connector.c?h=next-20240902#n419

Quoted 40 lines

Hi Maxime,

Thanks for your comments,

Quoted 36 lines

-----Original Message-----
From: dri-devel <[email protected]> On Behalf Of Maxime Ripard
Sent: 2024年7月2日 21:25
To: Sandor Yu <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; Laurent Pinchart <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; dl-linux-imx <[email protected]>; Oliver Brown <[email protected]>; [email protected]; [email protected]
Subject: [EXT] Re: [PATCH v16 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

Hi,

There's still the scrambler issue we discussed on v15, but I have some more comments.

Quoted 10 lines · Sandor Yu

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +10 -0

+enum drm_connector_status cdns_mhdp8501_detect(struct
+cdns_mhdp8501_device *mhdp) {
+	u8 hpd = 0xf;
+
+	hpd = cdns_mhdp8501_read_hpd(mhdp);
+	if (hpd == 1)
+		return connector_status_connected;
+	else if (hpd == 0)
+		return connector_status_disconnected;
+

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +8 -0

+	dev_warn(mhdp->dev, "Unknown cable status, hdp=%u\n", hpd);
+	return connector_status_unknown; }
+
+static void hotplug_work_func(struct work_struct *work) {
+	struct cdns_mhdp8501_device *mhdp = container_of(work,
+						     struct cdns_mhdp8501_device,
+						     hotplug_work.work);
+	enum drm_connector_status status =

cdns_mhdp8501_detect(mhdp);

Quoted 17 lines

Quoted 17 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c line 84

Quoted 13 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +13 -0

+
+	drm_bridge_hpd_notify(&mhdp->bridge, status);
+
+	if (status == connector_status_connected) {
+		/* Cable connected  */
+		DRM_INFO("HDMI/DP Cable Plug In\n");
+		enable_irq(mhdp->irq[IRQ_OUT]);
+	} else if (status == connector_status_disconnected) {
+		/* Cable Disconnected  */
+		DRM_INFO("HDMI/DP Cable Plug Out\n");
+		enable_irq(mhdp->irq[IRQ_IN]);
+	}
+}

You shouldn't play with the interrupt being enabled here: hotplug interrupts should always enabled.

Quoted 1 line

Quoted 1 line

If you can't for some reason, the reason should be documented in your

driver.

Quoted 1 line

Quoted 2 lines

iMX8MQ have two HPD interrupters, one for plugout and the other for plugin, because they could not be masked, so we have to enable one and

disable the other.

Quoted 1 line

I will add more comments here.

Right, but why do you need to enable and disable them? Do you get spurious interrupts?

Quoted 22 lines

Quoted 21 lines

Quoted 9 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +7 -0

+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);
+	ret = phy_init(mhdp->phy);
+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret) {
+		dev_err(dev, "Failed to initialize PHY: %d\n", ret);
+		goto clk_disable;

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c line 275

Quoted 10 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +10 -0

+	}
+
+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);
+	ret = phy_set_mode(mhdp->phy, phy_mode);
+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret) {
+		dev_err(dev, "Failed to configure PHY: %d\n", ret);
+		goto clk_disable;
+	}

Why do you need a shared mutex between the phy and HDMI controller?

Quoted 2 lines

Both PHY and HDMI controller could access to the HDMI firmware by mailbox, So add mutex to avoid race condition.

That should be handled at either the phy or mailbox level, not in your hdmi driver.

Quoted 37 lines

Quoted 29 lines

Quoted 29 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +28 -0

+static enum drm_mode_status
+cdns_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
+			       const struct drm_display_mode *mode,
+			       unsigned long long tmds_rate) {
+	struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
+	union phy_configure_opts phy_cfg;
+	int ret;
+
+	phy_cfg.hdmi.tmds_char_rate = tmds_rate;
+
+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);
+	ret = phy_validate(mhdp->phy, PHY_MODE_HDMI, 0, &phy_cfg);
+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret < 0)
+		return MODE_CLOCK_RANGE;
+
+	return MODE_OK;
+}
+
+static enum drm_mode_status
+cdns_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
+			    const struct drm_display_info *info,
+			    const struct drm_display_mode *mode) {
+	unsigned long long tmds_rate;
+
+	/* We don't support double-clocked and Interlaced modes */
+	if (mode->flags & DRM_MODE_FLAG_DBLCLK ||

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c line 584

Quoted 6 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c new file +6 -0

+	    mode->flags & DRM_MODE_FLAG_INTERLACE)
+		return MODE_BAD;
+
+	/* MAX support pixel clock rate 594MHz */
+	if (mode->clock > 594000)
+		return MODE_CLOCK_HIGH;

This needs to be in the tmds_char_rate_valid function

Quoted 2 lines

This clock rate check is covered by function tmds_char_rate_valid() It could be removed if keep function tmds_char_rate_valid() be called by

mode_valid.

Yeah, it's not something you should have to duplicate.

Quoted 12 lines

Quoted 11 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c line 430

Quoted 9 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +8 -0

+	if (mode->hdisplay > 3840)
+		return MODE_BAD_HVALUE;
+
+	if (mode->vdisplay > 2160)
+		return MODE_BAD_VVALUE;
+
+	tmds_rate = mode->clock * 1000ULL;
+	return cdns_hdmi_tmds_char_rate_valid(bridge, mode, tmds_rate);

It will already be called by the core so this is redundant.

Quoted 3 lines

mode_valid function is use to filter the mode list in drm_helper_probe_single_connector_modes(),
if function cdns_hdmi_tmds_char_rate_valid() is not called, unsupported

modes will in mode list.

It's probably something we should deal with in the core somehow. I'm not entirely sure how to reconcile drm_bridge_connector and the hdmi framework there, but we should at the very least provide a mode_valid helper for bridges.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

+static void cdns_hdmi_bridge_atomic_enable(struct drm_bridge

*bridge,

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +11 -0

+					   struct drm_bridge_state *old_state) {
+	struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
+	struct drm_atomic_state *state = old_state->base.state;
+	struct drm_connector *connector;
+	struct video_info *video = &mhdp->video_info;
+	struct drm_crtc_state *crtc_state;
+	struct drm_connector_state *conn_state;
+	struct drm_display_mode *mode = &mhdp->mode;
+	union phy_configure_opts phy_cfg;
+	int ret;
+

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c new file +8 -0

+	connector = drm_atomic_get_new_connector_for_encoder(state,
+							     bridge->encoder);
+	if (WARN_ON(!connector))
+		return;
+
+	mhdp->curr_conn = connector;
+
+	conn_state = drm_atomic_get_new_connector_state(state,

connector);

Quoted 4 lines

Quoted 4 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c

Quoted 4 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c new file +4 -0

+	if (WARN_ON(!conn_state))
+		return;
+
+	crtc_state = drm_atomic_get_new_crtc_state(state,

conn_state->crtc);

Quoted 11 lines

Quoted 11 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c line 494

Quoted 7 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +7 -0

+	if (WARN_ON(!crtc_state))
+		return;
+
+	video->color_fmt = conn_state->hdmi.output_format;
+	video->bpc = conn_state->hdmi.output_bpc;
+
+	drm_mode_copy(&mhdp->mode, &crtc_state->adjusted_mode);

Why do you need a copy of all these fields? You should pass the connector / bridge state around and not copy these fields.

Quoted 4 lines

OK, mode will be removed, and it will replace by drm_connector_state.

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	/* video mode check */
+	if (mode->clock == 0 || mode->hdisplay == 0 || mode->vdisplay ==

0)

Quoted 3 lines

Quoted 3 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+		return;

This should be checked in atomic_check, but I'm pretty sure it's

redundant.

Quoted 6 lines

OK, It will be removed.

Quoted 5 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c line 675

Quoted 5 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c new file +4 -0

+	dev_dbg(mhdp->dev, "Mode: %dx%dp%d\n",
+		mode->hdisplay, mode->vdisplay, mode->clock);
+
+

drm_atomic_helper_connector_hdmi_update_infoframes(connector,

Quoted 4 lines

Quoted 4 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c

Quoted 4 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c new file +4 -0

+state);
+
+	/* Line swapping */
+	cdns_mhdp_reg_write(&mhdp->base, LANES_CONFIG, 0x00400000

|

Quoted 44 lines

Quoted 9 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c line 510

Quoted 5 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +5 -0

+mhdp->lane_mapping);
+
+	mhdp->hdmi.char_rate = drm_hdmi_compute_mode_clock(mode,
+							   mhdp->video_info.bpc,
+							   mhdp->video_info.color_fmt);

The TMDS char rate is already available in the connector_state so there no need to recompute it.

Quoted 3 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c line 511

Quoted 1 line

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +1 -0

+	phy_cfg.hdmi.tmds_char_rate = mhdp->hdmi.char_rate;

And you shouldn't store a copy either.

OK, variable char_rate will be removed and will use the drm_connector_state-> hdmi.tmds_char_rate

Quoted 29 lines

Quoted 18 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +17 -0

+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);
+	ret = phy_configure(mhdp->phy, &phy_cfg);
+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret) {
+		dev_err(mhdp->dev, "%s: phy_configure() failed: %d\n",
+			__func__, ret);
+		return;
+	}
+
+	cdns_hdmi_sink_config(mhdp);
+
+	ret = cdns_hdmi_ctrl_init(mhdp);
+	if (ret < 0) {
+		dev_err(mhdp->dev, "%s, ret = %d\n", __func__, ret);
+		return;
+	}

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c

Quoted 10 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +10 -0

+
+	/* Config GCP */
+	if (mhdp->video_info.bpc == 8)
+		cdns_hdmi_disable_gcp(mhdp);
+	else
+		cdns_hdmi_enable_gcp(mhdp);
+
+	ret = cdns_hdmi_mode_config(mhdp, mode, &mhdp->video_info);
+	if (ret < 0) {
+		dev_err(mhdp->dev, "CDN_API_HDMITX_SetVic_blocking ret

= %d\n", ret);

Quoted 5 lines

Quoted 5 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c

Quoted 5 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +5 -0

+		return;
+	}
+}
+
+static int cdns_hdmi_bridge_clear_infoframe(struct drm_bridge

*bridge,

Quoted 5 lines

Quoted 5 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c line 291

Quoted 3 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +3 -0

+					    enum hdmi_infoframe_type type) {
+	return 0;
+}

Either implement it or don't, but an empty function is dead code.

Quoted 1 line

Must have function hdmi_clear_infoframe when set

DRM_BRIDGE_OP_HDMI

Quoted 1 line

flag in &drm_bridge->ops, otherwise the drm_bridge_connector_init() will

fail.

Quoted 2 lines

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tr
ee/drivers/gpu/drm/drm_bridge_connector.c?h=next-20240902#n419

That's a strong hint that you should implement it, not workaround it :)

Maxime

Quoted 68 lines · Sandor Yu

Quoted 36 lines

Quoted 36 lines

-----Original Message-----
From: dri-devel <[email protected]> On Behalf Of Maxime Ripard
Sent: 2024年7月2日 21:25
To: Sandor Yu <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; Laurent Pinchart <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; dl-linux-imx <[email protected]>; Oliver Brown <[email protected]>; [email protected]; [email protected]
Subject: [EXT] Re: [PATCH v16 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

Hi,

There's still the scrambler issue we discussed on v15, but I have some more comments.

Quoted 10 lines · Sandor Yu

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +10 -0

+enum drm_connector_status cdns_mhdp8501_detect(struct
+cdns_mhdp8501_device *mhdp) {
+	u8 hpd = 0xf;
+
+	hpd = cdns_mhdp8501_read_hpd(mhdp);
+	if (hpd == 1)
+		return connector_status_connected;
+	else if (hpd == 0)
+		return connector_status_disconnected;
+

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +8 -0

+	dev_warn(mhdp->dev, "Unknown cable status, hdp=%u\n", hpd);
+	return connector_status_unknown; }
+
+static void hotplug_work_func(struct work_struct *work) {
+	struct cdns_mhdp8501_device *mhdp = container_of(work,
+						     struct cdns_mhdp8501_device,
+						     hotplug_work.work);
+	enum drm_connector_status status =

cdns_mhdp8501_detect(mhdp);

Quoted 17 lines

Quoted 17 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c line 84

Quoted 13 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +13 -0

+
+	drm_bridge_hpd_notify(&mhdp->bridge, status);
+
+	if (status == connector_status_connected) {
+		/* Cable connected  */
+		DRM_INFO("HDMI/DP Cable Plug In\n");
+		enable_irq(mhdp->irq[IRQ_OUT]);
+	} else if (status == connector_status_disconnected) {
+		/* Cable Disconnected  */
+		DRM_INFO("HDMI/DP Cable Plug Out\n");
+		enable_irq(mhdp->irq[IRQ_IN]);
+	}
+}

You shouldn't play with the interrupt being enabled here: hotplug interrupts should always enabled.

Quoted 1 line

Quoted 1 line

If you can't for some reason, the reason should be documented in your

driver.

Quoted 3 lines

iMX8MQ have two HPD interrupters, one for plugout and the other for plugin, because they could not be masked, so we have to enable one and

Quoted 5 lines

disable the other.

Quoted 1 line

I will add more comments here.

Right, but why do you need to enable and disable them? Do you get spurious interrupts?

They don't have status registers and cannot be masked. If they are not disabled, they will continuously generate interrupts. Therefore, I have to disable one and enable the other.

Quoted 20 lines

Quoted 20 lines

Quoted 19 lines

Quoted 7 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +7 -0

+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);
+	ret = phy_init(mhdp->phy);
+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret) {
+		dev_err(dev, "Failed to initialize PHY: %d\n", ret);
+		goto clk_disable;

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c line 275

Quoted 10 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +10 -0

+	}
+
+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);
+	ret = phy_set_mode(mhdp->phy, phy_mode);
+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret) {
+		dev_err(dev, "Failed to configure PHY: %d\n", ret);
+		goto clk_disable;
+	}

Why do you need a shared mutex between the phy and HDMI controller?

Quoted 5 lines

Quoted 2 lines

Both PHY and HDMI controller could access to the HDMI firmware by mailbox, So add mutex to avoid race condition.

That should be handled at either the phy or mailbox level, not in your hdmi driver.

In both HDMI driver and PHY driver, every mailbox access had protected by its owns mutex. However, this mutex can only protect each mailbox access within their respective drivers, and it cannot provide protection for access between the HDMI and PHY drivers.

The PHY driver only provides two API functions, and these functions are only called in the HDMI driver. Therefore, when accessing these functions, we use a mutex to protect them. This ensures that mailbox access is protected across different PHY and HDMI drivers.

Quoted 58 lines

Quoted 37 lines

Quoted 29 lines

Quoted 29 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +28 -0

+static enum drm_mode_status
+cdns_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
+			       const struct drm_display_mode *mode,
+			       unsigned long long tmds_rate) {
+	struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
+	union phy_configure_opts phy_cfg;
+	int ret;
+
+	phy_cfg.hdmi.tmds_char_rate = tmds_rate;
+
+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);
+	ret = phy_validate(mhdp->phy, PHY_MODE_HDMI, 0, &phy_cfg);
+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret < 0)
+		return MODE_CLOCK_RANGE;
+
+	return MODE_OK;
+}
+
+static enum drm_mode_status
+cdns_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
+			    const struct drm_display_info *info,
+			    const struct drm_display_mode *mode) {
+	unsigned long long tmds_rate;
+
+	/* We don't support double-clocked and Interlaced modes */
+	if (mode->flags & DRM_MODE_FLAG_DBLCLK ||

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c line 584

Quoted 6 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c new file +6 -0

+	    mode->flags & DRM_MODE_FLAG_INTERLACE)
+		return MODE_BAD;
+
+	/* MAX support pixel clock rate 594MHz */
+	if (mode->clock > 594000)
+		return MODE_CLOCK_HIGH;

This needs to be in the tmds_char_rate_valid function

Quoted 2 lines

This clock rate check is covered by function tmds_char_rate_valid() It could be removed if keep function tmds_char_rate_valid() be called by

mode_valid.

Yeah, it's not something you should have to duplicate.

Quoted 12 lines

Quoted 11 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c line 430

Quoted 9 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +8 -0

+	if (mode->hdisplay > 3840)
+		return MODE_BAD_HVALUE;
+
+	if (mode->vdisplay > 2160)
+		return MODE_BAD_VVALUE;
+
+	tmds_rate = mode->clock * 1000ULL;
+	return cdns_hdmi_tmds_char_rate_valid(bridge, mode, tmds_rate);

It will already be called by the core so this is redundant.

Quoted 3 lines

mode_valid function is use to filter the mode list in drm_helper_probe_single_connector_modes(),
if function cdns_hdmi_tmds_char_rate_valid() is not called, unsupported

Quoted 6 lines

modes will in mode list.

It's probably something we should deal with in the core somehow. I'm not entirely sure how to reconcile drm_bridge_connector and the hdmi framework there, but we should at the very least provide a mode_valid helper for bridges.

I agree with that. In fact, I'm a bit confused about the current mode_valid and tmds_char_rate_valid functions. Ideally, we should find a way to make tmds_char_rate_valid also work for filtering out the mode list, rather than just during atomic_check.

Subject: Re: [EXT] Re: [PATCH v16 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

Quoted 12 lines · Sandor Yu

Quoted 12 lines · Sandor Yu

Quoted 11 lines

Quoted 11 lines

-----Original Message-----
From: dri-devel <[email protected]> On Behalf Of Maxime Ripard
Sent: 2024年7月2日 21:25
To: Sandor Yu <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; Laurent Pinchart <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected];

[email protected];

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

[email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; dl-linux-imx <[email protected]>; Oliver Brown <[email protected]>; [email protected]; [email protected]
Subject: [EXT] Re: [PATCH v16 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

Hi,

There's still the scrambler issue we discussed on v15, but I have some more comments.

On Tue, Jul 02, 2024 at 08:22:36PM GMT, Sandor Yu wrote:

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c

Quoted 11 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +11 -0

+enum drm_connector_status cdns_mhdp8501_detect(struct
+cdns_mhdp8501_device *mhdp) {
+	u8 hpd = 0xf;
+
+	hpd = cdns_mhdp8501_read_hpd(mhdp);
+	if (hpd == 1)
+		return connector_status_connected;
+	else if (hpd == 0)
+		return connector_status_disconnected;
+
+	dev_warn(mhdp->dev, "Unknown cable status, hdp=%u\n",

hpd);

Quoted 38 lines

Quoted 37 lines

Quoted 7 lines

Quoted 7 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c

Quoted 7 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +7 -0

+	return connector_status_unknown; }
+
+static void hotplug_work_func(struct work_struct *work) {
+	struct cdns_mhdp8501_device *mhdp = container_of(work,
+						     struct cdns_mhdp8501_device,
+						     hotplug_work.work);
+	enum drm_connector_status status =

cdns_mhdp8501_detect(mhdp);

Quoted 17 lines

Quoted 17 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c line 84

Quoted 13 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +13 -0

+
+	drm_bridge_hpd_notify(&mhdp->bridge, status);
+
+	if (status == connector_status_connected) {
+		/* Cable connected  */
+		DRM_INFO("HDMI/DP Cable Plug In\n");
+		enable_irq(mhdp->irq[IRQ_OUT]);
+	} else if (status == connector_status_disconnected) {
+		/* Cable Disconnected  */
+		DRM_INFO("HDMI/DP Cable Plug Out\n");
+		enable_irq(mhdp->irq[IRQ_IN]);
+	}
+}

You shouldn't play with the interrupt being enabled here:
hotplug interrupts should always enabled.

Quoted 2 lines

Quoted 2 lines

If you can't for some reason, the reason should be documented in your

driver.

Quoted 4 lines

iMX8MQ have two HPD interrupters, one for plugout and the other for plugin, because they could not be masked, so we have to enable one and

disable the other.

Quoted 1 line

I will add more comments here.

Right, but why do you need to enable and disable them? Do you get spurious interrupts?

Quoted 2 lines

They don't have status registers and cannot be masked. If they are not disabled, they will continuously generate interrupts. Therefore, I have to

disable one and enable the other.

Sorry, I still don't get it. How can it be useful to detect hotplug interrupts if it constantly sends spurious interrupts when it's enabled?

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +11 -0

+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);
+	ret = phy_init(mhdp->phy);
+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret) {
+		dev_err(dev, "Failed to initialize PHY: %d\n", ret);
+		goto clk_disable;
+	}
+
+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c line 275

Quoted 6 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +6 -0

+	ret = phy_set_mode(mhdp->phy, phy_mode);
+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret) {
+		dev_err(dev, "Failed to configure PHY: %d\n", ret);
+		goto clk_disable;
+	}

Why do you need a shared mutex between the phy and HDMI

controller?

Quoted 12 lines

Quoted 6 lines

Quoted 3 lines

Both PHY and HDMI controller could access to the HDMI firmware by mailbox, So add mutex to avoid race condition.

That should be handled at either the phy or mailbox level, not in your hdmi driver.

In both HDMI driver and PHY driver, every mailbox access had protected by its owns mutex. However, this mutex can only protect each mailbox access within their respective drivers, and it cannot provide protection for access between the HDMI and PHY drivers.

Quoted 4 lines

The PHY driver only provides two API functions, and these functions are only called in the HDMI driver. Therefore, when accessing these functions, we use a mutex to protect them. This ensures that mailbox access is protected across different PHY and HDMI drivers.

It's really about abstraction. You're using a publicly defined API, and change the semantics for your driver only, and that's not ok.

Why can't the mailbox driver itself serialize the accesses from any user, HDMI and PHY drivers included?

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c

Quoted 6 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +5 -0

+static enum drm_mode_status
+cdns_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
+			       const struct drm_display_mode *mode,
+			       unsigned long long tmds_rate) {
+	struct cdns_mhdp8501_device *mhdp =

bridge->driver_private;

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +8 -0

+	union phy_configure_opts phy_cfg;
+	int ret;
+
+	phy_cfg.hdmi.tmds_char_rate = tmds_rate;
+
+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);
+	ret = phy_validate(mhdp->phy, PHY_MODE_HDMI, 0,

&phy_cfg);

Quoted 54 lines

Quoted 47 lines

Quoted 23 lines

Quoted 15 lines

Quoted 15 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +15 -0

+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret < 0)
+		return MODE_CLOCK_RANGE;
+
+	return MODE_OK;
+}
+
+static enum drm_mode_status
+cdns_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
+			    const struct drm_display_info *info,
+			    const struct drm_display_mode *mode) {
+	unsigned long long tmds_rate;
+
+	/* We don't support double-clocked and Interlaced modes */
+	if (mode->flags & DRM_MODE_FLAG_DBLCLK ||

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c line 584

Quoted 6 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c new file +6 -0

+	    mode->flags & DRM_MODE_FLAG_INTERLACE)
+		return MODE_BAD;
+
+	/* MAX support pixel clock rate 594MHz */
+	if (mode->clock > 594000)
+		return MODE_CLOCK_HIGH;

This needs to be in the tmds_char_rate_valid function

Quoted 3 lines

This clock rate check is covered by function tmds_char_rate_valid() It could be removed if keep function tmds_char_rate_valid() be called by

mode_valid.

Yeah, it's not something you should have to duplicate.

Quoted 13 lines

Quoted 12 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c

Quoted 10 lines

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file +9 -0

+	if (mode->hdisplay > 3840)
+		return MODE_BAD_HVALUE;
+
+	if (mode->vdisplay > 2160)
+		return MODE_BAD_VVALUE;
+
+	tmds_rate = mode->clock * 1000ULL;
+	return cdns_hdmi_tmds_char_rate_valid(bridge, mode,
+tmds_rate);

It will already be called by the core so this is redundant.

Quoted 4 lines

mode_valid function is use to filter the mode list in drm_helper_probe_single_connector_modes(),
if function cdns_hdmi_tmds_char_rate_valid() is not called, unsupported

Quoted 6 lines

modes will in mode list.

It's probably something we should deal with in the core somehow. I'm not entirely sure how to reconcile drm_bridge_connector and the hdmi framework there, but we should at the very least provide a mode_valid helper for bridges.

Quoted 4 lines

I agree with that. In fact, I'm a bit confused about the current mode_valid and tmds_char_rate_valid functions. Ideally, we should find a way to make tmds_char_rate_valid also work for filtering out the mode list, rather than just during atomic_check.

Yeah, definitely. The way we did so on vc4 for example was to compute the rate for a 8bpc, RGB, output and try to validate that. I think it would be reasonable to start with that.

Maxime

Quoted 89 lines

Subject: Re: [EXT] Re: [PATCH v16 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

Quoted 12 lines · Sandor Yu

Quoted 12 lines · Sandor Yu

Quoted 11 lines

Quoted 11 lines

-----Original Message-----
From: dri-devel <[email protected]> On Behalf Of Maxime Ripard
Sent: 2024年7月2日 21:25
To: Sandor Yu <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; Laurent Pinchart <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected];

[email protected];

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

[email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; dl-linux-imx <[email protected]>; Oliver Brown <[email protected]>; [email protected]; [email protected]
Subject: [EXT] Re: [PATCH v16 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

Hi,

There's still the scrambler issue we discussed on v15, but I have some more comments.

On Tue, Jul 02, 2024 at 08:22:36PM GMT, Sandor Yu wrote:

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Review

Quoted 11 lines

(quoted diff) +11 -0

+enum drm_connector_status cdns_mhdp8501_detect(struct
+cdns_mhdp8501_device *mhdp) {
+   u8 hpd = 0xf;
+
+   hpd = cdns_mhdp8501_read_hpd(mhdp);
+   if (hpd == 1)
+           return connector_status_connected;
+   else if (hpd == 0)
+           return connector_status_disconnected;
+
+   dev_warn(mhdp->dev, "Unknown cable status, hdp=%u\n",

hpd);

Quoted 32 lines

Quoted 32 lines

Quoted 7 lines

Quoted 7 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+   return connector_status_unknown; }
+
+static void hotplug_work_func(struct work_struct *work) {
+   struct cdns_mhdp8501_device *mhdp = container_of(work,
+                                                struct cdns_mhdp8501_device,
+                                                hotplug_work.work);
+   enum drm_connector_status status =

cdns_mhdp8501_detect(mhdp);

Quoted 17 lines

Quoted 17 lines

Review

Quoted 13 lines

(quoted diff) +13 -0

+
+   drm_bridge_hpd_notify(&mhdp->bridge, status);
+
+   if (status == connector_status_connected) {
+           /* Cable connected  */
+           DRM_INFO("HDMI/DP Cable Plug In\n");
+           enable_irq(mhdp->irq[IRQ_OUT]);
+   } else if (status == connector_status_disconnected) {
+           /* Cable Disconnected  */
+           DRM_INFO("HDMI/DP Cable Plug Out\n");
+           enable_irq(mhdp->irq[IRQ_IN]);
+   }
+}

You shouldn't play with the interrupt being enabled here:
hotplug interrupts should always enabled.

Quoted 2 lines

Quoted 2 lines

If you can't for some reason, the reason should be documented in your

driver.

Quoted 4 lines

iMX8MQ have two HPD interrupters, one for plugout and the other for plugin, because they could not be masked, so we have to enable one and

Quoted 8 lines

Quoted 5 lines

disable the other.

Quoted 1 line

I will add more comments here.

Right, but why do you need to enable and disable them? Do you get spurious interrupts?

They don't have status registers and cannot be masked. If they are not disabled, they will continuously generate interrupts. Therefore, I have to

disable one and enable the other.

Quoted 2 lines

Sorry, I still don't get it. How can it be useful to detect hotplug interrupts if it constantly sends spurious interrupts when it's enabled?

Yes, this interrupt is different from a normal one; it's likely a design flaw. For instance, the plugin interrupt is continuously generated as long as the cable is plugged in, only stopping when the cable is unplugged.
That's why two interrupts are used to detect cable plugout and plugin separately. If interrupts aren't used, the only option is polling.

Quoted 39 lines

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 19 lines

(quoted diff) +17 -0

+   /* Mailbox protect for HDMI PHY access */
+   mutex_lock(&mhdp->mbox_mutex);
+   ret = phy_init(mhdp->phy);
+   mutex_unlock(&mhdp->mbox_mutex);
+   if (ret) {
+           dev_err(dev, "Failed to initialize PHY: %d\n", ret);
+           goto clk_disable;
+   }
+
+   /* Mailbox protect for HDMI PHY access */
+   mutex_lock(&mhdp->mbox_mutex);
+   ret = phy_set_mode(mhdp->phy, phy_mode);
+   mutex_unlock(&mhdp->mbox_mutex);
+   if (ret) {
+           dev_err(dev, "Failed to configure PHY: %d\n", ret);
+           goto clk_disable;
+   }

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Why do you need a shared mutex between the phy and HDMI

controller?

Quoted 12 lines

Quoted 6 lines

Quoted 3 lines

Both PHY and HDMI controller could access to the HDMI firmware by mailbox, So add mutex to avoid race condition.

That should be handled at either the phy or mailbox level, not in your hdmi driver.

In both HDMI driver and PHY driver, every mailbox access had protected by its owns mutex. However, this mutex can only protect each mailbox access within their respective drivers, and it cannot provide protection for access between the HDMI and PHY drivers.

Quoted 4 lines

The PHY driver only provides two API functions, and these functions are only called in the HDMI driver. Therefore, when accessing these functions, we use a mutex to protect them. This ensures that mailbox access is protected across different PHY and HDMI drivers.

Quoted 6 lines

It's really about abstraction. You're using a publicly defined API, and change the semantics for your driver only, and that's not ok.

Why can't the mailbox driver itself serialize the accesses from any user, HDMI and PHY drivers included?

In the current code implementation, cdns-mhdp-helper.c isn't a standalone driver but rather a library. It provides fundamental mailbox access functions and basic register read/write operations that rely on the mailbox. These functions are highly reusable across MHDP8501 and MHDP8546 and can be leveraged by future MHDP versions.

However, most MHDP firmware interactions involve a sequence of mailbox accesses, including sending commands and receiving firmware responses. These commands constitute a significant portion of all firmware interactions, encompassing operations like EDID reading and DP link training. Unfortunately, these commands cannot be reused between MHDP8501 and MHDP8546.

Creating a dedicated mailbox driver with its own mutex would effectively address race conditions. However, this would necessitate relocating all mailbox-related functions to this driver. Including these non-reusable functions would defeat the purpose of code reuse.

To strike a balance between code reusability and race condition mitigation, adding mutexes to PHY access functions seems like a reasonable solution.

Sandor

Quoted 78 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Review

Quoted 6 lines

(quoted diff) +5 -0

+static enum drm_mode_status
+cdns_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
+                          const struct drm_display_mode *mode,
+                          unsigned long long tmds_rate) {
+   struct cdns_mhdp8501_device *mhdp =

bridge->driver_private;

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+   union phy_configure_opts phy_cfg;
+   int ret;
+
+   phy_cfg.hdmi.tmds_char_rate = tmds_rate;
+
+   /* Mailbox protect for HDMI PHY access */
+   mutex_lock(&mhdp->mbox_mutex);
+   ret = phy_validate(mhdp->phy, PHY_MODE_HDMI, 0,

&phy_cfg);

Quoted 54 lines

Quoted 47 lines

Quoted 23 lines

Quoted 15 lines

Quoted 15 lines

(quoted diff) +15 -0

+   mutex_unlock(&mhdp->mbox_mutex);
+   if (ret < 0)
+           return MODE_CLOCK_RANGE;
+
+   return MODE_OK;
+}
+
+static enum drm_mode_status
+cdns_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
+                       const struct drm_display_info *info,
+                       const struct drm_display_mode *mode) {
+   unsigned long long tmds_rate;
+
+   /* We don't support double-clocked and Interlaced modes */
+   if (mode->flags & DRM_MODE_FLAG_DBLCLK ||

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+       mode->flags & DRM_MODE_FLAG_INTERLACE)
+           return MODE_BAD;
+
+   /* MAX support pixel clock rate 594MHz */
+   if (mode->clock > 594000)
+           return MODE_CLOCK_HIGH;

This needs to be in the tmds_char_rate_valid function

Quoted 3 lines

This clock rate check is covered by function tmds_char_rate_valid() It could be removed if keep function tmds_char_rate_valid() be called by

mode_valid.

Yeah, it's not something you should have to duplicate.

Quoted 13 lines

Quoted 12 lines

Review

Quoted 10 lines

(quoted diff) +9 -0

+   if (mode->hdisplay > 3840)
+           return MODE_BAD_HVALUE;
+
+   if (mode->vdisplay > 2160)
+           return MODE_BAD_VVALUE;
+
+   tmds_rate = mode->clock * 1000ULL;
+   return cdns_hdmi_tmds_char_rate_valid(bridge, mode,
+tmds_rate);

It will already be called by the core so this is redundant.

Quoted 4 lines

mode_valid function is use to filter the mode list in drm_helper_probe_single_connector_modes(),
if function cdns_hdmi_tmds_char_rate_valid() is not called, unsupported

Quoted 6 lines

modes will in mode list.

It's probably something we should deal with in the core somehow. I'm not entirely sure how to reconcile drm_bridge_connector and the hdmi framework there, but we should at the very least provide a mode_valid helper for bridges.

Quoted 4 lines

I agree with that. In fact, I'm a bit confused about the current mode_valid and tmds_char_rate_valid functions. Ideally, we should find a way to make tmds_char_rate_valid also work for filtering out the mode list, rather than just during atomic_check.

Yeah, definitely. The way we did so on vc4 for example was to compute the rate for a 8bpc, RGB, output and try to validate that. I think it would be reasonable to start with that.

Quoted 48 lines

Quoted 48 lines

Subject: Re: [EXT] Re: [PATCH v16 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

Quoted 13 lines · Sandor Yu

Quoted 5 lines · Sandor Yu

Quoted 4 lines

Quoted 4 lines

-----Original Message-----
From: dri-devel <[email protected]> On Behalf Of Maxime Ripard
Sent: 2024年7月2日 21:25

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

To: Sandor Yu <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; Laurent Pinchart <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected];
[email protected]; [email protected]; [email protected]; [email protected];

[email protected];

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

[email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; dl-linux-imx <[email protected]>; Oliver Brown <[email protected]>; [email protected]; [email protected]
Subject: [EXT] Re: [PATCH v16 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

Hi,

There's still the scrambler issue we discussed on v15, but I have some more comments.

On Tue, Jul 02, 2024 at 08:22:36PM GMT, Sandor Yu wrote:

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Review

Quoted 11 lines

(quoted diff) +11 -0

+enum drm_connector_status cdns_mhdp8501_detect(struct
+cdns_mhdp8501_device *mhdp) {
+   u8 hpd = 0xf;
+
+   hpd = cdns_mhdp8501_read_hpd(mhdp);
+   if (hpd == 1)
+           return connector_status_connected;
+   else if (hpd == 0)
+           return connector_status_disconnected;
+
+   dev_warn(mhdp->dev, "Unknown cable status, hdp=%u\n",

hpd);

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+   return connector_status_unknown; }
+
+static void hotplug_work_func(struct work_struct *work) {
+   struct cdns_mhdp8501_device *mhdp = container_of(work,
+                                                struct

cdns_mhdp8501_device,

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

+

hotplug_work.work);

Quoted 37 lines

Quoted 37 lines

Quoted 27 lines

Quoted 27 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

+ enum drm_connector_status status =

cdns_mhdp8501_detect(mhdp);

Quoted 17 lines

Quoted 17 lines

Review

Quoted 13 lines

(quoted diff) +13 -0

+
+   drm_bridge_hpd_notify(&mhdp->bridge, status);
+
+   if (status == connector_status_connected) {
+           /* Cable connected  */
+           DRM_INFO("HDMI/DP Cable Plug In\n");
+           enable_irq(mhdp->irq[IRQ_OUT]);
+   } else if (status == connector_status_disconnected) {
+           /* Cable Disconnected  */
+           DRM_INFO("HDMI/DP Cable Plug Out\n");
+           enable_irq(mhdp->irq[IRQ_IN]);
+   }
+}

You shouldn't play with the interrupt being enabled here:
hotplug interrupts should always enabled.

Quoted 2 lines

Quoted 2 lines

If you can't for some reason, the reason should be documented in your

driver.

Quoted 4 lines

iMX8MQ have two HPD interrupters, one for plugout and the other for plugin, because they could not be masked, so we have to enable one and

disable the other.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

I will add more comments here.

Right, but why do you need to enable and disable them? Do you get spurious interrupts?

They don't have status registers and cannot be masked. If they are not disabled, they will continuously generate interrupts. Therefore, I have to

disable one and enable the other.

Quoted 7 lines

Quoted 2 lines

Sorry, I still don't get it. How can it be useful to detect hotplug interrupts if it constantly sends spurious interrupts when it's enabled?

Yes, this interrupt is different from a normal one; it's likely a design flaw. For instance, the plugin interrupt is continuously generated as long as the cable is plugged in, only stopping when the cable is unplugged. That's why two interrupts are used to detect cable plugout and plugin

separately.

Quoted 1 line

If interrupts aren't used, the only option is polling.

I think I've seen such strange design on other platforms, level interrupt for HPD, which needs to be disabled via disable_irq().

Quoted 34 lines

Quoted 34 lines

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 19 lines

(quoted diff) +17 -0

+   /* Mailbox protect for HDMI PHY access */
+   mutex_lock(&mhdp->mbox_mutex);
+   ret = phy_init(mhdp->phy);
+   mutex_unlock(&mhdp->mbox_mutex);
+   if (ret) {
+           dev_err(dev, "Failed to initialize PHY: %d\n", ret);
+           goto clk_disable;
+   }
+
+   /* Mailbox protect for HDMI PHY access */
+   mutex_lock(&mhdp->mbox_mutex);
+   ret = phy_set_mode(mhdp->phy, phy_mode);
+   mutex_unlock(&mhdp->mbox_mutex);
+   if (ret) {
+           dev_err(dev, "Failed to configure PHY: %d\n", ret);
+           goto clk_disable;
+   }

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Why do you need a shared mutex between the phy and HDMI

controller?

Quoted 12 lines

Quoted 6 lines

Quoted 3 lines

Both PHY and HDMI controller could access to the HDMI firmware by mailbox, So add mutex to avoid race condition.

That should be handled at either the phy or mailbox level, not in your hdmi driver.

In both HDMI driver and PHY driver, every mailbox access had protected by its owns mutex. However, this mutex can only protect each mailbox access within their respective drivers, and it cannot provide protection for access between the HDMI and PHY drivers.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

The PHY driver only provides two API functions, and these functions are only called in the HDMI driver. Therefore, when accessing these functions, we use a mutex to protect them. This ensures that mailbox access is protected across different PHY and HDMI

drivers.

Quoted 9 lines

Quoted 7 lines

It's really about abstraction. You're using a publicly defined API, and change the semantics for your driver only, and that's not ok.

Why can't the mailbox driver itself serialize the accesses from any user, HDMI and PHY drivers included?

In the current code implementation, cdns-mhdp-helper.c isn't a standalone

driver but rather a library.

Quoted 1 line

It provides fundamental mailbox access functions and basic register

read/write operations that rely on the mailbox.

Quoted 1 line

These functions are highly reusable across MHDP8501 and MHDP8546 and

can be leveraged by future MHDP versions.

Quoted 2 lines

However, most MHDP firmware interactions involve a sequence of mailbox

accesses, including sending commands and receiving firmware responses.

Quoted 1 line

These commands constitute a significant portion of all firmware interactions,

encompassing operations like EDID reading and DP link training.

Quoted 1 line

Unfortunately, these commands cannot be reused between MHDP8501 and

MHDP8546.

Quoted 2 lines

Creating a dedicated mailbox driver with its own mutex would effectively

address race conditions.

Quoted 1 line

However, this would necessitate relocating all mailbox-related functions to

this driver.

Quoted 1 line

Including these non-reusable functions would defeat the purpose of code

reuse.

Quoted 2 lines

To strike a balance between code reusability and race condition mitigation,

adding mutexes to PHY access functions seems like a reasonable solution.

You seem to have two kinds of scenarios when talking to MHDP: just cdns_mhdp_mailbox_send(), no response needed and then the cdns_mhdp_mailbox_send() / cdns_mhdp_mailbox_recv_header() / cdns_mhdp_mailbox_recv_data() sequence. Extract those + the mutex access to separate functions, add a mutex to those sequences and use them as a high-level API for your HDMI and PHY drivers.

Adding mutexes around phy_foo() calls doesn't look like a proper solution _at_ _all_.

Quoted 6 lines

Sandor

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

+static enum drm_mode_status
+cdns_hdmi_tmds_char_rate_valid(const struct drm_bridge

*bridge,

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

+ const struct drm_display_mode

*mode,

Quoted 12 lines

Quoted 12 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+                          unsigned long long tmds_rate) {
+   struct cdns_mhdp8501_device *mhdp =

bridge->driver_private;

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+   union phy_configure_opts phy_cfg;
+   int ret;
+
+   phy_cfg.hdmi.tmds_char_rate = tmds_rate;
+
+   /* Mailbox protect for HDMI PHY access */
+   mutex_lock(&mhdp->mbox_mutex);
+   ret = phy_validate(mhdp->phy, PHY_MODE_HDMI, 0,

&phy_cfg);

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Review

Quoted 11 lines

(quoted diff) +11 -0

+   mutex_unlock(&mhdp->mbox_mutex);
+   if (ret < 0)
+           return MODE_CLOCK_RANGE;
+
+   return MODE_OK;
+}
+
+static enum drm_mode_status
+cdns_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
+                       const struct drm_display_info *info,
+                       const struct drm_display_mode

*mode) {

Quoted 48 lines

Quoted 48 lines

Quoted 47 lines

Quoted 36 lines

Quoted 12 lines

Quoted 4 lines

Quoted 4 lines

(quoted diff) +4 -0

+   unsigned long long tmds_rate;
+
+   /* We don't support double-clocked and Interlaced modes */
+   if (mode->flags & DRM_MODE_FLAG_DBLCLK ||

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+       mode->flags & DRM_MODE_FLAG_INTERLACE)
+           return MODE_BAD;
+
+   /* MAX support pixel clock rate 594MHz */
+   if (mode->clock > 594000)
+           return MODE_CLOCK_HIGH;

This needs to be in the tmds_char_rate_valid function

Quoted 3 lines

This clock rate check is covered by function tmds_char_rate_valid() It could be removed if keep function tmds_char_rate_valid() be called by

mode_valid.

Yeah, it's not something you should have to duplicate.

Quoted 13 lines

Quoted 12 lines

Review

Quoted 10 lines

(quoted diff) +9 -0

+   if (mode->hdisplay > 3840)
+           return MODE_BAD_HVALUE;
+
+   if (mode->vdisplay > 2160)
+           return MODE_BAD_VVALUE;
+
+   tmds_rate = mode->clock * 1000ULL;
+   return cdns_hdmi_tmds_char_rate_valid(bridge, mode,
+tmds_rate);

It will already be called by the core so this is redundant.

Quoted 4 lines

mode_valid function is use to filter the mode list in drm_helper_probe_single_connector_modes(),
if function cdns_hdmi_tmds_char_rate_valid() is not called, unsupported

Quoted 6 lines

modes will in mode list.

It's probably something we should deal with in the core somehow. I'm not entirely sure how to reconcile drm_bridge_connector and the hdmi framework there, but we should at the very least provide a mode_valid helper for bridges.

I agree with that. In fact, I'm a bit confused about the current mode_valid and tmds_char_rate_valid functions. Ideally, we should find a way to make tmds_char_rate_valid also work for filtering out the mode list, rather than just during atomic_check.

Quoted 3 lines

Quoted 3 lines

Yeah, definitely. The way we did so on vc4 for example was to compute the rate for a 8bpc, RGB, output and try to validate that. I think it would be reasonable to start with that.

+1, please extract this code as a helper. You can even submit it separately, reworking sun4i and vc4 to use a new helper.

--
With best wishes
Dmitry

Quoted 187 lines · Sandor Yu

Quoted 48 lines

Quoted 48 lines

Subject: Re: [EXT] Re: [PATCH v16 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

Quoted 13 lines · Sandor Yu

Quoted 5 lines · Sandor Yu

Quoted 4 lines

Quoted 4 lines

-----Original Message-----
From: dri-devel <[email protected]> On Behalf Of Maxime Ripard
Sent: 2024年7月2日 21:25

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

To: Sandor Yu <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; Laurent Pinchart <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected];
[email protected]; [email protected]; [email protected]; [email protected];

[email protected];

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

[email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; dl-linux-imx <[email protected]>; Oliver Brown <[email protected]>; [email protected]; [email protected]
Subject: [EXT] Re: [PATCH v16 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

Hi,

There's still the scrambler issue we discussed on v15, but I have some more comments.

On Tue, Jul 02, 2024 at 08:22:36PM GMT, Sandor Yu wrote:

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Review

Quoted 11 lines

(quoted diff) +11 -0

+enum drm_connector_status cdns_mhdp8501_detect(struct
+cdns_mhdp8501_device *mhdp) {
+   u8 hpd = 0xf;
+
+   hpd = cdns_mhdp8501_read_hpd(mhdp);
+   if (hpd == 1)
+           return connector_status_connected;
+   else if (hpd == 0)
+           return connector_status_disconnected;
+
+   dev_warn(mhdp->dev, "Unknown cable status, hdp=%u\n",

hpd);

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+   return connector_status_unknown; }
+
+static void hotplug_work_func(struct work_struct *work) {
+   struct cdns_mhdp8501_device *mhdp = container_of(work,
+                                                struct

cdns_mhdp8501_device,

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

+

hotplug_work.work);

Quoted 37 lines

Quoted 37 lines

Quoted 27 lines

Quoted 27 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

+ enum drm_connector_status status =

cdns_mhdp8501_detect(mhdp);

Quoted 17 lines

Quoted 17 lines

Review

Quoted 13 lines

(quoted diff) +13 -0

+
+   drm_bridge_hpd_notify(&mhdp->bridge, status);
+
+   if (status == connector_status_connected) {
+           /* Cable connected  */
+           DRM_INFO("HDMI/DP Cable Plug In\n");
+           enable_irq(mhdp->irq[IRQ_OUT]);
+   } else if (status == connector_status_disconnected) {
+           /* Cable Disconnected  */
+           DRM_INFO("HDMI/DP Cable Plug Out\n");
+           enable_irq(mhdp->irq[IRQ_IN]);
+   }
+}

You shouldn't play with the interrupt being enabled here:
hotplug interrupts should always enabled.

Quoted 2 lines

Quoted 2 lines

If you can't for some reason, the reason should be documented in your

driver.

Quoted 4 lines

iMX8MQ have two HPD interrupters, one for plugout and the other for plugin, because they could not be masked, so we have to enable one and

disable the other.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

I will add more comments here.

Right, but why do you need to enable and disable them? Do you get spurious interrupts?

They don't have status registers and cannot be masked. If they are not disabled, they will continuously generate interrupts. Therefore, I have to

disable one and enable the other.

Quoted 7 lines

Quoted 2 lines

Sorry, I still don't get it. How can it be useful to detect hotplug interrupts if it constantly sends spurious interrupts when it's enabled?

Yes, this interrupt is different from a normal one; it's likely a design flaw. For instance, the plugin interrupt is continuously generated as long as the cable is plugged in, only stopping when the cable is unplugged. That's why two interrupts are used to detect cable plugout and plugin

separately.

Quoted 1 line

If interrupts aren't used, the only option is polling.

I think I've seen such strange design on other platforms, level interrupt for HPD, which needs to be disabled via disable_irq().

Quoted 34 lines

Quoted 34 lines

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 19 lines

(quoted diff) +17 -0

+   /* Mailbox protect for HDMI PHY access */
+   mutex_lock(&mhdp->mbox_mutex);
+   ret = phy_init(mhdp->phy);
+   mutex_unlock(&mhdp->mbox_mutex);
+   if (ret) {
+           dev_err(dev, "Failed to initialize PHY: %d\n", ret);
+           goto clk_disable;
+   }
+
+   /* Mailbox protect for HDMI PHY access */
+   mutex_lock(&mhdp->mbox_mutex);
+   ret = phy_set_mode(mhdp->phy, phy_mode);
+   mutex_unlock(&mhdp->mbox_mutex);
+   if (ret) {
+           dev_err(dev, "Failed to configure PHY: %d\n", ret);
+           goto clk_disable;
+   }

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Why do you need a shared mutex between the phy and HDMI

controller?

Quoted 12 lines

Quoted 6 lines

Quoted 3 lines

Both PHY and HDMI controller could access to the HDMI firmware by mailbox, So add mutex to avoid race condition.

That should be handled at either the phy or mailbox level, not in your hdmi driver.

In both HDMI driver and PHY driver, every mailbox access had protected by its owns mutex. However, this mutex can only protect each mailbox access within their respective drivers, and it cannot provide protection for access between the HDMI and PHY drivers.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

The PHY driver only provides two API functions, and these functions are only called in the HDMI driver. Therefore, when accessing these functions, we use a mutex to protect them. This ensures that mailbox access is protected across different PHY and HDMI

drivers.

Quoted 9 lines

Quoted 7 lines

It's really about abstraction. You're using a publicly defined API, and change the semantics for your driver only, and that's not ok.

Why can't the mailbox driver itself serialize the accesses from any user, HDMI and PHY drivers included?

In the current code implementation, cdns-mhdp-helper.c isn't a standalone

driver but rather a library.

Quoted 1 line

It provides fundamental mailbox access functions and basic register

read/write operations that rely on the mailbox.

Quoted 1 line

These functions are highly reusable across MHDP8501 and MHDP8546 and

can be leveraged by future MHDP versions.

Quoted 2 lines

However, most MHDP firmware interactions involve a sequence of mailbox

accesses, including sending commands and receiving firmware responses.

Quoted 1 line

These commands constitute a significant portion of all firmware interactions,

encompassing operations like EDID reading and DP link training.

Quoted 1 line

Unfortunately, these commands cannot be reused between MHDP8501 and

MHDP8546.

Quoted 2 lines

Creating a dedicated mailbox driver with its own mutex would effectively

address race conditions.

Quoted 1 line

However, this would necessitate relocating all mailbox-related functions to

this driver.

Quoted 1 line

Including these non-reusable functions would defeat the purpose of code

reuse.

Quoted 2 lines

To strike a balance between code reusability and race condition mitigation,

adding mutexes to PHY access functions seems like a reasonable solution.

You seem to have two kinds of scenarios when talking to MHDP: just cdns_mhdp_mailbox_send(), no response needed and then the cdns_mhdp_mailbox_send() / cdns_mhdp_mailbox_recv_header() / cdns_mhdp_mailbox_recv_data() sequence. Extract those + the mutex access to separate functions, add a mutex to those sequences and use them as a high-level API for your HDMI and PHY drivers.

Quoted 3 lines

Adding mutexes around phy_foo() calls doesn't look like a proper solution _at_ _all_.

Because the sequence cdns_mhdp_mailbox_send() / cdns_mhdp_mailbox_recv_header() / cdns_mhdp_mailbox_recv_data() cannot be reused by different drivers, it's not suitable to abstract them into a separate function.

I've noticed some Linux kernel drivers use global mutexes, which can solve the current race conditions problem. I'll implement this in the next version.

Quoted 83 lines

Subject: Re: [EXT] Re: [PATCH v16 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

Quoted 12 lines · Sandor Yu

Quoted 12 lines · Sandor Yu

Quoted 11 lines

Quoted 11 lines

-----Original Message-----
From: dri-devel <[email protected]> On Behalf Of Maxime Ripard
Sent: 2024年7月2日 21:25
To: Sandor Yu <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; Laurent Pinchart <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected];

[email protected];

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

[email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; dl-linux-imx <[email protected]>; Oliver Brown <[email protected]>; [email protected]; [email protected]
Subject: [EXT] Re: [PATCH v16 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

Hi,

There's still the scrambler issue we discussed on v15, but I have some more comments.

On Tue, Jul 02, 2024 at 08:22:36PM GMT, Sandor Yu wrote:

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Review

Quoted 11 lines

(quoted diff) +11 -0

+enum drm_connector_status cdns_mhdp8501_detect(struct
+cdns_mhdp8501_device *mhdp) {
+	u8 hpd = 0xf;
+
+	hpd = cdns_mhdp8501_read_hpd(mhdp);
+	if (hpd == 1)
+		return connector_status_connected;
+	else if (hpd == 0)
+		return connector_status_disconnected;
+
+	dev_warn(mhdp->dev, "Unknown cable status, hdp=%u\n",

hpd);

Quoted 38 lines

Quoted 37 lines

Quoted 7 lines

Quoted 7 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+	return connector_status_unknown; }
+
+static void hotplug_work_func(struct work_struct *work) {
+	struct cdns_mhdp8501_device *mhdp = container_of(work,
+						     struct cdns_mhdp8501_device,
+						     hotplug_work.work);
+	enum drm_connector_status status =

cdns_mhdp8501_detect(mhdp);

Quoted 17 lines

Quoted 17 lines

Review

Quoted 13 lines

(quoted diff) +13 -0

+
+	drm_bridge_hpd_notify(&mhdp->bridge, status);
+
+	if (status == connector_status_connected) {
+		/* Cable connected  */
+		DRM_INFO("HDMI/DP Cable Plug In\n");
+		enable_irq(mhdp->irq[IRQ_OUT]);
+	} else if (status == connector_status_disconnected) {
+		/* Cable Disconnected  */
+		DRM_INFO("HDMI/DP Cable Plug Out\n");
+		enable_irq(mhdp->irq[IRQ_IN]);
+	}
+}

You shouldn't play with the interrupt being enabled here:
hotplug interrupts should always enabled.

Quoted 2 lines

Quoted 2 lines

If you can't for some reason, the reason should be documented in your

driver.

Quoted 4 lines

iMX8MQ have two HPD interrupters, one for plugout and the other for plugin, because they could not be masked, so we have to enable one and

disable the other.

Quoted 1 line

I will add more comments here.

Right, but why do you need to enable and disable them? Do you get spurious interrupts?

Quoted 6 lines

Quoted 2 lines

They don't have status registers and cannot be masked. If they are not disabled, they will continuously generate interrupts. Therefore, I have to

disable one and enable the other.

Sorry, I still don't get it. How can it be useful to detect hotplug interrupts if it constantly sends spurious interrupts when it's enabled?

Yes, this interrupt is different from a normal one; it's likely a design flaw. For instance, the plugin interrupt is continuously generated as long as the cable is plugged in, only stopping when the cable is unplugged.

That's why two interrupts are used to detect cable plugout and plugin separately. If interrupts aren't used, the only option is polling.

Quoted 37 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

(quoted diff) +11 -0

+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);
+	ret = phy_init(mhdp->phy);
+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret) {
+		dev_err(dev, "Failed to initialize PHY: %d\n", ret);
+		goto clk_disable;
+	}
+
+	/* Mailbox protect for HDMI PHY access */
+	mutex_lock(&mhdp->mbox_mutex);

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+	ret = phy_set_mode(mhdp->phy, phy_mode);
+	mutex_unlock(&mhdp->mbox_mutex);
+	if (ret) {
+		dev_err(dev, "Failed to configure PHY: %d\n", ret);
+		goto clk_disable;
+	}

Why do you need a shared mutex between the phy and HDMI

controller?

Quoted 12 lines

Quoted 6 lines

Quoted 3 lines

Both PHY and HDMI controller could access to the HDMI firmware by mailbox, So add mutex to avoid race condition.

That should be handled at either the phy or mailbox level, not in your hdmi driver.

In both HDMI driver and PHY driver, every mailbox access had protected by its owns mutex. However, this mutex can only protect each mailbox access within their respective drivers, and it cannot provide protection for access between the HDMI and PHY drivers.

Quoted 4 lines

The PHY driver only provides two API functions, and these functions are only called in the HDMI driver. Therefore, when accessing these functions, we use a mutex to protect them. This ensures that mailbox access is protected across different PHY and HDMI drivers.

Quoted 5 lines

It's really about abstraction. You're using a publicly defined API, and change the semantics for your driver only, and that's not ok.

Why can't the mailbox driver itself serialize the accesses from any user, HDMI and PHY drivers included?

In the current code implementation, cdns-mhdp-helper.c isn't a standalone driver but rather a library. It provides fundamental mailbox access functions and basic register read/write operations that rely on the mailbox. These functions are highly reusable across MHDP8501 and MHDP8546 and can be leveraged by future MHDP versions.

However, most MHDP firmware interactions involve a sequence of mailbox accesses, including sending commands and receiving firmware responses. These commands constitute a significant portion of all firmware interactions, encompassing operations like EDID reading and DP link training. Unfortunately, these commands cannot be reused between MHDP8501 and MHDP8546.

Creating a dedicated mailbox driver with its own mutex would effectively address race conditions. However, this would necessitate relocating all mailbox-related functions to this driver. Including these non-reusable functions would defeat the purpose of code reuse.

To strike a balance between code reusability and race condition mitigation, adding mutexes to PHY access functions seems like a reasonable solution.

Add a new DRM DisplayPort and HDMI bridge driver for Candence MHDP8501 used in i.MX8MQ SOC. MHDP8501 could support HDMI or DisplayPort standards according embedded Firmware running in the uCPU.

For iMX8MQ SOC, the DisplayPort/HDMI FW was loaded and activated by SOC's ROM code. Bootload binary included respective specific firmware is required.

Driver will check display connector type and
then load the corresponding driver.

Signed-off-by Sandor Yu <[email protected]>

Tested-by Alexander Stein <[email protected]>

---
v15->v16:
- rebase the patchset sits on top Dmitry's
'make use of the HDMI connector infrastructure' patchset ([2]).
- Add DRM_BRIDGE_OP_HDMI flags for HDMI driver,
- Introduce the hdmi info frame helper functions, added hdmi_clear_infoframe(), hdmi_write_infoframe() and hdmi_tmds_char_rate_valid().
- mode_fixup() was replaced by atomic_check().
- Fix video mode 4Kp30 did not work on some displays that support LTE_340Mcsc_scramble.
- updated for tmds_char_rate added in patch #2.

v13->v14:
- Rebase to next-20240219, replace get_edid function by edid_read function.

v12->v13:
- Explicitly include linux/platform_device.h for cdns-mhdp8501-core.c
- Fix build warning
- Order bit bpc and color_space in descending shit.

v11->v12:
- Replace DRM_INFO with dev_info or dev_warn.
- Replace DRM_ERROR with dev_err.
- Return ret when cdns_mhdp_dpcd_read failed in function cdns_dp_aux_transferi().
- Remove unused parmeter in function cdns_dp_get_msa_misc
and use two separate variables for color space and bpc.
- Add year 2024 to copyright.

  drivers/gpu/drm/bridge/cadence/Kconfig        |  16 +
  drivers/gpu/drm/bridge/cadence/Makefile       |   2 +
  .../drm/bridge/cadence/cdns-mhdp8501-core.c   | 330 +++++++++
  .../drm/bridge/cadence/cdns-mhdp8501-core.h   | 367 +++++++++
  .../gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c | 700 ++++++++++++++++++
  .../drm/bridge/cadence/cdns-mhdp8501-hdmi.c   | 595 +++++++++++++++
  6 files changed, 2010 insertions(+)

create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.h create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c

drivers/gpu/drm/bridge/cadence/Kconfig +16 -0

index e0973339e9e3..45848e741f5f 100644
@@ -51,3 +51,19 @@ config DRM_CDNS_MHDP8546_J721E
  	  initializes the J721E Display Port and sets up the
  	  clock and data muxes.
  endif
+
+config DRM_CDNS_MHDP8501
+	tristate "Cadence MHDP8501 DP/HDMI bridge"
+	select DRM_KMS_HELPER
+	select DRM_PANEL_BRIDGE
+	select DRM_DISPLAY_DP_HELPER
+	select DRM_DISPLAY_HELPER
+	select CDNS_MHDP_HELPER
+	select DRM_CDNS_AUDIO
+	depends on OF
+	help
+	  Support Cadence MHDP8501 DisplayPort/HDMI bridge.
+	  Cadence MHDP8501 support one or more protocols,
+	  including DisplayPort and HDMI.
+	  To use the DP and HDMI drivers, their respective
+	  specific firmware is required.

drivers/gpu/drm/bridge/cadence/Makefile +2 -0

index 087dc074820d..02c1a9f3cf6f 100644
@@ -6,3 +6,5 @@ obj-$(CONFIG_CDNS_MHDP_HELPER) += cdns-mhdp-helper.o
  obj-$(CONFIG_DRM_CDNS_MHDP8546) += cdns-mhdp8546.o
  cdns-mhdp8546-y := cdns-mhdp8546-core.o cdns-mhdp8546-hdcp.o
  cdns-mhdp8546-$(CONFIG_DRM_CDNS_MHDP8546_J721E) += cdns-mhdp8546-j721e.o
+obj-$(CONFIG_DRM_CDNS_MHDP8501) += cdns-mhdp8501.o
+cdns-mhdp8501-y := cdns-mhdp8501-core.o cdns-mhdp8501-dp.o cdns-mhdp8501-hdmi.o

drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file +213 -0

new file mode 100644
index 000000000000..c306db982b76
@@ -0,0 +1,330 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Cadence Display Port Interface (DP) driver
+ *
+ * Copyright (C) 2023, 2024 NXP Semiconductor, Inc.
+ *
+ */
+#include <drm/drm_of.h>
+#include <drm/drm_print.h>
+#include <linux/clk.h>
+#include <linux/irq.h>
+#include <linux/mutex.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/phy/phy.h>
+
+#include "cdns-mhdp8501-core.h"
+
+static int cdns_mhdp8501_read_hpd(struct cdns_mhdp8501_device *mhdp)
+{
+	u8 status;
+	int ret;
+
+	mutex_lock(&mhdp->mbox_mutex);
+
+	ret = cdns_mhdp_mailbox_send(&mhdp->base, MB_MODULE_ID_GENERAL,
+				     GENERAL_GET_HPD_STATE, 0, NULL);
+	if (ret)
+		goto err_get_hpd;
+
+	ret = cdns_mhdp_mailbox_recv_header(&mhdp->base, MB_MODULE_ID_GENERAL,
+					    GENERAL_GET_HPD_STATE,
+					    sizeof(status));
+	if (ret)
+		goto err_get_hpd;
+
+	ret = cdns_mhdp_mailbox_recv_data(&mhdp->base, &status, sizeof(status));
+	if (ret)
+		goto err_get_hpd;
+
+	mutex_unlock(&mhdp->mbox_mutex);
+
+	return status;
+
+err_get_hpd:
+	dev_err(mhdp->dev, "read hpd  failed: %d\n", ret);
+	mutex_unlock(&mhdp->mbox_mutex);
+
+	return ret;
+}
+
+enum drm_connector_status cdns_mhdp8501_detect(struct cdns_mhdp8501_device *mhdp)
+{
+	u8 hpd = 0xf;
+
+	hpd = cdns_mhdp8501_read_hpd(mhdp);
+	if (hpd == 1)
+		return connector_status_connected;
+	else if (hpd == 0)
+		return connector_status_disconnected;
+
+	dev_warn(mhdp->dev, "Unknown cable status, hdp=%u\n", hpd);
+	return connector_status_unknown;
+}
+
+static void hotplug_work_func(struct work_struct *work)
+{
+	struct cdns_mhdp8501_device *mhdp = container_of(work,
+						     struct cdns_mhdp8501_device,
+						     hotplug_work.work);
+	enum drm_connector_status status = cdns_mhdp8501_detect(mhdp);
+
+	drm_bridge_hpd_notify(&mhdp->bridge, status);
+
+	if (status == connector_status_connected) {
+		/* Cable connected  */
+		DRM_INFO("HDMI/DP Cable Plug In\n");
+		enable_irq(mhdp->irq[IRQ_OUT]);
+	} else if (status == connector_status_disconnected) {
+		/* Cable Disconnected  */
+		DRM_INFO("HDMI/DP Cable Plug Out\n");
+		enable_irq(mhdp->irq[IRQ_IN]);
+	}
+}
+
+static irqreturn_t cdns_mhdp8501_irq_thread(int irq, void *data)
+{
+	struct cdns_mhdp8501_device *mhdp = data;
+
+	disable_irq_nosync(irq);
+
+	mod_delayed_work(system_wq, &mhdp->hotplug_work,
+			 msecs_to_jiffies(HOTPLUG_DEBOUNCE_MS));
+
+	return IRQ_HANDLED;
+}
+
+static int cdns_mhdp8501_dt_parse(struct cdns_mhdp8501_device *mhdp,
+				  struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node *remote;
+
+	remote = of_graph_get_remote_node(np, 1, 0);
+	if (!remote) {
+		dev_err(dev, "fail to get remote node\n");
+		of_node_put(remote);
+		return -EINVAL;
+	}
+
+	/* get connector type */
+	if (of_device_is_compatible(remote, "hdmi-connector")) {
+		mhdp->connector_type = DRM_MODE_CONNECTOR_HDMIA;
+
+	} else if (of_device_is_compatible(remote, "dp-connector")) {
+		mhdp->connector_type = DRM_MODE_CONNECTOR_DisplayPort;
+
+	} else {
+		dev_err(dev, "Unknown connector type\n");
+		of_node_put(remote);
+		return -EINVAL;
+	}
+
+	of_node_put(remote);
+	return true;
+}
+
+static void cdns_mhdp8501_add_bridge(struct cdns_mhdp8501_device *mhdp)
+{
+	mhdp->bridge.type = mhdp->connector_type;
+	mhdp->bridge.driver_private = mhdp;
+	mhdp->bridge.of_node = mhdp->dev->of_node;
+	mhdp->bridge.vendor = "NXP";
+	mhdp->bridge.product = "i.MX8";
+	mhdp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
+			   DRM_BRIDGE_OP_HPD;
+
+	if (mhdp->connector_type == DRM_MODE_CONNECTOR_HDMIA) {
+		mhdp->bridge.funcs = &cdns_hdmi_bridge_funcs;
+		mhdp->bridge.ops |= DRM_BRIDGE_OP_HDMI;
+	} else if (mhdp->connector_type == DRM_MODE_CONNECTOR_DisplayPort) {
+		mhdp->bridge.funcs = &cdns_dp_bridge_funcs;
+	} else {
+		dev_err(mhdp->dev, "Unsupported connector type!\n");
+		return;
+	}
+
+	drm_bridge_add(&mhdp->bridge);
+}
+
+static int cdns_mhdp8501_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct cdns_mhdp8501_device *mhdp;
+	struct resource *res;
+	enum phy_mode phy_mode;
+	u32 reg;
+	int ret;
+
+	mhdp = devm_kzalloc(dev, sizeof(*mhdp), GFP_KERNEL);
+	if (!mhdp)
+		return -ENOMEM;
+
+	mutex_init(&mhdp->mbox_mutex);
+	mhdp->dev = dev;
+
+	INIT_DELAYED_WORK(&mhdp->hotplug_work, hotplug_work_func);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	mhdp->regs = devm_ioremap(dev, res->start, resource_size(res));
+	if (IS_ERR(mhdp->regs))
+		return PTR_ERR(mhdp->regs);
+
+	ret = cdns_mhdp8501_dt_parse(mhdp, pdev);
+	if (ret < 0)
+		return -EINVAL;
+
+	mhdp->phy = devm_of_phy_get_by_index(dev, pdev->dev.of_node, 0);
+	if (IS_ERR(mhdp->phy))
+		return dev_err_probe(dev, PTR_ERR(mhdp->phy), "no PHY configured\n");
+
+	mhdp->irq[IRQ_IN] = platform_get_irq_byname(pdev, "plug_in");
+	if (mhdp->irq[IRQ_IN] < 0)
+		return dev_err_probe(dev, mhdp->irq[IRQ_IN], "No plug_in irq number\n");
+
+	mhdp->irq[IRQ_OUT] = platform_get_irq_byname(pdev, "plug_out");
+	if (mhdp->irq[IRQ_OUT] < 0)
+		return dev_err_probe(dev, mhdp->irq[IRQ_OUT], "No plug_out irq number\n");
+
+	irq_set_status_flags(mhdp->irq[IRQ_IN], IRQ_NOAUTOEN);
+	ret = devm_request_threaded_irq(dev, mhdp->irq[IRQ_IN],
+					NULL, cdns_mhdp8501_irq_thread,
+					IRQF_ONESHOT, dev_name(dev), mhdp);
+	if (ret < 0) {
+		dev_err(dev, "can't claim irq %d\n", mhdp->irq[IRQ_IN]);
+		return -EINVAL;
+	}
+
+	irq_set_status_flags(mhdp->irq[IRQ_OUT], IRQ_NOAUTOEN);
+	ret = devm_request_threaded_irq(dev, mhdp->irq[IRQ_OUT],
+					NULL, cdns_mhdp8501_irq_thread,
+					IRQF_ONESHOT, dev_name(dev), mhdp);
+	if (ret < 0) {
+		dev_err(dev, "can't claim irq %d\n", mhdp->irq[IRQ_OUT]);
+		return -EINVAL;
+	}
+
+	/* set default lane mapping */
+	mhdp->lane_mapping = LANE_MAPPING_NORMAL;