Hi,
On Fri, Mar 25, 2022 at 8:54 AM Sankeerth Billakanti (QUIC) quic_sbillaka@quicinc.com wrote:
-----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.
Personally I'm not that upset about the 3 register reads. The problem Stephen pointed out is bigger. It's possible that a DP cable is unplugged _just_ as we started an AUX transaction. In that case we'll have a big delay here when we don't actually need one.
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.
Personally I wouldn't register two separate functions. You could just store a boolean in your structure and only wait for HPD if this is eDP. One extra "if" test doesn't seem like it justifies splitting off into two functions...
-Doug