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@ff96 { ... ... 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 = <_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 = <_panel_out>; }; }; port@1 { panel_in_channel1: endpoint { remote-endpoint = <_panel_out>; }; }; }; }; }; mipi_dsi1: mipi@ff968000 { ... ... ports { mipi1_in: port { ... ... }; mipi1_out: port { mipi1_panel_out: endpoint { remote-endpoint = <_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 = >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
Re: [PATCH v3 5/6] dt-bindings: add the rockchip, dual-channel for dw-mipi-dsi
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@ff96 { > > ... > > ... > > 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 = <_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 = <_panel_out>; > > }; > > }; > > port@1 { > > panel_in_channel1: endpoint { > > remote-endpoint = <_panel_out>; > > }; > > > > }; > > }; > > }; > >}; > > > >mipi_dsi1: mipi@ff968000 { > > ... > > ... > > ports { > > mipi1_in: port { > > ... > > ... > > }; > > mipi1_out: port { > > mipi1_panel_out: endpoint { > > remote-endpoint = <_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 = >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? > 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? > 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? > >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. It's unfortunate we have the anti-pattern already merged :( Brian ___ dri-devel mailing list
Re: [PATCH v3 5/6] dt-bindings: add the rockchip, dual-channel for dw-mipi-dsi
On 11/30/2017 11:02 PM, Nickey Yang wrote: Hi Archit, On 2017年10月26日 12:53, Archit Taneja wrote: On 10/25/2017 09:21 AM, Nickey Yang wrote: Configure dsi slave channel when driving a panel which needs 2 DSI links. Signed-off-by: Nickey Yang--- .../devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt index 6bb59ab..a2bea22 100644 --- a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt +++ b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt @@ -19,6 +19,8 @@ Optional properties: - power-domains: a phandle to mipi dsi power domain node. - resets: list of phandle + reset specifier pairs, as described in [3]. - reset-names: string reset name, must be "apb". +- rockchip,dual-channel: phandle to a 2nd DSI channel, useful as a slave +channel when driving a panel which needs 2 DSI links. The example below is how dual DSI bindings could look like. Let me know what you think of it. If both DSI outputs drive the same device (i.e, point to the same panel DT node), then I think it's reasonable enough to assume that the DSIs are operating in a 'dual-channel' mode. That being said, we still need DT to describe which of the DSIs generates the clock for both the channels. This is done with the 'clock-master' DT binding. Thanks, Archit mipi_dsi: mipi@ff96 { ... ... 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 = <_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 = <_panel_out>; }; }; port@1 { panel_in_channel1: endpoint { remote-endpoint = <_panel_out>; }; }; }; }; }; mipi_dsi1: mipi@ff968000 { ... ... ports { mipi1_in: port { ... ... }; mipi1_out: port { mipi1_panel_out: endpoint { remote-endpoint = <_in_channel1>; }; }; }; }; I try to follow as you suggested,use mipi_dsi: mipi@ff96 { ... ... 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 = <_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 = <_panel_out>; }; }; port@1 { panel_in_channel1: endpoint { remote-endpoint = <_panel_out>; }; }; }; }; }; mipi_dsi1: mipi@ff968000 { ... ... ports { mipi1_in: port { ... ... }; mipi1_out: port { mipi1_panel_out: endpoint { remote-endpoint = <_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
Re: [PATCH v3 5/6] dt-bindings: add the rockchip, dual-channel for dw-mipi-dsi
Hi Archit, On 2017年10月26日 12:53, Archit Taneja wrote: On 10/25/2017 09:21 AM, Nickey Yang wrote: Configure dsi slave channel when driving a panel which needs 2 DSI links. Signed-off-by: Nickey Yang--- .../devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt index 6bb59ab..a2bea22 100644 --- a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt +++ b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt @@ -19,6 +19,8 @@ Optional properties: - power-domains: a phandle to mipi dsi power domain node. - resets: list of phandle + reset specifier pairs, as described in [3]. - reset-names: string reset name, must be "apb". +- rockchip,dual-channel: phandle to a 2nd DSI channel, useful as a slave +channel when driving a panel which needs 2 DSI links. The example below is how dual DSI bindings could look like. Let me know what you think of it. If both DSI outputs drive the same device (i.e, point to the same panel DT node), then I think it's reasonable enough to assume that the DSIs are operating in a 'dual-channel' mode. That being said, we still need DT to describe which of the DSIs generates the clock for both the channels. This is done with the 'clock-master' DT binding. Thanks, Archit mipi_dsi: mipi@ff96 { ... ... 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 = <_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 = <_panel_out>; }; }; port@1 { panel_in_channel1: endpoint { remote-endpoint = <_panel_out>; }; }; }; }; }; mipi_dsi1: mipi@ff968000 { ... ... ports { mipi1_in: port { ... ... }; mipi1_out: port { mipi1_panel_out: endpoint { remote-endpoint = <_in_channel1>; }; }; }; }; I try to follow as you suggested,use mipi_dsi: mipi@ff96 { ... ... 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 = <_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 = <_panel_out>; }; }; port@1 { panel_in_channel1: endpoint { remote-endpoint = <_panel_out>; }; }; }; }; }; mipi_dsi1: mipi@ff968000 { ... ... ports { mipi1_in: port { ... ... }; mipi1_out: port { mipi1_panel_out: endpoint { remote-endpoint = <_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 =
Re: [PATCH v3 5/6] dt-bindings: add the rockchip, dual-channel for dw-mipi-dsi
On 10/25/2017 09:21 AM, Nickey Yang wrote: Configure dsi slave channel when driving a panel which needs 2 DSI links. Signed-off-by: Nickey Yang--- .../devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt index 6bb59ab..a2bea22 100644 --- a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt +++ b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt @@ -19,6 +19,8 @@ Optional properties: - power-domains: a phandle to mipi dsi power domain node. - resets: list of phandle + reset specifier pairs, as described in [3]. - reset-names: string reset name, must be "apb". +- rockchip,dual-channel: phandle to a 2nd DSI channel, useful as a slave +channel when driving a panel which needs 2 DSI links. The example below is how dual DSI bindings could look like. Let me know what you think of it. If both DSI outputs drive the same device (i.e, point to the same panel DT node), then I think it's reasonable enough to assume that the DSIs are operating in a 'dual-channel' mode. That being said, we still need DT to describe which of the DSIs generates the clock for both the channels. This is done with the 'clock-master' DT binding. Thanks, Archit mipi_dsi: mipi@ff96 { ... ... 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 = <_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 = <_panel_out>; }; }; port@1 { panel_in_channel1: endpoint { remote-endpoint = <_panel_out>; }; }; }; }; }; mipi_dsi1: mipi@ff968000 { ... ... ports { mipi1_in: port { ... ... }; mipi1_out: port { mipi1_panel_out: endpoint { remote-endpoint = <_in_channel1>; }; }; }; }; [1] Documentation/devicetree/bindings/clock/clock-bindings.txt [2] Documentation/devicetree/bindings/media/video-interfaces.txt -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 5/6] dt-bindings: add the rockchip, dual-channel for dw-mipi-dsi
Configure dsi slave channel when driving a panel which needs 2 DSI links. Signed-off-by: Nickey Yang--- .../devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt index 6bb59ab..a2bea22 100644 --- a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt +++ b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt @@ -19,6 +19,8 @@ Optional properties: - power-domains: a phandle to mipi dsi power domain node. - resets: list of phandle + reset specifier pairs, as described in [3]. - reset-names: string reset name, must be "apb". +- rockchip,dual-channel: phandle to a 2nd DSI channel, useful as a slave +channel when driving a panel which needs 2 DSI links. [1] Documentation/devicetree/bindings/clock/clock-bindings.txt [2] Documentation/devicetree/bindings/media/video-interfaces.txt -- 1.9.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel