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

[Maxime Ripard]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-phy/attachments/20240702/e408b66d/attachment.sig>