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

[Sandor Yu]

Hi Maxime,

Thanks for your comments,

> -----Original Message-----
> From: dri-devel <dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
> Maxime Ripard
> Sent: 2024年7月2日 21:25
> To: Sandor Yu <sandor.yu@xxxxxxx>
> Cc: dmitry.baryshkov@xxxxxxxxxx; andrzej.hajda@xxxxxxxxx;
> neil.armstrong@xxxxxxxxxx; Laurent Pinchart
> <laurent.pinchart@xxxxxxxxxxxxxxxx>; jonas@xxxxxxxxx;
> jernej.skrabec@xxxxxxxxx; airlied@xxxxxxxxx; daniel@xxxxxxxx;
> robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
> shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; festevam@xxxxxxxxx;
> vkoul@xxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; linux-phy@xxxxxxxxxxxxxxxxxxx;
> kernel@xxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; Oliver Brown
> <oliver.brown@xxxxxxx>; alexander.stein@xxxxxxxxxxxxxxx;
> sam@xxxxxxxxxxxx
> 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:
> > +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]);
> > +	}
> > +}
> 
> 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.

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.

> 
> > +	/* 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;
> > +	}
> 
> 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.

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

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

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

> > +	/* 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.
> 
> > +	dev_dbg(mhdp->dev, "Mode: %dx%dp%d\n",
> > +		mode->hdisplay, mode->vdisplay, mode->clock);
> > +
> > +	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.
> 
> > +	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

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

> 
> > +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);
> > +	}
> > +
> > +	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,
> > +conn_state->state); }
> 
> You should also call your mode_valid function here.
Function cdns_hdmi_tmds_char_rate_valid() is called in atomic_check().
Function cdns_hdmi_bridge_mode_valid is called by core, should not be called again.

Sandor
> 
> Maxime