By default, HPD was disabled on SN65DSI86 bridge. When the driver was
added (commit "a095f15c00e27"), the HPD_DISABLE bit was set in pre-enable
call which was moved to other function calls subsequently.
Later on, commit "c312b0df3b13" added detect utility for DP mode. But with
HPD_DISABLE bit set, all the HPD events are disabled[0] and the debounced
state always return 1 (always connected state).

Set HPD_DISABLE bit conditionally based on connector type.
Since the HPD_STATE is reflected correctly only after waiting for debounce
time (~100-400ms) and adding this delay in detect() is not feasible
owing to the performace impact (glitches and frame drop), remove runtime
calls in detect() and add hpd_enable()/disable() bridge hooks with runtime
calls, to detect hpd properly without any delay.

[0]: <https://www.ti.com/lit/gpn/SN65DSI86> (Pg. 32)

Fixes: c312b0df3b13 ("drm/bridge: ti-sn65dsi86: Implement bridge connector operations for DP")
Cc: Max Krummenacher <max.krummenacher@toradex.com>
Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
---

Changelog v3->v4:
- Remove "no-hpd" support due to backward compatibility issues
- Change the conditional from "no-hpd" back to connector type
  but still address [1]

v3 patch link:
<https://lore.kernel.org/all/20250529110418.481756-1-j-choudhary@ti.com/>

Changelog v2->v3:
- Change conditional based on no-hpd property to address [1]
- Remove runtime calls in detect() with appropriate comments
- Add hpd_enable() and hpd_disable() in drm_bridge_funcs

v2 patch link:
<https://lore.kernel.org/all/20250508115433.449102-1-j-choudhary@ti.com/>

Changelog v1->v2:
- Drop additional property in bindings and use conditional.
- Instead of register read for HPD state, use dpcd read which returns 0
  for success and error codes for no connection
- Add relevant history for the required change in commit message
- Drop RFC subject-prefix in v2
- Add "Cc:" tag

v1 patch link:
<https://lore.kernel.org/all/20250424105432.255309-1-j-choudhary@ti.com/>

[1]: <https://lore.kernel.org/all/mwh35anw57d6nvre3sguetzq3miu4kd43rokegvul7fk266lys@5h2euthpk7vq/>

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 49 ++++++++++++++++++++++-----
 1 file changed, 41 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 60224f476e1d..b674a1aa1a37 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -348,12 +348,20 @@ 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.
+	 * For DisplayPort connector type, now that HPD makes sense and is
+	 * required, use the connector type to conditionally disable HPD.
+	 *
+	 * NOTE: The bridge type is set in auxiliary_bridge probe but
+	 * enable_comms() can be called before. So for DisplayPort,
+	 * HPD will be enabled once bridge type is set.
+	 * "no-hpd" property is not used properly in devicetree description
+	 * and hence is unreliable. Therefore HPD is being enabled using
+	 * this conditional.
 	 */
-	regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
-			   HPD_DISABLE);
+
+	if (pdata->bridge.type != DRM_MODE_CONNECTOR_DisplayPort)
+		regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
+				   HPD_DISABLE);
 
 	pdata->comms_enabled = true;
 
@@ -1195,9 +1203,17 @@ static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 	int val = 0;
 
-	pm_runtime_get_sync(pdata->dev);
+	/*
+	 * The chip won't report HPD right after being powered on as
+	 * HPD_DEBOUNCED_STATE reflects correct state only after the
+	 * debounce time (~100-400 ms).
+	 * So having pm_runtime_get_sync() and immediately reading
+	 * the register in detect() won't work, and adding delay()
+	 * in detect will have performace impact in display.
+	 * So remove runtime calls here.
+	 */
+
 	regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
-	pm_runtime_put_autosuspend(pdata->dev);
 
 	return val & HPD_DEBOUNCED_STATE ? connector_status_connected
 					 : connector_status_disconnected;
@@ -1220,6 +1236,20 @@ static void ti_sn65dsi86_debugfs_init(struct drm_bridge *bridge, struct dentry *
 	debugfs_create_file("status", 0600, debugfs, pdata, &status_fops);
 }
 
+static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
+{
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+
+	pm_runtime_get_sync(pdata->dev);
+}
+
+static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
+{
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+
+	pm_runtime_put_sync(pdata->dev);
+}
+
 static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
 	.attach = ti_sn_bridge_attach,
 	.detach = ti_sn_bridge_detach,
@@ -1234,6 +1264,8 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 	.debugfs_init = ti_sn65dsi86_debugfs_init,
+	.hpd_enable = ti_sn_bridge_hpd_enable,
+	.hpd_disable = ti_sn_bridge_hpd_disable,
 };
 
 static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
@@ -1322,7 +1354,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 			   ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
 
 	if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
-		pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT;
+		pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT |
+				    DRM_BRIDGE_OP_HPD;
 
 	drm_bridge_add(&pdata->bridge);
 
-- 
2.34.1

Hi,

On 11/06/2025 08:29, Jayesh Choudhary wrote:
> By default, HPD was disabled on SN65DSI86 bridge. When the driver was
> added (commit "a095f15c00e27"), the HPD_DISABLE bit was set in pre-enable
> call which was moved to other function calls subsequently.
> Later on, commit "c312b0df3b13" added detect utility for DP mode. But with
> HPD_DISABLE bit set, all the HPD events are disabled[0] and the debounced
> state always return 1 (always connected state).
> 
> Set HPD_DISABLE bit conditionally based on connector type.
> Since the HPD_STATE is reflected correctly only after waiting for debounce
> time (~100-400ms) and adding this delay in detect() is not feasible
> owing to the performace impact (glitches and frame drop), remove runtime
> calls in detect() and add hpd_enable()/disable() bridge hooks with runtime
> calls, to detect hpd properly without any delay.
> 
> [0]: <https://www.ti.com/lit/gpn/SN65DSI86> (Pg. 32)
> 
> Fixes: c312b0df3b13 ("drm/bridge: ti-sn65dsi86: Implement bridge connector operations for DP")
> Cc: Max Krummenacher <max.krummenacher@toradex.com>
> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> ---
> 
> Changelog v3->v4:
> - Remove "no-hpd" support due to backward compatibility issues
> - Change the conditional from "no-hpd" back to connector type
>   but still address [1]
> 
> v3 patch link:
> <https://lore.kernel.org/all/20250529110418.481756-1-j-choudhary@ti.com/>
> 
> Changelog v2->v3:
> - Change conditional based on no-hpd property to address [1]
> - Remove runtime calls in detect() with appropriate comments
> - Add hpd_enable() and hpd_disable() in drm_bridge_funcs
> 
> v2 patch link:
> <https://lore.kernel.org/all/20250508115433.449102-1-j-choudhary@ti.com/>
> 
> Changelog v1->v2:
> - Drop additional property in bindings and use conditional.
> - Instead of register read for HPD state, use dpcd read which returns 0
>   for success and error codes for no connection
> - Add relevant history for the required change in commit message
> - Drop RFC subject-prefix in v2
> - Add "Cc:" tag
> 
> v1 patch link:
> <https://lore.kernel.org/all/20250424105432.255309-1-j-choudhary@ti.com/>
> 
> [1]: <https://lore.kernel.org/all/mwh35anw57d6nvre3sguetzq3miu4kd43rokegvul7fk266lys@5h2euthpk7vq/>
> 
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 49 ++++++++++++++++++++++-----
>  1 file changed, 41 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 60224f476e1d..b674a1aa1a37 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -348,12 +348,20 @@ 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.
> +	 * For DisplayPort connector type, now that HPD makes sense and is

This comment also is written like a commit description ("now that HPD
makes sense").

> +	 * required, use the connector type to conditionally disable HPD.
> +	 *
> +	 * NOTE: The bridge type is set in auxiliary_bridge probe but
> +	 * enable_comms() can be called before. So for DisplayPort,
> +	 * HPD will be enabled once bridge type is set.
> +	 * "no-hpd" property is not used properly in devicetree description
> +	 * and hence is unreliable. Therefore HPD is being enabled using
> +	 * this conditional.
>  	 */
> -	regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> -			   HPD_DISABLE);
> +
> +	if (pdata->bridge.type != DRM_MODE_CONNECTOR_DisplayPort)
> +		regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> +				   HPD_DISABLE);
>  
>  	pdata->comms_enabled = true;
>  
> @@ -1195,9 +1203,17 @@ static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
>  	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>  	int val = 0;
>  
> -	pm_runtime_get_sync(pdata->dev);
> +	/*
> +	 * The chip won't report HPD right after being powered on as
> +	 * HPD_DEBOUNCED_STATE reflects correct state only after the
> +	 * debounce time (~100-400 ms).
> +	 * So having pm_runtime_get_sync() and immediately reading
> +	 * the register in detect() won't work, and adding delay()
> +	 * in detect will have performace impact in display.
> +	 * So remove runtime calls here.
> +	 */
> +

As Doug mentioned, the style here is more like a commit message. But
also, in my opinion, it would make more sense to have the comment in
hpd_enable() rather than having it here, mentioning that the chip needs
to be powered to have a reliable HPD due to the long debounce time.

>  	regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> -	pm_runtime_put_autosuspend(pdata->dev);
>  
>  	return val & HPD_DEBOUNCED_STATE ? connector_status_connected
>  					 : connector_status_disconnected;
> @@ -1220,6 +1236,20 @@ static void ti_sn65dsi86_debugfs_init(struct drm_bridge *bridge, struct dentry *
>  	debugfs_create_file("status", 0600, debugfs, pdata, &status_fops);
>  }
>  
> +static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
> +{
> +	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +
> +	pm_runtime_get_sync(pdata->dev);
> +}
> +
> +static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
> +{
> +	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +
> +	pm_runtime_put_sync(pdata->dev);

No need for _sync, afaics. Why not pm_runtime_put_autosuspend()?

 Tomi

> +}
> +
>  static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>  	.attach = ti_sn_bridge_attach,
>  	.detach = ti_sn_bridge_detach,
> @@ -1234,6 +1264,8 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>  	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>  	.debugfs_init = ti_sn65dsi86_debugfs_init,
> +	.hpd_enable = ti_sn_bridge_hpd_enable,
> +	.hpd_disable = ti_sn_bridge_hpd_disable,
>  };
>  
>  static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
> @@ -1322,7 +1354,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
>  			   ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
>  
>  	if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> -		pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT;
> +		pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT |
> +				    DRM_BRIDGE_OP_HPD;
>  
>  	drm_bridge_add(&pdata->bridge);
>

Hello Tomi,

On 12/06/25 11:55, Tomi Valkeinen wrote:
> Hi,
> 
> On 11/06/2025 08:29, Jayesh Choudhary wrote:
>> By default, HPD was disabled on SN65DSI86 bridge. When the driver was
>> added (commit "a095f15c00e27"), the HPD_DISABLE bit was set in pre-enable
>> call which was moved to other function calls subsequently.
>> Later on, commit "c312b0df3b13" added detect utility for DP mode. But with
>> HPD_DISABLE bit set, all the HPD events are disabled[0] and the debounced
>> state always return 1 (always connected state).
>>
>> Set HPD_DISABLE bit conditionally based on connector type.
>> Since the HPD_STATE is reflected correctly only after waiting for debounce
>> time (~100-400ms) and adding this delay in detect() is not feasible
>> owing to the performace impact (glitches and frame drop), remove runtime
>> calls in detect() and add hpd_enable()/disable() bridge hooks with runtime
>> calls, to detect hpd properly without any delay.
>>
>> [0]: <https://www.ti.com/lit/gpn/SN65DSI86> (Pg. 32)
>>
>> Fixes: c312b0df3b13 ("drm/bridge: ti-sn65dsi86: Implement bridge connector operations for DP")
>> Cc: Max Krummenacher <max.krummenacher@toradex.com>
>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>> ---
>>
>> Changelog v3->v4:
>> - Remove "no-hpd" support due to backward compatibility issues
>> - Change the conditional from "no-hpd" back to connector type
>>    but still address [1]
>>
>> v3 patch link:
>> <https://lore.kernel.org/all/20250529110418.481756-1-j-choudhary@ti.com/>
>>
>> Changelog v2->v3:
>> - Change conditional based on no-hpd property to address [1]
>> - Remove runtime calls in detect() with appropriate comments
>> - Add hpd_enable() and hpd_disable() in drm_bridge_funcs
>>
>> v2 patch link:
>> <https://lore.kernel.org/all/20250508115433.449102-1-j-choudhary@ti.com/>
>>
>> Changelog v1->v2:
>> - Drop additional property in bindings and use conditional.
>> - Instead of register read for HPD state, use dpcd read which returns 0
>>    for success and error codes for no connection
>> - Add relevant history for the required change in commit message
>> - Drop RFC subject-prefix in v2
>> - Add "Cc:" tag
>>
>> v1 patch link:
>> <https://lore.kernel.org/all/20250424105432.255309-1-j-choudhary@ti.com/>
>>
>> [1]: <https://lore.kernel.org/all/mwh35anw57d6nvre3sguetzq3miu4kd43rokegvul7fk266lys@5h2euthpk7vq/>
>>
>>   drivers/gpu/drm/bridge/ti-sn65dsi86.c | 49 ++++++++++++++++++++++-----
>>   1 file changed, 41 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> index 60224f476e1d..b674a1aa1a37 100644
>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> @@ -348,12 +348,20 @@ 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.
>> +	 * For DisplayPort connector type, now that HPD makes sense and is
> 
> This comment also is written like a commit description ("now that HPD
> makes sense").

I will make it more apt.

> 
>> +	 * required, use the connector type to conditionally disable HPD.
>> +	 *
>> +	 * NOTE: The bridge type is set in auxiliary_bridge probe but
>> +	 * enable_comms() can be called before. So for DisplayPort,
>> +	 * HPD will be enabled once bridge type is set.
>> +	 * "no-hpd" property is not used properly in devicetree description
>> +	 * and hence is unreliable. Therefore HPD is being enabled using
>> +	 * this conditional.
>>   	 */
>> -	regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
>> -			   HPD_DISABLE);
>> +
>> +	if (pdata->bridge.type != DRM_MODE_CONNECTOR_DisplayPort)
>> +		regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
>> +				   HPD_DISABLE);
>>   
>>   	pdata->comms_enabled = true;
>>   
>> @@ -1195,9 +1203,17 @@ static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
>>   	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>>   	int val = 0;
>>   
>> -	pm_runtime_get_sync(pdata->dev);
>> +	/*
>> +	 * The chip won't report HPD right after being powered on as
>> +	 * HPD_DEBOUNCED_STATE reflects correct state only after the
>> +	 * debounce time (~100-400 ms).
>> +	 * So having pm_runtime_get_sync() and immediately reading
>> +	 * the register in detect() won't work, and adding delay()
>> +	 * in detect will have performace impact in display.
>> +	 * So remove runtime calls here.
>> +	 */
>> +
> 
> As Doug mentioned, the style here is more like a commit message. But
> also, in my opinion, it would make more sense to have the comment in
> hpd_enable() rather than having it here, mentioning that the chip needs
> to be powered to have a reliable HPD due to the long debounce time.
> 

So a comment here that runtime references are handled through
hpd_enable() and hpd_disable()

And then in enable(), mentioning that device needs to be powered on
for reliable HPD in detect() due to debounce time

>>   	regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
>> -	pm_runtime_put_autosuspend(pdata->dev);
>>   
>>   	return val & HPD_DEBOUNCED_STATE ? connector_status_connected
>>   					 : connector_status_disconnected;
>> @@ -1220,6 +1236,20 @@ static void ti_sn65dsi86_debugfs_init(struct drm_bridge *bridge, struct dentry *
>>   	debugfs_create_file("status", 0600, debugfs, pdata, &status_fops);
>>   }
>>   
>> +static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
>> +{
>> +	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>> +
>> +	pm_runtime_get_sync(pdata->dev);
>> +}
>> +
>> +static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
>> +{
>> +	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>> +
>> +	pm_runtime_put_sync(pdata->dev);
> 
> No need for _sync, afaics. Why not pm_runtime_put_autosuspend()?

Okay.

> 
>   Tomi
> 
>> +}
>> +
>>   static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>>   	.attach = ti_sn_bridge_attach,
>>   	.detach = ti_sn_bridge_detach,
>> @@ -1234,6 +1264,8 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>>   	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>>   	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>>   	.debugfs_init = ti_sn65dsi86_debugfs_init,
>> +	.hpd_enable = ti_sn_bridge_hpd_enable,
>> +	.hpd_disable = ti_sn_bridge_hpd_disable,
>>   };
>>   
>>   static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
>> @@ -1322,7 +1354,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
>>   			   ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
>>   
>>   	if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
>> -		pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT;
>> +		pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT |
>> +				    DRM_BRIDGE_OP_HPD;
>>   
>>   	drm_bridge_add(&pdata->bridge);
>>   
> 

Thanks,
Jayesh

Hi,

On Tue, Jun 10, 2025 at 10:29 PM Jayesh Choudhary <j-choudhary@ti.com> wrote:
>
> @@ -1195,9 +1203,17 @@ static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
>         struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>         int val = 0;
>
> -       pm_runtime_get_sync(pdata->dev);
> +       /*
> +        * The chip won't report HPD right after being powered on as
> +        * HPD_DEBOUNCED_STATE reflects correct state only after the
> +        * debounce time (~100-400 ms).
> +        * So having pm_runtime_get_sync() and immediately reading
> +        * the register in detect() won't work, and adding delay()
> +        * in detect will have performace impact in display.
> +        * So remove runtime calls here.

That last sentence makes sense in a commit message, but not long term.
Someone reading the code later won't understand what "remove" means.
If you change "remove" to "omit" then it all makes sense, though. You
could also say that a pm_runtime reference will be grabbed by
ti_sn_bridge_hpd_enable().


> +        */
> +
>         regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> -       pm_runtime_put_autosuspend(pdata->dev);
>
>         return val & HPD_DEBOUNCED_STATE ? connector_status_connected
>                                          : connector_status_disconnected;
> @@ -1220,6 +1236,20 @@ static void ti_sn65dsi86_debugfs_init(struct drm_bridge *bridge, struct dentry *
>         debugfs_create_file("status", 0600, debugfs, pdata, &status_fops);
>  }
>
> +static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
> +{
> +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +
> +       pm_runtime_get_sync(pdata->dev);
> +}
> +
> +static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
> +{
> +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +
> +       pm_runtime_put_sync(pdata->dev);
> +}
> +
>  static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>         .attach = ti_sn_bridge_attach,
>         .detach = ti_sn_bridge_detach,
> @@ -1234,6 +1264,8 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>         .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>         .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>         .debugfs_init = ti_sn65dsi86_debugfs_init,
> +       .hpd_enable = ti_sn_bridge_hpd_enable,
> +       .hpd_disable = ti_sn_bridge_hpd_disable,
>  };
>
>  static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
> @@ -1322,7 +1354,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
>                            ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
>
>         if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> -               pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT;
> +               pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT |
> +                                   DRM_BRIDGE_OP_HPD;

I think you also need this in the "DRM_MODE_CONNECTOR_DisplayPort" if test:

/*
 * If comms were already enabled they would have been enabled
 * with the wrong value of HPD_DISABLE. Update it now. Comms
 * could be enabled if anyone is holding a pm_runtime reference
 * (like if a GPIO is in use). Note that in most cases nobody
 * is doing AUX channel xfers before the bridge is added so
 * HPD doesn't _really_ matter then. The only exception is in
 * the eDP case where the panel wants to read the EDID before
 * the bridge is added. We always consistently have HPD disabled
 * for eDP.
 */
mutex_lock(&pdata->comms_mutex);
if (pdata->comms_enabled)
  regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
    HPD_DISABLE, 0);
mutex_unlock(&pdata->comms_mutex);

Does that sound right?

Hello Doug,

On 12/06/25 03:08, Doug Anderson wrote:
> Hi,
> 
> On Tue, Jun 10, 2025 at 10:29 PM Jayesh Choudhary <j-choudhary@ti.com> wrote:
>>
>> @@ -1195,9 +1203,17 @@ static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
>>          struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>>          int val = 0;
>>
>> -       pm_runtime_get_sync(pdata->dev);
>> +       /*
>> +        * The chip won't report HPD right after being powered on as
>> +        * HPD_DEBOUNCED_STATE reflects correct state only after the
>> +        * debounce time (~100-400 ms).
>> +        * So having pm_runtime_get_sync() and immediately reading
>> +        * the register in detect() won't work, and adding delay()
>> +        * in detect will have performace impact in display.
>> +        * So remove runtime calls here.
> 
> That last sentence makes sense in a commit message, but not long term.
> Someone reading the code later won't understand what "remove" means.
> If you change "remove" to "omit" then it all makes sense, though. You
> could also say that a pm_runtime reference will be grabbed by
> ti_sn_bridge_hpd_enable().

Okay. Will edit this.

> 
> 
>> +        */
>> +
>>          regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
>> -       pm_runtime_put_autosuspend(pdata->dev);
>>
>>          return val & HPD_DEBOUNCED_STATE ? connector_status_connected
>>                                           : connector_status_disconnected;
>> @@ -1220,6 +1236,20 @@ static void ti_sn65dsi86_debugfs_init(struct drm_bridge *bridge, struct dentry *
>>          debugfs_create_file("status", 0600, debugfs, pdata, &status_fops);
>>   }
>>
>> +static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
>> +{
>> +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>> +
>> +       pm_runtime_get_sync(pdata->dev);
>> +}
>> +
>> +static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
>> +{
>> +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>> +
>> +       pm_runtime_put_sync(pdata->dev);
>> +}
>> +
>>   static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>>          .attach = ti_sn_bridge_attach,
>>          .detach = ti_sn_bridge_detach,
>> @@ -1234,6 +1264,8 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>>          .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>>          .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>>          .debugfs_init = ti_sn65dsi86_debugfs_init,
>> +       .hpd_enable = ti_sn_bridge_hpd_enable,
>> +       .hpd_disable = ti_sn_bridge_hpd_disable,
>>   };
>>
>>   static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
>> @@ -1322,7 +1354,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
>>                             ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
>>
>>          if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
>> -               pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT;
>> +               pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT |
>> +                                   DRM_BRIDGE_OP_HPD;
> 
> I think you also need this in the "DRM_MODE_CONNECTOR_DisplayPort" if test:
> 
> /*
>   * If comms were already enabled they would have been enabled
>   * with the wrong value of HPD_DISABLE. Update it now. Comms
>   * could be enabled if anyone is holding a pm_runtime reference
>   * (like if a GPIO is in use). Note that in most cases nobody
>   * is doing AUX channel xfers before the bridge is added so
>   * HPD doesn't _really_ matter then. The only exception is in
>   * the eDP case where the panel wants to read the EDID before
>   * the bridge is added. We always consistently have HPD disabled
>   * for eDP.
>   */
> mutex_lock(&pdata->comms_mutex);
> if (pdata->comms_enabled)
>    regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
>      HPD_DISABLE, 0);
> mutex_unlock(&pdata->comms_mutex);
> 
> Does that sound right?


Here I don't think it is necessary to add this because enable_comms
will be called again after probe either in hpd_enable() (in case
refclk exist) or pre_enable() (in case it doesn't) with correct value.

If this seems okay then I will roll v5 with just the typo fixed in the
comments in detect().

Warm Regards,
Jayesh

Hi,

On Wed, Jun 11, 2025 at 9:39 PM Jayesh Choudhary <j-choudhary@ti.com> wrote:
>
> Hello Doug,
>
> On 12/06/25 03:08, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Jun 10, 2025 at 10:29 PM Jayesh Choudhary <j-choudhary@ti.com> wrote:
> >>
> >> @@ -1195,9 +1203,17 @@ static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
> >>          struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> >>          int val = 0;
> >>
> >> -       pm_runtime_get_sync(pdata->dev);
> >> +       /*
> >> +        * The chip won't report HPD right after being powered on as
> >> +        * HPD_DEBOUNCED_STATE reflects correct state only after the
> >> +        * debounce time (~100-400 ms).
> >> +        * So having pm_runtime_get_sync() and immediately reading
> >> +        * the register in detect() won't work, and adding delay()
> >> +        * in detect will have performace impact in display.
> >> +        * So remove runtime calls here.
> >
> > That last sentence makes sense in a commit message, but not long term.
> > Someone reading the code later won't understand what "remove" means.
> > If you change "remove" to "omit" then it all makes sense, though. You
> > could also say that a pm_runtime reference will be grabbed by
> > ti_sn_bridge_hpd_enable().
>
> Okay. Will edit this.
>
> >
> >
> >> +        */
> >> +
> >>          regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> >> -       pm_runtime_put_autosuspend(pdata->dev);
> >>
> >>          return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> >>                                           : connector_status_disconnected;
> >> @@ -1220,6 +1236,20 @@ static void ti_sn65dsi86_debugfs_init(struct drm_bridge *bridge, struct dentry *
> >>          debugfs_create_file("status", 0600, debugfs, pdata, &status_fops);
> >>   }
> >>
> >> +static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
> >> +{
> >> +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> >> +
> >> +       pm_runtime_get_sync(pdata->dev);
> >> +}
> >> +
> >> +static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
> >> +{
> >> +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> >> +
> >> +       pm_runtime_put_sync(pdata->dev);
> >> +}
> >> +
> >>   static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> >>          .attach = ti_sn_bridge_attach,
> >>          .detach = ti_sn_bridge_detach,
> >> @@ -1234,6 +1264,8 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> >>          .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> >>          .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> >>          .debugfs_init = ti_sn65dsi86_debugfs_init,
> >> +       .hpd_enable = ti_sn_bridge_hpd_enable,
> >> +       .hpd_disable = ti_sn_bridge_hpd_disable,
> >>   };
> >>
> >>   static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
> >> @@ -1322,7 +1354,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> >>                             ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
> >>
> >>          if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
> >> -               pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT;
> >> +               pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT |
> >> +                                   DRM_BRIDGE_OP_HPD;
> >
> > I think you also need this in the "DRM_MODE_CONNECTOR_DisplayPort" if test:
> >
> > /*
> >   * If comms were already enabled they would have been enabled
> >   * with the wrong value of HPD_DISABLE. Update it now. Comms
> >   * could be enabled if anyone is holding a pm_runtime reference
> >   * (like if a GPIO is in use). Note that in most cases nobody
> >   * is doing AUX channel xfers before the bridge is added so
> >   * HPD doesn't _really_ matter then. The only exception is in
> >   * the eDP case where the panel wants to read the EDID before
> >   * the bridge is added. We always consistently have HPD disabled
> >   * for eDP.
> >   */
> > mutex_lock(&pdata->comms_mutex);
> > if (pdata->comms_enabled)
> >    regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
> >      HPD_DISABLE, 0);
> > mutex_unlock(&pdata->comms_mutex);
> >
> > Does that sound right?
>
>
> Here I don't think it is necessary to add this because enable_comms
> will be called again after probe either in hpd_enable() (in case
> refclk exist) or pre_enable() (in case it doesn't) with correct value.

I don't think that's necessarily true, is it? From my memory, this happens:

1. Main driver probe and we create the sub-devices, like the GPIO,
backlight, and AUX.

2. As soon as the GPIO probe happens, someone could conceivably claim
one of the GPIOs and set it as an output, which would cause a
"pm_runtime" reference to be held indefinitely.

3. After AUX probes, we create the bridge sub-device.

4. When the bridge probe runs, comms will still be enabled because the
"pm_runtime" reference keeps them on.

...there's also the issue that we use "autosuspend" and thus comms can
still be left on for a chunk of time even after there are no
"pm_runtime" references left.

-Doug

Hello Doug,

On 12/06/25 20:21, Doug Anderson wrote:
> Hi,
> 
> On Wed, Jun 11, 2025 at 9:39 PM Jayesh Choudhary <j-choudhary@ti.com> wrote:
>>
>> Hello Doug,
>>
>> On 12/06/25 03:08, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Tue, Jun 10, 2025 at 10:29 PM Jayesh Choudhary <j-choudhary@ti.com> wrote:
>>>>
>>>> @@ -1195,9 +1203,17 @@ static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
>>>>           struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>>>>           int val = 0;
>>>>
>>>> -       pm_runtime_get_sync(pdata->dev);
>>>> +       /*
>>>> +        * The chip won't report HPD right after being powered on as
>>>> +        * HPD_DEBOUNCED_STATE reflects correct state only after the
>>>> +        * debounce time (~100-400 ms).
>>>> +        * So having pm_runtime_get_sync() and immediately reading
>>>> +        * the register in detect() won't work, and adding delay()
>>>> +        * in detect will have performace impact in display.
>>>> +        * So remove runtime calls here.
>>>
>>> That last sentence makes sense in a commit message, but not long term.
>>> Someone reading the code later won't understand what "remove" means.
>>> If you change "remove" to "omit" then it all makes sense, though. You
>>> could also say that a pm_runtime reference will be grabbed by
>>> ti_sn_bridge_hpd_enable().
>>
>> Okay. Will edit this.
>>
>>>
>>>
>>>> +        */
>>>> +
>>>>           regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
>>>> -       pm_runtime_put_autosuspend(pdata->dev);
>>>>
>>>>           return val & HPD_DEBOUNCED_STATE ? connector_status_connected
>>>>                                            : connector_status_disconnected;
>>>> @@ -1220,6 +1236,20 @@ static void ti_sn65dsi86_debugfs_init(struct drm_bridge *bridge, struct dentry *
>>>>           debugfs_create_file("status", 0600, debugfs, pdata, &status_fops);
>>>>    }
>>>>
>>>> +static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
>>>> +{
>>>> +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>>>> +
>>>> +       pm_runtime_get_sync(pdata->dev);
>>>> +}
>>>> +
>>>> +static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
>>>> +{
>>>> +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>>>> +
>>>> +       pm_runtime_put_sync(pdata->dev);
>>>> +}
>>>> +
>>>>    static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>>>>           .attach = ti_sn_bridge_attach,
>>>>           .detach = ti_sn_bridge_detach,
>>>> @@ -1234,6 +1264,8 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>>>>           .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>>>>           .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>>>>           .debugfs_init = ti_sn65dsi86_debugfs_init,
>>>> +       .hpd_enable = ti_sn_bridge_hpd_enable,
>>>> +       .hpd_disable = ti_sn_bridge_hpd_disable,
>>>>    };
>>>>
>>>>    static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
>>>> @@ -1322,7 +1354,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
>>>>                              ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
>>>>
>>>>           if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
>>>> -               pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT;
>>>> +               pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT |
>>>> +                                   DRM_BRIDGE_OP_HPD;
>>>
>>> I think you also need this in the "DRM_MODE_CONNECTOR_DisplayPort" if test:
>>>
>>> /*
>>>    * If comms were already enabled they would have been enabled
>>>    * with the wrong value of HPD_DISABLE. Update it now. Comms
>>>    * could be enabled if anyone is holding a pm_runtime reference
>>>    * (like if a GPIO is in use). Note that in most cases nobody
>>>    * is doing AUX channel xfers before the bridge is added so
>>>    * HPD doesn't _really_ matter then. The only exception is in
>>>    * the eDP case where the panel wants to read the EDID before
>>>    * the bridge is added. We always consistently have HPD disabled
>>>    * for eDP.
>>>    */
>>> mutex_lock(&pdata->comms_mutex);
>>> if (pdata->comms_enabled)
>>>     regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
>>>       HPD_DISABLE, 0);
>>> mutex_unlock(&pdata->comms_mutex);
>>>
>>> Does that sound right?
>>
>>
>> Here I don't think it is necessary to add this because enable_comms
>> will be called again after probe either in hpd_enable() (in case
>> refclk exist) or pre_enable() (in case it doesn't) with correct value.
> 
> I don't think that's necessarily true, is it? From my memory, this happens:
> 
> 1. Main driver probe and we create the sub-devices, like the GPIO,
> backlight, and AUX.
> 
> 2. As soon as the GPIO probe happens, someone could conceivably claim
> one of the GPIOs and set it as an output, which would cause a
> "pm_runtime" reference to be held indefinitely.

I did not consider this.

> 
> 3. After AUX probes, we create the bridge sub-device.
> 
> 4. When the bridge probe runs, comms will still be enabled because the
> "pm_runtime" reference keeps them on.
> 
> ...there's also the issue that we use "autosuspend" and thus comms can
> still be left on for a chunk of time even after there are no
> "pm_runtime" references left.


It makes sense.

I will add this change, edit a couple of comments mentioned and make
suspend asynchronous as suggested by Tomi (mark_last_bit and
put_autosuspend) in next revision.

Thanks,
Jayesh


> 
> -Doug