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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
 <PAXPR04MB944842A62DC21FC3F2530AF9F4DD2@PAXPR04MB9448.eurprd04.prod.outlook.com>
Date: Wed, 3 Jul 2024 15:43:39 +0000
From: Sandor Yu <sandor.yu@....com>
To: Maxime Ripard <mripard@...nel.org>
CC: Alexander Stein <alexander.stein@...tq-group.com>, Andrzej Hajda
	<andrzej.hajda@...el.com>, Neil Armstrong <neil.armstrong@...aro.org>, Robert
 Foss <rfoss@...nel.org>, Laurent Pinchart
	<laurent.pinchart@...asonboard.com>, Jonas Karlman <jonas@...boo.se>, Jernej
 Skrabec <jernej.skrabec@...il.com>, David Airlie <airlied@...il.com>, Daniel
 Vetter <daniel@...ll.ch>, Maarten Lankhorst
	<maarten.lankhorst@...ux.intel.com>, Thomas Zimmermann <tzimmermann@...e.de>,
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
	<krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>,
	Vinod Koul <vkoul@...nel.org>, Kishon Vijay Abraham I <kishon@...nel.org>,
	Shawn Guo <shawnguo@...nel.org>, Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>, Fabio Estevam
	<festevam@...il.com>, "dri-devel@...ts.freedesktop.org"
	<dri-devel@...ts.freedesktop.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-phy@...ts.infradead.org"
	<linux-phy@...ts.infradead.org>, "imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux@...tq-group.com"
	<linux@...tq-group.com>
Subject: Re: [PATCH v15 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

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@...nel.org>
> Sent: 2024年7月2日 20:40
> To: Sandor Yu <sandor.yu@....com>
> Cc: Alexander Stein <alexander.stein@...tq-group.com>; Andrzej Hajda
> <andrzej.hajda@...el.com>; Neil Armstrong <neil.armstrong@...aro.org>;
> Robert Foss <rfoss@...nel.org>; Laurent Pinchart
> <laurent.pinchart@...asonboard.com>; Jonas Karlman <jonas@...boo.se>;
> Jernej Skrabec <jernej.skrabec@...il.com>; David Airlie
> <airlied@...il.com>; Daniel Vetter <daniel@...ll.ch>; Maarten Lankhorst
> <maarten.lankhorst@...ux.intel.com>; Thomas Zimmermann
> <tzimmermann@...e.de>; Rob Herring <robh@...nel.org>; Krzysztof
> Kozlowski <krzysztof.kozlowski+dt@...aro.org>; Conor Dooley
> <conor+dt@...nel.org>; Vinod Koul <vkoul@...nel.org>; Kishon Vijay
> Abraham I <kishon@...nel.org>; Shawn Guo <shawnguo@...nel.org>; Sascha
> Hauer <s.hauer@...gutronix.de>; Pengutronix Kernel Team
> <kernel@...gutronix.de>; Fabio Estevam <festevam@...il.com>;
> dri-devel@...ts.freedesktop.org; devicetree@...r.kernel.org;
> linux-kernel@...r.kernel.org; linux-phy@...ts.infradead.org;
> imx@...ts.linux.dev; linux-arm-kernel@...ts.infradead.org;
> linux@...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@...z 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