| Message ID | 20240716084248.1393666-1-a-bhatia1@ti.com |
|---|---|
| Headers | show |
| Series | drm/tidss: Add OLDI bridge support | expand |
+Max Hello Aradhya, On Tue, Jul 16, 2024 at 02:12:44PM +0530, Aradhya Bhatia wrote: > The addition of the 2nd OLDI TX (and a 2nd DSS in AM62Px) creates a need > for some major changes for a full feature experience. > > 1. The OF graph needs to be updated to accurately show the data flow. > 2. The tidss and OLDI drivers now need to support the dual-link and the > cloned single-link OLDI video signals. > 3. The drivers also need to support the case where 2 OLDI TXes are > connected to 2 different VPs - thereby creating 2 independent streams > of single-link OLDI outputs. Have you considered/tested the use case in which only single link is used? You do not mention it here and to me this is a relevant use case. There is a workaround for this (use option 2, cloned, even if nothing is connected to the second link), but this seems not correct. We (Max in Cc here) noticed that this specific use case is broken on your downstream v6.6 TI branch. Francesco
Hi, On 06/09/2024 14:43, Francesco Dolcini wrote: > +Max > > Hello Aradhya, > > On Tue, Jul 16, 2024 at 02:12:44PM +0530, Aradhya Bhatia wrote: >> The addition of the 2nd OLDI TX (and a 2nd DSS in AM62Px) creates a need >> for some major changes for a full feature experience. >> >> 1. The OF graph needs to be updated to accurately show the data flow. >> 2. The tidss and OLDI drivers now need to support the dual-link and the >> cloned single-link OLDI video signals. >> 3. The drivers also need to support the case where 2 OLDI TXes are >> connected to 2 different VPs - thereby creating 2 independent streams >> of single-link OLDI outputs. > > Have you considered/tested the use case in which only single link is used? > You do not mention it here and to me this is a relevant use case. > > There is a workaround for this (use option 2, cloned, even if nothing is > connected to the second link), but this seems not correct. > > We (Max in Cc here) noticed that this specific use case is broken on > your downstream v6.6 TI branch. What if you set "fw_devlink=off" kernel boot parameter? Tomi
Hi, Thank you, Francesco and Max, for testing and reporting this! On 09/09/24 13:45, Tomi Valkeinen wrote: > Hi, > > On 06/09/2024 14:43, Francesco Dolcini wrote: >> +Max >> >> Hello Aradhya, >> >> On Tue, Jul 16, 2024 at 02:12:44PM +0530, Aradhya Bhatia wrote: >>> The addition of the 2nd OLDI TX (and a 2nd DSS in AM62Px) creates a need >>> for some major changes for a full feature experience. >>> >>> 1. The OF graph needs to be updated to accurately show the data flow. >>> 2. The tidss and OLDI drivers now need to support the dual-link and the >>> cloned single-link OLDI video signals. >>> 3. The drivers also need to support the case where 2 OLDI TXes are >>> connected to 2 different VPs - thereby creating 2 independent >>> streams >>> of single-link OLDI outputs. >> >> Have you considered/tested the use case in which only single link is >> used? >> You do not mention it here and to me this is a relevant use case. >> >> There is a workaround for this (use option 2, cloned, even if nothing is >> connected to the second link), but this seems not correct. I agree. The whole idea behind the series is to be able to accurately describe the hardware. Putting the devicetree in clone mode in order to get the single-link mode working is far from ideal. >> >> We (Max in Cc here) noticed that this specific use case is broken on >> your downstream v6.6 TI branch. Yes, it was been brought to my attention that the single-link usecase is not working over the downstream ti-6.6 kernel. As I have since discovered, it's not working on this series either. For some reason, the supplier-consumer dependency between the OLDI and the panel devicetree nodes is not getting flagged as `FWLINK_FLAG_CYCLE` in cases of single-link configuration. This flag allows the panel driver to continue to probe without waiting for the OLDI driver (panel's supplier). Absence of the flag getting set is causing these drivers to keep deferring probe in an endless cycle. Even with the flag, the OLDI (and tidss) cannot complete probe until the panel driver is probed and ready. That is because the OLDI (and tidss) need the panel to continue with the bridge-chain creation. However, over with the dual-lvds configuration (and as Francesco has now mentioned the clone configuration as well), the flag gets set by default, and everything works. This is what the debug has led to, so far. > > What if you set "fw_devlink=off" kernel boot parameter? > Yes! I haven't tested it, but it seems that this will bypass the supplier check and let the panel probe continue. Tomi, any idea, why is this issue happening only for single-link in the first place? It seems as if having 2 ports inside the panel devicetree lets the probe mechanism recognize the circular dependency and ignore the supplier OLDIs? This is the function where the difference comes down to, by the way - __fw_devlink_relax_cycles(), per my knowledge. I am still on my way to understand what exactly it is doing and why is it not relaxing the single-link case. Regards Aradhya
On Mon, Sep 09, 2024 at 11:15:28AM +0300, Tomi Valkeinen wrote: > Hi, > > On 06/09/2024 14:43, Francesco Dolcini wrote: > > +Max > > > > Hello Aradhya, > > > > On Tue, Jul 16, 2024 at 02:12:44PM +0530, Aradhya Bhatia wrote: > > > The addition of the 2nd OLDI TX (and a 2nd DSS in AM62Px) creates a need > > > for some major changes for a full feature experience. > > > > > > 1. The OF graph needs to be updated to accurately show the data flow. > > > 2. The tidss and OLDI drivers now need to support the dual-link and the > > > cloned single-link OLDI video signals. > > > 3. The drivers also need to support the case where 2 OLDI TXes are > > > connected to 2 different VPs - thereby creating 2 independent streams > > > of single-link OLDI outputs. > > > > Have you considered/tested the use case in which only single link is used? > > You do not mention it here and to me this is a relevant use case. > > > > There is a workaround for this (use option 2, cloned, even if nothing is > > connected to the second link), but this seems not correct. > > > > We (Max in Cc here) noticed that this specific use case is broken on > > your downstream v6.6 TI branch. > > What if you set "fw_devlink=off" kernel boot parameter? Hi Tomi My overlay specifing a single link which did not work by default results in a working panel with "fw_devlink=off" on the cmdline. Max > > Tomi >
On 09/09/2024 12:31, Aradhya Bhatia wrote: > Hi, > > Thank you, Francesco and Max, for testing and reporting this! > > On 09/09/24 13:45, Tomi Valkeinen wrote: >> Hi, >> >> On 06/09/2024 14:43, Francesco Dolcini wrote: >>> +Max >>> >>> Hello Aradhya, >>> >>> On Tue, Jul 16, 2024 at 02:12:44PM +0530, Aradhya Bhatia wrote: >>>> The addition of the 2nd OLDI TX (and a 2nd DSS in AM62Px) creates a need >>>> for some major changes for a full feature experience. >>>> >>>> 1. The OF graph needs to be updated to accurately show the data flow. >>>> 2. The tidss and OLDI drivers now need to support the dual-link and the >>>> cloned single-link OLDI video signals. >>>> 3. The drivers also need to support the case where 2 OLDI TXes are >>>> connected to 2 different VPs - thereby creating 2 independent >>>> streams >>>> of single-link OLDI outputs. >>> >>> Have you considered/tested the use case in which only single link is >>> used? >>> You do not mention it here and to me this is a relevant use case. >>> >>> There is a workaround for this (use option 2, cloned, even if nothing is >>> connected to the second link), but this seems not correct. > > I agree. The whole idea behind the series is to be able to accurately > describe the hardware. Putting the devicetree in clone mode in order to > get the single-link mode working is far from ideal. Btw, with the fw_devlink=off hack, and removing the second link from k3-am625-sk-microtips-mf101hie-panel.dtso, is still not enough, as the k3-am62-main.dtsi contains the ti,companion-oldi property which makes the driver think it's a cloning case. So, I think, either the ti,companion-oldi and ti,secondary-oldi should only be set in the overlay when setting up cloning/dual-link, or the companion-oldi property shouldn't actually make a difference, and the selection between clone and single-link should be done via some other means. >>> We (Max in Cc here) noticed that this specific use case is broken on >>> your downstream v6.6 TI branch. > > Yes, it was been brought to my attention that the single-link usecase is > not working over the downstream ti-6.6 kernel. As I have since > discovered, it's not working on this series either. > > For some reason, the supplier-consumer dependency between the OLDI and > the panel devicetree nodes is not getting flagged as `FWLINK_FLAG_CYCLE` > in cases of single-link configuration. > > This flag allows the panel driver to continue to probe without waiting > for the OLDI driver (panel's supplier). Absence of the flag getting set > is causing these drivers to keep deferring probe in an endless cycle. > > Even with the flag, the OLDI (and tidss) cannot complete probe until the > panel driver is probed and ready. That is because the OLDI (and tidss) > need the panel to continue with the bridge-chain creation. > > However, over with the dual-lvds configuration (and as Francesco has > now mentioned the clone configuration as well), the flag gets set by > default, and everything works. > > This is what the debug has led to, so far. Yes, I came to the same conclusion with my debug. >> >> What if you set "fw_devlink=off" kernel boot parameter? >> > > Yes! I haven't tested it, but it seems that this will bypass the > supplier check and let the panel probe continue. > > > Tomi, any idea, why is this issue happening only for single-link in the > first place? It seems as if having 2 ports inside the panel devicetree > lets the probe mechanism recognize the circular dependency and ignore > the supplier OLDIs? I have to say I have no idea... I don't really understand the devlink code here, but I'm guessing that the "cycle" part comes from the fact that with a media graph we have links (remote-endpoint) both ways in the DT data. So it's not possible to say which side is the consumer, which one is the supplier. Thus, it's marked as a cycle and, I think, basically ignored for the probing purpose. But why not here? I can see the links being created both ways: /display Linked as a fwnode consumer to /bus@f0000/dss@30200000/oldi-txes/oldi@0 /bus@f0000/dss@30200000/oldi-txes/oldi@0 Linked as a fwnode consumer to /display but it's never marked as a cycle. > This is the function where the difference comes down to, by the way - > __fw_devlink_relax_cycles(), per my knowledge. I am still on my way to > understand what exactly it is doing and why is it not relaxing the > single-link case. Yep. The answer is probably somewhere there =). Tomi
On 09/09/24 15:20, Tomi Valkeinen wrote: > On 09/09/2024 12:31, Aradhya Bhatia wrote: >> Hi, >> >> Thank you, Francesco and Max, for testing and reporting this! >> >> On 09/09/24 13:45, Tomi Valkeinen wrote: >>> Hi, >>> >>> On 06/09/2024 14:43, Francesco Dolcini wrote: >>>> +Max >>>> >>>> Hello Aradhya, >>>> >>>> On Tue, Jul 16, 2024 at 02:12:44PM +0530, Aradhya Bhatia wrote: >>>>> The addition of the 2nd OLDI TX (and a 2nd DSS in AM62Px) creates a >>>>> need >>>>> for some major changes for a full feature experience. >>>>> >>>>> 1. The OF graph needs to be updated to accurately show the data flow. >>>>> 2. The tidss and OLDI drivers now need to support the dual-link and >>>>> the >>>>> cloned single-link OLDI video signals. >>>>> 3. The drivers also need to support the case where 2 OLDI TXes are >>>>> connected to 2 different VPs - thereby creating 2 independent >>>>> streams >>>>> of single-link OLDI outputs. >>>> >>>> Have you considered/tested the use case in which only single link is >>>> used? >>>> You do not mention it here and to me this is a relevant use case. >>>> >>>> There is a workaround for this (use option 2, cloned, even if >>>> nothing is >>>> connected to the second link), but this seems not correct. >> >> I agree. The whole idea behind the series is to be able to accurately >> describe the hardware. Putting the devicetree in clone mode in order to >> get the single-link mode working is far from ideal. > > Btw, with the fw_devlink=off hack, and removing the second link from k3- > am625-sk-microtips-mf101hie-panel.dtso, is still not enough, as the k3- > am62-main.dtsi contains the ti,companion-oldi property which makes the > driver think it's a cloning case. Yes! > > So, I think, either the ti,companion-oldi and ti,secondary-oldi should > only be set in the overlay when setting up cloning/dual-link, or the > companion-oldi property shouldn't actually make a difference, and the > selection between clone and single-link should be done via some other > means. Yep, those properties need to be set in the overlay file, and not in the k3-am62-main.dtsi like it is the case in ti-6.6. > >>>> We (Max in Cc here) noticed that this specific use case is broken on >>>> your downstream v6.6 TI branch. >> >> Yes, it was been brought to my attention that the single-link usecase is >> not working over the downstream ti-6.6 kernel. As I have since >> discovered, it's not working on this series either. >> >> For some reason, the supplier-consumer dependency between the OLDI and >> the panel devicetree nodes is not getting flagged as `FWLINK_FLAG_CYCLE` >> in cases of single-link configuration. >> >> This flag allows the panel driver to continue to probe without waiting >> for the OLDI driver (panel's supplier). Absence of the flag getting set >> is causing these drivers to keep deferring probe in an endless cycle. >> >> Even with the flag, the OLDI (and tidss) cannot complete probe until the >> panel driver is probed and ready. That is because the OLDI (and tidss) >> need the panel to continue with the bridge-chain creation. >> >> However, over with the dual-lvds configuration (and as Francesco has >> now mentioned the clone configuration as well), the flag gets set by >> default, and everything works. >> >> This is what the debug has led to, so far. > > Yes, I came to the same conclusion with my debug. > >>> >>> What if you set "fw_devlink=off" kernel boot parameter? >>> >> >> Yes! I haven't tested it, but it seems that this will bypass the >> supplier check and let the panel probe continue. >> >> >> Tomi, any idea, why is this issue happening only for single-link in the >> first place? It seems as if having 2 ports inside the panel devicetree >> lets the probe mechanism recognize the circular dependency and ignore >> the supplier OLDIs? > > I have to say I have no idea... > > I don't really understand the devlink code here, but I'm guessing that > the "cycle" part comes from the fact that with a media graph we have > links (remote-endpoint) both ways in the DT data. So it's not possible > to say which side is the consumer, which one is the supplier. Thus, it's > marked as a cycle and, I think, basically ignored for the probing purpose. Okay! I am not too sure about the devlink code either, but this reasoning makes sense. > > But why not here? I can see the links being created both ways: > > /display Linked as a fwnode consumer to /bus@f0000/dss@30200000/oldi- > txes/oldi@0 > /bus@f0000/dss@30200000/oldi-txes/oldi@0 Linked as a fwnode consumer > to /display > > but it's never marked as a cycle. Yes, this matches my observations. > >> This is the function where the difference comes down to, by the way - >> __fw_devlink_relax_cycles(), per my knowledge. I am still on my way to >> understand what exactly it is doing and why is it not relaxing the >> single-link case. > > Yep. The answer is probably somewhere there =). > Alright! We have an interesting problem in our hands now. =) Regards Aradhya
Hello all, This patch series add support for the dual OLDI TXes supported in Texas Instruments' AM62x and AM62Px family of SoCs. The OLDI TXes support single-lvds, lvds-clone, and dual-lvds modes. These have now been represented through DRM bridges within TI-DSS. - Some history and hardware description for this patch series. This patch series is a complete re-vamp from the previously posted series[1] and hence, the version index has been reset to v1. The OLDI support from that series was dropped and only the base support for AM62x DSS was kept (and eventually merged)[2]. The OLDI display that the tidss driver today supports, could not be extended for the newer SoCs. The OLDI display in tidss is modelled after the DSS and OLDI hardware in the AM65x SoC. The DSS in AM65x SoC, has two video-ports. Both these video-ports (VP) output DPI video signals. One of the DPI output (from VP1) from the DSS connects to a singular OLDI TX present inside the SoC. There is no other way for the DPI from VP1 to be taken out of the SoC. The other DPI output however - the one from VP2 - is taken out of the SoC as is. Hence we have an OLDI bus output and a DPI bus output from the SoC. Since the VP1 and OLDI are tightly coupled, the tidss driver considers them as a single entity. That is why, any OLDI sink connects directly to the DSS ports in the OF graphs. The newer SoCs have varying DSS and OLDI integrations. The AM62x DSS also has 2 VPs. The 2nd VP, VP2, outputs DPI signals which are taken out of the SoC - similar to the AM65x above. For the VP1, there are 2 OLDI TXes. These OLDI TXes can only receive DPI signals from VP1, and don't connect to VP2 at all. The AM62Px SoC has 2 OLDI TXes like AM62x SoC. However, the AM62Px SoC also has 2 separate DSSes. The 2 OLDI TXes can now be shared between the 2 VPs of the 2 DSSes. The addition of the 2nd OLDI TX (and a 2nd DSS in AM62Px) creates a need for some major changes for a full feature experience. 1. The OF graph needs to be updated to accurately show the data flow. 2. The tidss and OLDI drivers now need to support the dual-link and the cloned single-link OLDI video signals. 3. The drivers also need to support the case where 2 OLDI TXes are connected to 2 different VPs - thereby creating 2 independent streams of single-link OLDI outputs. Note that the OLDI does not have registers of its own. Its still dependent on the parent VP. The VP that provides the DPI video signals to the OLDI TXes, also gives the OLDI TXes all the config data. That is to say, the hardware doesn't sit on the data bus directly - but does so via DSS. In light of all of these hardware variations, it was decided to have a separate OLDI driver (unlike AM65x) but not entirely separate so as to be a platform device. The OLDI TXes are now being represented as DRM bridges under the tidss. Also, since the DRM framework only really supports a linear encoder-bridge chain, the OLDI driver creates a DRM bridge ONLY for the primary OLDI TX in cases of dual-link or cloned single-link OLDI modes. That bridge then attaches to the tidss's display core - which consists of a CRTC, an Encoder (dummy) and a bridge (dummy). On the other end, it attaches to OLDI sinks (panels or other bridges). Since the OLDI TX have a hardware dependency with the VP, the OLDI configuration needs to happen before that VP is enabled for streaming. VP stream enable takes place in tidss_crtc_atomic_enable hook. I have posted a patch allowing DRM bridges to get pre-enabled before the CRTC of that bridge is enabled[0]. Without that patch, some warnings or glitches can be seen. These patches have been tested on AM625 based SK-AM625 EVM with a Microptis dual-lvds panel (SK-LCD1). The patches with complete support including the expected devicetree configuration of the OLDI TXes can be found in the "next_oldi-v3-tests" branch of my github fork[3]. Thanks, Aradhya Change Log: V3: - Fix the dt_binding_check warning in patch 3/4[4] by adding "additionalProperties" constraint. V2: - Add all the R-b and A-b tags from Laurent Pinchart, Rob Herring, and Tomi Valkeinen. - Reword the subject for patch 1/4. - Reword the commit descriptions to add proper hardware detail. - Drop the change in schema reference for port@0 in patch 3/4. - Lots of improvements for patch 4/4. * Refactor OLDI selection logic in tidss_oldi_tx_power(). * Add "companion_instance" support to identify the OLDI index in dual-link or cloned sinle-link modes. * De-initialize tidss_oldi during tidss removal. * Use dev_err_probe() instead of dev_err(). * Drop OLDI(n) macro. * Move OLDI Config register bits to tidss_dispc_regs.h. * Drop oldi bridge atomic_check(). * s/%d/%u for all print instances of "oldi_instance". * Move OLDI init after DISPC init in tidss_probe. * Use devm_drm_of_get_bridge() instead of drm_of_find_panel_or_bridge() to find the next bridge and drop all the drm_panel support from tidss_oldi. Previous revisions: V2: https://lore.kernel.org/all/20240715200953.1213284-1-a-bhatia1@ti.com/ V1: https://lore.kernel.org/all/20240511193055.1686149-1-a-bhatia1@ti.com/ [0]: Dependency Patch: ("drm/atomic-helper: Re-order bridge chain pre-enable and post-disable") https://lore.kernel.org/all/20240622110929.3115714-11-a-bhatia1@ti.com/ [1]: AM62 OLDI Series - v7 https://lore.kernel.org/all/20230125113529.13952-1-a-bhatia1@ti.com/ [2]: AM62 DSS Series - v9 https://lore.kernel.org/all/20230616150900.6617-1-a-bhatia1@ti.com/ [3]: GitHub Fork for OLDI tests https://github.com/aradhya07/linux-ab/tree/next_oldi-v3-tests/ [4]: ("ti,am65x-dss.yaml: oldi-txes: Missing additionalProperties/ unevaluatedProperties constraint") https://lore.kernel.org/all/172107979988.1595945.9666141982402158422.robh@kernel.org/ Aradhya Bhatia (4): dt-bindings: display: ti,am65x-dss: Re-indent the example dt-bindings: display: ti: Add schema for AM625 OLDI Transmitter dt-bindings: display: ti,am65x-dss: Add OLDI properties for AM625 DSS drm/tidss: Add OLDI bridge support .../bindings/display/ti/ti,am625-oldi.yaml | 153 +++++ .../bindings/display/ti/ti,am65x-dss.yaml | 177 +++++- MAINTAINERS | 1 + drivers/gpu/drm/tidss/Makefile | 3 +- drivers/gpu/drm/tidss/tidss_dispc.c | 20 +- drivers/gpu/drm/tidss/tidss_dispc.h | 4 + drivers/gpu/drm/tidss/tidss_dispc_regs.h | 14 + drivers/gpu/drm/tidss/tidss_drv.c | 9 + drivers/gpu/drm/tidss/tidss_drv.h | 5 + drivers/gpu/drm/tidss/tidss_oldi.c | 537 ++++++++++++++++++ drivers/gpu/drm/tidss/tidss_oldi.h | 51 ++ 11 files changed, 951 insertions(+), 23 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml create mode 100644 drivers/gpu/drm/tidss/tidss_oldi.c create mode 100644 drivers/gpu/drm/tidss/tidss_oldi.h base-commit: 3fe121b622825ff8cc995a1e6b026181c48188db