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

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

[Geert Uytterhoeven]

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?

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

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

[Krzysztof Kozlowski]

On 30/05/2025 09:55, 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/>


This does not work like that. You cannot change DTS in other projects,
other users of this ABI. What's more, you cannot change old DTS, unless
you have a time machine.


Best regards,
Krzysztof

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

[Jayesh Choudhary]

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
> 

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

[Doug Anderson]

Hi,

On Mon, Jun 2, 2025 at 4:05 AM Jayesh Choudhary <j-choudhary@ti.com> wrote:
>
> 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.

FWIW, we are in a little bit of a sticky situation here. We were in a
bit of a bad place from the start because the Linux driver ignored HPD
from the beginning but we didn't actually document that people should
be setting the "no-hpd" property until a little bit later. You can see
some discussion about this in commit 1dbc979172af ("dt-bindings:
drm/bridge: ti-sn65dsi86: Document no-hpd") where I noted "this is
somewhat of a backward-incompatible change." ...but, at the time, it
wasn't really a big deal because there were very few users (the one in
tree at the time was cheza, which was a dev board used internally at
Google).

...so, as of that change in May of 2020, it was documented that eDP
users were _supposed_ to be setting NO_HPD. I even remember Bjorn
requesting the "or is otherwise unusable" phrasing because we pretty
much wanted to set this property on everyone using sn65dsi86 as eDP
(even if they have HPD hooked up) because the debouncing time is so
long that it was better to hardcode the max delay instead of reading
the HPD line. Of course, even though we documented that they were
supposed to have the "no-hpd" property didn't necessarily mean that
everyone did. The code has never enforced it. I don't believe it even
checks the property...

So if there are dts files out there that don't set the property and
they were submitted after the bindings change in 2020, _technically_
they've been wrong the whole time. We're not changing history by
adding a new requirement so much as fixing broken DTS files. Although
the Linux driver always allowed them to get away with being broken,
technically DTS is separate from Linux so if they've been violating
the bindings then they've been wrong. :-P That being said, they've
been working and it would be nice to keep them working if we can, but
one could make an argument that maybe it would be OK to require them
to change...


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

While a bit on the ugly side, it seems like perhaps you could just do this:

1. If enable_comms is called before the bridge probe happens, just go
ahead and disable HPD.

2. When the bridge probe happens, if you notice that HPD should be
enabled and comms are on you can just enable HPD then (grabbing the
comms_mutex while doing it).

3. Any subsequent enable_comms called after the bridge probe happens
shouldn't disable HPD.

...you'd probably want a comment about the fact that "no-hpd" property
is unreliable, which is why we can't figure this out in a better way.


-Doug

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

[Jayesh Choudhary]

Hello Doug,

On 10/06/25 03:39, Doug Anderson wrote:
> Hi,
> 
> On Mon, Jun 2, 2025 at 4:05 AM Jayesh Choudhary <j-choudhary@ti.com> wrote:
>>
>> 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.
> 
> FWIW, we are in a little bit of a sticky situation here. We were in a
> bit of a bad place from the start because the Linux driver ignored HPD
> from the beginning but we didn't actually document that people should
> be setting the "no-hpd" property until a little bit later. You can see
> some discussion about this in commit 1dbc979172af ("dt-bindings:
> drm/bridge: ti-sn65dsi86: Document no-hpd") where I noted "this is
> somewhat of a backward-incompatible change." ...but, at the time, it
> wasn't really a big deal because there were very few users (the one in
> tree at the time was cheza, which was a dev board used internally at
> Google).
> 
> ...so, as of that change in May of 2020, it was documented that eDP
> users were _supposed_ to be setting NO_HPD. I even remember Bjorn
> requesting the "or is otherwise unusable" phrasing because we pretty
> much wanted to set this property on everyone using sn65dsi86 as eDP
> (even if they have HPD hooked up) because the debouncing time is so
> long that it was better to hardcode the max delay instead of reading
> the HPD line. Of course, even though we documented that they were
> supposed to have the "no-hpd" property didn't necessarily mean that
> everyone did. The code has never enforced it. I don't believe it even
> checks the property...
> 
> So if there are dts files out there that don't set the property and
> they were submitted after the bindings change in 2020, _technically_
> they've been wrong the whole time. We're not changing history by
> adding a new requirement so much as fixing broken DTS files. Although
> the Linux driver always allowed them to get away with being broken,
> technically DTS is separate from Linux so if they've been violating
> the bindings then they've been wrong. :-P That being said, they've
> been working and it would be nice to keep them working if we can, but
> one could make an argument that maybe it would be OK to require them
> to change...
> 
> 
>> Got it.
>> Let me try to figure out a way to fix it without messing it up.
> 
> While a bit on the ugly side, it seems like perhaps you could just do this:
> 
> 1. If enable_comms is called before the bridge probe happens, just go
> ahead and disable HPD.
> 
> 2. When the bridge probe happens, if you notice that HPD should be
> enabled and comms are on you can just enable HPD then (grabbing the
> comms_mutex while doing it).
> 
> 3. Any subsequent enable_comms called after the bridge probe happens
> shouldn't disable HPD.
> 
> ...you'd probably want a comment about the fact that "no-hpd" property
> is unreliable, which is why we can't figure this out in a better way.
> 
> 


Ernest mentioned in v2[3] that when pdata->bridge.type is not
set, the type field is 0 causing issue for eDP when enable_comms
is called before auxiliary_driver probe.

So it should be okay to check the bridge type for
DRM_MODE_CONNECTOR_Unknown (0) OR DRM_MODE_CONNECTOR_eDP (14) and
disable HPD in both case?
Or equivalently using !(DRM_MODE_CONNECTOR_DisplayPort) as this bridge
would support only these 2 connector types???

Then for DP case, it should behave like you mentioned: First disabling
HPD till types is set in auxiliary_driver probe. And once set to 10,
(for DRM_MODE_CONNECTOR_DisplayPort) enabling it for DisplayPort
connector type.


[3]: 
https://lore.kernel.org/all/mwh35anw57d6nvre3sguetzq3miu4kd43rokegvul7fk266lys@5h2euthpk7vq/

Warm Regards,
Jayesh

> -Doug

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

[Doug Anderson]

Hi,

On Tue, Jun 10, 2025 at 12:43 AM Jayesh Choudhary <j-choudhary@ti.com> wrote:
>
> Hello Doug,
>
> On 10/06/25 03:39, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Jun 2, 2025 at 4:05 AM Jayesh Choudhary <j-choudhary@ti.com> wrote:
> >>
> >> 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.
> >
> > FWIW, we are in a little bit of a sticky situation here. We were in a
> > bit of a bad place from the start because the Linux driver ignored HPD
> > from the beginning but we didn't actually document that people should
> > be setting the "no-hpd" property until a little bit later. You can see
> > some discussion about this in commit 1dbc979172af ("dt-bindings:
> > drm/bridge: ti-sn65dsi86: Document no-hpd") where I noted "this is
> > somewhat of a backward-incompatible change." ...but, at the time, it
> > wasn't really a big deal because there were very few users (the one in
> > tree at the time was cheza, which was a dev board used internally at
> > Google).
> >
> > ...so, as of that change in May of 2020, it was documented that eDP
> > users were _supposed_ to be setting NO_HPD. I even remember Bjorn
> > requesting the "or is otherwise unusable" phrasing because we pretty
> > much wanted to set this property on everyone using sn65dsi86 as eDP
> > (even if they have HPD hooked up) because the debouncing time is so
> > long that it was better to hardcode the max delay instead of reading
> > the HPD line. Of course, even though we documented that they were
> > supposed to have the "no-hpd" property didn't necessarily mean that
> > everyone did. The code has never enforced it. I don't believe it even
> > checks the property...
> >
> > So if there are dts files out there that don't set the property and
> > they were submitted after the bindings change in 2020, _technically_
> > they've been wrong the whole time. We're not changing history by
> > adding a new requirement so much as fixing broken DTS files. Although
> > the Linux driver always allowed them to get away with being broken,
> > technically DTS is separate from Linux so if they've been violating
> > the bindings then they've been wrong. :-P That being said, they've
> > been working and it would be nice to keep them working if we can, but
> > one could make an argument that maybe it would be OK to require them
> > to change...
> >
> >
> >> Got it.
> >> Let me try to figure out a way to fix it without messing it up.
> >
> > While a bit on the ugly side, it seems like perhaps you could just do this:
> >
> > 1. If enable_comms is called before the bridge probe happens, just go
> > ahead and disable HPD.
> >
> > 2. When the bridge probe happens, if you notice that HPD should be
> > enabled and comms are on you can just enable HPD then (grabbing the
> > comms_mutex while doing it).
> >
> > 3. Any subsequent enable_comms called after the bridge probe happens
> > shouldn't disable HPD.
> >
> > ...you'd probably want a comment about the fact that "no-hpd" property
> > is unreliable, which is why we can't figure this out in a better way.
> >
> >
>
>
> Ernest mentioned in v2[3] that when pdata->bridge.type is not
> set, the type field is 0 causing issue for eDP when enable_comms
> is called before auxiliary_driver probe.
>
> So it should be okay to check the bridge type for
> DRM_MODE_CONNECTOR_Unknown (0) OR DRM_MODE_CONNECTOR_eDP (14) and
> disable HPD in both case?
> Or equivalently using !(DRM_MODE_CONNECTOR_DisplayPort) as this bridge
> would support only these 2 connector types???

Yeah, I'd check for "not displayport".


> Then for DP case, it should behave like you mentioned: First disabling
> HPD till types is set in auxiliary_driver probe. And once set to 10,
> (for DRM_MODE_CONNECTOR_DisplayPort) enabling it for DisplayPort
> connector type.

Sounds reasonable to me.

-Doug