Re: [RFC PATCH] drm/bridge: ti-sn65dsi86: Enable HPD functionality

[Jayesh Choudhary]

Hello Max,

On 01/05/25 13:42, Max Krummenacher wrote:
> On Mon, Apr 28, 2025 at 02:15:12PM -0700, Doug Anderson wrote:
> Hello Jayesh,
>
> > Hi,
> >
> > On Thu, Apr 24, 2025 at 6:32 PM Kumar, Udit <u-kumar1@xxxxxx> wrote:
> > > Hello Jayesh,
> > >
> > > On 4/24/2025 4:24 PM, Jayesh Choudhary wrote:
> > > > For TI SoC J784S4, the display pipeline looks like:
> > > > TIDSS -> CDNS-DSI -> SN65DSI86 -> DisplayConnector -> DisplaySink
> > > > This requires HPD to detect connection form the connector.
> > > > By default, the HPD is disabled for eDP. So enable it conditionally
> > > > based on a new flag 'keep-hpd' as mentioned in the comments in the
> > > > driver.
> > > >
> > > > Signed-off-by: Jayesh Choudhary <j-choudhary@xxxxxx>
> > > > ---
> > > >
> > > > Hello All,
> > > >
> > > > Sending this RFC patch to get some thoughts on hpd for sn65dsi86.
> > > >
> > > > Now that we have a usecase for hpd in sn65dsi86, I wanted to get
> > > > some comments on this approach to "NOT DISABLE" hpd in the bridge.
> > > > As the driver considers the eDP case, it disables hpd by default.
> > > > So I have added another property in the binding for keeping hpd
> > > > functionality (the name used is still debatable) and used it in
> > > > the driver.
> > > >
> > > > Is this approach okay?
> > > > Also should this have a "Fixes" tag?
> > >
> > > >    .../bindings/display/bridge/ti,sn65dsi86.yaml      |  6 ++++++
> > > >    drivers/gpu/drm/bridge/ti-sn65dsi86.c              | 14 +++++++++-----
> > > >    2 files changed, 15 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > > > index c93878b6d718..5948be612849 100644
> > > > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > > > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > > > @@ -34,6 +34,12 @@ properties:
> > > >          Set if the HPD line on the bridge isn't hooked up to anything or is
> > > >          otherwise unusable.
> > > >
> > > > +  keep-hpd:
> > > > +    type: boolean
> > > > +    description:
> > > > +      HPD is disabled in the bridge by default. Set it if HPD line makes
> > > > +      sense and is used.
> > > > +
> > >
> > > Here are my suggestions
> > >
> > > 1) use interrupt in binding as optional instead of keep-hpd
> > >
> > > 2) use interrupt field (if present to enable of disable HPD functions in
> > > driver)
> >
> > Officially we've already got a "no-hpd" specified in the device tree.
> > You're supposed to be specifying this if HPD isn't hooked up. It would
> > be best if we could use that property if possible. If we think that
> > using the lack of "no-hpd" will break someone then we should be
> > explicit about that.
> >
> > I'd also note that unless you've figured out a way to turn off the
> > awful debouncing that ti-sn65dsi86 does on HPD that using HPD (at
> > least for initial panel power on) only really makes sense for when
> > we're using ti-sn65dsi86 in "DP" mode. For initial eDP panel poweron
> > it was almost always faster to just wait the maximum delay of the
> > panel than to wait for ti-sn65dsi86 to finally report that HPD was
> > asserted.
> >
> > I could also note that it's possible to use the ti-sn65dsi86's "HPD"
> > detection even if the interrupt isn't hooked up, so I don't totally
> > agree with Udit's suggestion.
> >
> > I guess the summary of my thoughts then: If you want to enable HPD for
> > eDP, please explain why in the commit message. Are you using this to
> > detect "panel interrupt"? Somehow using it for PSR? Using it during
> > panel power on? If using it for panel power on, have you confirmed
> > that this has a benefit compared to using the panel's maximum delay?
> >
> > -Doug
>
> I'm working on a similar issue where the bridge is used to provide a
> connector to a display port monitor and hot pluging would be needed.
>
> Related, but not the issue here: We have two display outputs and the
> reported connected display without an actual monitor to report a
> video mode then confuses the system to also not use the second display.
>
> As I already have a solution which fixes my issue, hopefully not
> affecting the eDP use case a proposed that here:
>
> https://lore.kernel.org/all/20250501074805.3069311-1-max.oss.09@xxxxxxxxx/

I was also planning to use connector type for conditionally setting the
HPD_DISABLE bit. But I see that renesas uses sn65dsi86 bridge
(arch/arm64/boot/dts/renesas/r8a779h0-gray-hawk-single.dts and two more
platforms) connected to mini-dp-connector which also had the connector
type DRM_MODE_CONNECTOR_DisplayPort.

After your changes, their platform will also get affected.
I would assume that even their platform needs HPD functionality. They
should also be getting "always connected" state for that connector.
But I don't see any reported issues for their platform.

As Doug proposed, we can use no-hpd in other platforms and not keep it
in our platforms.

(I will test your patch on TI platform.)

Warm Regards,
Jayesh


> Regards,
> Max