[PATCH v4] drm/bridge: ti-sn65dsi86: Add HPD for DisplayPort connector type

[Jayesh Choudhary]

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 at toradex.com>
>> Signed-off-by: Jayesh Choudhary <j-choudhary at 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