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

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

Hello Doug,

On 12/06/25 20:21, Doug Anderson wrote:
> Hi,
> 
> On Wed, Jun 11, 2025 at 9:39 PM Jayesh Choudhary <j-choudhary@ti.com> wrote:
>>
>> Hello Doug,
>>
>> On 12/06/25 03:08, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Tue, Jun 10, 2025 at 10:29 PM Jayesh Choudhary <j-choudhary@ti.com> wrote:
>>>>
>>>> @@ -1195,9 +1203,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.
>>>
>>> That last sentence makes sense in a commit message, but not long term.
>>> Someone reading the code later won't understand what "remove" means.
>>> If you change "remove" to "omit" then it all makes sense, though. You
>>> could also say that a pm_runtime reference will be grabbed by
>>> ti_sn_bridge_hpd_enable().
>>
>> Okay. Will edit this.
>>
>>>
>>>
>>>> +        */
>>>> +
>>>>           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 +1236,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 +1264,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 +1354,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;
>>>
>>> I think you also need this in the "DRM_MODE_CONNECTOR_DisplayPort" if test:
>>>
>>> /*
>>>    * If comms were already enabled they would have been enabled
>>>    * with the wrong value of HPD_DISABLE. Update it now. Comms
>>>    * could be enabled if anyone is holding a pm_runtime reference
>>>    * (like if a GPIO is in use). Note that in most cases nobody
>>>    * is doing AUX channel xfers before the bridge is added so
>>>    * HPD doesn't _really_ matter then. The only exception is in
>>>    * the eDP case where the panel wants to read the EDID before
>>>    * the bridge is added. We always consistently have HPD disabled
>>>    * for eDP.
>>>    */
>>> mutex_lock(&pdata->comms_mutex);
>>> if (pdata->comms_enabled)
>>>     regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
>>>       HPD_DISABLE, 0);
>>> mutex_unlock(&pdata->comms_mutex);
>>>
>>> Does that sound right?
>>
>>
>> Here I don't think it is necessary to add this because enable_comms
>> will be called again after probe either in hpd_enable() (in case
>> refclk exist) or pre_enable() (in case it doesn't) with correct value.
> 
> I don't think that's necessarily true, is it? From my memory, this happens:
> 
> 1. Main driver probe and we create the sub-devices, like the GPIO,
> backlight, and AUX.
> 
> 2. As soon as the GPIO probe happens, someone could conceivably claim
> one of the GPIOs and set it as an output, which would cause a
> "pm_runtime" reference to be held indefinitely.

I did not consider this.

> 
> 3. After AUX probes, we create the bridge sub-device.
> 
> 4. When the bridge probe runs, comms will still be enabled because the
> "pm_runtime" reference keeps them on.
> 
> ...there's also the issue that we use "autosuspend" and thus comms can
> still be left on for a chunk of time even after there are no
> "pm_runtime" references left.


It makes sense.

I will add this change, edit a couple of comments mentioned and make
suspend asynchronous as suggested by Tomi (mark_last_bit and
put_autosuspend) in next revision.

Thanks,
Jayesh


> 
> -Doug