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

[Sankeerth Billakanti (QUIC)]

> -----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.

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:
> 
> 1. 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".
> 
> 2. 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.
> 
> 3. 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