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

[Dmitry Baryshkov]

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 ?

>
> >> 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.
> >
> >>
> >>>        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".

>
> 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.

>
> 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.

--
With best wishes
Dmitry