Hello Geert, Krzysztof,

(continuing discussion from both patches on this thread...)

On 30/05/25 13:25, Geert Uytterhoeven wrote:

Hi Jayesh,

CC devicetree

On Fri, 30 May 2025 at 04:54, Jayesh Choudhary [off-list ref] wrote:

quoted

On 29/05/25 16:34, Jayesh Choudhary wrote:

quoted

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 <redacted>
Signed-off-by: Jayesh Choudhary <redacted>
---

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/ (local)>

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

Thanks for your patch!

quoted

quoted

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/ (local)>

On all Renesas platforms handled by that patch, the DP bridge's HPD pin
is wired to the HPD pin on the mini-DP connector.  What am I missing?

If the bridge's HPD is connected to that of the connector, then I am
pretty certain HPD will not work for renesas platform. The detect hook
always gives "connected" state in the driver (even if it is unplugged).
Do you have different observation on your end?
If not, then we do need something like this patch while addressing the
backwards-compatibility concerns.

During v1 RFC[2], I did observe that renesas also have DisplayPort 
connector type and might require hpd, but since the support was
already there and no issue was raised, I assumed it does not require
HPD.

[2]: 
https://lore.kernel.org/all/01b43a16-cffa-457f-a2e1-87dd27869d18@ti.com/ (local)

Regardless, breaking backwards-compatibility with existing DTBs is
definitely a no-go.

Got it.
Let me try to figure out a way to fix it without messing it up.

Warm Regards,
Jayesh

quoted

quoted

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

Gr{oetje,eeting}s,

                         Geert