Hi Marek,

On 23.12.2021 10:15, Marek Szyprowski wrote:

Hi Jagan,

On 18.12.2021 00:16, Marek Szyprowski wrote:

...

On 15.12.2021 15:56, Jagan Teki wrote:

...

On Wed, Dec 15, 2021 at 7:49 PM Marek Szyprowski m.szyprowski@samsung.com wrote:

...

On 15.12.2021 13:57, Jagan Teki wrote:

...

On Wed, Dec 15, 2021 at 5:31 PM Marek Szyprowski m.szyprowski@samsung.com wrote:

...

On 15.12.2021 11:15, Jagan Teki wrote: > Updated series about drm bridge conversion of exynos dsi. > Previous version can be accessible, here [1]. > > Patch 1: connector reset > > Patch 2: panel_bridge API > > Patch 3: Bridge conversion > > Patch 4: Atomic functions > > Patch 5: atomic_set > > Patch 6: DSI init in enable There is a little progress! :)

Devices with a simple display pipeline (only a DSI panel, like Trats/Trats2) works till the last patch. Then, after applying ("[PATCH v4 6/6] drm: exynos: dsi: Move DSI init in bridge enable"), I get no display at all.

A TM2e board with in-bridge (Exynos MIC) stops displaying anything after applying patch "[PATCH v4 2/6] drm: exynos: dsi: Use drm panel_bridge API".

In case of the Arndale board with tc358764 bridge, no much progress. The display is broken just after applying the "[PATCH v2] drm: bridge: tc358764: Use drm panel_bridge API" patch on top of linux-next.

In all cases the I had "drm: of: Lookup if child node has panel or bridge" patch applied.

Just skip the 6/6 for now.

Apply

https://protect2.fireeye.com/v1/url?k=a24f3f76-fdd40659-a24eb439-0cc47a31cdf...

https://protect2.fireeye.com/v1/url?k=a226360f-fdbd0f20-a227bd40-0cc47a31cdf...

Then apply 1/6 to 5/6.  and update the status?

Okay, my fault, I didn't check that case on Arndale.

I've checked and indeed, Trats/Trats2 and Arndale works after the above 2 patches AND patches 1-5.

The only problem is now on TM2e, which uses Exynos MIC as in-bridge for Exynos DSI:

[    4.068866] [drm] Exynos DRM: using 13800000.decon device for DMA mapping operations [    4.069183] exynos-drm exynos-drm: bound 13800000.decon (ops decon_component_ops) [    4.128983] exynos-drm exynos-drm: bound 13880000.decon (ops decon_component_ops) [    4.129261] exynos-drm exynos-drm: bound 13930000.mic (ops exynos_mic_component_ops) [    4.133508] exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] *ERROR* failed to find the bridge: -19 [    4.136392] exynos-drm exynos-drm: bound 13900000.dsi (ops exynos_dsi_component_ops) [    4.145499] rc_core: Couldn't load IR keymap rc-cec [    4.145666] Registered IR keymap rc-empty [    4.148402] rc rc0: sii8620 as /devices/virtual/rc/rc0 [    4.156051] input: sii8620 as /devices/virtual/rc/rc0/input1 [    4.160647] exynos-drm exynos-drm: bound 13970000.hdmi (ops hdmi_component_ops) [    4.169923] exynos-drm exynos-drm: [drm] Cannot find any crtc or sizes [    4.173958] exynos-drm exynos-drm: [drm] Cannot find any crtc or sizes [    4.182304] [drm] Initialized exynos 1.1.0 20180330 for exynos-drm on minor 0

The display pipeline for TM2e is:

Exynos5433 Decon -> Exynos MIC -> Exynos DSI -> s6e3ha2 DSI panel

If Trats/Trats2 is working then it has to work. I don't see any difference in output pipeline. Can you please share the full log, I cannot see host_attach print saying "Attached.."

Well, there is a failure message about the panel:

exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] *ERROR* failed to find the bridge: -19

however it looks that something might be broken in dts. The in-bridge (Exynos MIC) is on port 0 and the panel is @0, what imho might cause the issue.

I've tried to change in in-bridge ('mic_to_dsi') port to 1 in exynos5433.dtsi. Then the panel has been attached:

exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] Attached s6e3hf2 device

but the display is still not working, probably due to lack of proper Exynos MIC handling. I will investigate it later and let You know.

I've played a bit with the Exynos DRM code and finally I made it working on TM2(e). There are basically 3 different issues that need to be fixed to get it working with the $subject patchset:

1. Port numbers in exynos5433 dsi/dts are broken. For all pre-Exynos5433

boards the panel was defined as a DSI node child (at 'reg 0'),

True, emphasis that it is reg 0 of DSI bus.

what means it used port 0.

And this does not seems true to me. Established practice is (unless I have not noticed change in bindings :) ) that in case of data bus shared with control bus the port node is optional. In such case host knows already who is connected to its data bus, it does not need port node. And if there is no port node there is no port number as well.

As I quickly looked at the exynos bindings they seems generally OK to me, if there is something wrong/suspicious let me know.

Then, Exynos5433 introduced so called RGB-in at port 0 and panel at port 1 (as described in the dt bindings).However the committed Exynos5433 dtsi and TM2(e) dts still defined panel as a DSI child (with reg=0, so port 0) and Exynos MIC as of-graph at port 0. The Exynos DSI code however always searched for a panel as a DSI child node, so it worked fine, even though the panel and exynos mic used in fact the 'port 0'. IMHO this can be fixed by the following patch:

diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi b/arch/arm64/boot/dts/exynos/exynos5433.dtsi index bfe4ed8a23d6..2718c752d916 100644 --- a/arch/arm64/boot/dts/exynos/exynos5433.dtsi +++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi @@ -1046,8 +1046,8 @@                                 #address-cells = <1>;                                 #size-cells = <0>;

-                               port@0 { -                                       reg = <0>; +                               port@1 { +                                       reg = <1>;                                         dsi_to_mic: endpoint {                                                 remote-endpoint = <&mic_to_dsi>;                                         }; diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index d2933a70c01f..e8e2df339c5f 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -220,8 +220,8 @@ enum exynos_dsi_transfer_type {  };

enum { -       DSI_PORT_IN, -       DSI_PORT_OUT +       DSI_PORT_OUT, +       DSI_PORT_IN  };

struct exynos_dsi_transfer {

2. (devm_)drm_of_get_bridge() ignores panel's 'reg' property and it is

I guess drm_of_get_bridge should not be used in exynos_dsi_host_attach at all - there are no ports here, only of_node of the sink.

Since there is no helper to workaround the dualism panel/bridge you should still use of_drm_find_bridge and of_drm_find_panel pair.

unable to find a panel if there is a 'ports' child node (even if it describes completely different port than the one requested by the caller). I don't have time now to fix this, a simple and ugly workaround for that is to define the complete of-graph for the DSI child panel:

diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts b/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts index 22d26460f3dd..4726c325bd2c 100644 --- a/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts +++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts @@ -58,6 +58,27 @@                 vci-supply = <&ldo28_reg>;                 reset-gpios = <&gpg0 0 GPIO_ACTIVE_LOW>;                 enable-gpios = <&gpf1 5 GPIO_ACTIVE_HIGH>;

+               ports { +                       #address-cells = <1>; +                       #size-cells = <0>;

+                       port@0 { +                               reg = <0>; +                               panel_to_dsi: endpoint { +                                       remote-endpoint = <&dsi_to_panel>; +                               }; +                       }; +               }; +       };

+       ports { +               port@0 { +                       reg = <0>; +                       dsi_to_panel: endpoint { +                               remote-endpoint = <&panel_to_dsi>; +                       }; +               };         };  };

--

3. Lack of proper handling of the 'in-bridge' in the Exynos DSI after

Your conversion. Initially I thought that this could be fixed with the following simple patch:

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index e8e2df339c5f..2fe9c995549f 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -259,6 +259,7 @@ struct exynos_dsi {         struct mipi_dsi_host dsi_host;         struct drm_bridge bridge;         struct drm_bridge *out_bridge; +       struct drm_bridge *in_bridge;         struct device *dev;

void __iomem *reg_base; @@ -1535,7 +1536,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,

drm_bridge_add(&dsi->bridge);

-       drm_bridge_attach(encoder, &dsi->bridge, NULL, 0); +       drm_bridge_attach(encoder, &dsi->bridge, dsi->in_bridge, 0);

/*          * This is a temporary solution and should be made by more generic way. @@ -1674,7 +1675,6 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,         struct drm_encoder *encoder = &dsi->encoder;         struct drm_device *drm_dev = data;         struct device_node *in_bridge_node; -       struct drm_bridge *in_bridge;         int ret;

printk("drm: %s enter\n", __func__); @@ -1688,9 +1688,10 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,         }         in_bridge_node = of_graph_get_remote_node(dev->of_node, DSI_PORT_IN, 0);         if (in_bridge_node) { -               in_bridge = of_drm_find_bridge(in_bridge_node); -               if (in_bridge) -                       drm_bridge_attach(encoder, in_bridge, NULL, 0); +               dsi->in_bridge = of_drm_find_bridge(in_bridge_node); +               if (dsi->in_bridge) { +                       drm_bridge_attach(encoder, dsi->in_bridge, NULL, 0); +               }                 of_node_put(in_bridge_node);         }

--

but it turned out that in such case the order of the bridge 'enable' calls is not correct for display pipeline operation. The one that actually works is (sorry for the hacky code):

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index e8e2df339c5f..51b568556fce 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -259,6 +259,7 @@ struct exynos_dsi {         struct mipi_dsi_host dsi_host;         struct drm_bridge bridge;         struct drm_bridge *out_bridge; +       struct drm_bridge *in_bridge;         struct device *dev;

void __iomem *reg_base; @@ -1424,6 +1425,9 @@ static void exynos_dsi_atomic_pre_enable(struct drm_bridge *bridge,

dsi->state |= DSIM_STATE_ENABLED;         printk("drm: %s finish: %d\n", __func__, 0);

+       if (dsi->in_bridge)

• dsi->in_bridge->funcs->pre_enable(dsi->in_bridge);  }

  static void exynos_dsi_atomic_enable(struct drm_bridge *bridge,

@@ -1439,6 +1443,9 @@ static void exynos_dsi_atomic_enable(struct drm_bridge *bridge,

printk("drm: %s finish: %d\n", __func__, 0);

+       if (dsi->in_bridge)

• dsi->in_bridge->funcs->enable(dsi->in_bridge);
• return;  }

@@ -1455,6 +1462,8 @@ static void exynos_dsi_atomic_disable(struct drm_bridge *bridge,         dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;         printk("drm: %s finish: %d\n", __func__, 0);

+       if (dsi->in_bridge)

• dsi->in_bridge->funcs->disable(dsi->in_bridge);  }

  static void exynos_dsi_atomic_post_disable(struct drm_bridge *bridge,

@@ -1470,6 +1479,8 @@ static void exynos_dsi_atomic_post_disable(struct drm_bridge *bridge,

printk("drm: %s finish: %d\n", __func__, 0);

+       if (dsi->in_bridge)

• dsi->in_bridge->funcs->post_disable(dsi->in_bridge);  }

  static void exynos_dsi_mode_set(struct drm_bridge *bridge,

@@ -1482,6 +1493,10 @@ static void exynos_dsi_mode_set(struct drm_bridge *bridge,

drm_mode_copy(&dsi->mode, adjusted_mode);         printk("drm: %s finish: %d\n", __func__, ret);

+       if (dsi->in_bridge)

• dsi->in_bridge->funcs->mode_set(dsi->in_bridge, mode,

+                                               adjusted_mode);  }

static int exynos_dsi_attach(struct drm_bridge *bridge,  @@ -1674,7 +1689,6 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,         struct drm_encoder *encoder = &dsi->encoder;         struct drm_device *drm_dev = data;         struct device_node *in_bridge_node; -       struct drm_bridge *in_bridge;         int ret;

printk("drm: %s enter\n", __func__); @@ -1688,9 +1702,11 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,         }         in_bridge_node = of_graph_get_remote_node(dev->of_node, DSI_PORT_IN, 0);         if (in_bridge_node) { -               in_bridge = of_drm_find_bridge(in_bridge_node); -               if (in_bridge) -                       drm_bridge_attach(encoder, in_bridge, NULL, 0); +               dsi->in_bridge = of_drm_find_bridge(in_bridge_node); +               if (dsi->in_bridge) { +                       drm_bridge_attach(encoder, dsi->in_bridge, NULL, 0);

• INIT_LIST_HEAD(&encoder->bridge_chain);

+               }                 of_node_put(in_bridge_node);         }

--

I hope the above findings helps somehow in finishing this patchset. I'm not deep in the DRM bridge development, but it looks that there is something seriously broken in the design or Exynos MIC and DSI should not be modeled as bridges.

I guess the most 'correct' way of handling mic would be dropping completely in_bridge from exynos_dsi, instead use bridge chain.

This would require touching decon driver and re-think the role of almost dummy 'encoder' code.

I suppose original author of mic bridge did this way because bridge framework did not supported attaching bridge between crtc and encoder, but now with exynos_dsi as a bridge it should be possible.

But this could be next step.

Regards

Andrzej

I will be on holidays till 10th Jan 2022.

Best regards

Hi Jagan,

On 11.01.2022 10:32, Jagan Teki wrote:

Hi Andrzej,

On Tue, Dec 28, 2021 at 4:18 PM Andrzej Hajda andrzej.hajda@intel.com wrote:

...

Hi Marek,

On 23.12.2021 10:15, Marek Szyprowski wrote:

...

Hi Jagan,

On 18.12.2021 00:16, Marek Szyprowski wrote:

...

On 15.12.2021 15:56, Jagan Teki wrote:

...

On Wed, Dec 15, 2021 at 7:49 PM Marek Szyprowski m.szyprowski@samsung.com wrote:

...

On 15.12.2021 13:57, Jagan Teki wrote: > On Wed, Dec 15, 2021 at 5:31 PM Marek Szyprowski > m.szyprowski@samsung.com wrote: >> On 15.12.2021 11:15, Jagan Teki wrote: >>> Updated series about drm bridge conversion of exynos dsi. >>> Previous version can be accessible, here [1]. >>> >>> Patch 1: connector reset >>> >>> Patch 2: panel_bridge API >>> >>> Patch 3: Bridge conversion >>> >>> Patch 4: Atomic functions >>> >>> Patch 5: atomic_set >>> >>> Patch 6: DSI init in enable >> There is a little progress! :) >> >> Devices with a simple display pipeline (only a DSI panel, like >> Trats/Trats2) works till the last patch. Then, after applying >> ("[PATCH >> v4 6/6] drm: exynos: dsi: Move DSI init in bridge enable"), I get no >> display at all. >> >> A TM2e board with in-bridge (Exynos MIC) stops displaying anything >> after >> applying patch "[PATCH v4 2/6] drm: exynos: dsi: Use drm >> panel_bridge API". >> >> In case of the Arndale board with tc358764 bridge, no much >> progress. The >> display is broken just after applying the "[PATCH v2] drm: bridge: >> tc358764: Use drm panel_bridge API" patch on top of linux-next. >> >> In all cases the I had "drm: of: Lookup if child node has panel or >> bridge" patch applied. > Just skip the 6/6 for now. > > Apply > - > https://protect2.fireeye.com/v1/url?k=a24f3f76-fdd40659-a24eb439-0cc47a31cdf... > - > https://protect2.fireeye.com/v1/url?k=a226360f-fdbd0f20-a227bd40-0cc47a31cdf... > > Then apply 1/6 to 5/6. and update the status? Okay, my fault, I didn't check that case on Arndale.

I've checked and indeed, Trats/Trats2 and Arndale works after the above 2 patches AND patches 1-5.

The only problem is now on TM2e, which uses Exynos MIC as in-bridge for Exynos DSI:

[ 4.068866] [drm] Exynos DRM: using 13800000.decon device for DMA mapping operations [ 4.069183] exynos-drm exynos-drm: bound 13800000.decon (ops decon_component_ops) [ 4.128983] exynos-drm exynos-drm: bound 13880000.decon (ops decon_component_ops) [ 4.129261] exynos-drm exynos-drm: bound 13930000.mic (ops exynos_mic_component_ops) [ 4.133508] exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] *ERROR* failed to find the bridge: -19 [ 4.136392] exynos-drm exynos-drm: bound 13900000.dsi (ops exynos_dsi_component_ops) [ 4.145499] rc_core: Couldn't load IR keymap rc-cec [ 4.145666] Registered IR keymap rc-empty [ 4.148402] rc rc0: sii8620 as /devices/virtual/rc/rc0 [ 4.156051] input: sii8620 as /devices/virtual/rc/rc0/input1 [ 4.160647] exynos-drm exynos-drm: bound 13970000.hdmi (ops hdmi_component_ops) [ 4.169923] exynos-drm exynos-drm: [drm] Cannot find any crtc or sizes [ 4.173958] exynos-drm exynos-drm: [drm] Cannot find any crtc or sizes [ 4.182304] [drm] Initialized exynos 1.1.0 20180330 for exynos-drm on minor 0

The display pipeline for TM2e is:

Exynos5433 Decon -> Exynos MIC -> Exynos DSI -> s6e3ha2 DSI panel

If Trats/Trats2 is working then it has to work. I don't see any difference in output pipeline. Can you please share the full log, I cannot see host_attach print saying "Attached.."

Well, there is a failure message about the panel:

exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] *ERROR* failed to find the bridge: -19

however it looks that something might be broken in dts. The in-bridge (Exynos MIC) is on port 0 and the panel is @0, what imho might cause the issue.

I've tried to change in in-bridge ('mic_to_dsi') port to 1 in exynos5433.dtsi. Then the panel has been attached:

exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] Attached s6e3hf2 device

but the display is still not working, probably due to lack of proper Exynos MIC handling. I will investigate it later and let You know.

I've played a bit with the Exynos DRM code and finally I made it working on TM2(e). There are basically 3 different issues that need to be fixed to get it working with the $subject patchset:

1. Port numbers in exynos5433 dsi/dts are broken. For all pre-Exynos5433

boards the panel was defined as a DSI node child (at 'reg 0'),

True, emphasis that it is reg 0 of DSI bus.

...

what means it used port 0.

And this does not seems true to me. Established practice is (unless I have not noticed change in bindings :) ) that in case of data bus shared with control bus the port node is optional. In such case host knows already who is connected to its data bus, it does not need port node. And if there is no port node there is no port number as well.

As I quickly looked at the exynos bindings they seems generally OK to me, if there is something wrong/suspicious let me know.

...

Then, Exynos5433 introduced so called RGB-in at port 0 and panel at port 1 (as described in the dt bindings).However the committed Exynos5433 dtsi and TM2(e) dts still defined panel as a DSI child (with reg=0, so port 0) and Exynos MIC as of-graph at port 0. The Exynos DSI code however always searched for a panel as a DSI child node, so it worked fine, even though the panel and exynos mic used in fact the 'port 0'. IMHO this can be fixed by the following patch:

diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi b/arch/arm64/boot/dts/exynos/exynos5433.dtsi index bfe4ed8a23d6..2718c752d916 100644 --- a/arch/arm64/boot/dts/exynos/exynos5433.dtsi +++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi @@ -1046,8 +1046,8 @@ #address-cells = <1>; #size-cells = <0>;

•                           port@0 {
  

•                                   reg = <0>;
  

•                           port@1 {
  

•                                   reg = <1>;
                                       dsi_to_mic: endpoint {
                                               remote-endpoint =
  

<&mic_to_dsi>; }; diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index d2933a70c01f..e8e2df339c5f 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -220,8 +220,8 @@ enum exynos_dsi_transfer_type { };

enum {

•   DSI_PORT_IN,
  

•   DSI_PORT_OUT
  

•   DSI_PORT_OUT,
  

•   DSI_PORT_IN
  

  };

  struct exynos_dsi_transfer {

--

2. (devm_)drm_of_get_bridge() ignores panel's 'reg' property and it is

I guess drm_of_get_bridge should not be used in exynos_dsi_host_attach at all - there are no ports here, only of_node of the sink.

Since there is no helper to workaround the dualism panel/bridge you should still use of_drm_find_bridge and of_drm_find_panel pair.

We have 2 use cases so far for adding input and outputs for a given host node.

1. with ports

   dsi { compatible = "samsung,exynos5433-mipi-dsi"; #address-cells = <1>; #size-cells = <0>;

          ports {
                             #address-cells = <1>;
                             #size-cells = <0>;
   
                             port@0 {
                                     reg = <0>;
                                     dsi_to_mic: endpoint {
                                             remote-endpoint = <&mic_to_dsi>;
                                     };
                             };
   
                        port@1 {
                                    reg = <1>;
   
                                   dsi_out_panel: endpoint {
                                             remote-endpoint =
   

<&dsi_in_panel>; }; }; };

         panel@0 {
                 compatible = "samsung,s6e3hf2";
                 reg = <0>;
                 vdd3-supply = <&ldo27_reg>;
                 vci-supply = <&ldo28_reg>;
                 reset-gpios = <&gpg0 0 GPIO_ACTIVE_LOW>;
                 enable-gpios = <&gpf1 5 GPIO_ACTIVE_HIGH>;

                 port {
                                 dsi_in_panel: endpoint {
                                        remote-endpoint = <&dsi_out_panel>;
                                  };
                 };
         };
 };

2. with port

   dsi { compatible = "samsung,exynos5433-mipi-dsi"; #address-cells = <1>; #size-cells = <0>;

         port {
                 dsi_to_mic: endpoint {
                         remote-endpoint = <&mic_to_dsi>;
                 };
         };
   
         panel@0 {
                 compatible = "samsung,s6e3hf2";
                 reg = <0>;
                 vdd3-supply = <&ldo27_reg>;
                 vci-supply = <&ldo28_reg>;
                 reset-gpios = <&gpg0 0 GPIO_ACTIVE_LOW>;
                 enable-gpios = <&gpf1 5 GPIO_ACTIVE_HIGH>;
         };
   

   };

We have a patch which do find the panel/bridge as a child node[1] via devm_drm_of_get_bridge. However that based on the above use cases where child panel/bridge added as per 2 use case and there is no possibility of child node in 1 use case as it has a feasibility to add outputs via 'ports'.

Since exynos5433 has 'ports' in host node, this patches [2] added panel via port@1.

[1] https://patchwork.amarulasolutions.com/patch/1823/ [2] https://patchwork.amarulasolutions.com/patch/1836/

Maybe I am missing something, but you do not have to 'find' panel/bridge, you have it already - it is 'device' argument of exynos_dsi_host_attach.

For me it looks odd when panel calls 'hey I am here, ready to proceed' (exynos_dsi_host_attach callback), and then dsi host ignores this call, instead it look for panel using different mechanism.

Regards

Andrzej

Thanks, Jagan.

Hi Marek,

On Thu, Dec 23, 2021 at 2:45 PM Marek Szyprowski m.szyprowski@samsung.com wrote:

Hi Jagan,

On 18.12.2021 00:16, Marek Szyprowski wrote:

...

On 15.12.2021 15:56, Jagan Teki wrote:

...

On Wed, Dec 15, 2021 at 7:49 PM Marek Szyprowski m.szyprowski@samsung.com wrote:

...

On 15.12.2021 13:57, Jagan Teki wrote:

...

On Wed, Dec 15, 2021 at 5:31 PM Marek Szyprowski m.szyprowski@samsung.com wrote:

...

On 15.12.2021 11:15, Jagan Teki wrote: > Updated series about drm bridge conversion of exynos dsi. > Previous version can be accessible, here [1]. > > Patch 1: connector reset > > Patch 2: panel_bridge API > > Patch 3: Bridge conversion > > Patch 4: Atomic functions > > Patch 5: atomic_set > > Patch 6: DSI init in enable There is a little progress! :)

Devices with a simple display pipeline (only a DSI panel, like Trats/Trats2) works till the last patch. Then, after applying ("[PATCH v4 6/6] drm: exynos: dsi: Move DSI init in bridge enable"), I get no display at all.

A TM2e board with in-bridge (Exynos MIC) stops displaying anything after applying patch "[PATCH v4 2/6] drm: exynos: dsi: Use drm panel_bridge API".

In case of the Arndale board with tc358764 bridge, no much progress. The display is broken just after applying the "[PATCH v2] drm: bridge: tc358764: Use drm panel_bridge API" patch on top of linux-next.

In all cases the I had "drm: of: Lookup if child node has panel or bridge" patch applied.

Just skip the 6/6 for now.

Apply

https://protect2.fireeye.com/v1/url?k=a24f3f76-fdd40659-a24eb439-0cc47a31cdf...

https://protect2.fireeye.com/v1/url?k=a226360f-fdbd0f20-a227bd40-0cc47a31cdf...

Then apply 1/6 to 5/6. and update the status?

Okay, my fault, I didn't check that case on Arndale.

I've checked and indeed, Trats/Trats2 and Arndale works after the above 2 patches AND patches 1-5.

The only problem is now on TM2e, which uses Exynos MIC as in-bridge for Exynos DSI:

[ 4.068866] [drm] Exynos DRM: using 13800000.decon device for DMA mapping operations [ 4.069183] exynos-drm exynos-drm: bound 13800000.decon (ops decon_component_ops) [ 4.128983] exynos-drm exynos-drm: bound 13880000.decon (ops decon_component_ops) [ 4.129261] exynos-drm exynos-drm: bound 13930000.mic (ops exynos_mic_component_ops) [ 4.133508] exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] *ERROR* failed to find the bridge: -19 [ 4.136392] exynos-drm exynos-drm: bound 13900000.dsi (ops exynos_dsi_component_ops) [ 4.145499] rc_core: Couldn't load IR keymap rc-cec [ 4.145666] Registered IR keymap rc-empty [ 4.148402] rc rc0: sii8620 as /devices/virtual/rc/rc0 [ 4.156051] input: sii8620 as /devices/virtual/rc/rc0/input1 [ 4.160647] exynos-drm exynos-drm: bound 13970000.hdmi (ops hdmi_component_ops) [ 4.169923] exynos-drm exynos-drm: [drm] Cannot find any crtc or sizes [ 4.173958] exynos-drm exynos-drm: [drm] Cannot find any crtc or sizes [ 4.182304] [drm] Initialized exynos 1.1.0 20180330 for exynos-drm on minor 0

The display pipeline for TM2e is:

Exynos5433 Decon -> Exynos MIC -> Exynos DSI -> s6e3ha2 DSI panel

If Trats/Trats2 is working then it has to work. I don't see any difference in output pipeline. Can you please share the full log, I cannot see host_attach print saying "Attached.."

Well, there is a failure message about the panel:

exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] *ERROR* failed to find the bridge: -19

however it looks that something might be broken in dts. The in-bridge (Exynos MIC) is on port 0 and the panel is @0, what imho might cause the issue.

I've tried to change in in-bridge ('mic_to_dsi') port to 1 in exynos5433.dtsi. Then the panel has been attached:

exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] Attached s6e3hf2 device

but the display is still not working, probably due to lack of proper Exynos MIC handling. I will investigate it later and let You know.

I've played a bit with the Exynos DRM code and finally I made it working on TM2(e). There are basically 3 different issues that need to be fixed to get it working with the $subject patchset:

1. Port numbers in exynos5433 dsi/dts are broken. For all pre-Exynos5433

boards the panel was defined as a DSI node child (at 'reg 0'), what means it used port 0. Then, Exynos5433 introduced so called RGB-in at port 0 and panel at port 1 (as described in the dt bindings). However the committed Exynos5433 dtsi and TM2(e) dts still defined panel as a DSI child (with reg=0, so port 0) and Exynos MIC as of-graph at port 0. The Exynos DSI code however always searched for a panel as a DSI child node, so it worked fine, even though the panel and exynos mic used in fact the 'port 0'. IMHO this can be fixed by the following patch:

diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi b/arch/arm64/boot/dts/exynos/exynos5433.dtsi index bfe4ed8a23d6..2718c752d916 100644 --- a/arch/arm64/boot/dts/exynos/exynos5433.dtsi +++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi @@ -1046,8 +1046,8 @@ #address-cells = <1>; #size-cells = <0>;

•                           port@0 {
  

•                                   reg = <0>;
  

•                           port@1 {
  

•                                   reg = <1>;
                                     dsi_to_mic: endpoint {
                                             remote-endpoint =
  

<&mic_to_dsi>; }; diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index d2933a70c01f..e8e2df339c5f 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -220,8 +220,8 @@ enum exynos_dsi_transfer_type { };

enum {

•   DSI_PORT_IN,
  

•   DSI_PORT_OUT
  

•   DSI_PORT_OUT,
  

•   DSI_PORT_IN
  

  };

  struct exynos_dsi_transfer {

--

port@1 of mic is invalid, it is input to DSI so it has to be port@0 with proper pipeline on dts file can fix the things.

--- a/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts +++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts @@ -51,6 +51,16 @@ &cmu_disp { };

&dsi { + ports { + port@1 { + reg = <1>; + + dsi_out_panel: endpoint { + remote-endpoint = <&dsi_in_panel>; + }; + }; + }; + panel@0 { compatible = "samsung,s6e3hf2"; reg = <0>; @@ -58,6 +68,12 @@ panel@0 { vci-supply = <&ldo28_reg>; reset-gpios = <&gpg0 0 GPIO_ACTIVE_LOW>; enable-gpios = <&gpf1 5 GPIO_ACTIVE_HIGH>; + + port { + dsi_in_panel: endpoint { + remote-endpoint = <&dsi_out_panel>; + }; + }; }; };

2. (devm_)drm_of_get_bridge() ignores panel's 'reg' property and it is

unable to find a panel if there is a 'ports' child node (even if it describes completely different port than the one requested by the caller). I don't have time now to fix this, a simple and ugly workaround for that is to define the complete of-graph for the DSI child panel:

diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts b/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts index 22d26460f3dd..4726c325bd2c 100644 --- a/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts +++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts @@ -58,6 +58,27 @@ vci-supply = <&ldo28_reg>; reset-gpios = <&gpg0 0 GPIO_ACTIVE_LOW>; enable-gpios = <&gpf1 5 GPIO_ACTIVE_HIGH>;

•           ports {
  

•                   #address-cells = <1>;
  

•                   #size-cells = <0>;
  

•                   port@0 {
  

•                           reg = <0>;
  

•                           panel_to_dsi: endpoint {
  

•                                   remote-endpoint = <&dsi_to_panel>;
  

•                           };
  

•                   };
  

•           };
  

•   };
  

•   ports {
  

•           port@0 {
  

•                   reg = <0>;
  

•                   dsi_to_panel: endpoint {
  

•                           remote-endpoint = <&panel_to_dsi>;
  

•                   };
  

•           };
     };
  

  };

--

3. Lack of proper handling of the 'in-bridge' in the Exynos DSI after

Your conversion. Initially I thought that this could be fixed with the following simple patch:

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index e8e2df339c5f..2fe9c995549f 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -259,6 +259,7 @@ struct exynos_dsi { struct mipi_dsi_host dsi_host; struct drm_bridge bridge; struct drm_bridge *out_bridge;

•   struct drm_bridge *in_bridge;
     struct device *dev;
  
     void __iomem *reg_base;
  

@@ -1535,7 +1536,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,

     drm_bridge_add(&dsi->bridge);

•   drm_bridge_attach(encoder, &dsi->bridge, NULL, 0);
  

•   drm_bridge_attach(encoder, &dsi->bridge, dsi->in_bridge, 0);
  
     /*
      * This is a temporary solution and should be made by more
  

generic way. @@ -1674,7 +1675,6 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, struct drm_encoder *encoder = &dsi->encoder; struct drm_device *drm_dev = data; struct device_node *in_bridge_node;

•   struct drm_bridge *in_bridge;
     int ret;
  
     printk("drm: %s enter\n", __func__);
  

@@ -1688,9 +1688,10 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, } in_bridge_node = of_graph_get_remote_node(dev->of_node, DSI_PORT_IN, 0); if (in_bridge_node) {

•           in_bridge = of_drm_find_bridge(in_bridge_node);
  

•           if (in_bridge)
  

•                   drm_bridge_attach(encoder, in_bridge, NULL, 0);
  

•           dsi->in_bridge = of_drm_find_bridge(in_bridge_node);
  

•           if (dsi->in_bridge) {
  

•                   drm_bridge_attach(encoder, dsi->in_bridge, NULL, 0);
  

•           }
             of_node_put(in_bridge_node);
     }
  
  

--

but it turned out that in such case the order of the bridge 'enable' calls is not correct for display pipeline operation. The one that actually works is (sorry for the hacky code):

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index e8e2df339c5f..51b568556fce 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -259,6 +259,7 @@ struct exynos_dsi { struct mipi_dsi_host dsi_host; struct drm_bridge bridge; struct drm_bridge *out_bridge;

•   struct drm_bridge *in_bridge;
     struct device *dev;
  
     void __iomem *reg_base;
  

@@ -1424,6 +1425,9 @@ static void exynos_dsi_atomic_pre_enable(struct drm_bridge *bridge,

     dsi->state |= DSIM_STATE_ENABLED;
     printk("drm: %s finish: %d\n", __func__, 0);

•   if (dsi->in_bridge)
  

• dsi->in_bridge->funcs->pre_enable(dsi->in_bridge); }

  static void exynos_dsi_atomic_enable(struct drm_bridge *bridge,

@@ -1439,6 +1443,9 @@ static void exynos_dsi_atomic_enable(struct drm_bridge *bridge,

     printk("drm: %s finish: %d\n", __func__, 0);

•   if (dsi->in_bridge)
  

• dsi->in_bridge->funcs->enable(dsi->in_bridge);

•    return;
  

  }

@@ -1455,6 +1462,8 @@ static void exynos_dsi_atomic_disable(struct drm_bridge *bridge, dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE; printk("drm: %s finish: %d\n", __func__, 0);

•   if (dsi->in_bridge)
  

• dsi->in_bridge->funcs->disable(dsi->in_bridge); }

  static void exynos_dsi_atomic_post_disable(struct drm_bridge *bridge,

@@ -1470,6 +1479,8 @@ static void exynos_dsi_atomic_post_disable(struct drm_bridge *bridge,

     printk("drm: %s finish: %d\n", __func__, 0);

•   if (dsi->in_bridge)
  

• dsi->in_bridge->funcs->post_disable(dsi->in_bridge); }

  static void exynos_dsi_mode_set(struct drm_bridge *bridge,

@@ -1482,6 +1493,10 @@ static void exynos_dsi_mode_set(struct drm_bridge *bridge,

     drm_mode_copy(&dsi->mode, adjusted_mode);
     printk("drm: %s finish: %d\n", __func__, ret);

•   if (dsi->in_bridge)
  

• dsi->in_bridge->funcs->mode_set(dsi->in_bridge, mode,

•                                           adjusted_mode);
  

  }

  static int exynos_dsi_attach(struct drm_bridge *bridge, @@ -1674,7 +1689,6 @@ static int exynos_dsi_bind(struct device *dev,

struct device *master, struct drm_encoder *encoder = &dsi->encoder; struct drm_device *drm_dev = data; struct device_node *in_bridge_node;

•   struct drm_bridge *in_bridge;
     int ret;
  
     printk("drm: %s enter\n", __func__);
  

@@ -1688,9 +1702,11 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, } in_bridge_node = of_graph_get_remote_node(dev->of_node, DSI_PORT_IN, 0); if (in_bridge_node) {

•           in_bridge = of_drm_find_bridge(in_bridge_node);
  

•           if (in_bridge)
  

•                   drm_bridge_attach(encoder, in_bridge, NULL, 0);
  

•           dsi->in_bridge = of_drm_find_bridge(in_bridge_node);
  

•           if (dsi->in_bridge) {
  

•                   drm_bridge_attach(encoder, dsi->in_bridge, NULL, 0);
  

• INIT_LIST_HEAD(&encoder->bridge_chain);

•           }
             of_node_put(in_bridge_node);
     }
  
  

--

I'm not quite sure of having in_bridge require explicit chain handling. It is the job of drm code as it finds the chain and invoke it. On the other hand, if it MIC is registered as bridge then the respective bridge calls has to be in their driver.

Let me fix the exynos5433 and send apart of the same series. then we can continue to see the trouble necks.

will that be fine?

Thanks, Jagan.