Hi Maxime,

Thanks for your comments.

On Tue, Sep 24, 2024 at 03:36:49PM GMT, Sandor Yu wrote:

quoted

+static int cdns_mhdp8501_read_hpd(struct cdns_mhdp8501_device

*mhdp)

quoted

+{
+	u8 status;
+	int ret;
+
+	mutex_lock(&mhdp_mailbox_mutex);
+
+	ret = cdns_mhdp_mailbox_send(&mhdp->base,

MB_MODULE_ID_GENERAL,

quoted

+				     GENERAL_GET_HPD_STATE, 0, NULL);
+	if (ret)
+		goto err_get_hpd;
+
+	ret = cdns_mhdp_mailbox_recv_header(&mhdp->base,

MB_MODULE_ID_GENERAL,

quoted

+					    GENERAL_GET_HPD_STATE,
+					    sizeof(status));
+	if (ret)
+		goto err_get_hpd;
+
+	ret = cdns_mhdp_mailbox_recv_data(&mhdp->base, &status,

sizeof(status));

quoted

+	if (ret)
+		goto err_get_hpd;
+
+	mutex_unlock(&mhdp_mailbox_mutex);

That's better I guess, but it's still not a good API design. If you can't have
concurrent accesses, then cdns_mhdp_mailbox_send et al.
should take the mutex themselves.

I will check Dmitry's comments to add API like,
 int cdns_mhdp_mailbox_send_recv(struct cdns_mhdp_device *mhdp,
                         u8 module_id, u8 opcode,
                         u16 size, const u8 *message,
                         u16 buf_size, u8 *buf);

quoted

+	return status;
+
+err_get_hpd:
+	dev_err(mhdp->dev, "read hpd  failed: %d\n", ret);
+	mutex_unlock(&mhdp_mailbox_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);

This is already logged, there's no need to add a message there.

OK, it will be removed.

quoted

+	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");

You might want to log it using drm_dev_debug, but anything else is a big
stretch.

I will use the dev_dbg function to maintain consistency with the rest of the driver.

quoted

+		enable_irq(mhdp->irq[IRQ_OUT]);
+
+		/* Reset HDMI/DP link with sink */
+		if (mhdp->connector_type == DRM_MODE_CONNECTOR_HDMIA)
+			cdns_hdmi_reset_link(mhdp);
+		else
+			cdns_dp_check_link_state(mhdp);
+
+	} 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;
+}

disable_irq_nosync doesn't guarantee that you won't be called before
interrupts are reenabled. What will happen if this handler is called multiple
times?

If this handler is called multiple times, function will hotplug_work_func() excute multiple times too,
driver will working in the latest HPD status.

quoted

+
+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);
+
+	if (of_property_read_u32(np, "lane-mapping", &mhdp->lane_mapping)) {
+		dev_warn(dev, "Failed to get lane_mapping - using default\n");

debug logging as well.

use dev_dbg here too，

quoted

+		mhdp->lane_mapping = LANE_MAPPING_FLIPPED;
+	}
+
+	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

|

quoted

+			   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) {

quoted

+		mhdp->bridge.funcs = &cdns_dp_bridge_funcs;
+	} else {
+		dev_err(mhdp->dev, "Unsupported connector type!\n");
+		return;
+	}

If this function can error out, then it should return an error code.

OK.

quoted

+static int cdns_hdmi_scdc_write(struct cdns_mhdp8501_device *mhdp, u8
+addr, u8 value) {
+	u8 msg[5], reg[5];
+	int ret;
+
+	msg[0] = 0x54;
+	msg[1] = addr;
+	msg[2] = 0;
+	msg[3] = 1;
+	msg[4] = value;
+
+	mutex_lock(&mhdp_mailbox_mutex);
+
+	ret = cdns_mhdp_mailbox_send(&mhdp->base,

MB_MODULE_ID_HDMI_TX, HDMI_TX_WRITE,

quoted

+				     sizeof(msg), msg);
+	if (ret)
+		goto err_scdc_write;
+
+	ret = cdns_mhdp_mailbox_recv_header(&mhdp->base,

MB_MODULE_ID_HDMI_TX,

quoted

+					    HDMI_TX_WRITE, sizeof(reg));
+	if (ret)
+		goto err_scdc_write;
+
+	ret = cdns_mhdp_mailbox_recv_data(&mhdp->base, reg, sizeof(reg));
+	if (ret)
+		goto err_scdc_write;
+
+	if (reg[0] != 0)
+		ret = -EINVAL;
+
+err_scdc_write:
+
+	mutex_unlock(&mhdp_mailbox_mutex);
+
+	if (ret)
+		dev_err(mhdp->dev, "scdc write failed: %d\n", ret);
+	return ret;
+}

You should be using SCDC helpers there instead of hand crafting your
messages.

OK, I will try create i2c_adapter/DDC for SCDC.

quoted

+static void cdns_hdmi_sink_config(struct cdns_mhdp8501_device *mhdp)
+{
+	struct drm_display_info *display = &mhdp->curr_conn->display_info;
+	struct drm_connector_state *conn_state = mhdp->curr_conn->state;

That looks a bit hackish to me. We should probably provide a helper to get the
connector state the bridge is attached to.

How about code change to followed, is it more clear?
370         struct drm_connector *connector = mhdp->curr_conn;
371         struct drm_connector_state *conn_state = connector->state;
372         struct drm_display_info *display = &connector->display_info;
373         struct drm_scdc *scdc = &display->hdmi.scdc;

quoted

+	struct drm_scdc *scdc = &mhdp->curr_conn->display_info.hdmi.scdc;
+	u8 buff = 0;
+
+	/* check sink type (HDMI or DVI) */
+	if (!display->is_hdmi) {
+		mhdp->hdmi.hdmi_type = MODE_DVI;
+		return;
+	}
+
+	/* Default work in HDMI1.4 */
+	mhdp->hdmi.hdmi_type = MODE_HDMI_1_4;
+
+	/* check sink support SCDC or not */
+	if (!scdc->supported) {
+		dev_info(mhdp->dev, "Sink Not Support SCDC\n");

drm_dev_debug

dev_dbg

quoted

+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;
+
+	ret = phy_validate(mhdp->phy, PHY_MODE_HDMI, 0, &phy_cfg);
+	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 ||
+	    mode->flags & DRM_MODE_FLAG_INTERLACE)
+		return MODE_BAD;
+
+	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); }

Didn't we agree on creating a mode_valid helper?

In fact, now I'm no idea where should add the mode_valid helper function.

In struct drm_bridge_funcs, it had mode_valid() and hdmi_tmds_char_rate_valid().

If create a new mode_valid helper function in struct drm_connector_hdmi_funcs,
Is it appropriate to call another API function(tmds_char_rate_valid)
at the same level within this API function?

quoted

+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_info = &mhdp->video_info;
+	struct drm_crtc_state *crtc_state;
+	struct drm_connector_state *conn_state;
+	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;
+
+	video_info->color_fmt = conn_state->hdmi.output_format;
+	video_info->bpc = conn_state->hdmi.output_bpc;
+
+	drm_atomic_helper_connector_hdmi_update_infoframes(connector,
+state);
+
+	/* Line swapping */
+	cdns_mhdp_reg_write(&mhdp->base, LANES_CONFIG, 0x00400000 |
+mhdp->lane_mapping);
+
+	phy_cfg.hdmi.tmds_char_rate = conn_state->hdmi.tmds_char_rate;
+
+	ret = phy_configure(mhdp->phy, &phy_cfg);
+	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, "hdmi ctrl init failed = %d\n",  ret);
+		return;
+	}
+
+	/* Config GCP */
+	if (video_info->bpc == 8)
+		cdns_hdmi_disable_gcp(mhdp);
+	else
+		cdns_hdmi_enable_gcp(mhdp);
+
+	ret = cdns_hdmi_mode_config(mhdp, &crtc_state->adjusted_mode,

video_info);

quoted

+	if (ret < 0) {
+		dev_err(mhdp->dev, "CDN_API_HDMITX_SetVic_blocking ret = %d\n",

ret);

quoted

+		return;
+	}

If the only thing you're doing is filling video_info and passing it there, why do
you need that structure at all? Just pass the connector_state pointer.

OK, video_info will remove from hdmi driver.

B.R
Sandor

Maxime