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

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

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:
>>> 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