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

[Jayesh Choudhary <j-choudhary@ti.com>]

On 29/05/25 16:34, 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 "no-hpd" property.
> 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 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
> - Not picking up "Tested-by" tag as there are new changes
> 
> v2 patch link:
> <https://lore.kernel.org/all/20250508115433.449102-1-j-choudhary@ti.com/>
> 
> [1]: <https://lore.kernel.org/all/mwh35anw57d6nvre3sguetzq3miu4kd43rokegvul7fk266lys@5h2euthpk7vq/>
> 
> This would also require dts changes in all the nodes of sn65dsi86
> to ensure that they have no-hpd property.

DTS patch is posted now:
<https://lore.kernel.org/all/20250529112423.484232-1-j-choudhary@ti.com/>

> 
>   drivers/gpu/drm/bridge/ti-sn65dsi86.c | 40 +++++++++++++++++++++++----
>   1 file changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 60224f476e1d..e9ffc58acf58 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -190,6 +190,7 @@ struct ti_sn65dsi86 {
>   	u8				ln_assign;
>   	u8				ln_polrs;
>   	bool				comms_enabled;
> +	bool				no_hpd;
>   	struct mutex			comms_mutex;
>   
>   #if defined(CONFIG_OF_GPIO)
> @@ -352,8 +353,10 @@ static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata,
>   	 * change this to be conditional on someone specifying that HPD should
>   	 * be used.
>   	 */
> -	regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> -			   HPD_DISABLE);
> +
> +	if (pdata->no_hpd)
> +		regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> +				   HPD_DISABLE);
>   
>   	pdata->comms_enabled = true;
>   
> @@ -1195,9 +1198,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 +1231,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 +1259,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 +1349,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);
>   
> @@ -1935,6 +1963,8 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
>   		return dev_err_probe(dev, PTR_ERR(pdata->refclk),
>   				     "failed to get reference clock\n");
>   
> +	pdata->no_hpd = of_property_read_bool(dev->of_node, "no-hpd");
> +
>   	pm_runtime_enable(dev);
>   	pm_runtime_set_autosuspend_delay(pdata->dev, 500);
>   	pm_runtime_use_autosuspend(pdata->dev);