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

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

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 <j-choudhary@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.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/>
> 
> 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-choudhary@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?

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/


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


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