| Message ID | 20240520-dpu-handle-te-signal-v1-0-f273b42a089c@linaro.org |
|---|---|
| Headers | show |
| Series | drm/msm/dpu: handle non-default TE source pins | expand |
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. > }; > }; > }; >
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,
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>
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>
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,
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. > > }; > > }; > > }; > >
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? >>> }; >>> }; >>> }; >>> >
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
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
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, > > >
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, > > > > > >
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 > > >
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. > >
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
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.
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
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
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 >
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
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,