Hi, On Tue, Jun 10, 2025 at 12:43 AM Jayesh Choudhary <j-choudh...@ti.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-choudh...@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-choudh...@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.krummenac...@toradex.com> > >>>>> Signed-off-by: Jayesh Choudhary <j-choudh...@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-choudh...@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-choudh...@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-87dd27869...@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
