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@xxxxxxxxxx>
> Sent: 2024年7月2日 20:40
> To: Sandor Yu <sandor.yu@xxxxxxx>
> Cc: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>; Andrzej Hajda
> <andrzej.hajda@xxxxxxxxx>; Neil Armstrong <neil.armstrong@xxxxxxxxxx>;
> Robert Foss <rfoss@xxxxxxxxxx>; Laurent Pinchart
> <laurent.pinchart@xxxxxxxxxxxxxxxx>; Jonas Karlman <jonas@xxxxxxxxx>;
> Jernej Skrabec <jernej.skrabec@xxxxxxxxx>; David Airlie
> <airlied@xxxxxxxxx>; Daniel Vetter <daniel@xxxxxxxx>; Maarten Lankhorst
> <maarten.lankhorst@xxxxxxxxxxxxxxx>; Thomas Zimmermann
> <tzimmermann@xxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Krzysztof
> Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; Vinod Koul <vkoul@xxxxxxxxxx>; Kishon Vijay
> Abraham I <kishon@xxxxxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>; Sascha
> Hauer <s.hauer@xxxxxxxxxxxxxx>; Pengutronix Kernel Team
> <kernel@xxxxxxxxxxxxxx>; Fabio Estevam <festevam@xxxxxxxxx>;
> dri-devel@xxxxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; linux-phy@xxxxxxxxxxxxxxxxxxx;
> imx@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> linux@xxxxxxxxxxxxxxx
> 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@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
