[PATCH v3] drm/bridge: ti-sn65dsi86: Add HPD for DisplayPort connector type

[Doug Anderson]

Hi,

On Mon, Jun 2, 2025 at 4:05 AM Jayesh Choudhary <j-choudhary at ti.com> wrote:
>
> Hello Geert, Krzysztof,
>
> (continuing discussion from both patches on this thread...)
>
> On 30/05/25 13:25, Geert Uytterhoeven wrote:
> > Hi Jayesh,
> >
> > CC devicetree
> >
> > On Fri, 30 May 2025 at 04:54, Jayesh Choudhary <j-choudhary at ti.com> wrote:
> >> On 29/05/25 16:34, Jayesh Choudhary wrote:
> >>> By default, HPD was disabled on SN65DSI86 bridge. When the driver was
> >>> added (commit "a095f15c00e27"), the HPD_DISABLE bit was set in pre-enable
> >>> call which was moved to other function calls subsequently.
> >>> Later on, commit "c312b0df3b13" added detect utility for DP mode. But with
> >>> HPD_DISABLE bit set, all the HPD events are disabled[0] and the debounced
> >>> state always return 1 (always connected state).
> >>>
> >>> Set HPD_DISABLE bit conditionally based on "no-hpd" property.
> >>> Since the HPD_STATE is reflected correctly only after waiting for debounce
> >>> time (~100-400ms) and adding this delay in detect() is not feasible
> >>> owing to the performace impact (glitches and frame drop), remove runtime
> >>> calls in detect() and add hpd_enable()/disable() bridge hooks with runtime
> >>> calls, to detect hpd properly without any delay.
> >>>
> >>> [0]: <https://www.ti.com/lit/gpn/SN65DSI86> (Pg. 32)
> >>>
> >>> Fixes: c312b0df3b13 ("drm/bridge: ti-sn65dsi86: Implement bridge connector operations for DP")
> >>> Cc: Max Krummenacher <max.krummenacher at toradex.com>
> >>> Signed-off-by: Jayesh Choudhary <j-choudhary at ti.com>
> >>> ---
> >>>
> >>> Changelog v2->v3:
> >>> - Change conditional based on no-hpd property to address [1]
> >>> - Remove runtime calls in detect() with appropriate comments
> >>> - Add hpd_enable() and hpd_disable() in drm_bridge_funcs
> >>> - Not picking up "Tested-by" tag as there are new changes
> >>>
> >>> v2 patch link:
> >>> <https://lore.kernel.org/all/20250508115433.449102-1-j-choudhary@ti.com/>
> >>>
> >>> [1]: <https://lore.kernel.org/all/mwh35anw57d6nvre3sguetzq3miu4kd43rokegvul7fk266lys@5h2euthpk7vq/>
> >
> > Thanks for your patch!
> >
> >>> This would also require dts changes in all the nodes of sn65dsi86
> >>> to ensure that they have no-hpd property.
> >>
> >> DTS patch is posted now:
> >> <https://lore.kernel.org/all/20250529112423.484232-1-j-choudhary@ti.com/>
> >
> > On all Renesas platforms handled by that patch, the DP bridge's HPD pin
> > is wired to the HPD pin on the mini-DP connector.  What am I missing?
>
> If the bridge's HPD is connected to that of the connector, then I am
> pretty certain HPD will not work for renesas platform. The detect hook
> always gives "connected" state in the driver (even if it is unplugged).
> Do you have different observation on your end?
> If not, then we do need something like this patch while addressing the
> backwards-compatibility concerns.
>
> During v1 RFC[2], I did observe that renesas also have DisplayPort
> connector type and might require hpd, but since the support was
> already there and no issue was raised, I assumed it does not require
> HPD.
>
> [2]:
> https://lore.kernel.org/all/01b43a16-cffa-457f-a2e1-87dd27869d18@ti.com/
>
>
> >
> > Regardless, breaking backwards-compatibility with existing DTBs is
> > definitely a no-go.

FWIW, we are in a little bit of a sticky situation here. We were in a
bit of a bad place from the start because the Linux driver ignored HPD
from the beginning but we didn't actually document that people should
be setting the "no-hpd" property until a little bit later. You can see
some discussion about this in commit 1dbc979172af ("dt-bindings:
drm/bridge: ti-sn65dsi86: Document no-hpd") where I noted "this is
somewhat of a backward-incompatible change." ...but, at the time, it
wasn't really a big deal because there were very few users (the one in
tree at the time was cheza, which was a dev board used internally at
Google).

...so, as of that change in May of 2020, it was documented that eDP
users were _supposed_ to be setting NO_HPD. I even remember Bjorn
requesting the "or is otherwise unusable" phrasing because we pretty
much wanted to set this property on everyone using sn65dsi86 as eDP
(even if they have HPD hooked up) because the debouncing time is so
long that it was better to hardcode the max delay instead of reading
the HPD line. Of course, even though we documented that they were
supposed to have the "no-hpd" property didn't necessarily mean that
everyone did. The code has never enforced it. I don't believe it even
checks the property...

So if there are dts files out there that don't set the property and
they were submitted after the bindings change in 2020, _technically_
they've been wrong the whole time. We're not changing history by
adding a new requirement so much as fixing broken DTS files. Although
the Linux driver always allowed them to get away with being broken,
technically DTS is separate from Linux so if they've been violating
the bindings then they've been wrong. :-P That being said, they've
been working and it would be nice to keep them working if we can, but
one could make an argument that maybe it would be OK to require them
to change...


> Got it.
> Let me try to figure out a way to fix it without messing it up.

While a bit on the ugly side, it seems like perhaps you could just do this:

1. If enable_comms is called before the bridge probe happens, just go
ahead and disable HPD.

2. When the bridge probe happens, if you notice that HPD should be
enabled and comms are on you can just enable HPD then (grabbing the
comms_mutex while doing it).

3. Any subsequent enable_comms called after the bridge probe happens
shouldn't disable HPD.

...you'd probably want a comment about the fact that "no-hpd" property
is unreliable, which is why we can't figure this out in a better way.


-Doug