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

Wed Jul 3 15:43:39 UTC 2024

Hi Maxime,

Thanks for your detail explain, it is make sense to reset the HDMI link.
I will implement it later.

B.R
Sandor

> -----Original Message-----
> From: Maxime Ripard <mripard at kernel.org>
> Sent: 2024年7月2日 20:40
> To: Sandor Yu <sandor.yu at nxp.com>
> Cc: Alexander Stein <alexander.stein at ew.tq-group.com>; Andrzej Hajda
> <andrzej.hajda at intel.com>; Neil Armstrong <neil.armstrong at linaro.org>;
> Robert Foss <rfoss at kernel.org>; Laurent Pinchart
> <laurent.pinchart at ideasonboard.com>; Jonas Karlman <jonas at kwiboo.se>;
> Jernej Skrabec <jernej.skrabec at gmail.com>; David Airlie
> <airlied at gmail.com>; Daniel Vetter <daniel at ffwll.ch>; Maarten Lankhorst
> <maarten.lankhorst at linux.intel.com>; Thomas Zimmermann
> <tzimmermann at suse.de>; Rob Herring <robh at kernel.org>; Krzysztof
> Kozlowski <krzysztof.kozlowski+dt at linaro.org>; Conor Dooley
> <conor+dt at kernel.org>; Vinod Koul <vkoul at kernel.org>; Kishon Vijay
> Abraham I <kishon at kernel.org>; Shawn Guo <shawnguo at kernel.org>; Sascha
> Hauer <s.hauer at pengutronix.de>; Pengutronix Kernel Team
> <kernel at pengutronix.de>; Fabio Estevam <festevam at gmail.com>;
> dri-devel at lists.freedesktop.org; devicetree at vger.kernel.org;
> linux-kernel at vger.kernel.org; linux-phy at lists.infradead.org;
> imx at lists.linux.dev; linux-arm-kernel at lists.infradead.org;
> linux at ew.tq-group.com
> Subject: [EXT] Re: [PATCH v15 4/8] drm: bridge: Cadence: Add MHDP8501
> DP/HDMI driver
> 
> Hi,
> 
> On Tue, Jul 02, 2024 at 12:13:16PM GMT, Sandor Yu wrote:
> > > Subject: [EXT] Re: [PATCH v15 4/8] drm: bridge: Cadence: Add
> > > MHDP8501 DP/HDMI driver
> > >
> > > Hi,
> > >
> > > On Wed, Mar 06, 2024 at 11:16:21AM +0100, Alexander Stein wrote:
> > > > +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;
> > > > +}
> > >
> > > AFAICT from the rest of the driver, you support HDMI modes with a
> > > character rate > 340Mchar/s, and thus you need to enable the scrambler.
> > >
> > > If you unplug/replug a monitor with the scrambler enabled though,
> > > and if there's no userspace component to react to the userspace
> > > event, the code you have here won't enable the scrambler again.
> > >
> > > You can test that by using modetest with a 4k at 60Hz mode or
> > > something, and then disconnecting / reconnecting the monitor.
> > >
> > > We addressed it in vc4 in commit 6bed2ea3cb38 ("drm/vc4: hdmi: Reset
> > > link on hotplug").
> > >
> > > Arguably, the whole handling of the HDMI scrambling setup should be
> > > turned into a helper, and I wanted to extend the work I've been
> > > doing around the HDMI infra to support the scrambler setup once it
> landed.
> > >
> >
> > Yes, for userspace components that do not handle HPD events (such as
> > modetest), if they are connected to a 4K display and enable scramble
> > then the cable is unplugged/plugged, HDMI will not work. However, this
> > is a userspace component limitation.
> 
> No, it's not.
> 
> I mean, yes, it's something the userspace *could* do. But it doesn't have to,
> and the expectation is very much that the display keeps working.
> 
> > fbdev and weston could work well for this case.
> 
> Yeah, they could work well. We don't want them to possibly work, we want
> them to work, period. That's why amdgpu, i915 and vc4 all have that code.
> 
> > The patch for vc4 in commit 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link
> > on hotplug") assumes unplug/replug the same monitor as stated in its
> > commit log.
> 
> Indeed.
> 
> > It does not support the case where unplug/plug to different displays.
> > For example, if the cable is unplugged from a 4K monitor and then
> > plugged into a 1080p monitor, 4K video mode will be output to the 1080p
> monitor because this userspace component cannot respond to the monitor
> change.
> > Therefore, for userspace components that do not handle HPD events
> > (such as modetest), this patch can only partially solve the limitation, but it
> is not a perfect solution.
> 
> You're looking at it from the wrong PoV. What matters is the behaviour we
> offer from userspace. Userspace is in charge of setting the mode, and it's
> expectation is that the mode is going to be output until it either changes the
> mode or disables the display.
> 
> Reenabling the scrambler when a display is disconnected and reconnected
> matches that expectation. If we ignore the case where the display has
> changed, we still match that expectation: the userspace is in control of the
> mode.
> 
> If it wants to react to monitors being unplugged, it can. But it doesn't have to,
> and it should keep working as long as you don't change the moniter / panel.
> 
> And you're also completely ignoring things like AV amplifiers that really like
> to do those kind of short HPD pulses.
> 
> > If helper functions are used to restore the HDMI scramble bit after
> > hotplug, I would like to use it.
> 
> Those helpers don't exist yet, so feel free to work on them.
> 
> Maxime