Re: [PATCH v3 5/6] dt-bindings: add the rockchip, dual-channel for dw-mipi-dsi
From: Archit Taneja
Date: Tue Dec 05 2017 - 00:16:56 EST
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@ff960000 {
ÂÂÂ ...
ÂÂÂ ...
ÂÂÂ 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@ff968000 {
ÂÂÂ ...
ÂÂÂ ...
ÂÂÂ 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