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