Re: [RFC PATCH] drm/bridge: ti-sn65dsi86: Enable HPD functionality

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

Hello Max,

On 01/05/25 13:42, Max Krummenacher wrote:
> On Mon, Apr 28, 2025 at 02:15:12PM -0700, Doug Anderson wrote:
> Hello Jayesh,
> 
>> Hi,
>>
>> On Thu, Apr 24, 2025 at 6:32 PM Kumar, Udit <u-kumar1@ti.com> wrote:
>>>
>>> Hello Jayesh,
>>>
>>> On 4/24/2025 4:24 PM, Jayesh Choudhary wrote:
>>>> For TI SoC J784S4, the display pipeline looks like:
>>>> TIDSS -> CDNS-DSI -> SN65DSI86 -> DisplayConnector -> DisplaySink
>>>> This requires HPD to detect connection form the connector.
>>>> By default, the HPD is disabled for eDP. So enable it conditionally
>>>> based on a new flag 'keep-hpd' as mentioned in the comments in the
>>>> driver.
>>>>
>>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>>> ---
>>>>
>>>> Hello All,
>>>>
>>>> Sending this RFC patch to get some thoughts on hpd for sn65dsi86.
>>>>
>>>> Now that we have a usecase for hpd in sn65dsi86, I wanted to get
>>>> some comments on this approach to "NOT DISABLE" hpd in the bridge.
>>>> As the driver considers the eDP case, it disables hpd by default.
>>>> So I have added another property in the binding for keeping hpd
>>>> functionality (the name used is still debatable) and used it in
>>>> the driver.
>>>>
>>>> Is this approach okay?
>>>> Also should this have a "Fixes" tag?
>>>
>>>>
>>>>    .../bindings/display/bridge/ti,sn65dsi86.yaml      |  6 ++++++
>>>>    drivers/gpu/drm/bridge/ti-sn65dsi86.c              | 14 +++++++++-----
>>>>    2 files changed, 15 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
>>>> index c93878b6d718..5948be612849 100644
>>>> --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
>>>> +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
>>>> @@ -34,6 +34,12 @@ properties:
>>>>          Set if the HPD line on the bridge isn't hooked up to anything or is
>>>>          otherwise unusable.
>>>>
>>>> +  keep-hpd:
>>>> +    type: boolean
>>>> +    description:
>>>> +      HPD is disabled in the bridge by default. Set it if HPD line makes
>>>> +      sense and is used.
>>>> +
>>>
>>> Here are my suggestions
>>>
>>> 1) use interrupt in binding as optional instead of keep-hpd
>>>
>>> 2) use interrupt field (if present to enable of disable HPD functions in
>>> driver)
>>
>> Officially we've already got a "no-hpd" specified in the device tree.
>> You're supposed to be specifying this if HPD isn't hooked up. It would
>> be best if we could use that property if possible. If we think that
>> using the lack of "no-hpd" will break someone then we should be
>> explicit about that.
>>
>> I'd also note that unless you've figured out a way to turn off the
>> awful debouncing that ti-sn65dsi86 does on HPD that using HPD (at
>> least for initial panel power on) only really makes sense for when
>> we're using ti-sn65dsi86 in "DP" mode. For initial eDP panel poweron
>> it was almost always faster to just wait the maximum delay of the
>> panel than to wait for ti-sn65dsi86 to finally report that HPD was
>> asserted.
>>
>> I could also note that it's possible to use the ti-sn65dsi86's "HPD"
>> detection even if the interrupt isn't hooked up, so I don't totally
>> agree with Udit's suggestion.
>>
>> I guess the summary of my thoughts then: If you want to enable HPD for
>> eDP, please explain why in the commit message. Are you using this to
>> detect "panel interrupt"? Somehow using it for PSR? Using it during
>> panel power on? If using it for panel power on, have you confirmed
>> that this has a benefit compared to using the panel's maximum delay?
>>
>> -Doug
> 
> I'm working on a similar issue where the bridge is used to provide a
> connector to a display port monitor and hot pluging would be needed.
> 
> Related, but not the issue here: We have two display outputs and the
> reported connected display without an actual monitor to report a
> video mode then confuses the system to also not use the second display.
> 
> As I already have a solution which fixes my issue, hopefully not
> affecting the eDP use case a proposed that here:
> 
> https://lore.kernel.org/all/20250501074805.3069311-1-max.oss.09@gmail.com/
> 

I was also planning to use connector type for conditionally setting the
HPD_DISABLE bit. But I see that renesas uses sn65dsi86 bridge
(arch/arm64/boot/dts/renesas/r8a779h0-gray-hawk-single.dts and two more
platforms) connected to mini-dp-connector which also had the connector
type DRM_MODE_CONNECTOR_DisplayPort.

After your changes, their platform will also get affected.
I would assume that even their platform needs HPD functionality. They
should also be getting "always connected" state for that connector.
But I don't see any reported issues for their platform.

As Doug proposed, we can use no-hpd in other platforms and not keep it
in our platforms.

(I will test your patch on TI platform.)

Warm Regards,
Jayesh


> Regards,
> Max