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

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

[Jayesh Choudhary]

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@toradex.com>
Signed-off-by: Jayesh Choudhary <j-choudhary@ti.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(-)

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);
 
 	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;
 }
 
 static const struct drm_edid *ti_sn_bridge_edid_read(struct drm_bridge *bridge,
-- 
2.34.1

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

[Max Krummenacher]

Hi

On Thu, May 08, 2025 at 05:24:33PM +0530, 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)
> 
> 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@toradex.com>
> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.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(-)
> 
> 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);
>  
>  	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;
>  }
>  
>  static const struct drm_edid *ti_sn_bridge_edid_read(struct drm_bridge *bridge,
> -- 
> 2.34.1
> 

This fixes the issues I have with detecting a not connected monitor on
boot.

As my HW has enable under software control but the power supplies not there
seem to be an issue to properly bring up the bridge after a disconnect and
then reconnect. I can work around that in the device tree by not adding the
optional enable gpio.

As such on a HW with a DP connector and a DP monitor:
Tested-by: Max Krummenacher <max.krummenacher@toradex.com>

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

[Doug Anderson]

Hi,

On Thu, May 8, 2025 at 4:54 AM Jayesh Choudhary <j-choudhary@ti.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@toradex.com>
> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.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

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

[Dmitry Baryshkov]

On Wed, May 21, 2025 at 06:10:59PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Thu, May 8, 2025 at 4:54 AM Jayesh Choudhary <j-choudhary@ti.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@toradex.com>
> > Signed-off-by: Jayesh Choudhary <j-choudhary@ti.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.

What about:
- keeping pm_runtime_get()/put_autosuspend() in detect, but..
- also adding .hpd_enable() / .hpd_disable() callbacks which would also
  get and put the runtime PM, making sure that there is no additional
  delay in .detect()?

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

-- 
With best wishes
Dmitry

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

[Jayesh Choudhary]

Hello Dmitry, Doug,

Thanks a lot for the review.

On 22/05/25 18:44, Dmitry Baryshkov wrote:
> On Wed, May 21, 2025 at 06:10:59PM -0700, Doug Anderson wrote:
>> Hi,
>>
>> On Thu, May 8, 2025 at 4:54 AM Jayesh Choudhary <j-choudhary@ti.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@toradex.com>
>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.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.
> 
> What about:
> - keeping pm_runtime_get()/put_autosuspend() in detect, but..
> - also adding .hpd_enable() / .hpd_disable() callbacks which would also
>    get and put the runtime PM, making sure that there is no additional
>    delay in .detect()?
> 

Keeping a reference alive via hpd_enable() fixes the issue.
Things works with the previous detect logic and I do not need to
rely on reading link status.


In hpd_enable()/disable(), I do not need to add anything else:

+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);
+}
+

Posting v3 with these changes.

Warm Regards,
Jayesh

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

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

[Doug Anderson]

Hi,

On Thu, May 22, 2025 at 6:14 AM Dmitry Baryshkov
<dmitry.baryshkov@oss.qualcomm.com> wrote:
>
> > > @@ -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.
>
> What about:
> - keeping pm_runtime_get()/put_autosuspend() in detect, but..

I guess? The problem is that if the calls in pm_runtime_get() actually
cause the device to be resumed then detect() will not actually work.
The chip simply won't report HPD right after being powered on because
it needs the debouncing time. ...so having the calls there is
misleading. Instead, I'd rather have a comment in there about why we
_don't_ have the pm_runtime_get() calls there...


> - also adding .hpd_enable() / .hpd_disable() callbacks which would also
>   get and put the runtime PM, making sure that there is no additional
>   delay in .detect()?

Sounds reasonable to me and sounds like it works.

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

[Ernest Van Hoecke]

Hi Jayesh,

First of all, thanks for your patch. I applied it to our 6.6-based
downstream kernel supporting a board I have here, and noticed some
strange behaviour with eDP now.

On Thu, May 08, 2025 at 05:24:33PM +0530, Jayesh Choudhary wrote:
> +	if (pdata->bridge.type == DRM_MODE_CONNECTOR_eDP)
> +		regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> +				   HPD_DISABLE);
>  

On my setup it seems that `pdata->bridge.type` is not yet set here,
because it executes before `ti_sn_bridge_probe`. For the DP use case,
this is not a problem because the type field is 0 (i.e., not
DRM_MODE_CONNECTOR_eDP) in that case. But for eDP, it means that we are
unexpectedly not disabling HDP.

With working HDP, everything is fine in the end for both DP and eDP. But
when the HDP line is not connected, eDP no longer works. So I wonder if
this breaks some functionality for weird eDP panels or board
implementations.

I could certainly be missing something; from my understanding it looks
like without a good HPD signal, the `ti_sn_bridge_probe` and quoted code
are stuck in a loop. `ti_sn65dsi86_enable_comms` runs but does not
disable HDP, after which the probe runs but fails and does not set the
type field, so the next `enable_comms` run fails to disable HDP again,
etc.

Kind regards,
Ernest

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

[Doug Anderson]

Hi,

On Mon, May 26, 2025 at 1:41 AM Ernest Van Hoecke
<ernestvanhoecke@gmail.com> wrote:
>
> Hi Jayesh,
>
> First of all, thanks for your patch. I applied it to our 6.6-based
> downstream kernel supporting a board I have here, and noticed some
> strange behaviour with eDP now.
>
> On Thu, May 08, 2025 at 05:24:33PM +0530, Jayesh Choudhary wrote:
> > +     if (pdata->bridge.type == DRM_MODE_CONNECTOR_eDP)
> > +             regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> > +                                HPD_DISABLE);
> >
>
> On my setup it seems that `pdata->bridge.type` is not yet set here,
> because it executes before `ti_sn_bridge_probe`. For the DP use case,
> this is not a problem because the type field is 0 (i.e., not
> DRM_MODE_CONNECTOR_eDP) in that case. But for eDP, it means that we are
> unexpectedly not disabling HDP.
>
> With working HDP, everything is fine in the end for both DP and eDP. But
> when the HDP line is not connected, eDP no longer works. So I wonder if
> this breaks some functionality for weird eDP panels or board
> implementations.
>
> I could certainly be missing something; from my understanding it looks
> like without a good HPD signal, the `ti_sn_bridge_probe` and quoted code
> are stuck in a loop. `ti_sn65dsi86_enable_comms` runs but does not
> disable HDP, after which the probe runs but fails and does not set the
> type field, so the next `enable_comms` run fails to disable HDP again,
> etc.

This does sound like a real problem.

I'm not sure I'll have the time to analyze it and come up with a
proposal myself right now, but Jayesh: you should make sure you
consider and address this issue before you send your next version.

Thanks!

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

[Jayesh Choudhary]

Hello Doug, Ernest,

On 27/05/25 21:14, Doug Anderson wrote:
> Hi,
> 
> On Mon, May 26, 2025 at 1:41 AM Ernest Van Hoecke
> <ernestvanhoecke@gmail.com> wrote:
>>
>> Hi Jayesh,
>>
>> First of all, thanks for your patch. I applied it to our 6.6-based
>> downstream kernel supporting a board I have here, and noticed some
>> strange behaviour with eDP now.
>>
>> On Thu, May 08, 2025 at 05:24:33PM +0530, Jayesh Choudhary wrote:
>>> +     if (pdata->bridge.type == DRM_MODE_CONNECTOR_eDP)
>>> +             regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
>>> +                                HPD_DISABLE);
>>>
>>
>> On my setup it seems that `pdata->bridge.type` is not yet set here,
>> because it executes before `ti_sn_bridge_probe`. For the DP use case,
>> this is not a problem because the type field is 0 (i.e., not
>> DRM_MODE_CONNECTOR_eDP) in that case. But for eDP, it means that we are
>> unexpectedly not disabling HDP.
>>
>> With working HDP, everything is fine in the end for both DP and eDP. But
>> when the HDP line is not connected, eDP no longer works. So I wonder if
>> this breaks some functionality for weird eDP panels or board
>> implementations.
>>
>> I could certainly be missing something; from my understanding it looks
>> like without a good HPD signal, the `ti_sn_bridge_probe` and quoted code
>> are stuck in a loop. `ti_sn65dsi86_enable_comms` runs but does not
>> disable HDP, after which the probe runs but fails and does not set the
>> type field, so the next `enable_comms` run fails to disable HDP again,
>> etc.
> 
> This does sound like a real problem.
> 
> I'm not sure I'll have the time to analyze it and come up with a
> proposal myself right now, but Jayesh: you should make sure you
> consider and address this issue before you send your next version.
> 
> Thanks!


I see that i2c_driver probe ti_sn65dsi86_probe() is called first
which calls ti_sn65dsi86_resume() and ti_sn65dsi86_enable_comms()
and after that auxiliary_driver probe ti_sn_bridge_probe() is called
where pdata->bridge.type is set.

As per the bindings, I see that we should have "no-hpd" property in
the device description for platforms with bad HPD or disconnected HPD.

Then we can read it in ti_sn65dsi86_probe() before resume call and use
it as a conditional instead.
Since I do not have any "bad HPD signal" board, I would need some
help validating this on such boards from Ernest.

Also, that would mean adding "no-hpd" to all the platforms using
sn65dsi86 for baseline first, even before the driver changes are in.
Because if driver changes go in first, this would enable HPD for all
the platforms. Whereas, the dts changes alone are harmless.
But still I am doubtful about dts changes getting in before driver
changes.
Considering this, please let me know the order of changes and I will
send out the patches accordingly.

Thanks,
Jayesh

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

[Ernest Van Hoecke]

Hi Jayesh,

On Wed, May 28, 2025 at 05:48:56PM +0530, Jayesh Choudhary wrote:
> As per the bindings, I see that we should have "no-hpd" property in
> the device description for platforms with bad HPD or disconnected HPD.
> 
> Then we can read it in ti_sn65dsi86_probe() before resume call and use
> it as a conditional instead.
> Since I do not have any "bad HPD signal" board, I would need some
> help validating this on such boards from Ernest.

This sounds like a good approach to me, during my investigation I also
thought the "no-hpd" property should enter into the story.

I will gladly help with testing and will add a jumper to my board so I
can turn it into a bad/good HPD signal board.

Thanks for the efforts and kind regards,
Ernest