> -----Original Message----- > From: Doug Anderson <diand...@chromium.org> > Sent: Friday, March 25, 2022 9:36 PM > To: Sankeerth Billakanti (QUIC) <quic_sbill...@quicinc.com> > Cc: Stephen Boyd <swb...@chromium.org>; David Airlie <airl...@linux.ie>; > dri-devel <dri-de...@lists.freedesktop.org>; bjorn.anders...@linaro.org; > Thierry Reding <thierry.red...@gmail.com>; Sam Ravnborg > <s...@ravnborg.org>; Kuogee Hsieh (QUIC) <quic_khs...@quicinc.com>; > Andy Gross <agr...@kernel.org>; open list:OPEN FIRMWARE AND > FLATTENED DEVICE TREE BINDINGS <devicet...@vger.kernel.org>; > quic_vproddut <quic_vprod...@quicinc.com>; linux-arm-msm <linux-arm- > m...@vger.kernel.org>; Abhinav Kumar (QUIC) > <quic_abhin...@quicinc.com>; Rob Herring <robh...@kernel.org>; Sean > Paul <seanp...@chromium.org>; Sean Paul <s...@poorly.run>; > quic_kalyant <quic_kaly...@quicinc.com>; LKML <linux- > ker...@vger.kernel.org>; dmitry.barysh...@linaro.org; > krzk...@kernel.org; freedreno <freedreno@lists.freedesktop.org> > Subject: Re: [PATCH v5 6/9] drm/msm/dp: wait for hpd high before any sink > interaction > > WARNING: This email originated from outside of Qualcomm. Please be wary > of any links or attachments, and do not enable macros. > > Hi, > > On Fri, Mar 25, 2022 at 8:54 AM Sankeerth Billakanti (QUIC) > <quic_sbill...@quicinc.com> wrote: > > > > > -----Original Message----- > > > From: Doug Anderson <diand...@chromium.org> > > > Sent: Saturday, March 19, 2022 5:26 AM > > > To: Stephen Boyd <swb...@chromium.org> > > > Cc: Sankeerth Billakanti (QUIC) <quic_sbill...@quicinc.com>; open > > > list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS > > > <devicet...@vger.kernel.org>; dri-devel > > > <dri-de...@lists.freedesktop.org>; > > > freedreno <freedreno@lists.freedesktop.org>; linux-arm-msm > > > <linux-arm- m...@vger.kernel.org>; LKML > > > <linux-ker...@vger.kernel.org>; Rob Clark <robdcl...@gmail.com>; > > > Sean Paul <seanp...@chromium.org>; quic_kalyant > > > <quic_kaly...@quicinc.com>; Abhinav Kumar (QUIC) > > > <quic_abhin...@quicinc.com>; Kuogee Hsieh (QUIC) > > > <quic_khs...@quicinc.com>; Andy Gross <agr...@kernel.org>; > > > bjorn.anders...@linaro.org; Rob Herring <robh...@kernel.org>; > > > krzk...@kernel.org; Sean Paul <s...@poorly.run>; David Airlie > > > <airl...@linux.ie>; Daniel Vetter <dan...@ffwll.ch>; Thierry Reding > > > <thierry.red...@gmail.com>; Sam Ravnborg <s...@ravnborg.org>; > > > dmitry.barysh...@linaro.org; quic_vproddut > > > <quic_vprod...@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 <swb...@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. > > Okay. Got it
> > 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 Okay. I will make that change. Thank you.
