On Thu, Mar 02, 2023 at 03:17:04PM -0800, Bjorn Andersson wrote: > On Wed, Mar 01, 2023 at 02:58:50PM +0100, Johan Hovold wrote: > > So after debugging this issue a third time, I can conclude that it is > > still very much present in 6.2. > > > > It appears you looked at the linux-next tree when you concluded that > > this patch was not needed. In 6.2 the bridge->hpd_cb callback is set > > before mode_config.funcs is initialised as part of > > kms->funcs->hw_init(kms). > > > > The hpd DRM changes heading into 6.3 do appear to avoid the NULL-pointer > > dereference by moving the bridge->hpd_cb initialisation to > > drm_kms_helper_poll_init() as you mention above. I can confirm that as expected my reproducer no longer triggers with 6.3-rc1. > > The PMIC GLINK altmode driver still happily forwards notifications > > regardless of the DRM driver state though, which can lead to missed > > hotplug events. It seems you need to implement the > > hpd_enable()/disable() callbacks and either cache or not enable events > > in fw until the DRM driver is ready. > > > > It's not clear to me what the expectation from the DRM framework is on > this point. We register a drm_bridge which is only capable of signaling > HPD events (DRM_BRIDGE_OP_HPD), not querying HPD state (DRM_BRIDGE_OP_DETECT). I think the assumption is that any bridge that can generate hotplug events also has a way of detecting whether it is connected (i.e. DRM_BRIDGE_OP_HPD => DRM_BRIDGE_OP_DETECT). The pmic_glink_altmode driver appears to be the only driver that sets DRM_BRIDGE_OP_HPD but not DRM_BRIDGE_OP_DETECT. > Does this imply that any such bridge must ensure that hpd events are > re-delivered once hpd_enable() has been invoked (we can't invoke it from > hpd_enable...)? > > Is it reasonable to do this retriggering in the altmode driver? Or is it > the job of the TCPM (it seems reasonable to not send the PAN_EN message > until we get hpd_enable()...)? Are you sure there is no way to query the firmware about the connected state? Otherwise, enabling the notification messages when hpd_enable() is called looks like it should work as the fw currently appears to always send a disconnected event followed by a connect event if connected. But that's not going to be enough unless you can also disable events in fw on hpd_disable() so that the state can again be updated on the next hpd_enable(). If that's not possible, it seems you need to cache the state in the driver and hope you get a notification after a suspend cycle if the state has changed. But in any case, the DRM documentation is pretty clear on that a bridge driver should not be calling drm_bridge_hpd_notify() until hpd_enable() is called (and also not after hpd_disable()) as the pmic_glink_altmode driver currently do. hpd_enable Enable hot plug detection. From now on the bridge shall call drm_bridge_hpd_notify() each time a change is detected in the output connection status, until hot plug detection gets disabled with hpd_disable. This callback is optional and shall only be implemented by bridges that support hot-plug notification without polling. Bridges that implement it shall also implement the hpd_disable callback and set the DRM_BRIDGE_OP_HPD flag in their drm_bridge->ops. https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html#c.drm_bridge_funcs Johan
