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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAD=FV=WytPZCF-jcWFgXoAOoXOV61bw2_ftJbdbWZviHQqap5w@mail.gmail.com>
Date: Mon, 28 Apr 2025 14:15:12 -0700
From: Doug Anderson <dianders@...omium.org>
To: "Kumar, Udit" <u-kumar1@...com>
Cc: Jayesh Choudhary <j-choudhary@...com>, andrzej.hajda@...el.com, neil.armstrong@...aro.org, 
	rfoss@...nel.org, Laurent.pinchart@...asonboard.com, 
	dri-devel@...ts.freedesktop.org, tomi.valkeinen@...asonboard.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, 
	linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] drm/bridge: ti-sn65dsi86: Enable HPD functionality

Hi,

On Thu, Apr 24, 2025 at 6:32 PM Kumar, Udit <u-kumar1@...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@...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