Hi Jayesh, CC devicetree
On Fri, 30 May 2025 at 04:54, Jayesh Choudhary <j-choudh...@ti.com> wrote: > 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/> Thanks for your patch! > > 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/> 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? Regardless, breaking backwards-compatibility with existing DTBs is definitely a no-go. > > 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 -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
