On Thu, 23 May 2024 at 02:57, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
>
>
>
> On 5/22/2024 1:05 PM, Dmitry Baryshkov wrote:
> > On Wed, 22 May 2024 at 21:38, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> 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@xxxxxxxxxx>
> >>> ---
> >>> .../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@0
> >>> - port@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
