-----Original Message----- From: Doug Anderson dianders@chromium.org Sent: Saturday, March 19, 2022 5:26 AM To: Stephen Boyd swboyd@chromium.org Cc: Sankeerth Billakanti (QUIC) quic_sbillaka@quicinc.com; open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS devicetree@vger.kernel.org; dri-devel dri-devel@lists.freedesktop.org; freedreno freedreno@lists.freedesktop.org; linux-arm-msm <linux-arm- msm@vger.kernel.org>; LKML linux-kernel@vger.kernel.org; Rob Clark robdclark@gmail.com; Sean Paul seanpaul@chromium.org; quic_kalyant quic_kalyant@quicinc.com; Abhinav Kumar (QUIC) quic_abhinavk@quicinc.com; Kuogee Hsieh (QUIC) quic_khsieh@quicinc.com; Andy Gross agross@kernel.org; bjorn.andersson@linaro.org; Rob Herring robh+dt@kernel.org; krzk+dt@kernel.org; Sean Paul sean@poorly.run; David Airlie airlied@linux.ie; Daniel Vetter daniel@ffwll.ch; Thierry Reding thierry.reding@gmail.com; Sam Ravnborg sam@ravnborg.org; dmitry.baryshkov@linaro.org; quic_vproddut quic_vproddut@quicinc.com Subject: Re: [PATCH v5 6/9] drm/msm/dp: wait for hpd high before any sink interaction
Hi,
On Fri, Mar 18, 2022 at 4:27 PM Stephen Boyd swboyd@chromium.org wrote:
Pushing hpd state checking into aux transactions looks like the wrong direction. Also, as I said up above I am concerned that even checking the GPIO won't work and we need some way to ask the bridge if HPD is asserted or not and then fallback to the GPIO method if the display phy/controller doesn't have support to check HPD internally. Something on top of DRM_BRIDGE_OP_HPD?
If we could somehow get the HPD status from the bridge in the panel driver it definitely would be convenient. It does feel like that's an improvement that could be done later, though. We've already landed a few instances of doing what's done here, like for parade-ps8640 and analogix_dp. I suspect designing a new mechanism might not be the most trivial.
What is done in the bridge drivers is to wait for a fixed timeout and assume aux is ready? Or is it something else? If there's just a fixed timeout for the eDP case it sounds OK to do that for now and we can fine tune it later to actually check HPD status register before the panel tries to read EDID.
Right. For the parade chip (which is only used for eDP as far as I know--never DP) waits for up to 200 ms. See ps8640_ensure_hpd().
So I guess tl;dr to Sankeerth that it's OK for his patch to have the wait in the aux transfer function, but only for eDP. Other discussions here are about how we could make it better in future patches.
The aux transactions for external DP are initiated by the dp_display driver only after the display is hot plugged to the connector. The phy_init is necessary for the aux transactions to take place. So, for the DP case, like Doug mentioned below, this patch is introducing an overhead of three register reads to detect hpd_high before performing aux transactions. So, we felt this was okay to do for DP.
On the other hand, for eDP, it is necessary to wait for panel ready through this hpd connect status. Currently there is no way to know which type of connector it is in the dp_aux sub-module.
However, as the discussion suggested, to have the wait only for eDP, I am thinking to pass the connector_type information to aux sub-module and register different aux_transfer functions for eDP and DP. The eDP transfer function will wait for hpd_high and the DP transfer function will be same as the one before this patch.
What do you think?
I haven't actually tried it, but I suspect that to get something like what you're talking about we'd have to get the rest of drm to know that for eDP ports that it should assume something is connected _regardless_ of what the "detect" / "HPD" options say. Then we'd have to extend the edp-panel code to be able to be able to query the next bridge in the chain if a GPIO wasn't provided.
Can the panel interrogate the bridge chain somehow? It feels like either something in the chain should know the status of HPD (the case here where display controller in the SoC knows) or it should be a gpio to the panel (trogdor case). The bridge ops can implement DRM_BRIDGE_OP_HPD and the first bridge from the encoder that supports HPD can implement some sort of "wait for hpd asserted" function that the panel then uses once it powers up the panel during probe. If the panel has a gpio and nothing else in the chain can detect hpd then the panel polls the gpio, or it waits for the amount of time delay after powering on the panel if the panel's hpd function is called.
Yeah, there ought to be some way to make something like that work. I don't think it's just DRM_BRIDGE_OP_HPD, though, for a few reasons:
- That operation actually specifically means that HPD can cause an interrupt
and that the bridge promises to call drm_bridge_hpd_notify() when the interrupt occurs. It seems to work hand-in-hand with "DRM_BRIDGE_OP_DETECT". I know it's legit to advertise "detect" without "HPD" (you have an HPD line that can be polled but not cause interrupts) but I'd have to research whether it's legal to advertise "HPD" without "detect".
- If it were up to me, I'd rather avoid conflating what we need with the
existing "HPD" and "DETECT" ops. While the line is called "HPD" in the eDP spec, what we're looking for here is really a different concept. eDP panels are never hot plugged and are always present, so I'd personally rather it be a new OP like "OP_PANEL_READY". Of course, in whatever future patch we could always debate this.
- The main reason I'd prefer a different op is that I think using the existing
ops will confuse DRM (not just because I'm being pedantic). If DRM sees that the eDP controller driver advertises that it can "detect" and support "hpd" then it will use these functions to decide whether it should start turning on the panel. ...and it won't even try using the panel until one is detected, right? ...but that means that it won't be powered on and will never be detected. ;-) This is what I'm trying to get at: it's a different concept. The panel is always there and never hotplugged. The existing DRM ops (IMO) are for knowing whether a panel is physically present. For eDP the answer is always a resounding "yes", even if we have no actual evidence (because we can't gather evidence for an "off" panel). On eDP the HPD line simply means something different than on DP.
For the DP case this should not cause any significant overhead, right? HPD should always be asserted so this is basically just one extra IO read confirming that HPD is asserted which should be almost
nothing...
You're just about to do a whole bunch of IO reads/writes in order to program the AUX transaction anyway.
In the DP case the dongle/cable can be disconnected in the middle of aux transactions. If that happens we could be waiting a while in this transfer function to timeout looking for the status bit. The driver already gets an "unplug" irq when the cable is disconnected though so it would be better to figure out a way to stop the aux transactions quickly when that happens without having to read the hardware and poll the bit that we already know is doomed to timeout. I think apple dongles throw this logic for a loop though because the HDMI cable can be disconnected from the dongle and then we don't see an "unplug" irq, just the number of sinks
becomes 0. Maybe there's an irq_hpd event, not sure.
Ah, interesting. Having a DP cable unplugged in the middle of an aux transaction does seem like it could be a problem. What if we just wait in the case our bridge.type is "DRM_MODE_CONNECTOR_eDP"?
That
should be easy, right?
Sounds like it would work. Is this supposed to fix some DP case as well though? There were some patches to speed up aux failures when the dongle was unplugged but I haven't checked after that. I guess this waiting is only important for eDP because the edp-panel code is trying to read EDID and it isn't waiting for HPD to be asserted before doing that.
Right, I think this is only important for eDP.
That doesn't sound right. My understanding is that HPD will be asserted after the panel is powered up. Before that HPD is deasserted. Similarly, when we power down the panel, HPD will be deasserted. I guess DRM wants to assume that an eDP panel is always connected? That sounds like it might be OK as long as userspace doesn't use "connected" to know that it's OK to do things like read/write aux or push pixels to the panel
when HPD is deasserted.
IMO having userspace reading / writing aux directly and expecting it to work is a terrible idea anyway. It's _maybe_ sorta OK in the DP case, but it's really not good in the eDP case. To me it's sorta like expecting things to be amazing and foolproof when you go behind the kernel's back and write to an i2c device using `i2cset -f`. Sure, it might work, but it can also confuse the heck out of things. It also turns out to be a huge problem when you get to PSR because userspace will get errors if it tries to write to the AUX channel and the panel is in PSR mode. This came up in the context of Brian's analogix dp patches [1]. The right answer, in my mind, is to treat userspace accessing the AUX channel directly as more of a debug feature, at least for eDP panels.
If it's a debug feature then it should be removed from the system. The flow of data is passing through the kernel so if the kernel is getting confused about backdoor access over aux it should snoop the transactions and block things it doesn't like. I don't know the backstory on aux being exposed to userspace, but leaving it in a broken
state isn't good.
Agreed, it's not a great situation. :(
-Doug