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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD=FV=WvH73d78De3PrbiG7b6OaS_BysGtxQ=mJTj4z-h0LYWA@mail.gmail.com>
Date: Wed, 11 Jun 2025 14:38:43 -0700
From: Doug Anderson <dianders@...omium.org>
To: Jayesh Choudhary <j-choudhary@...com>
Cc: andrzej.hajda@...el.com, neil.armstrong@...aro.org, rfoss@...nel.org, 
	Laurent.pinchart@...asonboard.com, dri-devel@...ts.freedesktop.org, 
	tomi.valkeinen@...asonboard.com, max.krummenacher@...adex.com, 
	ernestvanhoecke@...il.com, jonas@...boo.se, jernej.skrabec@...il.com, 
	maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de, 
	airlied@...il.com, simona@...ll.ch, kieran.bingham+renesas@...asonboard.com, 
	linux-kernel@...r.kernel.org, max.oss.09@...il.com, devarsht@...com, 
	geert@...ux-m68k.org
Subject: Re: [PATCH v4] drm/bridge: ti-sn65dsi86: Add HPD for DisplayPort
 connector type

Hi,

On Tue, Jun 10, 2025 at 10:29 PM Jayesh Choudhary <j-choudhary@...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().


> +        */
> +
>         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?