[Freedreno] [PATCH v5 6/9] drm/msm/dp: wait for hpd high before any sink interaction

[Sankeerth Billakanti]

> -----Original Message-----
> From: Doug Anderson <dianders at chromium.org>
> Sent: Friday, March 25, 2022 9:36 PM
> To: Sankeerth Billakanti (QUIC) <quic_sbillaka at quicinc.com>
> Cc: Stephen Boyd <swboyd at chromium.org>; David Airlie <airlied at linux.ie>;
> dri-devel <dri-devel at lists.freedesktop.org>; bjorn.andersson at linaro.org;
> Thierry Reding <thierry.reding at gmail.com>; Sam Ravnborg
> <sam at ravnborg.org>; Kuogee Hsieh (QUIC) <quic_khsieh at quicinc.com>;
> Andy Gross <agross at kernel.org>; open list:OPEN FIRMWARE AND
> FLATTENED DEVICE TREE BINDINGS <devicetree at vger.kernel.org>;
> quic_vproddut <quic_vproddut at quicinc.com>; linux-arm-msm <linux-arm-
> msm at vger.kernel.org>; Abhinav Kumar (QUIC)
> <quic_abhinavk at quicinc.com>; Rob Herring <robh+dt at kernel.org>; Sean
> Paul <seanpaul at chromium.org>; Sean Paul <sean at poorly.run>;
> quic_kalyant <quic_kalyant at quicinc.com>; LKML <linux-
> kernel at vger.kernel.org>; dmitry.baryshkov at linaro.org;
> krzk+dt at kernel.org; freedreno <freedreno at 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_sbillaka at quicinc.com> wrote:
> >
> > > -----Original Message-----
> > > From: Doug Anderson <dianders at chromium.org>
> > > Sent: Saturday, March 19, 2022 5:26 AM
> > > To: Stephen Boyd <swboyd at chromium.org>
> > > Cc: Sankeerth Billakanti (QUIC) <quic_sbillaka at quicinc.com>; open
> > > list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
> > > <devicetree at vger.kernel.org>; dri-devel
> > > <dri-devel at lists.freedesktop.org>;
> > > freedreno <freedreno at lists.freedesktop.org>; linux-arm-msm
> > > <linux-arm- msm at vger.kernel.org>; LKML
> > > <linux-kernel at vger.kernel.org>; Rob Clark <robdclark at gmail.com>;
> > > Sean Paul <seanpaul at chromium.org>; quic_kalyant
> > > <quic_kalyant at quicinc.com>; Abhinav Kumar (QUIC)
> > > <quic_abhinavk at quicinc.com>; Kuogee Hsieh (QUIC)
> > > <quic_khsieh at quicinc.com>; Andy Gross <agross at kernel.org>;
> > > bjorn.andersson at linaro.org; Rob Herring <robh+dt at kernel.org>;
> > > krzk+dt at kernel.org; Sean Paul <sean at poorly.run>; David Airlie
> > > <airlied at linux.ie>; Daniel Vetter <daniel at ffwll.ch>; Thierry Reding
> > > <thierry.reding at gmail.com>; Sam Ravnborg <sam at ravnborg.org>;
> > > dmitry.baryshkov at linaro.org; quic_vproddut
> > > <quic_vproddut at 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 at 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.