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

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

[Jayesh 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

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

[Kumar, Udit]

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;
>   

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

[Krzysztof Kozlowski]

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

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

[Jayesh Choudhary]

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

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

[kernel test robot]

Hi Jayesh,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on drm-exynos/exynos-drm-next linus/master drm/drm-next drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-misc/drm-misc-next drm-tip/drm-tip v6.15-rc3 next-20250424]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jayesh-Choudhary/drm-bridge-ti-sn65dsi86-Enable-HPD-functionality/20250424-185532
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20250424105432.255309-1-j-choudhary%40ti.com
patch subject: [RFC PATCH] drm/bridge: ti-sn65dsi86: Enable HPD functionality
config: s390-randconfig-002-20250425 (https://download.01.org/0day-ci/archive/20250425/202504252020.Sr5yxEZ5-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250425/202504252020.Sr5yxEZ5-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504252020.Sr5yxEZ5-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/bridge/ti-sn65dsi86.c:205: warning: Function parameter or struct member 'keep_hpd' not described in 'ti_sn65dsi86'


vim +205 drivers/gpu/drm/bridge/ti-sn65dsi86.c

cea86c5bb4425c Bjorn Andersson  2021-10-25  131  
27ed2b3f22ed60 Douglas Anderson 2020-05-07  132  /**
db0036db4851df Douglas Anderson 2021-04-23  133   * struct ti_sn65dsi86 - Platform data for ti-sn65dsi86 driver.
bf73537f411b0d Douglas Anderson 2021-04-23  134   * @bridge_aux:   AUX-bus sub device for MIPI-to-eDP bridge functionality.
bf73537f411b0d Douglas Anderson 2021-04-23  135   * @gpio_aux:     AUX-bus sub device for GPIO controller functionality.
a1e3667a9835e1 Douglas Anderson 2021-06-11  136   * @aux_aux:      AUX-bus sub device for eDP AUX channel functionality.
cea86c5bb4425c Bjorn Andersson  2021-10-25  137   * @pwm_aux:      AUX-bus sub device for PWM controller functionality.
bf73537f411b0d Douglas Anderson 2021-04-23  138   *
bf73537f411b0d Douglas Anderson 2021-04-23  139   * @dev:          Pointer to the top level (i2c) device.
27ed2b3f22ed60 Douglas Anderson 2020-05-07  140   * @regmap:       Regmap for accessing i2c.
27ed2b3f22ed60 Douglas Anderson 2020-05-07  141   * @aux:          Our aux channel.
27ed2b3f22ed60 Douglas Anderson 2020-05-07  142   * @bridge:       Our bridge.
27ed2b3f22ed60 Douglas Anderson 2020-05-07  143   * @connector:    Our connector.
27ed2b3f22ed60 Douglas Anderson 2020-05-07  144   * @host_node:    Remote DSI node.
27ed2b3f22ed60 Douglas Anderson 2020-05-07  145   * @dsi:          Our MIPI DSI source.
27ed2b3f22ed60 Douglas Anderson 2020-05-07  146   * @refclk:       Our reference clock.
4e5763f03e105f Laurent Pinchart 2021-06-24  147   * @next_bridge:  The bridge on the eDP side.
27ed2b3f22ed60 Douglas Anderson 2020-05-07  148   * @enable_gpio:  The GPIO we toggle to enable the bridge.
27ed2b3f22ed60 Douglas Anderson 2020-05-07  149   * @supplies:     Data for bulk enabling/disabling our regulators.
27ed2b3f22ed60 Douglas Anderson 2020-05-07  150   * @dp_lanes:     Count of dp_lanes we're using.
5bebaeadb30e8d Douglas Anderson 2020-05-18  151   * @ln_assign:    Value to program to the LN_ASSIGN register.
c42fb724cdf608 Douglas Anderson 2020-06-12  152   * @ln_polrs:     Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
b137406d9679ac Douglas Anderson 2021-04-23  153   * @comms_enabled: If true then communication over the aux channel is enabled.
b137406d9679ac Douglas Anderson 2021-04-23  154   * @comms_mutex:   Protects modification of comms_enabled.
27ed2b3f22ed60 Douglas Anderson 2020-05-07  155   *
27ed2b3f22ed60 Douglas Anderson 2020-05-07  156   * @gchip:        If we expose our GPIOs, this is used.
27ed2b3f22ed60 Douglas Anderson 2020-05-07  157   * @gchip_output: A cache of whether we've set GPIOs to output.  This
27ed2b3f22ed60 Douglas Anderson 2020-05-07  158   *                serves double-duty of keeping track of the direction and
27ed2b3f22ed60 Douglas Anderson 2020-05-07  159   *                also keeping track of whether we've incremented the
27ed2b3f22ed60 Douglas Anderson 2020-05-07  160   *                pm_runtime reference count for this pin, which we do
27ed2b3f22ed60 Douglas Anderson 2020-05-07  161   *                whenever a pin is configured as an output.  This is a
27ed2b3f22ed60 Douglas Anderson 2020-05-07  162   *                bitmap so we can do atomic ops on it without an extra
27ed2b3f22ed60 Douglas Anderson 2020-05-07  163   *                lock so concurrent users of our 4 GPIOs don't stomp on
27ed2b3f22ed60 Douglas Anderson 2020-05-07  164   *                each other's read-modify-write.
cea86c5bb4425c Bjorn Andersson  2021-10-25  165   *
cea86c5bb4425c Bjorn Andersson  2021-10-25  166   * @pchip:        pwm_chip if the PWM is exposed.
cea86c5bb4425c Bjorn Andersson  2021-10-25  167   * @pwm_enabled:  Used to track if the PWM signal is currently enabled.
cea86c5bb4425c Bjorn Andersson  2021-10-25  168   * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
cea86c5bb4425c Bjorn Andersson  2021-10-25  169   * @pwm_refclk_freq: Cache for the reference clock input to the PWM.
27ed2b3f22ed60 Douglas Anderson 2020-05-07  170   */
db0036db4851df Douglas Anderson 2021-04-23  171  struct ti_sn65dsi86 {
7aa83fbd712a6f Douglas Anderson 2023-06-13  172  	struct auxiliary_device		*bridge_aux;
7aa83fbd712a6f Douglas Anderson 2023-06-13  173  	struct auxiliary_device		*gpio_aux;
7aa83fbd712a6f Douglas Anderson 2023-06-13  174  	struct auxiliary_device		*aux_aux;
7aa83fbd712a6f Douglas Anderson 2023-06-13  175  	struct auxiliary_device		*pwm_aux;
bf73537f411b0d Douglas Anderson 2021-04-23  176  
a095f15c00e278 Sandeep Panda    2018-07-20  177  	struct device			*dev;
a095f15c00e278 Sandeep Panda    2018-07-20  178  	struct regmap			*regmap;
b814ec6d453577 Sean Paul        2018-08-13  179  	struct drm_dp_aux		aux;
a095f15c00e278 Sandeep Panda    2018-07-20  180  	struct drm_bridge		bridge;
e283820cbf8092 Douglas Anderson 2022-02-04  181  	struct drm_connector		*connector;
a095f15c00e278 Sandeep Panda    2018-07-20  182  	struct device_node		*host_node;
a095f15c00e278 Sandeep Panda    2018-07-20  183  	struct mipi_dsi_device		*dsi;
a095f15c00e278 Sandeep Panda    2018-07-20  184  	struct clk			*refclk;
4e5763f03e105f Laurent Pinchart 2021-06-24  185  	struct drm_bridge		*next_bridge;
a095f15c00e278 Sandeep Panda    2018-07-20  186  	struct gpio_desc		*enable_gpio;
a095f15c00e278 Sandeep Panda    2018-07-20  187  	struct regulator_bulk_data	supplies[SN_REGULATOR_SUPPLY_NUM];
fa8a66c68755ce Douglas Anderson 2019-12-18  188  	int				dp_lanes;
5bebaeadb30e8d Douglas Anderson 2020-05-18  189  	u8				ln_assign;
5bebaeadb30e8d Douglas Anderson 2020-05-18  190  	u8				ln_polrs;
b137406d9679ac Douglas Anderson 2021-04-23  191  	bool				comms_enabled;
b137406d9679ac Douglas Anderson 2021-04-23  192  	struct mutex			comms_mutex;
7d7f75fa49ee7f Jayesh Choudhary 2025-04-24  193  	bool				keep_hpd;
27ed2b3f22ed60 Douglas Anderson 2020-05-07  194  
9e4f358313920d Douglas Anderson 2020-06-12  195  #if defined(CONFIG_OF_GPIO)
27ed2b3f22ed60 Douglas Anderson 2020-05-07  196  	struct gpio_chip		gchip;
27ed2b3f22ed60 Douglas Anderson 2020-05-07  197  	DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
9e4f358313920d Douglas Anderson 2020-06-12  198  #endif
ed531feda7852d Uwe Kleine-König 2025-02-17  199  #if IS_REACHABLE(CONFIG_PWM)
596de87ddfc72f Uwe Kleine-König 2024-02-14  200  	struct pwm_chip			*pchip;
cea86c5bb4425c Bjorn Andersson  2021-10-25  201  	bool				pwm_enabled;
cea86c5bb4425c Bjorn Andersson  2021-10-25  202  	atomic_t			pwm_pin_busy;
cea86c5bb4425c Bjorn Andersson  2021-10-25  203  #endif
cea86c5bb4425c Bjorn Andersson  2021-10-25  204  	unsigned int			pwm_refclk_freq;
a095f15c00e278 Sandeep Panda    2018-07-20 @205  };
a095f15c00e278 Sandeep Panda    2018-07-20  206  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Doug Anderson]

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

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

[Max Krummenacher]

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

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