Messages in this thread | | | From | Doug Anderson <> | Date | Fri, 25 Mar 2022 09:05:51 -0700 | Subject | Re: [PATCH v5 6/9] drm/msm/dp: wait for hpd high before any sink interaction |
| |
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
| |