[PATCH 0/7] drm/msm/dpu: handle non-default TE source pins

[PATCH 0/7] drm/msm/dpu: handle non-default TE source pins

[Dmitry Baryshkov]

Command-mode DSI panels need to signal the display controlller when
vsync happens, so that the device can start sending the next frame. Some
devices (Google Pixel 3) use a non-default pin, so additional
configuration is required. Add a way to specify this information in DT
and handle it in the DSI and DPU drivers.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
Dmitry Baryshkov (7):
      dt-bindings: display/msm/dsi: allow specifying TE source
      drm/msm/dpu: convert vsync source defines to the enum
      drm/msm/dsi: drop unused GPIOs handling
      drm/msm/dpu: pull the is_cmd_mode out of _dpu_encoder_update_vsync_source()
      drm/msm/dpu: rework vsync_source handling
      drm/msm/dsi: parse vsync source from device tree
      drm/msm/dpu: support setting the TE source

 .../bindings/display/msm/dsi-controller-main.yaml  | 16 ++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 11 ++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h        |  5 +--
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c        |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h        | 26 ++++++------
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h         |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c            | 44 ++++++++++++++++++++
 drivers/gpu/drm/msm/dsi/dsi.h                      |  1 +
 drivers/gpu/drm/msm/dsi/dsi_host.c                 | 48 +++++-----------------
 drivers/gpu/drm/msm/dsi/dsi_manager.c              |  5 +++
 drivers/gpu/drm/msm/msm_drv.h                      |  6 +++
 12 files changed, 106 insertions(+), 62 deletions(-)
---
base-commit: 75fa778d74b786a1608d55d655d42b480a6fa8bd
change-id: 20240514-dpu-handle-te-signal-82663c0211bd

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

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

[Dmitry Baryshkov]

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.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@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.
+                enum:
+                  - mdp_gpio0
+                  - mdp_gpio1
+                  - mdp_gpio2
+                  - timer0
+                  - timer1
+                  - timer2
+                  - timer3
+                  - timer4
+
     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";
                           };
                   };
            };

-- 
2.39.2

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

[Rob Herring]

On Mon, May 20, 2024 at 03:12:43PM +0300, 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.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@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.

default: mdp_gpio0

With that,

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

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

[Abhinav Kumar]

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@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?

In that case wouldnt it be better to name it like that?

> +                  - timer0
> +                  - timer1
> +                  - timer2
> +                  - timer3
> +                  - timer4
> +

These are indicating watchdog timer sources right?

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

>                             };
>                     };
>              };
> 

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

[Dmitry Baryshkov]

On Wed, 22 May 2024 at 21:38, Abhinav Kumar <quic_abhinavk@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@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.

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

> >                             };
> >                     };
> >              };
> >

-- 
With best wishes
Dmitry

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

[Abhinav Kumar]

On 5/22/2024 1:05 PM, Dmitry Baryshkov wrote:
> On Wed, 22 May 2024 at 21:38, Abhinav Kumar <quic_abhinavk@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@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

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

I didnt follow why this is a link property. Sorry , I didnt follow the 
split part.

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?

>>>                              };
>>>                      };
>>>               };
>>>
> 

Re: [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@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@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@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@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

Re: [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@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@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@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@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".
> 

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

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

[Dmitry Baryshkov]

On Thu, 30 May 2024 at 00:57, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 5/23/2024 2:58 AM, Dmitry Baryshkov wrote:
> > On Thu, 23 May 2024 at 02:57, Abhinav Kumar <quic_abhinavk@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@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@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.

OK, what about the DT? I like the vsync_p/_s/_e idea.

>
> >>
> >>>> 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@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".
> >
>
> The ports are labeled dsi0_in and dsi0_out. Putting te source in
> dsi0_out really looks very confusing to me.

dsi0_in is a port that connects DSI and DPU, so we should not be
putting panel-related data there.

I see two ports: mdss_dsi0_out and panel_in. Neither of them is
logical from this point of view. The TE source likewise isn't an input
to the panel, so we should not be using the panel_in port.

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

It denotes how the panel signals DPU that it finished processing the
data (please excuse me for possibly inaccurate description). However
there is no direct link between the panel and the DPU. So we should be
using a link between DSI host and the panel.

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



-- 
With best wishes
Dmitry

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

[Abhinav Kumar]

On 5/29/2024 5:02 PM, Dmitry Baryshkov wrote:
> On Thu, 30 May 2024 at 00:57, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 5/23/2024 2:58 AM, Dmitry Baryshkov wrote:
>>> On Thu, 23 May 2024 at 02:57, Abhinav Kumar <quic_abhinavk@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@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@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.
> 
> OK, what about the DT? I like the vsync_p/_s/_e idea.
> 

Yes, vsync_p/_s/_e for DT is fine with me.

My comment was only for driver.

So driver would do:

vsync_p -> gpio0
vsync_s -> gpio1
vsync_e -> gpio2

>>
>>>>
>>>>>> 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@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".
>>>
>>
>> The ports are labeled dsi0_in and dsi0_out. Putting te source in
>> dsi0_out really looks very confusing to me.
> 
> dsi0_in is a port that connects DSI and DPU, so we should not be
> putting panel-related data there.
> 

Yes, true. But here we are using the "out" port which like you mentioned 
is not logical either. Thats why I am not convinced or not sure if this 
is the right way to model this.

> I see two ports: mdss_dsi0_out and panel_in. Neither of them is
> logical from this point of view. The TE source likewise isn't an input
> to the panel, so we should not be using the panel_in port.
> 

>>
>>>>
>>>> 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.
> 
> It denotes how the panel signals DPU that it finished processing the
> data (please excuse me for possibly inaccurate description). However
> there is no direct link between the panel and the DPU. So we should be
> using a link between DSI host and the panel.
>

Yes, I totally agree that we should be using a link between DSI host and 
the panel.

My question from the beginning has been why the output port?

It looks like to me we need to have another input port to the controller 
then?

One from DPU and the other from panel?

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

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

[Dmitry Baryshkov]

On Wed, May 29, 2024 at 06:08:21PM -0700, Abhinav Kumar wrote:
> 
> 
> On 5/29/2024 5:02 PM, Dmitry Baryshkov wrote:
> > On Thu, 30 May 2024 at 00:57, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > > 
> > > 
> > > 
> > > On 5/23/2024 2:58 AM, Dmitry Baryshkov wrote:
> > > > On Thu, 23 May 2024 at 02:57, Abhinav Kumar <quic_abhinavk@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@quicinc.com> wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote:
> > > > > > > 
> > > > > > > >          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".
> > > > 
> > > 
> > > The ports are labeled dsi0_in and dsi0_out. Putting te source in
> > > dsi0_out really looks very confusing to me.
> > 
> > dsi0_in is a port that connects DSI and DPU, so we should not be
> > putting panel-related data there.
> > 
> 
> Yes, true. But here we are using the "out" port which like you mentioned is
> not logical either. Thats why I am not convinced or not sure if this is the
> right way to model this.
> 
> > I see two ports: mdss_dsi0_out and panel_in. Neither of them is
> > logical from this point of view. The TE source likewise isn't an input
> > to the panel, so we should not be using the panel_in port.
> > 
> 
> > > 
> > > > > 
> > > > > 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.
> > 
> > It denotes how the panel signals DPU that it finished processing the
> > data (please excuse me for possibly inaccurate description). However
> > there is no direct link between the panel and the DPU. So we should be
> > using a link between DSI host and the panel.
> > 
> 
> Yes, I totally agree that we should be using a link between DSI host and the
> panel.
> 
> My question from the beginning has been why the output port?
> 
> It looks like to me we need to have another input port to the controller
> then?
> 
> One from DPU and the other from panel?

Dear DT maintainers, could you please comment on the OF graph entries?
Are they considered to be unidirectional or bidirectional?

Would you suggest adding another arc to the OF graph in our case or is
it fine to have a signal generated by the panel in the 'panel_in' port?


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

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

[Krzysztof Kozlowski]

On 04/06/2024 17:14, Dmitry Baryshkov wrote:
>>>>>>
>>>>>> 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.
>>>
>>> It denotes how the panel signals DPU that it finished processing the
>>> data (please excuse me for possibly inaccurate description). However
>>> there is no direct link between the panel and the DPU. So we should be
>>> using a link between DSI host and the panel.
>>>
>>
>> Yes, I totally agree that we should be using a link between DSI host and the
>> panel.
>>
>> My question from the beginning has been why the output port?
>>
>> It looks like to me we need to have another input port to the controller
>> then?
>>
>> One from DPU and the other from panel?
> 
> Dear DT maintainers, could you please comment on the OF graph entries?
> Are they considered to be unidirectional or bidirectional?
> 
> Would you suggest adding another arc to the OF graph in our case or is
> it fine to have a signal generated by the panel in the 'panel_in' port?

Which pin are we talking about? DSI or panel? Commit msg suggests DSI,
so property is in DSI node part. Seems logical to me.

Best regards,
Krzysztof

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

[Dmitry Baryshkov]

On Tue, Jun 04, 2024 at 05:22:03PM +0200, Krzysztof Kozlowski wrote:
> On 04/06/2024 17:14, Dmitry Baryshkov wrote:
> >>>>>>
> >>>>>> 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.
> >>>
> >>> It denotes how the panel signals DPU that it finished processing the
> >>> data (please excuse me for possibly inaccurate description). However
> >>> there is no direct link between the panel and the DPU. So we should be
> >>> using a link between DSI host and the panel.
> >>>
> >>
> >> Yes, I totally agree that we should be using a link between DSI host and the
> >> panel.
> >>
> >> My question from the beginning has been why the output port?
> >>
> >> It looks like to me we need to have another input port to the controller
> >> then?
> >>
> >> One from DPU and the other from panel?
> > 
> > Dear DT maintainers, could you please comment on the OF graph entries?
> > Are they considered to be unidirectional or bidirectional?
> > 
> > Would you suggest adding another arc to the OF graph in our case or is
> > it fine to have a signal generated by the panel in the 'panel_in' port?
> 
> Which pin are we talking about? DSI or panel? Commit msg suggests DSI,
> so property is in DSI node part. Seems logical to me.

Input pin on the DSI side.

-- 
With best wishes
Dmitry

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

[Krzysztof Kozlowski]

On 04/06/2024 17:32, Dmitry Baryshkov wrote:
> On Tue, Jun 04, 2024 at 05:22:03PM +0200, Krzysztof Kozlowski wrote:
>> On 04/06/2024 17:14, Dmitry Baryshkov wrote:
>>>>>>>>
>>>>>>>> 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.
>>>>>
>>>>> It denotes how the panel signals DPU that it finished processing the
>>>>> data (please excuse me for possibly inaccurate description). However
>>>>> there is no direct link between the panel and the DPU. So we should be
>>>>> using a link between DSI host and the panel.
>>>>>
>>>>
>>>> Yes, I totally agree that we should be using a link between DSI host and the
>>>> panel.
>>>>
>>>> My question from the beginning has been why the output port?
>>>>
>>>> It looks like to me we need to have another input port to the controller
>>>> then?
>>>>
>>>> One from DPU and the other from panel?
>>>
>>> Dear DT maintainers, could you please comment on the OF graph entries?
>>> Are they considered to be unidirectional or bidirectional?
>>>
>>> Would you suggest adding another arc to the OF graph in our case or is
>>> it fine to have a signal generated by the panel in the 'panel_in' port?
>>
>> Which pin are we talking about? DSI or panel? Commit msg suggests DSI,
>> so property is in DSI node part. Seems logical to me.
> 
> Input pin on the DSI side.

So adding it to panel schema is not even possible thus I am not sure if
we discuss this option (maybe not, because it would be odd, considering
you got Rb tag!).

Adding some input node to DSI connecting panel output and DSI input...
for what? I mean, what sort of data would it represent?

Best regards,
Krzysztof

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

[Abhinav Kumar]

On 6/4/2024 8:36 AM, Krzysztof Kozlowski wrote:
> On 04/06/2024 17:32, Dmitry Baryshkov wrote:
>> On Tue, Jun 04, 2024 at 05:22:03PM +0200, Krzysztof Kozlowski wrote:
>>> On 04/06/2024 17:14, Dmitry Baryshkov wrote:
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>
>>>>>> It denotes how the panel signals DPU that it finished processing the
>>>>>> data (please excuse me for possibly inaccurate description). However
>>>>>> there is no direct link between the panel and the DPU. So we should be
>>>>>> using a link between DSI host and the panel.
>>>>>>
>>>>>
>>>>> Yes, I totally agree that we should be using a link between DSI host and the
>>>>> panel.
>>>>>
>>>>> My question from the beginning has been why the output port?
>>>>>
>>>>> It looks like to me we need to have another input port to the controller
>>>>> then?
>>>>>
>>>>> One from DPU and the other from panel?
>>>>
>>>> Dear DT maintainers, could you please comment on the OF graph entries?
>>>> Are they considered to be unidirectional or bidirectional?
>>>>
>>>> Would you suggest adding another arc to the OF graph in our case or is
>>>> it fine to have a signal generated by the panel in the 'panel_in' port?
>>>
>>> Which pin are we talking about? DSI or panel? Commit msg suggests DSI,
>>> so property is in DSI node part. Seems logical to me.
>>
>> Input pin on the DSI side.
> 
> So adding it to panel schema is not even possible thus I am not sure if
> we discuss this option (maybe not, because it would be odd, considering
> you got Rb tag!).
> 
> Adding some input node to DSI connecting panel output and DSI input...
> for what? I mean, what sort of data would it represent?
> 

TE pin is an input signal from the panel to the DSI host.

Today we have two ports in the DSI host node:

1) input to DSI node from DPU. This represents the pixel stream from DPU 
to DSI

2) DSI output node to represent pixel stream from DSI host to panel in

Now, please explain to me how does TE pin belongs to (2) because thats 
where this property has been added.

> Best regards,
> Krzysztof
> 

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

[Krzysztof Kozlowski]

On 04/06/2024 19:52, Abhinav Kumar wrote:
> 
> 
> On 6/4/2024 8:36 AM, Krzysztof Kozlowski wrote:
>> On 04/06/2024 17:32, Dmitry Baryshkov wrote:
>>> On Tue, Jun 04, 2024 at 05:22:03PM +0200, Krzysztof Kozlowski wrote:
>>>> On 04/06/2024 17:14, Dmitry Baryshkov wrote:
>>>>>>>>>>
>>>>>>>>>> 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.
>>>>>>>
>>>>>>> It denotes how the panel signals DPU that it finished processing the
>>>>>>> data (please excuse me for possibly inaccurate description). However
>>>>>>> there is no direct link between the panel and the DPU. So we should be
>>>>>>> using a link between DSI host and the panel.
>>>>>>>
>>>>>>
>>>>>> Yes, I totally agree that we should be using a link between DSI host and the
>>>>>> panel.
>>>>>>
>>>>>> My question from the beginning has been why the output port?
>>>>>>
>>>>>> It looks like to me we need to have another input port to the controller
>>>>>> then?
>>>>>>
>>>>>> One from DPU and the other from panel?
>>>>>
>>>>> Dear DT maintainers, could you please comment on the OF graph entries?
>>>>> Are they considered to be unidirectional or bidirectional?
>>>>>
>>>>> Would you suggest adding another arc to the OF graph in our case or is
>>>>> it fine to have a signal generated by the panel in the 'panel_in' port?
>>>>
>>>> Which pin are we talking about? DSI or panel? Commit msg suggests DSI,
>>>> so property is in DSI node part. Seems logical to me.
>>>
>>> Input pin on the DSI side.
>>
>> So adding it to panel schema is not even possible thus I am not sure if
>> we discuss this option (maybe not, because it would be odd, considering
>> you got Rb tag!).
>>
>> Adding some input node to DSI connecting panel output and DSI input...
>> for what? I mean, what sort of data would it represent?
>>
> 
> TE pin is an input signal from the panel to the DSI host.
> 
> Today we have two ports in the DSI host node:
> 
> 1) input to DSI node from DPU. This represents the pixel stream from DPU 
> to DSI
> 
> 2) DSI output node to represent pixel stream from DSI host to panel in
> 
> Now, please explain to me how does TE pin belongs to (2) because thats 
> where this property has been added.

Don't get what is a DPU, but anyway connections are kind of
bi-directional. So TE belongs there because this is the connection
between the DSI and the panel, with generic meaning that data flows from
the DSI to the panel.

Why we keep discussing this? You really need 3rd ack or this is just
bikeschedding?

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 20/05/2024 14:12, 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.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Maybe we need third DT maintainer review/ack...

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH 2/7] drm/msm/dpu: convert vsync source defines to the enum

[Dmitry Baryshkov]

Add enum dpu_vsync_source instead of a series of defines. Use this enum
to pass vsync information.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 26 ++++++++++++++------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h  |  2 +-
 5 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 119f3ea50a7c..4988a1029431 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -747,7 +747,7 @@ static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt *dpu_enc,
 		if (disp_info->is_te_using_watchdog_timer)
 			vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_WD_TIMER_0;
 		else
-			vsync_cfg.vsync_source = DPU_VSYNC0_SOURCE_GPIO;
+			vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_GPIO_0;
 
 		hw_mdptop->ops.setup_vsync_source(hw_mdptop, &vsync_cfg);
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
index 225c1c7768ff..96f6160cf607 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -462,7 +462,7 @@ static int dpu_hw_intf_get_vsync_info(struct dpu_hw_intf *intf,
 }
 
 static void dpu_hw_intf_vsync_sel(struct dpu_hw_intf *intf,
-		u32 vsync_source)
+				  enum dpu_vsync_source vsync_source)
 {
 	struct dpu_hw_blk_reg_map *c;
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
index f9015c67a574..ac244f0b33fb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -107,7 +107,7 @@ struct dpu_hw_intf_ops {
 
 	int (*connect_external_te)(struct dpu_hw_intf *intf, bool enable_external_te);
 
-	void (*vsync_sel)(struct dpu_hw_intf *intf, u32 vsync_source);
+	void (*vsync_sel)(struct dpu_hw_intf *intf, enum dpu_vsync_source vsync_source);
 
 	/**
 	 * Disable autorefresh if enabled
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
index 66759623fc42..a2eff36a2224 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
@@ -54,18 +54,20 @@
 #define DPU_BLEND_BG_INV_MOD_ALPHA	(1 << 12)
 #define DPU_BLEND_BG_TRANSP_EN		(1 << 13)
 
-#define DPU_VSYNC0_SOURCE_GPIO		0
-#define DPU_VSYNC1_SOURCE_GPIO		1
-#define DPU_VSYNC2_SOURCE_GPIO		2
-#define DPU_VSYNC_SOURCE_INTF_0		3
-#define DPU_VSYNC_SOURCE_INTF_1		4
-#define DPU_VSYNC_SOURCE_INTF_2		5
-#define DPU_VSYNC_SOURCE_INTF_3		6
-#define DPU_VSYNC_SOURCE_WD_TIMER_4	11
-#define DPU_VSYNC_SOURCE_WD_TIMER_3	12
-#define DPU_VSYNC_SOURCE_WD_TIMER_2	13
-#define DPU_VSYNC_SOURCE_WD_TIMER_1	14
-#define DPU_VSYNC_SOURCE_WD_TIMER_0	15
+enum dpu_vsync_source {
+	DPU_VSYNC_SOURCE_GPIO_0,
+	DPU_VSYNC_SOURCE_GPIO_1,
+	DPU_VSYNC_SOURCE_GPIO_2,
+	DPU_VSYNC_SOURCE_INTF_0 = 3,
+	DPU_VSYNC_SOURCE_INTF_1,
+	DPU_VSYNC_SOURCE_INTF_2,
+	DPU_VSYNC_SOURCE_INTF_3,
+	DPU_VSYNC_SOURCE_WD_TIMER_4 = 11,
+	DPU_VSYNC_SOURCE_WD_TIMER_3,
+	DPU_VSYNC_SOURCE_WD_TIMER_2,
+	DPU_VSYNC_SOURCE_WD_TIMER_1,
+	DPU_VSYNC_SOURCE_WD_TIMER_0,
+};
 
 enum dpu_hw_blk_type {
 	DPU_HW_BLK_TOP = 0,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h
index 6f3dc98087df..5c9a7ede991e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h
@@ -64,7 +64,7 @@ struct dpu_vsync_source_cfg {
 	u32 pp_count;
 	u32 frame_rate;
 	u32 ppnumber[PINGPONG_MAX];
-	u32 vsync_source;
+	enum dpu_vsync_source vsync_source;
 };
 
 /**

-- 
2.39.2

Re: [PATCH 2/7] drm/msm/dpu: convert vsync source defines to the enum

[Abhinav Kumar]

On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote:
> Add enum dpu_vsync_source instead of a series of defines. Use this enum
> to pass vsync information.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  2 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c |  2 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h |  2 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 26 ++++++++++++++------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h  |  2 +-
>   5 files changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 119f3ea50a7c..4988a1029431 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -747,7 +747,7 @@ static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt *dpu_enc,
>   		if (disp_info->is_te_using_watchdog_timer)
>   			vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_WD_TIMER_0;
>   		else
> -			vsync_cfg.vsync_source = DPU_VSYNC0_SOURCE_GPIO;
> +			vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_GPIO_0;
>   
>   		hw_mdptop->ops.setup_vsync_source(hw_mdptop, &vsync_cfg);
>   
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> index 225c1c7768ff..96f6160cf607 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -462,7 +462,7 @@ static int dpu_hw_intf_get_vsync_info(struct dpu_hw_intf *intf,
>   }
>   
>   static void dpu_hw_intf_vsync_sel(struct dpu_hw_intf *intf,
> -		u32 vsync_source)
> +				  enum dpu_vsync_source vsync_source)
>   {
>   	struct dpu_hw_blk_reg_map *c;
>   
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> index f9015c67a574..ac244f0b33fb 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> @@ -107,7 +107,7 @@ struct dpu_hw_intf_ops {
>   
>   	int (*connect_external_te)(struct dpu_hw_intf *intf, bool enable_external_te);
>   
> -	void (*vsync_sel)(struct dpu_hw_intf *intf, u32 vsync_source);
> +	void (*vsync_sel)(struct dpu_hw_intf *intf, enum dpu_vsync_source vsync_source);
>   
>   	/**
>   	 * Disable autorefresh if enabled
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> index 66759623fc42..a2eff36a2224 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> @@ -54,18 +54,20 @@
>   #define DPU_BLEND_BG_INV_MOD_ALPHA	(1 << 12)
>   #define DPU_BLEND_BG_TRANSP_EN		(1 << 13)
>   
> -#define DPU_VSYNC0_SOURCE_GPIO		0
> -#define DPU_VSYNC1_SOURCE_GPIO		1
> -#define DPU_VSYNC2_SOURCE_GPIO		2
> -#define DPU_VSYNC_SOURCE_INTF_0		3
> -#define DPU_VSYNC_SOURCE_INTF_1		4
> -#define DPU_VSYNC_SOURCE_INTF_2		5
> -#define DPU_VSYNC_SOURCE_INTF_3		6
> -#define DPU_VSYNC_SOURCE_WD_TIMER_4	11
> -#define DPU_VSYNC_SOURCE_WD_TIMER_3	12
> -#define DPU_VSYNC_SOURCE_WD_TIMER_2	13
> -#define DPU_VSYNC_SOURCE_WD_TIMER_1	14
> -#define DPU_VSYNC_SOURCE_WD_TIMER_0	15
> +enum dpu_vsync_source {
> +	DPU_VSYNC_SOURCE_GPIO_0,
> +	DPU_VSYNC_SOURCE_GPIO_1,
> +	DPU_VSYNC_SOURCE_GPIO_2,
> +	DPU_VSYNC_SOURCE_INTF_0 = 3,

Do we need this assignment to 3?

Rest LGTM,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Re: [PATCH 2/7] drm/msm/dpu: convert vsync source defines to the enum

[Dmitry Baryshkov]

On Wed, 22 May 2024 at 21:41, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote:
> > Add enum dpu_vsync_source instead of a series of defines. Use this enum
> > to pass vsync information.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  2 +-
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c |  2 +-
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h |  2 +-
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 26 ++++++++++++++------------
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h  |  2 +-
> >   5 files changed, 18 insertions(+), 16 deletions(-)
> >

> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> > index 66759623fc42..a2eff36a2224 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> > @@ -54,18 +54,20 @@
> >   #define DPU_BLEND_BG_INV_MOD_ALPHA  (1 << 12)
> >   #define DPU_BLEND_BG_TRANSP_EN              (1 << 13)
> >
> > -#define DPU_VSYNC0_SOURCE_GPIO               0
> > -#define DPU_VSYNC1_SOURCE_GPIO               1
> > -#define DPU_VSYNC2_SOURCE_GPIO               2
> > -#define DPU_VSYNC_SOURCE_INTF_0              3
> > -#define DPU_VSYNC_SOURCE_INTF_1              4
> > -#define DPU_VSYNC_SOURCE_INTF_2              5
> > -#define DPU_VSYNC_SOURCE_INTF_3              6
> > -#define DPU_VSYNC_SOURCE_WD_TIMER_4  11
> > -#define DPU_VSYNC_SOURCE_WD_TIMER_3  12
> > -#define DPU_VSYNC_SOURCE_WD_TIMER_2  13
> > -#define DPU_VSYNC_SOURCE_WD_TIMER_1  14
> > -#define DPU_VSYNC_SOURCE_WD_TIMER_0  15
> > +enum dpu_vsync_source {
> > +     DPU_VSYNC_SOURCE_GPIO_0,
> > +     DPU_VSYNC_SOURCE_GPIO_1,
> > +     DPU_VSYNC_SOURCE_GPIO_2,
> > +     DPU_VSYNC_SOURCE_INTF_0 = 3,
>
> Do we need this assignment to 3?

It is redundant, but it points out that if at some point another GPIO
is added, it should go to the end (or to some other place, having the
proper value).

>
> Rest LGTM,
>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

-- 
With best wishes
Dmitry

Re: [PATCH 2/7] drm/msm/dpu: convert vsync source defines to the enum

[Abhinav Kumar]

On 5/22/2024 1:01 PM, Dmitry Baryshkov wrote:
> On Wed, 22 May 2024 at 21:41, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote:
>>> Add enum dpu_vsync_source instead of a series of defines. Use this enum
>>> to pass vsync information.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  2 +-
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c |  2 +-
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h |  2 +-
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 26 ++++++++++++++------------
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h  |  2 +-
>>>    5 files changed, 18 insertions(+), 16 deletions(-)
>>>
> 
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
>>> index 66759623fc42..a2eff36a2224 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
>>> @@ -54,18 +54,20 @@
>>>    #define DPU_BLEND_BG_INV_MOD_ALPHA  (1 << 12)
>>>    #define DPU_BLEND_BG_TRANSP_EN              (1 << 13)
>>>
>>> -#define DPU_VSYNC0_SOURCE_GPIO               0
>>> -#define DPU_VSYNC1_SOURCE_GPIO               1
>>> -#define DPU_VSYNC2_SOURCE_GPIO               2
>>> -#define DPU_VSYNC_SOURCE_INTF_0              3
>>> -#define DPU_VSYNC_SOURCE_INTF_1              4
>>> -#define DPU_VSYNC_SOURCE_INTF_2              5
>>> -#define DPU_VSYNC_SOURCE_INTF_3              6
>>> -#define DPU_VSYNC_SOURCE_WD_TIMER_4  11
>>> -#define DPU_VSYNC_SOURCE_WD_TIMER_3  12
>>> -#define DPU_VSYNC_SOURCE_WD_TIMER_2  13
>>> -#define DPU_VSYNC_SOURCE_WD_TIMER_1  14
>>> -#define DPU_VSYNC_SOURCE_WD_TIMER_0  15
>>> +enum dpu_vsync_source {
>>> +     DPU_VSYNC_SOURCE_GPIO_0,
>>> +     DPU_VSYNC_SOURCE_GPIO_1,
>>> +     DPU_VSYNC_SOURCE_GPIO_2,
>>> +     DPU_VSYNC_SOURCE_INTF_0 = 3,
>>
>> Do we need this assignment to 3?
> 
> It is redundant, but it points out that if at some point another GPIO
> is added, it should go to the end (or to some other place, having the
> proper value).
> 

Ack, makes sense.

>>
>> Rest LGTM,
>>
>> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> 

[PATCH 3/7] drm/msm/dsi: drop unused GPIOs handling

[Dmitry Baryshkov]

Neither disp-enable-gpios nor disp-te-gpios are defined in the schema.
None of the board DT files use those GPIO pins. Drop them from the
driver.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 37 -------------------------------------
 1 file changed, 37 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index a50f4dda5941..c4d72562c95a 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -7,7 +7,6 @@
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
-#include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
 #include <linux/mfd/syscon.h>
 #include <linux/of.h>
@@ -130,9 +129,6 @@ struct msm_dsi_host {
 
 	unsigned long src_clk_rate;
 
-	struct gpio_desc *disp_en_gpio;
-	struct gpio_desc *te_gpio;
-
 	const struct msm_dsi_cfg_handler *cfg_hnd;
 
 	struct completion dma_comp;
@@ -1613,28 +1609,6 @@ static irqreturn_t dsi_host_irq(int irq, void *ptr)
 	return IRQ_HANDLED;
 }
 
-static int dsi_host_init_panel_gpios(struct msm_dsi_host *msm_host,
-			struct device *panel_device)
-{
-	msm_host->disp_en_gpio = devm_gpiod_get_optional(panel_device,
-							 "disp-enable",
-							 GPIOD_OUT_LOW);
-	if (IS_ERR(msm_host->disp_en_gpio)) {
-		DBG("cannot get disp-enable-gpios %ld",
-				PTR_ERR(msm_host->disp_en_gpio));
-		return PTR_ERR(msm_host->disp_en_gpio);
-	}
-
-	msm_host->te_gpio = devm_gpiod_get_optional(panel_device, "disp-te",
-								GPIOD_IN);
-	if (IS_ERR(msm_host->te_gpio)) {
-		DBG("cannot get disp-te-gpios %ld", PTR_ERR(msm_host->te_gpio));
-		return PTR_ERR(msm_host->te_gpio);
-	}
-
-	return 0;
-}
-
 static int dsi_host_attach(struct mipi_dsi_host *host,
 					struct mipi_dsi_device *dsi)
 {
@@ -1651,11 +1625,6 @@ static int dsi_host_attach(struct mipi_dsi_host *host,
 	if (dsi->dsc)
 		msm_host->dsc = dsi->dsc;
 
-	/* Some gpios defined in panel DT need to be controlled by host */
-	ret = dsi_host_init_panel_gpios(msm_host, &dsi->dev);
-	if (ret)
-		return ret;
-
 	ret = dsi_dev_attach(msm_host->pdev);
 	if (ret)
 		return ret;
@@ -2422,9 +2391,6 @@ int msm_dsi_host_power_on(struct mipi_dsi_host *host,
 	dsi_sw_reset(msm_host);
 	dsi_ctrl_enable(msm_host, phy_shared_timings, phy);
 
-	if (msm_host->disp_en_gpio)
-		gpiod_set_value(msm_host->disp_en_gpio, 1);
-
 	msm_host->power_on = true;
 	mutex_unlock(&msm_host->dev_mutex);
 
@@ -2454,9 +2420,6 @@ int msm_dsi_host_power_off(struct mipi_dsi_host *host)
 
 	dsi_ctrl_disable(msm_host);
 
-	if (msm_host->disp_en_gpio)
-		gpiod_set_value(msm_host->disp_en_gpio, 0);
-
 	pinctrl_pm_select_sleep_state(&msm_host->pdev->dev);
 
 	cfg_hnd->ops->link_clk_disable(msm_host);

-- 
2.39.2

Re: [PATCH 3/7] drm/msm/dsi: drop unused GPIOs handling

[Abhinav Kumar]

On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote:
> Neither disp-enable-gpios nor disp-te-gpios are defined in the schema.
> None of the board DT files use those GPIO pins. Drop them from the
> driver.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/dsi/dsi_host.c | 37 -------------------------------------
>   1 file changed, 37 deletions(-)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

[PATCH 4/7] drm/msm/dpu: pull the is_cmd_mode out of _dpu_encoder_update_vsync_source()

[Dmitry Baryshkov]

Setting vsync source makes sense only for DSI CMD panels. Pull the
is_cmd_mode condition out of the function into the calling code, so that
it becomes more explicit.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 4988a1029431..bd37a56b4d03 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -736,8 +736,7 @@ static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt *dpu_enc,
 		return;
 	}
 
-	if (hw_mdptop->ops.setup_vsync_source &&
-			disp_info->is_cmd_mode) {
+	if (hw_mdptop->ops.setup_vsync_source) {
 		for (i = 0; i < dpu_enc->num_phys_encs; i++)
 			vsync_cfg.ppnumber[i] = dpu_enc->hw_pp[i]->idx;
 
@@ -1226,7 +1225,8 @@ static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
 		dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select(
 			dpu_enc->cur_master->hw_mdptop);
 
-	_dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info);
+	if (dpu_enc->disp_info.is_cmd_mode)
+		_dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info);
 
 	if (dpu_enc->disp_info.intf_type == INTF_DSI &&
 			!WARN_ON(dpu_enc->num_phys_encs == 0)) {

-- 
2.39.2

Re: [PATCH 4/7] drm/msm/dpu: pull the is_cmd_mode out of _dpu_encoder_update_vsync_source()

[Abhinav Kumar]

On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote:
> Setting vsync source makes sense only for DSI CMD panels. Pull the
> is_cmd_mode condition out of the function into the calling code, so that
> it becomes more explicit.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

[PATCH 5/7] drm/msm/dpu: rework vsync_source handling

[Dmitry Baryshkov]

The struct msm_display_info has is_te_using_watchdog_timer field which
is neither used anywhere nor is flexible enough to specify different
sources. Replace it with the field specifying the vsync source using
enum dpu_vsync_source.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 +----
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 5 ++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 2 ++
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index bd37a56b4d03..b147f8814a18 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -743,10 +743,7 @@ static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt *dpu_enc,
 		vsync_cfg.pp_count = dpu_enc->num_phys_encs;
 		vsync_cfg.frame_rate = drm_mode_vrefresh(&dpu_enc->base.crtc->state->adjusted_mode);
 
-		if (disp_info->is_te_using_watchdog_timer)
-			vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_WD_TIMER_0;
-		else
-			vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_GPIO_0;
+		vsync_cfg.vsync_source = disp_info->vsync_source;
 
 		hw_mdptop->ops.setup_vsync_source(hw_mdptop, &vsync_cfg);
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 76be77e30954..cb59bd4436f4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -26,15 +26,14 @@
  * @h_tile_instance:    Controller instance used per tile. Number of elements is
  *                      based on num_of_h_tiles
  * @is_cmd_mode		Boolean to indicate if the CMD mode is requested
- * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
- *				 used instead of panel TE in cmd mode panels
+ * @vsync_source:	Source of the TE signal for DSI CMD devices
  */
 struct msm_display_info {
 	enum dpu_intf_type intf_type;
 	uint32_t num_of_h_tiles;
 	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
 	bool is_cmd_mode;
-	bool is_te_using_watchdog_timer;
+	enum dpu_vsync_source vsync_source;
 };
 
 /**
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 1955848b1b78..e9991f3756d4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -543,6 +543,8 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 
 		info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]);
 
+		info.vsync_source = DPU_VSYNC_SOURCE_GPIO_0;
+
 		encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI, &info);
 		if (IS_ERR(encoder)) {
 			DPU_ERROR("encoder init failed for dsi display\n");

-- 
2.39.2

Re: [PATCH 5/7] drm/msm/dpu: rework vsync_source handling

[Abhinav Kumar]

On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote:
> The struct msm_display_info has is_te_using_watchdog_timer field which
> is neither used anywhere nor is flexible enough to specify different
> sources. Replace it with the field specifying the vsync source using
> enum dpu_vsync_source.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 +----
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 5 ++---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 2 ++
>   3 files changed, 5 insertions(+), 7 deletions(-)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

[PATCH 6/7] drm/msm/dsi: parse vsync source from device tree

[Dmitry Baryshkov]

Allow board's device tree to specify the vsync source (aka TE source).
If the property is omitted, the display controller driver will use the
default setting.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dsi/dsi.h         |  1 +
 drivers/gpu/drm/msm/dsi/dsi_host.c    | 11 +++++++++++
 drivers/gpu/drm/msm/dsi/dsi_manager.c |  5 +++++
 drivers/gpu/drm/msm/msm_drv.h         |  6 ++++++
 4 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index afc290408ba4..87496db203d6 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -37,6 +37,7 @@ struct msm_dsi {
 
 	struct mipi_dsi_host *host;
 	struct msm_dsi_phy *phy;
+	const char *te_source;
 
 	struct drm_bridge *next_bridge;
 
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index c4d72562c95a..c26ad0fed54d 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -1786,9 +1786,11 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
 
 static int dsi_host_parse_dt(struct msm_dsi_host *msm_host)
 {
+	struct msm_dsi *msm_dsi = platform_get_drvdata(msm_host->pdev);
 	struct device *dev = &msm_host->pdev->dev;
 	struct device_node *np = dev->of_node;
 	struct device_node *endpoint;
+	const char *te_source;
 	int ret = 0;
 
 	/*
@@ -1811,6 +1813,15 @@ static int dsi_host_parse_dt(struct msm_dsi_host *msm_host)
 		goto err;
 	}
 
+	ret = of_property_read_string(endpoint, "qcom,te-source", &te_source);
+	if (ret && ret != -EINVAL) {
+		DRM_DEV_ERROR(dev, "%s: invalid TE source configuration %d\n",
+			__func__, ret);
+		goto err;
+	}
+	if (!ret)
+		msm_dsi->te_source = devm_kstrdup(dev, te_source, GFP_KERNEL);
+
 	if (of_property_read_bool(np, "syscon-sfpb")) {
 		msm_host->sfpb = syscon_regmap_lookup_by_phandle(np,
 					"syscon-sfpb");
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 5b3f3068fd92..a210b7c9e5ca 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -603,3 +603,8 @@ bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi)
 {
 	return IS_MASTER_DSI_LINK(msm_dsi->id);
 }
+
+const char *msm_dsi_get_te_source(struct msm_dsi *msm_dsi)
+{
+	return msm_dsi->te_source;
+}
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 912ebaa5df84..afd98dffea99 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -330,6 +330,7 @@ bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
 bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
 bool msm_dsi_wide_bus_enabled(struct msm_dsi *msm_dsi);
 struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi);
+const char *msm_dsi_get_te_source(struct msm_dsi *msm_dsi);
 #else
 static inline void __init msm_dsi_register(void)
 {
@@ -367,6 +368,11 @@ static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_
 {
 	return NULL;
 }
+
+static inline const char *msm_dsi_get_te_source(struct msm_dsi *msm_dsi)
+{
+	return NULL;
+}
 #endif
 
 #ifdef CONFIG_DRM_MSM_DP

-- 
2.39.2

Re: [PATCH 6/7] drm/msm/dsi: parse vsync source from device tree

[Abhinav Kumar]

On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote:
> Allow board's device tree to specify the vsync source (aka TE source).
> If the property is omitted, the display controller driver will use the
> default setting.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/dsi/dsi.h         |  1 +
>   drivers/gpu/drm/msm/dsi/dsi_host.c    | 11 +++++++++++
>   drivers/gpu/drm/msm/dsi/dsi_manager.c |  5 +++++
>   drivers/gpu/drm/msm/msm_drv.h         |  6 ++++++
>   4 files changed, 23 insertions(+)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

[PATCH 7/7] drm/msm/dpu: support setting the TE source

[Dmitry Baryshkov]

Make the DPU driver use the TE source specified in the DT. If none is
specified, the driver defaults to the first GPIO (mdp_vsync0).

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 44 ++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index e9991f3756d4..932d0bb41d7e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -505,6 +505,44 @@ static void dpu_kms_wait_flush(struct msm_kms *kms, unsigned crtc_mask)
 		dpu_kms_wait_for_commit_done(kms, crtc);
 }
 
+static const char *dpu_vsync_sources[] = {
+	[DPU_VSYNC_SOURCE_GPIO_0] = "mdp_gpio0",
+	[DPU_VSYNC_SOURCE_GPIO_1] = "mdp_gpio1",
+	[DPU_VSYNC_SOURCE_GPIO_2] = "mdp_gpio2",
+	[DPU_VSYNC_SOURCE_INTF_0] = "mdp_intf0",
+	[DPU_VSYNC_SOURCE_INTF_1] = "mdp_intf1",
+	[DPU_VSYNC_SOURCE_INTF_2] = "mdp_intf2",
+	[DPU_VSYNC_SOURCE_INTF_3] = "mdp_intf3",
+	[DPU_VSYNC_SOURCE_WD_TIMER_0] = "timer0",
+	[DPU_VSYNC_SOURCE_WD_TIMER_1] = "timer1",
+	[DPU_VSYNC_SOURCE_WD_TIMER_2] = "timer2",
+	[DPU_VSYNC_SOURCE_WD_TIMER_3] = "timer3",
+	[DPU_VSYNC_SOURCE_WD_TIMER_4] = "timer4",
+};
+
+static int dpu_kms_dsi_set_te_source(struct msm_display_info *info,
+				     struct msm_dsi *dsi)
+{
+	const char *te_source = msm_dsi_get_te_source(dsi);
+	int i;
+
+	if (!te_source) {
+		info->vsync_source = DPU_VSYNC_SOURCE_GPIO_0;
+		return 0;
+	}
+
+	/* we can not use match_string since dpu_vsync_sources is a sparse array */
+	for (i = 0; i < ARRAY_SIZE(dpu_vsync_sources); i++) {
+		if (dpu_vsync_sources[i] &&
+		    !strcmp(dpu_vsync_sources[i], te_source)) {
+			info->vsync_source = i;
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
 static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 				    struct msm_drm_private *priv,
 				    struct dpu_kms *dpu_kms)
@@ -543,7 +581,11 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 
 		info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]);
 
-		info.vsync_source = DPU_VSYNC_SOURCE_GPIO_0;
+		rc = dpu_kms_dsi_set_te_source(&info, priv->dsi[i]);
+		if (rc) {
+			DPU_ERROR("failed to identify TE source for dsi display\n");
+			return rc;
+		}
 
 		encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI, &info);
 		if (IS_ERR(encoder)) {

-- 
2.39.2

Re: [PATCH 0/7] drm/msm/dpu: handle non-default TE source pins

[Abhinav Kumar]

On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote:
> Command-mode DSI panels need to signal the display controlller when
> vsync happens, so that the device can start sending the next frame. Some
> devices (Google Pixel 3) use a non-default pin, so additional
> configuration is required. Add a way to specify this information in DT
> and handle it in the DSI and DPU drivers.
> 

Which pin is the pixel 3 using? Just wanted to know .. is it gpio0 or gpio1?

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> Dmitry Baryshkov (7):
>        dt-bindings: display/msm/dsi: allow specifying TE source
>        drm/msm/dpu: convert vsync source defines to the enum
>        drm/msm/dsi: drop unused GPIOs handling
>        drm/msm/dpu: pull the is_cmd_mode out of _dpu_encoder_update_vsync_source()
>        drm/msm/dpu: rework vsync_source handling
>        drm/msm/dsi: parse vsync source from device tree
>        drm/msm/dpu: support setting the TE source
> 
>   .../bindings/display/msm/dsi-controller-main.yaml  | 16 ++++++++
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 11 ++---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h        |  5 +--
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c        |  2 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  2 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h        | 26 ++++++------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h         |  2 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c            | 44 ++++++++++++++++++++
>   drivers/gpu/drm/msm/dsi/dsi.h                      |  1 +
>   drivers/gpu/drm/msm/dsi/dsi_host.c                 | 48 +++++-----------------
>   drivers/gpu/drm/msm/dsi/dsi_manager.c              |  5 +++
>   drivers/gpu/drm/msm/msm_drv.h                      |  6 +++
>   12 files changed, 106 insertions(+), 62 deletions(-)
> ---
> base-commit: 75fa778d74b786a1608d55d655d42b480a6fa8bd
> change-id: 20240514-dpu-handle-te-signal-82663c0211bd
> 
> Best regards,

Re: [PATCH 0/7] drm/msm/dpu: handle non-default TE source pins

[Dmitry Baryshkov]

On Wed, 22 May 2024 at 21:39, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote:
> > Command-mode DSI panels need to signal the display controlller when
> > vsync happens, so that the device can start sending the next frame. Some
> > devices (Google Pixel 3) use a non-default pin, so additional
> > configuration is required. Add a way to specify this information in DT
> > and handle it in the DSI and DPU drivers.
> >
>
> Which pin is the pixel 3 using? Just wanted to know .. is it gpio0 or gpio1?

gpio2. If it was gpio0 then there were no issues at all.

>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> > Dmitry Baryshkov (7):
> >        dt-bindings: display/msm/dsi: allow specifying TE source
> >        drm/msm/dpu: convert vsync source defines to the enum
> >        drm/msm/dsi: drop unused GPIOs handling
> >        drm/msm/dpu: pull the is_cmd_mode out of _dpu_encoder_update_vsync_source()
> >        drm/msm/dpu: rework vsync_source handling
> >        drm/msm/dsi: parse vsync source from device tree
> >        drm/msm/dpu: support setting the TE source
> >
> >   .../bindings/display/msm/dsi-controller-main.yaml  | 16 ++++++++
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 11 ++---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h        |  5 +--
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c        |  2 +-
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  2 +-
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h        | 26 ++++++------
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h         |  2 +-
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c            | 44 ++++++++++++++++++++
> >   drivers/gpu/drm/msm/dsi/dsi.h                      |  1 +
> >   drivers/gpu/drm/msm/dsi/dsi_host.c                 | 48 +++++-----------------
> >   drivers/gpu/drm/msm/dsi/dsi_manager.c              |  5 +++
> >   drivers/gpu/drm/msm/msm_drv.h                      |  6 +++
> >   12 files changed, 106 insertions(+), 62 deletions(-)
> > ---
> > base-commit: 75fa778d74b786a1608d55d655d42b480a6fa8bd
> > change-id: 20240514-dpu-handle-te-signal-82663c0211bd
> >
> > Best regards,



-- 
With best wishes
Dmitry

Re: [PATCH 0/7] drm/msm/dpu: handle non-default TE source pins

[Abhinav Kumar]

On 5/22/2024 12:59 PM, Dmitry Baryshkov wrote:
> On Wed, 22 May 2024 at 21:39, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote:
>>> Command-mode DSI panels need to signal the display controlller when
>>> vsync happens, so that the device can start sending the next frame. Some
>>> devices (Google Pixel 3) use a non-default pin, so additional
>>> configuration is required. Add a way to specify this information in DT
>>> and handle it in the DSI and DPU drivers.
>>>
>>
>> Which pin is the pixel 3 using? Just wanted to know .. is it gpio0 or gpio1?
> 
> gpio2. If it was gpio0 then there were no issues at all.
> 

Got it. Instead of asking gpio1 or gpio2, I mistyped and asked gpio0 or 
gpio1.

While reviewing the code , I think the function 
dpu_hw_setup_vsync_source is poorly named . It really doesnt configured 
vsync_source. It actually configured watchdog timer.

Can you pls include one more patch in this series to rename 
dpu_hw_setup_vsync_source ---> dpu_hw_setup_wd_timer()

>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>> Dmitry Baryshkov (7):
>>>         dt-bindings: display/msm/dsi: allow specifying TE source
>>>         drm/msm/dpu: convert vsync source defines to the enum
>>>         drm/msm/dsi: drop unused GPIOs handling
>>>         drm/msm/dpu: pull the is_cmd_mode out of _dpu_encoder_update_vsync_source()
>>>         drm/msm/dpu: rework vsync_source handling
>>>         drm/msm/dsi: parse vsync source from device tree
>>>         drm/msm/dpu: support setting the TE source
>>>
>>>    .../bindings/display/msm/dsi-controller-main.yaml  | 16 ++++++++
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 11 ++---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h        |  5 +--
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c        |  2 +-
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  2 +-
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h        | 26 ++++++------
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h         |  2 +-
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c            | 44 ++++++++++++++++++++
>>>    drivers/gpu/drm/msm/dsi/dsi.h                      |  1 +
>>>    drivers/gpu/drm/msm/dsi/dsi_host.c                 | 48 +++++-----------------
>>>    drivers/gpu/drm/msm/dsi/dsi_manager.c              |  5 +++
>>>    drivers/gpu/drm/msm/msm_drv.h                      |  6 +++
>>>    12 files changed, 106 insertions(+), 62 deletions(-)
>>> ---
>>> base-commit: 75fa778d74b786a1608d55d655d42b480a6fa8bd
>>> change-id: 20240514-dpu-handle-te-signal-82663c0211bd
>>>
>>> Best regards,
> 
> 
> 

Re: [PATCH 0/7] drm/msm/dpu: handle non-default TE source pins

[Dmitry Baryshkov]

On Thu, 30 May 2024 at 01:11, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 5/22/2024 12:59 PM, Dmitry Baryshkov wrote:
> > On Wed, 22 May 2024 at 21:39, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote:
> >>> Command-mode DSI panels need to signal the display controlller when
> >>> vsync happens, so that the device can start sending the next frame. Some
> >>> devices (Google Pixel 3) use a non-default pin, so additional
> >>> configuration is required. Add a way to specify this information in DT
> >>> and handle it in the DSI and DPU drivers.
> >>>
> >>
> >> Which pin is the pixel 3 using? Just wanted to know .. is it gpio0 or gpio1?
> >
> > gpio2. If it was gpio0 then there were no issues at all.
> >
>
> Got it. Instead of asking gpio1 or gpio2, I mistyped and asked gpio0 or
> gpio1.
>
> While reviewing the code , I think the function
> dpu_hw_setup_vsync_source is poorly named . It really doesnt configured
> vsync_source. It actually configured watchdog timer.
>
> Can you pls include one more patch in this series to rename
> dpu_hw_setup_vsync_source ---> dpu_hw_setup_wd_timer()

Ack, sounds like a good idea.

>
> >>
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>> ---
> >>> Dmitry Baryshkov (7):
> >>>         dt-bindings: display/msm/dsi: allow specifying TE source
> >>>         drm/msm/dpu: convert vsync source defines to the enum
> >>>         drm/msm/dsi: drop unused GPIOs handling
> >>>         drm/msm/dpu: pull the is_cmd_mode out of _dpu_encoder_update_vsync_source()
> >>>         drm/msm/dpu: rework vsync_source handling
> >>>         drm/msm/dsi: parse vsync source from device tree
> >>>         drm/msm/dpu: support setting the TE source
> >>>
> >>>    .../bindings/display/msm/dsi-controller-main.yaml  | 16 ++++++++
> >>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 11 ++---
> >>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h        |  5 +--
> >>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c        |  2 +-
> >>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  2 +-
> >>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h        | 26 ++++++------
> >>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h         |  2 +-
> >>>    drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c            | 44 ++++++++++++++++++++
> >>>    drivers/gpu/drm/msm/dsi/dsi.h                      |  1 +
> >>>    drivers/gpu/drm/msm/dsi/dsi_host.c                 | 48 +++++-----------------
> >>>    drivers/gpu/drm/msm/dsi/dsi_manager.c              |  5 +++
> >>>    drivers/gpu/drm/msm/msm_drv.h                      |  6 +++
> >>>    12 files changed, 106 insertions(+), 62 deletions(-)
> >>> ---
> >>> base-commit: 75fa778d74b786a1608d55d655d42b480a6fa8bd
> >>> change-id: 20240514-dpu-handle-te-signal-82663c0211bd
> >>>
> >>> Best regards,
> >
> >
> >



-- 
With best wishes
Dmitry