.../bindings/display/bridge/ti,sn65dsi86.yaml | 6 ++++++ drivers/gpu/drm/bridge/ti-sn65dsi86.c | 14 +++++++++----- 2 files changed, 15 insertions(+), 5 deletions(-)
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.1On 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
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
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;
>
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
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
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
© 2016 - 2026 Red Hat, Inc.