Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

From: Dmitry Baryshkov
Date: Tue Apr 05 2022 - 21:10:52 EST

On 05/04/2022 20:02, Doug Anderson wrote:
Hi,

On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:
3. 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.

--
With best wishes
Dmitry