[PATCH 1/7] dt-bindings: display/msm/dsi: allow specifying TE source

[Abhinav Kumar]

On 5/23/2024 2:58 AM, Dmitry Baryshkov wrote:
> On Thu, 23 May 2024 at 02:57, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>
>>
>>
>> On 5/22/2024 1:05 PM, Dmitry Baryshkov wrote:
>>> On Wed, 22 May 2024 at 21:38, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote:
>>>>> Command mode panels provide TE signal back to the DSI host to signal
>>>>> that the frame display has completed and update of the image will not
>>>>> cause tearing. Usually it is connected to the first GPIO with the
>>>>> mdp_vsync function, which is the default. In such case the property can
>>>>> be skipped.
>>>>>
>>>>
>>>> This is a good addition overall. Some comments below.
>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>>> ---
>>>>>     .../bindings/display/msm/dsi-controller-main.yaml        | 16 ++++++++++++++++
>>>>>     1 file changed, 16 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
>>>>> index 1fa28e976559..c1771c69b247 100644
>>>>> --- a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
>>>>> +++ b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
>>>>> @@ -162,6 +162,21 @@ properties:
>>>>>                     items:
>>>>>                       enum: [ 0, 1, 2, 3 ]
>>>>>
>>>>> +              qcom,te-source:
>>>>> +                $ref: /schemas/types.yaml#/definitions/string
>>>>> +                description:
>>>>> +                  Specifies the source of vsync signal from the panel used for
>>>>> +                  tearing elimination. The default is mdp_gpio0.
>>>>
>>>> panel --> command mode panel?
>>>>
>>>>> +                enum:
>>>>> +                  - mdp_gpio0
>>>>> +                  - mdp_gpio1
>>>>> +                  - mdp_gpio2
>>>>
>>>> are gpio0, gpio1 and gpio2 referring to the vsync_p, vsync_s and vsync_e
>>>> sources?
>>>
>>> No idea, unfortunately. They are gpioN or just mdp_vsync all over the
>>> place. For the reference, in case of the SDM845 and Pixel3 the signal
>>> is routed through SoC GPIO12.
>>>
>>
>> GPIO12 on sdm845 is mdp_vsync_e.
>>
>> Thats why I think its better we use mdp_vsync_p/s/e instead of mdp_gpio0/1/2
> 
> Sure. This matches pins description. Are you fine with changing
> defines in DPU driver to VSYNC_P / _S / _E too ?
> 

Sorry for the delay in responding.

As per the software docs, the registers still use GPIO0/1/2.

Only the pin descriptions use vsync_p/s/e.

Hence I think we can make DPU driver to use 0/1/2.

>>
>>>> In that case wouldnt it be better to name it like that?
>>>>
>>>>> +                  - timer0
>>>>> +                  - timer1
>>>>> +                  - timer2
>>>>> +                  - timer3
>>>>> +                  - timer4
>>>>> +
>>>>
>>>> These are indicating watchdog timer sources right?
>>>
>>> Yes.
>>>

ack.

>>>>
>>>>>         required:
>>>>>           - port at 0
>>>>>           - port at 1
>>>>> @@ -452,6 +467,7 @@ examples:
>>>>>                               dsi0_out: endpoint {
>>>>>                                        remote-endpoint = <&sn65dsi86_in>;
>>>>>                                        data-lanes = <0 1 2 3>;
>>>>> +                                   qcom,te-source = "mdp_gpio2";
>>>>
>>>> I have a basic doubt on this. Should te-source should be in the input
>>>> port or the output one for the controller? Because TE is an input to the
>>>> DSI. And if the source is watchdog timer then it aligns even more as a
>>>> property of the input endpoint.
>>>
>>> I don't really want to split this. Both data-lanes and te-source are
>>> properties of the link between the DSI and panel. You can not really
>>> say which side has which property.
>>>
>>
>> TE is an input to the DSI from the panel. Between input and output port,
>> I think it belongs more to the input port.
> 
> Technically we don't have in/out ports. There are two ports which
> define a link between two instances. For example, if the panel
> supports getting information through DCS commands, then "panel input"
> also becomes "panel output".
> 

The ports are labeled dsi0_in and dsi0_out. Putting te source in 
dsi0_out really looks very confusing to me.

>>
>> I didnt follow why this is a link property. Sorry , I didnt follow the
>> split part.
> 
> There is a link between the DSI host and the panel. I don't want to
> end up in a situation when the properties of the link are split
> between two different nodes.
> 

It really depends on what the property denotes. I do not think this 
should be the reason to do it this way.

>>
>> If we are unsure about input vs output port, do you think its better we
>> make it a property of the main dsi node instead?
> 
> No, it's not a property of the DSI node at all. If the vendor rewires
> the panel GPIOs or (just for example regulators), it has nothing to do
> with the DSI host.

Ack to this.

> 
> --
> With best wishes
> Dmitry