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

[Max Krummenacher <max.oss.09@gmail.com>]

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@ti.com> 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@ti.com>
> > > ---
> > >
> > > 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@gmail.com/

Regards,
Max