Message-ID: <20240903-gay-capable-hound-3cbef2@houat>
Date: Tue, 3 Sep 2024 10:28:09 +0200
From: Maxime Ripard <mripard@...nel.org>
To: Sandor Yu <sandor.yu@....com>
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 v16 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

On Tue, Sep 03, 2024 at 06:07:25AM GMT, Sandor Yu wrote:
> Hi Maxime,
> 
> Thanks for your comments,
> 
> > -----Original Message-----
> > From: dri-devel <dri-devel-bounces@...ts.freedesktop.org> On Behalf Of
> > Maxime Ripard
> > Sent: 2024年7月2日 21:25
> > To: Sandor Yu <sandor.yu@....com>
> > Cc: dmitry.baryshkov@...aro.org; andrzej.hajda@...el.com;
> > neil.armstrong@...aro.org; Laurent Pinchart
> > <laurent.pinchart@...asonboard.com>; jonas@...boo.se;
> > jernej.skrabec@...il.com; airlied@...il.com; daniel@...ll.ch;
> > robh+dt@...nel.org; krzysztof.kozlowski+dt@...aro.org;
> > shawnguo@...nel.org; s.hauer@...gutronix.de; festevam@...il.com;
> > vkoul@...nel.org; dri-devel@...ts.freedesktop.org;
> > devicetree@...r.kernel.org; linux-arm-kernel@...ts.infradead.org;
> > linux-kernel@...r.kernel.org; linux-phy@...ts.infradead.org;
> > kernel@...gutronix.de; dl-linux-imx <linux-imx@....com>; Oliver Brown
> > <oliver.brown@....com>; alexander.stein@...tq-group.com;
> > sam@...nborg.org
> > 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.

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

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

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

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

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

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

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.

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

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

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)

Powered by blists - more mailing lists