dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH] drm/bridge: ti-sn65dsi86: Enable HPD functionality
@ 2025-04-24 10:54 Jayesh Choudhary
  2025-04-25  1:32 ` Kumar, Udit
  2025-04-25  5:34 ` Krzysztof Kozlowski
  0 siblings, 2 replies; 7+ messages in thread
From: Jayesh Choudhary @ 2025-04-24 10:54 UTC (permalink / raw)
  To: dianders, andrzej.hajda, neil.armstrong, rfoss, Laurent.pinchart,
	dri-devel, tomi.valkeinen
  Cc: jonas, jernej.skrabec, maarten.lankhorst, mripard, tzimmermann,
	airlied, simona, linux-kernel, j-choudhary

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.
+
   vccio-supply:
     description: A 1.8V supply that powers the digital IOs.
 
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index f72675766e01..4081cc957c6c 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -191,6 +191,7 @@ struct ti_sn65dsi86 {
 	u8				ln_polrs;
 	bool				comms_enabled;
 	struct mutex			comms_mutex;
+	bool				keep_hpd;
 
 #if defined(CONFIG_OF_GPIO)
 	struct gpio_chip		gchip;
@@ -348,12 +349,15 @@ static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata,
 	 * 200 ms.  We'll assume that the panel driver will have the hardcoded
 	 * delay in its prepare and always disable HPD.
 	 *
-	 * If HPD somehow makes sense on some future panel we'll have to
-	 * change this to be conditional on someone specifying that HPD should
-	 * be used.
+	 * If needed, use 'keep-hpd' property in the hardware description in
+	 * board file as a conditional specifying that HPD should be used.
 	 */
-	regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
-			   HPD_DISABLE);
+
+	pdata->keep_hpd = of_property_read_bool(pdata->dev->of_node, "keep-hpd");
+
+	if (!pdata->keep_hpd)
+		regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
+				   HPD_DISABLE);
 
 	pdata->comms_enabled = true;
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] drm/bridge: ti-sn65dsi86: Enable HPD functionality
  2025-04-24 10:54 [RFC PATCH] drm/bridge: ti-sn65dsi86: Enable HPD functionality Jayesh Choudhary
@ 2025-04-25  1:32 ` Kumar, Udit
  2025-04-28 21:15   ` Doug Anderson
  2025-04-25  5:34 ` Krzysztof Kozlowski
  1 sibling, 1 reply; 7+ messages in thread
From: Kumar, Udit @ 2025-04-25  1:32 UTC (permalink / raw)
  To: Jayesh Choudhary, dianders, andrzej.hajda, neil.armstrong, rfoss,
	Laurent.pinchart, dri-devel, tomi.valkeinen
  Cc: jonas, jernej.skrabec, maarten.lankhorst, mripard, tzimmermann,
	airlied, simona, linux-kernel, u-kumar1

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)


>     vccio-supply:
>       description: A 1.8V supply that powers the digital IOs.
>   
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index f72675766e01..4081cc957c6c 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -191,6 +191,7 @@ struct ti_sn65dsi86 {
>   	u8				ln_polrs;
>   	bool				comms_enabled;
>   	struct mutex			comms_mutex;
> +	bool				keep_hpd;
>   
>   #if defined(CONFIG_OF_GPIO)
>   	struct gpio_chip		gchip;
> @@ -348,12 +349,15 @@ static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata,
>   	 * 200 ms.  We'll assume that the panel driver will have the hardcoded
>   	 * delay in its prepare and always disable HPD.
>   	 *
> -	 * If HPD somehow makes sense on some future panel we'll have to
> -	 * change this to be conditional on someone specifying that HPD should
> -	 * be used.
> +	 * If needed, use 'keep-hpd' property in the hardware description in
> +	 * board file as a conditional specifying that HPD should be used.
>   	 */
> -	regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> -			   HPD_DISABLE);
> +
> +	pdata->keep_hpd = of_property_read_bool(pdata->dev->of_node, "keep-hpd");
> +
> +	if (!pdata->keep_hpd)
> +		regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> +				   HPD_DISABLE);

you may want to extend interrupt support in this driver if HPD is enabled.


>   
>   	pdata->comms_enabled = true;
>   

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] drm/bridge: ti-sn65dsi86: Enable HPD functionality
  2025-04-24 10:54 [RFC PATCH] drm/bridge: ti-sn65dsi86: Enable HPD functionality Jayesh Choudhary
  2025-04-25  1:32 ` Kumar, Udit
@ 2025-04-25  5:34 ` Krzysztof Kozlowski
  2025-04-25 11:42   ` Jayesh Choudhary
  1 sibling, 1 reply; 7+ messages in thread
From: Krzysztof Kozlowski @ 2025-04-25  5:34 UTC (permalink / raw)
  To: Jayesh Choudhary, dianders, andrzej.hajda, neil.armstrong, rfoss,
	Laurent.pinchart, dri-devel, tomi.valkeinen
  Cc: jonas, jernej.skrabec, maarten.lankhorst, mripard, tzimmermann,
	airlied, simona, linux-kernel

On 24/04/2025 12:54, 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.

Please run scripts/checkpatch.pl on the patches and fix reported
warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
patches and (probably) fix more warnings. Some warnings can be ignored,
especially from --strict run, but the code here looks like it needs a
fix. Feel free to get in touch if the warning is not clear.

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.
</form letter>

(But if intention was not to get review, then of course it is fine)

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] drm/bridge: ti-sn65dsi86: Enable HPD functionality
  2025-04-25  5:34 ` Krzysztof Kozlowski
@ 2025-04-25 11:42   ` Jayesh Choudhary
  0 siblings, 0 replies; 7+ messages in thread
From: Jayesh Choudhary @ 2025-04-25 11:42 UTC (permalink / raw)
  To: Krzysztof Kozlowski, dianders, andrzej.hajda, neil.armstrong,
	rfoss, Laurent.pinchart, dri-devel, tomi.valkeinen
  Cc: jonas, jernej.skrabec, maarten.lankhorst, mripard, tzimmermann,
	airlied, simona, linux-kernel

Hello Krzysztof,

On 25/04/25 11:04, Krzysztof Kozlowski wrote:
> On 24/04/2025 12:54, 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.
> 
> Please run scripts/checkpatch.pl on the patches and fix reported
> warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
> patches and (probably) fix more warnings. Some warnings can be ignored,
> especially from --strict run, but the code here looks like it needs a
> fix. Feel free to get in touch if the warning is not clear.
> 
> <form letter>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC. It might happen, that command when run on an older
> kernel, gives you outdated entries. Therefore please be sure you base
> your patches on recent Linux kernel.
> 
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> people, so fix your workflow. Tools might also fail if you work on some
> ancient tree (don't, instead use mainline) or work on fork of kernel
> (don't, instead use mainline). Just use b4 and everything should be
> fine, although remember about `b4 prep --auto-to-cc` if you added new
> patches to the patchset.
> 
> You missed at least devicetree list (maybe more), so this won't be
> tested by automated tooling. Performing review on untested code might be
> a waste of time.
> 
> Please kindly resend and include all necessary To/Cc entries.
> </form letter>
> 
> (But if intention was not to get review, then of course it is fine)
> 


I might have have taken unwarranted liberty while sending this patch
thinking that it was an RFC PATCH.
I was looking for comments from bridge driver maintainers to see
if this was correct approach or not. It was not ready for bindings
review yet.
This was misjudgement from my end.
(DO-NOT-MERGE tag should have been there I guess..)

I will adhere to proper flow for RFCs as well from here on.

Will resend this properly.

Thanks.
-Jayesh

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] drm/bridge: ti-sn65dsi86: Enable HPD functionality
  2025-04-25  1:32 ` Kumar, Udit
@ 2025-04-28 21:15   ` Doug Anderson
  2025-05-01  8:12     ` Max Krummenacher
  0 siblings, 1 reply; 7+ messages in thread
From: Doug Anderson @ 2025-04-28 21:15 UTC (permalink / raw)
  To: Kumar, Udit
  Cc: Jayesh Choudhary, andrzej.hajda, neil.armstrong, rfoss,
	Laurent.pinchart, dri-devel, tomi.valkeinen, jonas,
	jernej.skrabec, maarten.lankhorst, mripard, tzimmermann, airlied,
	simona, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] drm/bridge: ti-sn65dsi86: Enable HPD functionality
  2025-04-28 21:15   ` Doug Anderson
@ 2025-05-01  8:12     ` Max Krummenacher
  2025-05-01 12:11       ` Jayesh Choudhary
  0 siblings, 1 reply; 7+ messages in thread
From: Max Krummenacher @ 2025-05-01  8:12 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Kumar, Udit, Jayesh Choudhary, andrzej.hajda, neil.armstrong,
	rfoss, Laurent.pinchart, dri-devel, tomi.valkeinen, jonas,
	jernej.skrabec, maarten.lankhorst, mripard, tzimmermann, airlied,
	simona, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] drm/bridge: ti-sn65dsi86: Enable HPD functionality
  2025-05-01  8:12     ` Max Krummenacher
@ 2025-05-01 12:11       ` Jayesh Choudhary
  0 siblings, 0 replies; 7+ messages in thread
From: Jayesh Choudhary @ 2025-05-01 12:11 UTC (permalink / raw)
  To: Max Krummenacher, Doug Anderson
  Cc: Kumar, Udit, andrzej.hajda, neil.armstrong, rfoss,
	Laurent.pinchart, dri-devel, tomi.valkeinen, jonas,
	jernej.skrabec, maarten.lankhorst, mripard, tzimmermann, airlied,
	simona, linux-kernel

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@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/
> 

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2025-05-01 12:11 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-24 10:54 [RFC PATCH] drm/bridge: ti-sn65dsi86: Enable HPD functionality Jayesh Choudhary
2025-04-25  1:32 ` Kumar, Udit
2025-04-28 21:15   ` Doug Anderson
2025-05-01  8:12     ` Max Krummenacher
2025-05-01 12:11       ` Jayesh Choudhary
2025-04-25  5:34 ` Krzysztof Kozlowski
2025-04-25 11:42   ` Jayesh Choudhary

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).