Message-ID: <51bbc12a-d601-339d-fa07-95a8744ccbfa@codeaurora.org>
Date:   Tue, 5 Dec 2017 10:46:36 +0530
From:   Archit Taneja <architt@...eaurora.org>
To:     Brian Norris <briannorris@...omium.org>
Cc:     Nickey Yang <nickey.yang@...k-chips.com>, robh+dt@...nel.org,
        heiko@...ech.de, mark.rutland@....com, airlied@...ux.ie,
        hl@...k-chips.com, zyw@...k-chips.comg,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linux-rockchip@...ts.infradead.org, xbl@...k-chips.com
Subject: Re: [PATCH v3 5/6] dt-bindings: add the rockchip, dual-channel for
 dw-mipi-dsi



On 12/05/2017 06:49 AM, Brian Norris wrote:
> Hi Archit,
> 
> I'm a relative n00b here, but I'm trying to follow along and I have some
> questions:
> 
> On Fri, Dec 01, 2017 at 06:29:04PM +0530, Archit Taneja wrote:
>> On 11/30/2017 11:02 PM, Nickey Yang wrote:
>>> I try to follow as you suggested,use
>>>
>>> mipi_dsi: mipi@...60000 {
>>>      ...
>>>      ...
>>>      clock-master;    /* implies that this DSI instance drivers the clock
>>>               * for both the DSIs.
>>>               */
>>>      ports {
>>>          mipi_in: port {
>>>              ...
>>>              ...
>>>          };
>>>          /* add extra output ports for both DSIs */
>>>          mipi_out: port {
>>>              mipi_panel_out: endpoint {
>>>                  remote-endpoint = <&panel_in_channel0>;
>>>              };
>>>          };
>>>      };
>>>      panel {
>>>          ...
>>>          ...
>>>          /*
>>>           * panel node can describe its input ports, if both the DSIs output
>>>           * ports are connected to the same device (i.e, the same DSI panel),
>>>           * we can assume that the DSIs need to operate in dual DSI mode
>>>           */
>>>          ports {
>>>              ...
>>>              port@0 {
>>>                  panel_in_channel0: endpoint {
>>>                      remote-endpoint = <&mipi_panel_out>;
>>>                  };
>>>              };
>>>              port@1 {
>>>                  panel_in_channel1: endpoint {
>>>                      remote-endpoint = <&mipi1_panel_out>;
>>>                  };
>>>
>>>              };
>>>          };
>>>      };
>>> };
>>>
>>> mipi_dsi1: mipi@...68000 {
>>>      ...
>>>      ...
>>>      ports {
>>>          mipi1_in: port {
>>>              ...
>>>              ...
>>>          };
>>>          mipi1_out: port {
>>>              mipi1_panel_out: endpoint {
>>>                  remote-endpoint = <&panel_in_channel1>;
>>>              };
>>>          };
>>>      };
>>> }
>>>
>>> But it seems we can not use of_drm_find_panel(like below)
>>>
>>> /*
>>>          port = of_graph_get_port_by_id(dev->of_node, 1);
>>>          if (port) {
>>>                  endpoint = of_get_child_by_name(port, "endpoint");
>>>                  of_node_put(port);
>>>                  if (!endpoint) {
>>>                          dev_err(dev, "no output endpoint found\n");
>>>                          return -EINVAL;
>>>                  }
>>>                  panel_node = of_graph_get_remote_port_parent(endpoint);
>>>                  of_node_put(endpoint);
>>>                  if (!panel_node) {
>>>                          dev_err(dev, "no output node found\n");
>>>                          return -EINVAL;
>>>                  }
>>>                  panel = of_drm_find_panel(panel_node);
>>>                  of_node_put(panel_node);
>>>                  if (!panel)
>>>                          return -EPROBE_DEFER;
>>>          }
>>> */
>>> to get DSI1 outputs,because of_drm_find_panel need compare
>>>
>>> if (panel->dev->of_node == np)
>>>
>>> in dsi_panel driver innolux->base.dev = &innolux->link->dev;
>>> dsi->dev
>>
>> Yes, we should only have 1 drm_panel in the global panel list.
>> Shouldn't it be possible to modify the dsi driver such that dsi1
>> doesn't care whether it has a drm_panel for it or not, if we are
>> in dual dsi mode?
>>
>> I imagine a sequence like this:
>>
>> 1. dsi0 probes, parses the of-graph, finds the panel and saves its device
>> node.
> 
> Does this mean we depend on probe order? Or would we be able to
> -EPROBE_DEFER or similar if dsi1 binds first?

I don't think the probe order should matter. However, I also don't know what
challenges it might bring up once we actually try to implement it. I can see
the the driver's of-graph parsing code getting a bit complicated. The first dsi
instance that probes/binds (say dsi1) should peek into the panels other ports
and see if it is the slave DSI instance in a dual DSI set-up. If so, it could
defer until DSI0 first probes and registers the panel.

Btw, full disclosure, I work on the drm/msm driver, and the code uses a binding
called "qcom,dual-dsi-mode" done by someone in the past, but thankfully it isn't
used in any dts file. I plan to remove these and use the bindings I've suggested
here.

Also, the bindings I've shared above are more a proof of concept, and based on
how dual DSI is implemented on the MSM chipsets. If the HW requires special
properties while operating in Dual DSI mode, then it might be okay to have
additional bindings. However, it seems strange to have a DT prop that says
"operate in dual DSI mode" if it can be inferred from the port connections.

I'll post a RFC explaining the bindings and copy all the people with kms drivers
that support DSI. Maybe we'd come up with a better consensus.

> 
>> 2. dsi1 probes, parses the of-graph, find the panel's device node
>>    - dsi1 checks if it is the same as the panel attached to dsi0.
>>    - If so, it just takes the drm_panel pointer from dsi0.
>>    - If not, it tries a of_drm_find_panel() on the panel's device node.
> 
> So, that all means we'd need a new variant of
> drm_of_find_panel_or_bridge() for "dual" drivers like this? Or else
> open-code this logic in dw-mipi-dsi.c?

Yeah. It would be nice to have these in helpers.

> 
>> A dual DSI panel driver would also be a bit different. It will be a
>> mipi_dsi_driver with the master DSI (dsi0) as the mipi_dsi_device. Using
>> the of-graph helpers, we would get the device node of dsi1 using
>> of_find_mipi_dsi_host_by_node(), and create another DSI device using
>> mipi_dsi_device_register_full(). Then, we call mipi_dsi_attach() on
>> both the dsi devices.
> 
> That seems...interesting. I guess that sounds like it could work, but
> someone would have to play with that a bit more.
> 
> I assume one wouldn't want to do all this in every dual DSI driver that
> needs this, right?

Yes. I agree.

> 
>>> struct innolux_panel {
>>>          struct drm_panel base;
>>>          struct mipi_dsi_device *link;
>>> };
>>> It means one panel can only be found in his dsi node,(like dsi0 above).
>>>
>>> I'm doubting about it, Or  may we follow tegra_dsi_ganged_probe
>>> (drivers/gpu/drm/tergra/dsi.c) method.
>>
>> This method will add a new binding similar to "nvidia,ganged-mode", which
>> is something we don't want to do.

Btw, we already have a dual DSI panel driver, which has a special phandle called
"link2":

Documentation/devicetree/bindings/display/panel/sharp,lq101r1sx01.txt

I don't think we should continue using a prop like link2, it seems like something
that should be replaced by of-graph usage.

Thanks,
Archit

> 
> It's unfortunate we have the anti-pattern already merged :(
> 
> Brian
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Powered by blists - more mailing lists