On 05/04/2022 20:02, Doug Anderson wrote:
Hi,
On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
- For DP and eDP HPD means something a little different. Essentially
there are two concepts: a) is a display physically connected and b) is the display powered up and ready. For DP, the two are really tied together. From the kernel's point of view you never "power down" a DP display and you can't detect that it's physically connected until it's ready. Said another way, on you tie "is a display there" to the HPD line and the moment a display is there it's ready for you to do AUX transfers. For eDP, in the lowest power state of a display it _won't_ assert its "HPD" signal. However, it's still physically present. For eDP you simply have to _assume_ it's present without any actual proof since you can't get proof until you power it up. Thus for eDP, you report that the display is there as soon as we're asked. We can't _talk_ to the display yet, though. So in get_modes() we need to be able to power the display on enough to talk over the AUX channel to it. As part of this, we wait for the signal named "HPD" which really means "panel finished powering on" in this context.
NOTE: for aux transfer, we don't have the _display_ pipe and clocks running. We only have enough stuff running to do the AUX transfer. We're not clocking out pixels. We haven't fully powered on the display. The AUX transfer is designed to be something that can be done early _before_ you turn on the display.
OK, so basically that was a longwinded way of saying: yes, we could avoid the AUX transfer in probe, but we can't wait all the way to enable. We have to be able to transfer in get_modes(). If you think that's helpful I think it'd be a pretty easy patch to write even if it would look a tad bit awkward IMO. Let me know if you want me to post it up.
I think it would be a good idea. At least it will allow us to judge, which is the more correct way.
I'm still happy to prototype this, but the more I think about it the more it feels like a workaround for the Qualcomm driver. The eDP panel driver is actually given a pointer to the AUX bus at probe time. It's really weird to say that we can't do a transfer on it yet... As you said, this is a little sideband bus. It should be able to be used without all the full blown infra of the rest of the driver.
Yes, I have that feeling too. However I also have a feeling that just powering up the PHY before the bus probe is ... a hack. There are no obvious stopgaps for the driver not to power it down later.
A cleaner design might be to split all hotplug event handling from the dp_display, provide a lightweight state machine for the eDP and select which state machine to use depending on the hardware connector type. The dp_display_bind/unbind would probably also be duplicated and receive correct code flows for calling dp_parser_get_next_bridge, etc. Basically that means that depending on the device data we'd use either dp_display_comp_ops or (new) edp_comp_ops.
WDYT?
And I also think it might help the ti,sn65dsi86 driver, as it won't have to ensure that gpio is available during the AUX bus probe.
The ti,sn65dsi86 GPIO issue has been solved for a while, though so not sure why we need to do something there? I'm also unclear how it would have helped. In this discussion, we've agreed that the panel driver would still acquire resources during its probe time and the only thing that would be delayed would be the first AUX transfer. The GPIO is a resource here and it's ideal to acquire it at probe time so we could EPROBE_DEFER if needed.
Ack. I agree here. The world at 5 a.m. looks differently :D
BTW, another random idea, before you start coding.
We have the bridge's hpd_notify call. Currently it is called only by the means of drm_bridge_connector's HPD mechanism, tied to the bridge registering as DRM_BRIDGE_OP_HPD. It looks to me like it might be a perfect fit for the first aux-bus related reads.
We'd need to trigger it manually once and tie it to the new drm_panel_funcs callback, which in turn would probe the aux bus, create backlight, etc.
Regarding the Sankeerth's patch. I have been comparing it with the hpd_event_thread()'s calls. It looks to me like we should reuse dp_display_config_hpd() /EV_HPD_INIT_SETUP and maybe others.
What I'm trying to say is that if we split AUX probing and first AUX transfers, it would be possible to reuse a significant part of MSM DP HPD machine rather than hacking around it and replicating it manually.
I'm not sure I completely understand, but I'm pretty wary here. It's my assertion that all of the current "HPD" infrastructure in DRM all relates to the physical presence of the panel. If you start implementing these functions for eDP I think you're going to confuse the heck out of everything. The kernel will think that this is a display that's sometimes not there. Whenever the display is powered off then HPD will be low and it will look like there's no display. Nothing will ever try to power it on because it looks like there's no display.
I think your idea is to "trigger once" at bootup and then it all magically works, right? ...but what about after bootup? If you turn the display off for whatever reason (modeset or you simply close the lid of your laptop because you're using an external display) and then you want to use the eDP display again, how do you kickstart the process another time? You can't reboot, and when the display is off the HPD line is low.
I can't say it enough times, HPD on eDP _does not mean hot plug detect_. The panel is always there. HPD is really a "panel ready / panel notify" signal for eDP. That's fully what its function is.
Too many things being called HPD. I meant to trigger drm's hpd handling, which is not tied to the HPD pin for panel-edp. HPD pin is checked during panel runtime resume. However... let's probably go a simpler way.