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 <mrip...@kernel.org> > Sent: 2024年7月2日 20:40 > To: Sandor Yu <sandor...@nxp.com> > Cc: Alexander Stein <alexander.st...@ew.tq-group.com>; Andrzej Hajda > <andrzej.ha...@intel.com>; Neil Armstrong <neil.armstr...@linaro.org>; > Robert Foss <rf...@kernel.org>; Laurent Pinchart > <laurent.pinch...@ideasonboard.com>; Jonas Karlman <jo...@kwiboo.se>; > Jernej Skrabec <jernej.skra...@gmail.com>; David Airlie > <airl...@gmail.com>; Daniel Vetter <dan...@ffwll.ch>; Maarten Lankhorst > <maarten.lankho...@linux.intel.com>; Thomas Zimmermann > <tzimmerm...@suse.de>; Rob Herring <r...@kernel.org>; Krzysztof > Kozlowski <krzysztof.kozlowski...@linaro.org>; Conor Dooley > <conor...@kernel.org>; Vinod Koul <vk...@kernel.org>; Kishon Vijay > Abraham I <kis...@kernel.org>; Shawn Guo <shawn...@kernel.org>; Sascha > Hauer <s.ha...@pengutronix.de>; Pengutronix Kernel Team > <ker...@pengutronix.de>; Fabio Estevam <feste...@gmail.com>; > dri-devel@lists.freedesktop.org; devicet...@vger.kernel.org; > linux-ker...@vger.kernel.org; linux-...@lists.infradead.org; > i...@lists.linux.dev; linux-arm-ker...@lists.infradead.org; > li...@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@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