Message-ID:
 <DB9PR04MB945284B95FAE4FE127E6568FF4752@DB9PR04MB9452.eurprd04.prod.outlook.com>
Date: Sun, 29 Sep 2024 02:34:36 +0000
From: Sandor Yu <sandor.yu@....com>
To: Maxime Ripard <mripard@...nel.org>
CC: "dmitry.baryshkov@...aro.org" <dmitry.baryshkov@...aro.org>,
	"andrzej.hajda@...el.com" <andrzej.hajda@...el.com>,
	"neil.armstrong@...aro.org" <neil.armstrong@...aro.org>, Laurent Pinchart
	<laurent.pinchart@...asonboard.com>, "jonas@...boo.se" <jonas@...boo.se>,
	"jernej.skrabec@...il.com" <jernej.skrabec@...il.com>, "airlied@...il.com"
	<airlied@...il.com>, "daniel@...ll.ch" <daniel@...ll.ch>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	"krzysztof.kozlowski+dt@...aro.org" <krzysztof.kozlowski+dt@...aro.org>,
	"shawnguo@...nel.org" <shawnguo@...nel.org>, "s.hauer@...gutronix.de"
	<s.hauer@...gutronix.de>, "festevam@...il.com" <festevam@...il.com>,
	"vkoul@...nel.org" <vkoul@...nel.org>, "dri-devel@...ts.freedesktop.org"
	<dri-devel@...ts.freedesktop.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-phy@...ts.infradead.org"
	<linux-phy@...ts.infradead.org>, "kernel@...gutronix.de"
	<kernel@...gutronix.de>, dl-linux-imx <linux-imx@....com>, Oliver Brown
	<oliver.brown@....com>, "alexander.stein@...tq-group.com"
	<alexander.stein@...tq-group.com>, "sam@...nborg.org" <sam@...nborg.org>
Subject: Re: [PATCH v17 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

Hi Maxime,

Thanks for your comments.

> 
> On Tue, Sep 24, 2024 at 03:36:49PM GMT, Sandor Yu wrote:
> > +static int cdns_mhdp8501_read_hpd(struct cdns_mhdp8501_device
> *mhdp)
> > +{
> > +	u8 status;
> > +	int ret;
> > +
> > +	mutex_lock(&mhdp_mailbox_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_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);

> 
> > +	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.

> 
> > +	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.

> 
> > +		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.

> 
> > +
> > +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，
> 
> > +		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
> |
> > +			   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;
> > +	}
> 
> If this function can error out, then it should return an error code.
OK.

> 
> > +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,
> > +				     sizeof(msg), msg);
> > +	if (ret)
> > +		goto err_scdc_write;
> > +
> > +	ret = cdns_mhdp_mailbox_recv_header(&mhdp->base,
> MB_MODULE_ID_HDMI_TX,
> > +					    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.

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

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

> 
> > +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?

> 
> > +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);
> > +	if (ret < 0) {
> > +		dev_err(mhdp->dev, "CDN_API_HDMITX_SetVic_blocking ret = %d\n",
> ret);
> > +		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

Powered by blists - more mailing lists