Message-ID: <CAD=FV=X2npwru2=2XYpzbvMTc-jFAGzG3xVDRuCp1_D=QoRWcQ@mail.gmail.com>
Date: Tue, 10 Jun 2025 09:22:20 -0700
From: Doug Anderson <dianders@...omium.org>
To: Jayesh Choudhary <j-choudhary@...com>
Cc: ernestvanhoecke@...il.com, Geert Uytterhoeven <geert@...ux-m68k.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, andrzej.hajda@...el.com, neil.armstrong@...aro.org, 
	rfoss@...nel.org, Laurent.pinchart@...asonboard.com, 
	dri-devel@...ts.freedesktop.org, tomi.valkeinen@...asonboard.com, 
	max.krummenacher@...adex.com, jonas@...boo.se, jernej.skrabec@...il.com, 
	maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de, 
	airlied@...il.com, simona@...ll.ch, kieran.bingham+renesas@...asonboard.com, 
	linux-kernel@...r.kernel.org, max.oss.09@...il.com, devarsht@...com, 
	Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@...r.kernel.org>
Subject: Re: [PATCH v3] drm/bridge: ti-sn65dsi86: Add HPD for DisplayPort
 connector type

Hi,

On Tue, Jun 10, 2025 at 12:43 AM Jayesh Choudhary <j-choudhary@...com> wrote:
>
> Hello Doug,
>
> On 10/06/25 03:39, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Jun 2, 2025 at 4:05 AM Jayesh Choudhary <j-choudhary@...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@...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@...adex.com>
> >>>>> Signed-off-by: Jayesh Choudhary <j-choudhary@...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.
> >
> >
>
>
> Ernest mentioned in v2[3] that when pdata->bridge.type is not
> set, the type field is 0 causing issue for eDP when enable_comms
> is called before auxiliary_driver probe.
>
> So it should be okay to check the bridge type for
> DRM_MODE_CONNECTOR_Unknown (0) OR DRM_MODE_CONNECTOR_eDP (14) and
> disable HPD in both case?
> Or equivalently using !(DRM_MODE_CONNECTOR_DisplayPort) as this bridge
> would support only these 2 connector types???

Yeah, I'd check for "not displayport".


> Then for DP case, it should behave like you mentioned: First disabling
> HPD till types is set in auxiliary_driver probe. And once set to 10,
> (for DRM_MODE_CONNECTOR_DisplayPort) enabling it for DisplayPort
> connector type.

Sounds reasonable to me.

-Doug

Powered by blists - more mailing lists