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

[Jayesh Choudhary]

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.krummenac...@toradex.com>
Signed-off-by: Jayesh Choudhary <j-choudh...@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-choudh...@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-choudh...@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);