Re: [RFC v2 2/2] dt-bindings: mipi-dsi: Add dual-channel DSI related info

[Heiko Stuebner <heiko@sntech.de>]

Am Mittwoch, 6. Juni 2018, 18:07:46 CEST schrieb Archit Taneja:
> 
> On Wednesday 06 June 2018 04:16 PM, Heiko Stübner wrote:
> > Hi Archit,
> > 
> > Am Mittwoch, 6. Juni 2018, 12:21:16 CEST schrieb Archit Taneja:
> >> On Wednesday 06 June 2018 02:00 PM, Heiko Stübner wrote:
> >>> Am Mittwoch, 6. Juni 2018, 07:59:29 CEST schrieb Archit Taneja:
> >>>> On Monday 04 June 2018 05:47 PM, Heiko Stuebner wrote:
> >>>>> Am Donnerstag, 18. Januar 2018, 05:53:55 CEST schrieb Archit Taneja:
> >>>>>> Add binding info for peripherals that support dual-channel DSI. Add
> >>>>>> corresponding optional bindings for DSI host controllers that may
> >>>>>> be configured in this mode. Add an example of an I2C controlled
> >>>>>> device operating in dual-channel DSI mode.
> >>>>>>
> >>>>>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> >>>>>
> >>>>> Looks like a great solution for that problem, so
> >>>>> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> >>>>>
> >>>>> As I'm looking into that for my rk3399-scarlet device right now and
> >>>>> couldn't find this patchset in the kernel yet, is it planned to
> >>>>> merge or refresh these binding changes or were problems encountered.
> >>>>>
> >>>>> At least an Ack/Review from Rob seems to be missing.
> >>>>
> >>>> I forgot about these patches. Rob had reviewed the first one in
> >>>> the set the second one still needed an Ack. I'll post a v3
> >>>> that adds the Reviewed-bys and fixes a small typo.
> >>>
> >>> very nice ... because it looks like yesterday I managed to make the
> >>> Rockchip dsi work in dual mode following this.
> >>>
> >>> But one question came up, do you really want two input ports on the panel
> >>> side? I.e. hardware-wise, I guess the panel will have one 8-lane or so
> >>> input thatonly gets split up on the soc side onto 2 dsi controllers?
> >>
> >> I think all dual DSI panels actually have 2 DSI controllers/parsers
> >> within them, one on each port. The MIPI DSI spec doesn't support 8
> >> lanes. Also, the pixels coming out of the host are distributed among
> >> the lanes differently than what would have been the case with a
> >> 'theoretical' 8 lane receiver.
> >>
> >> Other than that, some dual DSI panels only accept DSI commands on the
> >> 'master' port, where as others expect the same command to be sent across
> >> both the ports.
> >>
> >> Therefore, I think it's better to represent dual DSI panels having 2
> >> DSI input ports.
> >>
> >> Your DT looks good to me.
> > 
> > Hmm, that doesn't match up then ;-) ... as my dt uses 2 endpoints
> > in one port for the dsi-links.
> > 
> 
> Sorry, I didn't notice you'd created two endpoints within a single port.
> 
> I don't think I'm particular about 2 ports vs 1 port with 2 endpoints.
> They both seem okay to me as long as we follow it consistently. I'm
> myself not 100% sure of how to figure where one should prefer endpoints
> over ports. Maybe someone more familiar with the of graph bindings
> could comment here.
> 
> 
> > The issue I see with using ports and not endpoints for dual-dsi links
> > is with distinguishing between input and output ports.
> > 
> > For a panel that's easy, as you every port will be an input port and if
> > you have 2, it's supposed dual-dsi. But for example I guess there might
> > exist some dual-dsi-to-something bridges, where you would end up
> > with say 3 (or even more) ports ... two dual-dsi inputs and 1 or more
> > outputs.
> 
> Okay, I get your point here. Although, even if the remote device had
> exactly 2 ports. Is it safe to assume that port #0 is an input port and
> port #1 is an output port? Is that the norm?

I don't think that anything like that is specified anywhere, so you cannot
assume anything about what a port contains.


> I've at least seen one device (toshiba,tc358767 bridge) that can 
> actually take either DPI as input or DSI based on how you configure it.
> There are 2 input ports here, port #0 for DSI and port #1 for DPI. Would
> it have made sense here to have a single port and 2 endpoints here too?

Nope, I guess having a port for DPI input, one port for DSI input makes
quite a lot of sense. And then you can have the input-specifics living in
the endpoints like dual links or so.


> > While the following argument might not be 100% valid from a dt-purity
> > standpoint implementing this might get hairy for _any_ operating system,
> > as you will need each panel/bridge to tell what the ports are used for.
> 
> Yeah.
> 
> > 
> > I.e. in my endpoint based mapping, right now I have this nice and generic
> > WIP function to parse the of_graph and get the master+slave nodes:
> > 
> > https://github.com/mmind/linux-rockchip/blob/tmp/rk3399-gru-bob-scarlet/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c#L697
> > [0]
> 
> I'd tried out something locally before posting this patch, I don't have
> the code for it, but I can describe the steps I took. This code expects
> the panel/bridge to have 2 input ports.

Which again would be very much panel-specific as you cannot assume
to much about the ports.


> 1. DSI0 host driver looks up its output port, gets the remote endpoint,
> and get this endpoint's parent (using
> of_graph_get_remote_port_parent()). Keeps the parent device node in a
> temp variable.
> 
> 2. DSI1 host driver does the same thing.
> 
> 3. DSI0 and DSI1 check whether their outputs are connected to the
> same device. If so, they're in dual DSI mode. If not, they are
> operating independently.
> 
> The positive of this approach is that we don't need to make any
> assumptions about the panel/bridge's port numbers, which is great.
> The negative is that our DSI controller instances now need to query
> each other, which can be messy, but not too hard to implement.
> 
> I think the choice finally boils down to what makes more sense w.r.t
> representing the HW correctly. We'd need Rob's comment on that.

Taking your DPI+DSI example from above actually shows quite nicely
how ports+endpoints could play together.

panel-or-bridge@0 {
	compatible = "something,something";
	reg = <0>;

	ports {
		/* DPI input */
		port@0 {
			endpoint {
				remote-endpoint = <&dpi_out>;
			};
		};

		/* DSI input */
		port@1 {
			/* from first dsi controller */
			endpoint@0 {
				remote-endpoint = <&dsi0_out>;
			};

			/* from second dsi controller */
			endpoint@1 {
				remote-endpoint = <&dsi1_out>;
			};
		};

		/* another input, or an output for a bridge */
		port@2 {
			endpoint {
				remote-endpoint = <&somewhere>;
			}
		};
	};
};

But yeah, hopefully we can entice Rob for comments :-) .

Heiko

> Thanks,
> Archit
> 
> > 
> > So I guess my proposal would be to have one port for inputs
> > and one port for outputs for dsi peripherals, with possibly
> > multiple endpoints in each.
> > 
> > 
> > Heiko
> > 
> > 
> > [0] github seems to have reliability problems, so for reference my
> > parsing function:
> > 
> > static int dw_mipi_dsi_is_dual(struct dw_mipi_dsi_rockchip *dsi,
> > 			struct device_node **master, struct device_node **slave)
> > {
> > 	struct device_node *local_ep, *remote_port, *ep;
> > 	struct device_node *ctrls[2] = { NULL, NULL };
> > 	int num = 0, ret = 0, idx;
> > 
> > 	/* get local panel endpoint of the dsi controller */
> > 	local_ep = of_graph_get_endpoint_by_regs(dsi->dev->of_node, 1, 0);
> > 	if (!local_ep) {
> > 		DRM_DEV_ERROR(dsi->dev, "couldn't find local panel endpoint\n");
> > 		return -ENXIO;
> > 	}
> > 
> > 	/* get panel port */
> > 	remote_port = of_graph_get_remote_port_parent(local_ep);
> > 	of_node_put(local_ep);
> > 	if (!remote_port) {
> > 		DRM_DEV_ERROR(dsi->dev, "couldn't find panel port\n");
> > 		return -ENXIO;
> > 	}
> > 
> > 	/* check other endpoints */
> > 	for_each_endpoint_of_node(remote_port, ep) {
> > 		struct device_node *np = of_graph_get_remote_port_parent(ep);
> > 
> > 		if (!np)
> > 			continue;
> > 
> > 		idx = of_property_read_bool(np, "clock-master");
> > 
> > 		/*
> > 		 * Either master or slave already defined, drop refcnt
> > 		 * but catch errors only after the full loop.
> > 		 */
> > 		if (ctrls[idx])
> > 			of_node_put(np);
> > 		else
> > 			ctrls[idx] = np;
> > 
> > 		num++;
> > 	}
> > 	of_node_put(remote_port);
> > 
> > 	if (num > 2) {
> > 		DRM_DEV_ERROR(dsi->dev, "too many dsi devices linked\n");
> > 		ret = -EINVAL;
> > 		goto cleanup;
> > 	}
> > 
> > 	/* nothing to do */
> > 	if (num < 1) {
> > 		ret = 0;
> > 		goto cleanup;
> > 	}
> > 
> > 	if (!ctrls[1]) {
> > 		DRM_DEV_ERROR(dsi->dev, "no master defined in dual-dsi\n");
> > 		ret = -ENODEV;
> > 		goto cleanup;
> > 	}
> > 
> > 	if (!ctrls[0]) {
> > 		DRM_DEV_ERROR(dsi->dev, "no slave defined in dual-dsi\n");
> > 		ret = -ENODEV;
> > 		goto cleanup;
> > 	}
> > 
> > 	*master = ctrls[1];
> > 	*slave = ctrls[0];
> > 
> > 	return 1;
> > 
> > cleanup:
> > 	for (idx = 0; idx < 2; idx++)
> > 		if (ctrls[idx])
> > 			of_node_put(ctrls[idx]);
> > 	return ret;
> > }
> > 
> >>> So right now I'm operating with a devicetree like
> >>>
> >>> &mipi_dsi {
> >>>
> >>>           status = "okay";
> >>>           clock-master;
> >>>           
> >>>           ports {
> >>>           
> >>>                   mipi_out: port@1 {
> >>>                   
> >>>                           reg = <1>;
> >>>                           
> >>>                           mipi_out_panel: endpoint {
> >>>                           
> >>>                                   remote-endpoint = <&mipi_in_panel>;
> >>>                           
> >>>                           };
> >>>                   
> >>>                   };
> >>>           
> >>>           };
> >>>           
> >>>           mipi_panel: panel@0 {
> >>> 		
> >>> 		  compatible = "innolux,p097pfg";
> >>> 		
> >>>                   reg = <0>;
> >>>                   backlight = <&backlight>;
> >>>                   enable-gpios = <&gpio4 25 GPIO_ACTIVE_HIGH>;
> >>>                   pinctrl-names = "default";
> >>>                   pinctrl-0 = <&display_rst_l>;
> >>>                   
> >>>                   port {
> >>>                   
> >>>                           #address-cells = <1>;
> >>>                           #size-cells = <0>;
> >>>                           
> >>>                           mipi_in_panel: endpoint@0 {
> >>>                           
> >>>                                   reg = <0>;
> >>>                                   remote-endpoint = <&mipi_out_panel>;
> >>>                           
> >>>                           };
> >>>                           
> >>>                           mipi1_in_panel: endpoint@1 {
> >>>                           
> >>>                                   reg = <1>;
> >>>                                   remote-endpoint = <&mipi1_out_panel>;
> >>>                           
> >>>                           };
> >>>                   
> >>>                   };
> >>>           
> >>>           };
> >>>
> >>> };
> >>>
> >>> &mipi_dsi1 {
> >>>
> >>>           status = "okay";
> >>>           
> >>>           ports {
> >>>           
> >>>                   mipi1_out: port@1 {
> >>>                   
> >>>                           reg = <1>;
> >>>                           
> >>>                           mipi1_out_panel: endpoint {
> >>>                           
> >>>                                   remote-endpoint = <&mipi1_in_panel>;
> >>>                           
> >>>                           };
> >>>                   
> >>>                   };
> >>>           
> >>>           };
> >>>
> >>> };
> >>>
> >>>
> >>> I guess it is a matter of preference on what reflects the hardware
> >>> best, so maybe that's Robs call?
> >>>
> >>>
> >>> Heiko
> >>>
> >>>>>> ---
> >>>>>> v2:
> >>>>>> - Specify that clock-master is a boolean property.
> >>>>>> - Drop/add unit-address and #*-cells where applicable.
> >>>>>>
> >>>>>>     .../devicetree/bindings/display/mipi-dsi-bus.txt   | 80
> >>>>>>     ++++++++++++++++++++++ 1 file changed, 80 insertions(+)
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
> >>>>>> b/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt index
> >>>>>> 94fb72cb916f..7a3abbedb3fa 100644
> >>>>>> --- a/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
> >>>>>> +++ b/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
> >>>>>>
> >>>>>> @@ -29,6 +29,13 @@ Required properties:
> >>>>>>     - #size-cells: Should be 0. There are cases where it makes sense to
> >>>>>>     use
> >>>>>>     a
> >>>>>>     
> >>>>>>       different value here. See below.
> >>>>>>
> >>>>>> +Optional properties:
> >>>>>> +- clock-master: boolean. Should be enabled if the host is being used
> >>>>>> in
> >>>>>> +  conjunction with another DSI host to drive the same peripheral.
> >>>>>> Hardware
> >>>>>> +  supporting such a configuration generally requires the data on both
> >>>>>> the busses +  to be driven by the same clock. Only the DSI host
> >>>>>> instance
> >>>>>> controlling this +  clock should contain this property.
> >>>>>> +
> >>>>>>
> >>>>>>     DSI peripheral
> >>>>>>     ==============
> >>>>>>
> >>>>>> @@ -62,6 +69,16 @@ primary control bus, but are also connected to a DSI
> >>>>>> bus (mostly for the data>>
> >>>>>>
> >>>>>>     path). Connections between such peripherals and a DSI host can be
> >>>>>>     represented using the graph bindings [1], [2].
> >>>>>>
> >>>>>> +Peripherals that support dual channel DSI
> >>>>>> +-----------------------------------------
> >>>>>> +
> >>>>>> +Peripherals with higher bandwidth requirements can be connected to 2
> >>>>>> DSI
> >>>>>> +busses. Each DSI bus/channel drives some portion of the pixel data
> >>>>>> (generally +left/right half of each line of the display, or even/odd
> >>>>>> lines of the display). +The graph bindings should be used to represent
> >>>>>> the multiple DSI busses that are +connected to this peripheral. Each
> >>>>>> DSI
> >>>>>> host's output endpoint can be linked to +an input endpoint of the DSI
> >>>>>> peripheral.
> >>>>>> +
> >>>>>>
> >>>>>>     [1] Documentation/devicetree/bindings/graph.txt
> >>>>>>     [2] Documentation/devicetree/bindings/media/video-interfaces.txt
> >>>>>>
> >>>>>> @@ -71,6 +88,8 @@ Examples
> >>>>>>
> >>>>>>       with different virtual channel configurations.
> >>>>>>     
> >>>>>>     - (4) is an example of a peripheral on a I2C control bus connected
> >>>>>>     with
> >>>>>>     to
> >>>>>>     
> >>>>>>       a DSI host using of-graph bindings.
> >>>>>>
> >>>>>> +- (5) is an example of 2 DSI hosts driving a dual-channel DSI
> >>>>>> peripheral,
> >>>>>> +  which uses I2C as its primary control bus.
> >>>>>>
> >>>>>>     1)
> >>>>>>     
> >>>>>>     	dsi-host {
> >>>>>>
> >>>>>> @@ -153,3 +172,64 @@ Examples
> >>>>>>
> >>>>>>     			};
> >>>>>>     		
> >>>>>>     		};
> >>>>>>     	
> >>>>>>     	};
> >>>>>>
> >>>>>> +
> >>>>>> +5)
> >>>>>> +	i2c-host {
> >>>>>> +		dsi-bridge@35 {
> >>>>>> +			compatible = "...";
> >>>>>> +			reg = <0x35>;
> >>>>>> +
> >>>>>> +			ports {
> >>>>>> +				#address-cells = <1>;
> >>>>>> +				#size-cells = <0>;
> >>>>>> +
> >>>>>> +				port@0 {
> >>>>>> +					reg = <0>;
> >>>>>> +					dsi0_in: endpoint {
> >>>>>> +						remote-endpoint = <&dsi0_out>;
> >>>>>> +					};
> >>>>>> +				};
> >>>>>> +
> >>>>>> +				port@1 {
> >>>>>> +					reg = <1>;
> >>>>>> +					dsi1_in: endpoint {
> >>>>>> +						remote-endpoint = <&dsi1_out>;
> >>>>>> +					};
> >>>>>> +				};
> >>>>>> +			};
> >>>>>> +		};
> >>>>>> +	};
> >>>>>> +
> >>>>>> +	dsi0-host {
> >>>>>> +		...
> >>>>>> +
> >>>>>> +		/*
> >>>>>> +		 * this DSI instance drives the clock for both the host
> >>>>>> +		 * controllers
> >>>>>> +		 */
> >>>>>> +		clock-master;
> >>>>>> +
> >>>>>> +		ports {
> >>>>>> +			...
> >>>>>> +
> >>>>>> +			port {
> >>>>>> +				dsi0_out: endpoint {
> >>>>>> +					remote-endpoint = <&dsi0_in>;
> >>>>>> +				};
> >>>>>> +			};
> >>>>>> +		};
> >>>>>> +	};
> >>>>>> +
> >>>>>> +	dsi1-host {
> >>>>>> +		...
> >>>>>> +
> >>>>>> +		ports {
> >>>>>> +			...
> >>>>>> +
> >>>>>> +			port {
> >>>>>> +				dsi1_out: endpoint {
> >>>>>> +					remote-endpoint = <&dsi1_in>;
> >>>>>> +				};
> >>>>>> +			};
> >>>>>> +		};
> >>>>>> +	};
> >>>>>
> >>>>> --
> >>>>> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm"
> >>>>> in
> >>>>> the body of a message to majordomo@vger.kernel.org
> >>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > 
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> 




_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel