Message-ID: <CAD=FV=V1mNX-WidTAaENH66-2ExN=F_ovuX818uQGfc+Gsym1Q@mail.gmail.com>
Date: Wed, 21 May 2025 18:10:59 -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, 
	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
Subject: Re: [PATCH v2] drm/bridge: ti-sn65dsi86: Add HPD for DisplayPort
 connector type

Hi,

On Thu, May 8, 2025 at 4:54 AM Jayesh Choudhary <j-choudhary@...com> 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)
>
> Also, with the suspend and resume calls before every register access, the
> bridge starts with disconnected state and the HPD state is reflected
> correctly only after debounce time (400ms). However, adding this delay
> in the detect function causes frame drop and visible glitch in display.
>
> So to get the detect utility working properly for DP mode without any
> performance issues in display, instead of reading HPD state from the
> register, rely on aux communication. Use 'drm_dp_dpcd_read_link_status'
> to find if we have something connected at the sink.
>
> [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@...adex.com>
> Signed-off-by: Jayesh Choudhary <j-choudhary@...com>
> ---
>
> v1 patch link which was sent as RFC:
> <https://patchwork.kernel.org/project/dri-devel/patch/20250424105432.255309-1-j-choudhary@ti.com/>
>
> Changelog v1->v2:
> - Drop additional property in bindings and use conditional.
> - Instead of register read for HPD state, use dpcd read which returns 0
>   for success and error codes for no connection
> - Add relevant history for the required change in commit message
> - Drop RFC subject-prefix in v2 as v2 is in better state after discussion
>   in v1 and Max's mail thread
> - Add "Cc:" tag
>
> This approach does not make suspend/resume no-op and no additional
> delay needs to be added in the detect hook which causes frame drops.
>
> Here, I am adding conditional to HPD_DISABLE bit even when we are
> not using the register read to get HPD state. This is to prevent
> unnecessary register updates in every resume call.
> (It was adding to latency and leading to ~2-3 frame drop every 10 sec)
>
> Tested and verified on TI's J784S4-EVM platform:
> - Display comes up
> - Detect utility works with a couple of seconds latency.
>   But I guess without interrupt support, this is acceptable.
> - No frame-drop observed
>
> Discussion thread for Max's patch:
> <https://patchwork.kernel.org/project/dri-devel/patch/20250501074805.3069311-1-max.oss.09@gmail.com/>
>
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)

Sorry for the delay in responding. Things got a little crazy over the
last few weeks.


> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 60224f476e1d..9489e78b6da3 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -352,8 +352,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->bridge.type == DRM_MODE_CONNECTOR_eDP)
> +               regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> +                                  HPD_DISABLE);

Given your an Max's testing, I'm totally on-board with the above.

>
>         pdata->comms_enabled = true;
>
> @@ -1194,13 +1196,14 @@ 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;
> +       u8 link_status[DP_LINK_STATUS_SIZE];
>
> -       pm_runtime_get_sync(pdata->dev);
> -       regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
> -       pm_runtime_put_autosuspend(pdata->dev);
> +       val = drm_dp_dpcd_read_link_status(&pdata->aux, link_status);
>
> -       return val & HPD_DEBOUNCED_STATE ? connector_status_connected
> -                                        : connector_status_disconnected;
> +       if (val < 0)
> +               return connector_status_disconnected;
> +       else
> +               return connector_status_connected;

I'd really rather not do this. It took me a little while to realize
why this was working and also not being slow like your 400ms delay
was. I believe that each time you do the AUX transfer it grabs a
pm_runtime reference and then puts it with "autosuspend". Then you're
relying on the fact that detect is called often enough so that the
autosuspend doesn't actually hit so the next time your function runs
then it's fast. Is that accurate?

I'd rather see something like this in the bridge's probe (untested)

  if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) {
    pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT;

    /*
     * In order for DRM_BRIDGE_OP_DETECT to work in a reasonable
     * way we need to keep the bridge powered on all the time.
     * The bridge takes hundreds of milliseconds to debounce HPD
     * and we simply can't wait that amount of time in every call
     * to detect.
     */
    pm_runtime_get_sync(pdata->dev);
  }

...obviously you'd also need to find the right times to undo this in
error handling and in remove.

Nicely, this would be the same type of solution needed for if we ever
enabled interrupts.

-Doug

Powered by blists - more mailing lists