[PATCH] ARM: dts: imx6q-utilite-pro: enable 2nd display pipeline

[PATCH] ARM: dts: imx6q-utilite-pro: enable 2nd display pipeline

[christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg]

From: Christopher Spinrath <christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org>

Apart from the already enabled Designware HDMI port, the Utilite Pro
has a second display pipeline which has the following shape:

  IPU1 DI0 --> Parallel display --> tfp410 rgb24 to DVI encoder
                                --> HDMI connector.
Enable support for it.

In addition, since this pipeline is hardwired to IPU1, sever the link
between IPU1 and the SoC-internal Designware HDMI encoder forcing the
latter to be connected to IPU2 instead of IPU1. Otherwise, it is not
possible to drive both displays at high resolution due to the bandwidth
limitations of a single IPU.

Signed-off-by: Christopher Spinrath <christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org>
---

Hi all,

the removal of the link between IPU1 and the Designware HDMI encoder is the
result of a discussion I had with Philipp Zabel:

  https://lists.freedesktop.org/archives/dri-devel/2016-November/125399.html .

Altough it is not possible to connect anything else to IPU1 on the Utilite, this
approach has at least one disadvantage: if the resolution is low enough such 
that a single IPU can handle both displays then muxing both displays to IPU1
would reduce the power consumption.

However, IMHO omitting the link IPU1 <--> DW HDMI is still the preferrable
solution since I'm not aware of any OS/driver that is capable of switching IPUs
or can handle the bandwidth limitation in a sane way. In particular, Linux is
unusable when both displays are supposed to be driven at high resolution and
both muxing options for the DW HDMI are available (this is not a userspace
issue; the system becomes almost unresponsive as soon as the kernel sets the
initial resolution).

Cheers,
Christopher

P.S.: this patch depends on the tfp410 bridge driver which has recently been
merged into drm-next.

 arch/arm/boot/dts/imx6q-utilite-pro.dts | 115 ++++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

diff --git a/arch/arm/boot/dts/imx6q-utilite-pro.dts b/arch/arm/boot/dts/imx6q-utilite-pro.dts
index 2200994..69bdd82 100644
--- a/arch/arm/boot/dts/imx6q-utilite-pro.dts
+++ b/arch/arm/boot/dts/imx6q-utilite-pro.dts
@@ -59,6 +59,33 @@
 		rtc1 = &snvs_rtc;
 	};
 
+	encoder {
+		compatible = "ti,tfp410";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+
+				tfp410_in: endpoint {
+					remote-endpoint = <&parallel_display_out>;
+				};
+			};
+
+			port@1 {
+				reg = <1>;
+
+				tfp410_out: endpoint {
+					remote-endpoint = <&hdmi_connector_in>;
+				};
+			};
+		};
+	};
+
 	gpio-keys {
 		compatible = "gpio-keys";
 		pinctrl-names = "default";
@@ -72,6 +99,19 @@
 		};
 	};
 
+	hdmi-connector {
+		compatible = "hdmi-connector";
+
+		type = "a";
+		ddc-i2c-bus = <&i2c_dvi_ddc>;
+
+		port {
+			hdmi_connector_in: endpoint {
+				remote-endpoint = <&tfp410_out>;
+			};
+		};
+	};
+
 	i2cmux {
 		compatible = "i2c-mux-gpio";
 		pinctrl-names = "default";
@@ -105,8 +145,46 @@
 			#size-cells = <0>;
 		};
 	};
+
+	parallel-display {
+		compatible = "fsl,imx-parallel-display";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_ipu1>;
+
+		interface-pix-fmt = "rgb24";
+
+		port@0 {
+			reg = <0>;
+
+			parallel_display_in: endpoint {
+				remote-endpoint = <&ipu1_di0_disp0>;
+			};
+		};
+
+		port@1 {
+			reg = <1>;
+
+			parallel_display_out: endpoint {
+				remote-endpoint = <&tfp410_in>;
+			};
+		};
+	};
 };
 
+/*
+ * A single IPU is not able to drive both display interfaces available on the
+ * Utilite Pro at high resolution due to its bandwidth limitation. Since the
+ * tfp410 encoder is wired up to IPU1, sever the link between IPU1 and the
+ * SoC-internal Designware HDMI encoder forcing the latter to be connected to
+ * IPU2 instead of IPU1.
+ */
+/delete-node/&ipu1_di0_hdmi;
+/delete-node/&hdmi_mux_0;
+/delete-node/&ipu1_di1_hdmi;
+/delete-node/&hdmi_mux_1;
+
 &hdmi {
 	ddc-i2c-bus = <&i2c2>;
 	status = "okay";
@@ -151,6 +229,39 @@
 		>;
 	};
 
+	pinctrl_ipu1: ipu1grp {
+		fsl,pins = <
+			MX6QDL_PAD_DI0_DISP_CLK__IPU1_DI0_DISP_CLK 0x38
+			MX6QDL_PAD_DI0_PIN15__IPU1_DI0_PIN15       0x38
+			MX6QDL_PAD_DI0_PIN2__IPU1_DI0_PIN02        0x38
+			MX6QDL_PAD_DI0_PIN3__IPU1_DI0_PIN03        0x38
+			MX6QDL_PAD_DISP0_DAT0__IPU1_DISP0_DATA00   0x38
+			MX6QDL_PAD_DISP0_DAT1__IPU1_DISP0_DATA01   0x38
+			MX6QDL_PAD_DISP0_DAT2__IPU1_DISP0_DATA02   0x38
+			MX6QDL_PAD_DISP0_DAT3__IPU1_DISP0_DATA03   0x38
+			MX6QDL_PAD_DISP0_DAT4__IPU1_DISP0_DATA04   0x38
+			MX6QDL_PAD_DISP0_DAT5__IPU1_DISP0_DATA05   0x38
+			MX6QDL_PAD_DISP0_DAT6__IPU1_DISP0_DATA06   0x38
+			MX6QDL_PAD_DISP0_DAT7__IPU1_DISP0_DATA07   0x38
+			MX6QDL_PAD_DISP0_DAT8__IPU1_DISP0_DATA08   0x38
+			MX6QDL_PAD_DISP0_DAT9__IPU1_DISP0_DATA09   0x38
+			MX6QDL_PAD_DISP0_DAT10__IPU1_DISP0_DATA10  0x38
+			MX6QDL_PAD_DISP0_DAT11__IPU1_DISP0_DATA11  0x38
+			MX6QDL_PAD_DISP0_DAT12__IPU1_DISP0_DATA12  0x38
+			MX6QDL_PAD_DISP0_DAT13__IPU1_DISP0_DATA13  0x38
+			MX6QDL_PAD_DISP0_DAT14__IPU1_DISP0_DATA14  0x38
+			MX6QDL_PAD_DISP0_DAT15__IPU1_DISP0_DATA15  0x38
+			MX6QDL_PAD_DISP0_DAT16__IPU1_DISP0_DATA16  0x38
+			MX6QDL_PAD_DISP0_DAT17__IPU1_DISP0_DATA17  0x38
+			MX6QDL_PAD_DISP0_DAT18__IPU1_DISP0_DATA18  0x38
+			MX6QDL_PAD_DISP0_DAT19__IPU1_DISP0_DATA19  0x38
+			MX6QDL_PAD_DISP0_DAT20__IPU1_DISP0_DATA20  0x38
+			MX6QDL_PAD_DISP0_DAT21__IPU1_DISP0_DATA21  0x38
+			MX6QDL_PAD_DISP0_DAT22__IPU1_DISP0_DATA22  0x38
+			MX6QDL_PAD_DISP0_DAT23__IPU1_DISP0_DATA23  0x38
+		>;
+	};
+
 	pinctrl_uart2: uart2grp {
 		fsl,pins = <
 			MX6QDL_PAD_GPIO_7__UART2_TX_DATA 0x1b0b1
@@ -194,6 +305,10 @@
 	};
 };
 
+&ipu1_di0_disp0 {
+	remote-endpoint = <&parallel_display_in>;
+};
+
 &pcie {
 	pcie@0,0 {
 		reg = <0x000000 0 0 0 0>;
-- 
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: dts: imx6q-utilite-pro: enable 2nd display pipeline

[Shawn Guo]

On Fri, Dec 02, 2016 at 03:37:22PM +0100, christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org wrote:
> From: Christopher Spinrath <christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org>
> 
> Apart from the already enabled Designware HDMI port, the Utilite Pro
> has a second display pipeline which has the following shape:
> 
>   IPU1 DI0 --> Parallel display --> tfp410 rgb24 to DVI encoder
>                                 --> HDMI connector.
> Enable support for it.
> 
> In addition, since this pipeline is hardwired to IPU1, sever the link
> between IPU1 and the SoC-internal Designware HDMI encoder forcing the
> latter to be connected to IPU2 instead of IPU1. Otherwise, it is not
> possible to drive both displays at high resolution due to the bandwidth
> limitations of a single IPU.
> 
> Signed-off-by: Christopher Spinrath <christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org>

@Philipp, can you help review the changes?

> ---
> 
> Hi all,
> 
> the removal of the link between IPU1 and the Designware HDMI encoder is the
> result of a discussion I had with Philipp Zabel:
> 
>   https://lists.freedesktop.org/archives/dri-devel/2016-November/125399.html .
> 
> Altough it is not possible to connect anything else to IPU1 on the Utilite, this
> approach has at least one disadvantage: if the resolution is low enough such 
> that a single IPU can handle both displays then muxing both displays to IPU1
> would reduce the power consumption.
> 
> However, IMHO omitting the link IPU1 <--> DW HDMI is still the preferrable
> solution since I'm not aware of any OS/driver that is capable of switching IPUs
> or can handle the bandwidth limitation in a sane way. In particular, Linux is
> unusable when both displays are supposed to be driven at high resolution and
> both muxing options for the DW HDMI are available (this is not a userspace
> issue; the system becomes almost unresponsive as soon as the kernel sets the
> initial resolution).
> 
> Cheers,
> Christopher
> 
> P.S.: this patch depends on the tfp410 bridge driver which has recently been
> merged into drm-next.

v4.10-rc1 has the driver, so the dependency is gone now, I guess.

> 
>  arch/arm/boot/dts/imx6q-utilite-pro.dts | 115 ++++++++++++++++++++++++++++++++
>  1 file changed, 115 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx6q-utilite-pro.dts b/arch/arm/boot/dts/imx6q-utilite-pro.dts
> index 2200994..69bdd82 100644
> --- a/arch/arm/boot/dts/imx6q-utilite-pro.dts
> +++ b/arch/arm/boot/dts/imx6q-utilite-pro.dts
> @@ -59,6 +59,33 @@
>  		rtc1 = &snvs_rtc;
>  	};
>  
> +	encoder {
> +		compatible = "ti,tfp410";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
> +				reg = <0>;
> +
> +				tfp410_in: endpoint {
> +					remote-endpoint = <&parallel_display_out>;
> +				};
> +			};
> +
> +			port@1 {
> +				reg = <1>;
> +
> +				tfp410_out: endpoint {
> +					remote-endpoint = <&hdmi_connector_in>;
> +				};
> +			};
> +		};
> +	};
> +
>  	gpio-keys {
>  		compatible = "gpio-keys";
>  		pinctrl-names = "default";
> @@ -72,6 +99,19 @@
>  		};
>  	};
>  
> +	hdmi-connector {
> +		compatible = "hdmi-connector";
> +

The newline is unnecessary.

> +		type = "a";
> +		ddc-i2c-bus = <&i2c_dvi_ddc>;
> +
> +		port {
> +			hdmi_connector_in: endpoint {
> +				remote-endpoint = <&tfp410_out>;
> +			};
> +		};
> +	};
> +
>  	i2cmux {
>  		compatible = "i2c-mux-gpio";
>  		pinctrl-names = "default";
> @@ -105,8 +145,46 @@
>  			#size-cells = <0>;
>  		};
>  	};
> +
> +	parallel-display {
> +		compatible = "fsl,imx-parallel-display";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_ipu1>;
> +

Ditto

I can fix them up if I get a Reviewed-by tag from Philipp on this
version.

Shawn

> +		interface-pix-fmt = "rgb24";
> +
> +		port@0 {
> +			reg = <0>;
> +
> +			parallel_display_in: endpoint {
> +				remote-endpoint = <&ipu1_di0_disp0>;
> +			};
> +		};
> +
> +		port@1 {
> +			reg = <1>;
> +
> +			parallel_display_out: endpoint {
> +				remote-endpoint = <&tfp410_in>;
> +			};
> +		};
> +	};
>  };
>  
> +/*
> + * A single IPU is not able to drive both display interfaces available on the
> + * Utilite Pro at high resolution due to its bandwidth limitation. Since the
> + * tfp410 encoder is wired up to IPU1, sever the link between IPU1 and the
> + * SoC-internal Designware HDMI encoder forcing the latter to be connected to
> + * IPU2 instead of IPU1.
> + */
> +/delete-node/&ipu1_di0_hdmi;
> +/delete-node/&hdmi_mux_0;
> +/delete-node/&ipu1_di1_hdmi;
> +/delete-node/&hdmi_mux_1;
> +
>  &hdmi {
>  	ddc-i2c-bus = <&i2c2>;
>  	status = "okay";
> @@ -151,6 +229,39 @@
>  		>;
>  	};
>  
> +	pinctrl_ipu1: ipu1grp {
> +		fsl,pins = <
> +			MX6QDL_PAD_DI0_DISP_CLK__IPU1_DI0_DISP_CLK 0x38
> +			MX6QDL_PAD_DI0_PIN15__IPU1_DI0_PIN15       0x38
> +			MX6QDL_PAD_DI0_PIN2__IPU1_DI0_PIN02        0x38
> +			MX6QDL_PAD_DI0_PIN3__IPU1_DI0_PIN03        0x38
> +			MX6QDL_PAD_DISP0_DAT0__IPU1_DISP0_DATA00   0x38
> +			MX6QDL_PAD_DISP0_DAT1__IPU1_DISP0_DATA01   0x38
> +			MX6QDL_PAD_DISP0_DAT2__IPU1_DISP0_DATA02   0x38
> +			MX6QDL_PAD_DISP0_DAT3__IPU1_DISP0_DATA03   0x38
> +			MX6QDL_PAD_DISP0_DAT4__IPU1_DISP0_DATA04   0x38
> +			MX6QDL_PAD_DISP0_DAT5__IPU1_DISP0_DATA05   0x38
> +			MX6QDL_PAD_DISP0_DAT6__IPU1_DISP0_DATA06   0x38
> +			MX6QDL_PAD_DISP0_DAT7__IPU1_DISP0_DATA07   0x38
> +			MX6QDL_PAD_DISP0_DAT8__IPU1_DISP0_DATA08   0x38
> +			MX6QDL_PAD_DISP0_DAT9__IPU1_DISP0_DATA09   0x38
> +			MX6QDL_PAD_DISP0_DAT10__IPU1_DISP0_DATA10  0x38
> +			MX6QDL_PAD_DISP0_DAT11__IPU1_DISP0_DATA11  0x38
> +			MX6QDL_PAD_DISP0_DAT12__IPU1_DISP0_DATA12  0x38
> +			MX6QDL_PAD_DISP0_DAT13__IPU1_DISP0_DATA13  0x38
> +			MX6QDL_PAD_DISP0_DAT14__IPU1_DISP0_DATA14  0x38
> +			MX6QDL_PAD_DISP0_DAT15__IPU1_DISP0_DATA15  0x38
> +			MX6QDL_PAD_DISP0_DAT16__IPU1_DISP0_DATA16  0x38
> +			MX6QDL_PAD_DISP0_DAT17__IPU1_DISP0_DATA17  0x38
> +			MX6QDL_PAD_DISP0_DAT18__IPU1_DISP0_DATA18  0x38
> +			MX6QDL_PAD_DISP0_DAT19__IPU1_DISP0_DATA19  0x38
> +			MX6QDL_PAD_DISP0_DAT20__IPU1_DISP0_DATA20  0x38
> +			MX6QDL_PAD_DISP0_DAT21__IPU1_DISP0_DATA21  0x38
> +			MX6QDL_PAD_DISP0_DAT22__IPU1_DISP0_DATA22  0x38
> +			MX6QDL_PAD_DISP0_DAT23__IPU1_DISP0_DATA23  0x38
> +		>;
> +	};
> +
>  	pinctrl_uart2: uart2grp {
>  		fsl,pins = <
>  			MX6QDL_PAD_GPIO_7__UART2_TX_DATA 0x1b0b1
> @@ -194,6 +305,10 @@
>  	};
>  };
>  
> +&ipu1_di0_disp0 {
> +	remote-endpoint = <&parallel_display_in>;
> +};
> +
>  &pcie {
>  	pcie@0,0 {
>  		reg = <0x000000 0 0 0 0>;
> -- 
> 2.10.2
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: dts: imx6q-utilite-pro: enable 2nd display pipeline

[Shawn Guo]

On Fri, Dec 02, 2016 at 03:37:22PM +0100, christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org wrote:
> From: Christopher Spinrath <christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org>
> 
> Apart from the already enabled Designware HDMI port, the Utilite Pro
> has a second display pipeline which has the following shape:
> 
>   IPU1 DI0 --> Parallel display --> tfp410 rgb24 to DVI encoder
>                                 --> HDMI connector.
> Enable support for it.
> 
> In addition, since this pipeline is hardwired to IPU1, sever the link
> between IPU1 and the SoC-internal Designware HDMI encoder forcing the
> latter to be connected to IPU2 instead of IPU1. Otherwise, it is not
> possible to drive both displays at high resolution due to the bandwidth
> limitations of a single IPU.
> 
> Signed-off-by: Christopher Spinrath <christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org>

Applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: dts: imx6q-utilite-pro: enable 2nd display pipeline

[Christopher Spinrath]

Hi Shawn,

thanks for the review!

On 12/30/2016 03:27 PM, Shawn Guo wrote:
> On Fri, Dec 02, 2016 at 03:37:22PM +0100, christopher.spinrath@rwth-aachen.de wrote:
>> From: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
>>
>> Apart from the already enabled Designware HDMI port, the Utilite Pro
>> has a second display pipeline which has the following shape:
>>
>>   IPU1 DI0 --> Parallel display --> tfp410 rgb24 to DVI encoder
>>                                 --> HDMI connector.
>> Enable support for it.
>>
>> In addition, since this pipeline is hardwired to IPU1, sever the link
>> between IPU1 and the SoC-internal Designware HDMI encoder forcing the
>> latter to be connected to IPU2 instead of IPU1. Otherwise, it is not
>> possible to drive both displays at high resolution due to the bandwidth
>> limitations of a single IPU.
>>
>> Signed-off-by: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
>
> @Philipp, can you help review the changes?
>
>> ---
>>
>> Hi all,
>>
>> the removal of the link between IPU1 and the Designware HDMI encoder is the
>> result of a discussion I had with Philipp Zabel:
>>
>>   https://lists.freedesktop.org/archives/dri-devel/2016-November/125399.html .
>>
>> Altough it is not possible to connect anything else to IPU1 on the Utilite, this
>> approach has at least one disadvantage: if the resolution is low enough such
>> that a single IPU can handle both displays then muxing both displays to IPU1
>> would reduce the power consumption.
>>
>> However, IMHO omitting the link IPU1 <--> DW HDMI is still the preferrable
>> solution since I'm not aware of any OS/driver that is capable of switching IPUs
>> or can handle the bandwidth limitation in a sane way. In particular, Linux is
>> unusable when both displays are supposed to be driven at high resolution and
>> both muxing options for the DW HDMI are available (this is not a userspace
>> issue; the system becomes almost unresponsive as soon as the kernel sets the
>> initial resolution).
>>
>> Cheers,
>> Christopher
>>
>> P.S.: this patch depends on the tfp410 bridge driver which has recently been
>> merged into drm-next.
>
> v4.10-rc1 has the driver, so the dependency is gone now, I guess.

That's correct.

>>
>>  arch/arm/boot/dts/imx6q-utilite-pro.dts | 115 ++++++++++++++++++++++++++++++++
>>  1 file changed, 115 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/imx6q-utilite-pro.dts b/arch/arm/boot/dts/imx6q-utilite-pro.dts
>> index 2200994..69bdd82 100644
>> --- a/arch/arm/boot/dts/imx6q-utilite-pro.dts
>> +++ b/arch/arm/boot/dts/imx6q-utilite-pro.dts
>> @@ -59,6 +59,33 @@
>>  		rtc1 = &snvs_rtc;
>>  	};
>>
>> +	encoder {
>> +		compatible = "ti,tfp410";
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		ports {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +
>> +			port@0 {
>> +				reg = <0>;
>> +
>> +				tfp410_in: endpoint {
>> +					remote-endpoint = <&parallel_display_out>;
>> +				};
>> +			};
>> +
>> +			port@1 {
>> +				reg = <1>;
>> +
>> +				tfp410_out: endpoint {
>> +					remote-endpoint = <&hdmi_connector_in>;
>> +				};
>> +			};
>> +		};
>> +	};
>> +
>>  	gpio-keys {
>>  		compatible = "gpio-keys";
>>  		pinctrl-names = "default";
>> @@ -72,6 +99,19 @@
>>  		};
>>  	};
>>
>> +	hdmi-connector {
>> +		compatible = "hdmi-connector";
>> +
>
> The newline is unnecessary.
>
>> +		type = "a";
>> +		ddc-i2c-bus = <&i2c_dvi_ddc>;
>> +
>> +		port {
>> +			hdmi_connector_in: endpoint {
>> +				remote-endpoint = <&tfp410_out>;
>> +			};
>> +		};
>> +	};
>> +
>>  	i2cmux {
>>  		compatible = "i2c-mux-gpio";
>>  		pinctrl-names = "default";
>> @@ -105,8 +145,46 @@
>>  			#size-cells = <0>;
>>  		};
>>  	};
>> +
>> +	parallel-display {
>> +		compatible = "fsl,imx-parallel-display";
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_ipu1>;
>> +
>
> Ditto
>
> I can fix them up if I get a Reviewed-by tag from Philipp on this
> version.

Thanks. I'll bear that in mind for future patches (and, of course, I 
will fix them up myself if I have to send a v2).

Cheers,
Christopher

> Shawn
>
>> +		interface-pix-fmt = "rgb24";
>> +
>> +		port@0 {
>> +			reg = <0>;
>> +
>> +			parallel_display_in: endpoint {
>> +				remote-endpoint = <&ipu1_di0_disp0>;
>> +			};
>> +		};
>> +
>> +		port@1 {
>> +			reg = <1>;
>> +
>> +			parallel_display_out: endpoint {
>> +				remote-endpoint = <&tfp410_in>;
>> +			};
>> +		};
>> +	};
>>  };
>>
>> +/*
>> + * A single IPU is not able to drive both display interfaces available on the
>> + * Utilite Pro at high resolution due to its bandwidth limitation. Since the
>> + * tfp410 encoder is wired up to IPU1, sever the link between IPU1 and the
>> + * SoC-internal Designware HDMI encoder forcing the latter to be connected to
>> + * IPU2 instead of IPU1.
>> + */
>> +/delete-node/&ipu1_di0_hdmi;
>> +/delete-node/&hdmi_mux_0;
>> +/delete-node/&ipu1_di1_hdmi;
>> +/delete-node/&hdmi_mux_1;
>> +
>>  &hdmi {
>>  	ddc-i2c-bus = <&i2c2>;
>>  	status = "okay";
>> @@ -151,6 +229,39 @@
>>  		>;
>>  	};
>>
>> +	pinctrl_ipu1: ipu1grp {
>> +		fsl,pins = <
>> +			MX6QDL_PAD_DI0_DISP_CLK__IPU1_DI0_DISP_CLK 0x38
>> +			MX6QDL_PAD_DI0_PIN15__IPU1_DI0_PIN15       0x38
>> +			MX6QDL_PAD_DI0_PIN2__IPU1_DI0_PIN02        0x38
>> +			MX6QDL_PAD_DI0_PIN3__IPU1_DI0_PIN03        0x38
>> +			MX6QDL_PAD_DISP0_DAT0__IPU1_DISP0_DATA00   0x38
>> +			MX6QDL_PAD_DISP0_DAT1__IPU1_DISP0_DATA01   0x38
>> +			MX6QDL_PAD_DISP0_DAT2__IPU1_DISP0_DATA02   0x38
>> +			MX6QDL_PAD_DISP0_DAT3__IPU1_DISP0_DATA03   0x38
>> +			MX6QDL_PAD_DISP0_DAT4__IPU1_DISP0_DATA04   0x38
>> +			MX6QDL_PAD_DISP0_DAT5__IPU1_DISP0_DATA05   0x38
>> +			MX6QDL_PAD_DISP0_DAT6__IPU1_DISP0_DATA06   0x38
>> +			MX6QDL_PAD_DISP0_DAT7__IPU1_DISP0_DATA07   0x38
>> +			MX6QDL_PAD_DISP0_DAT8__IPU1_DISP0_DATA08   0x38
>> +			MX6QDL_PAD_DISP0_DAT9__IPU1_DISP0_DATA09   0x38
>> +			MX6QDL_PAD_DISP0_DAT10__IPU1_DISP0_DATA10  0x38
>> +			MX6QDL_PAD_DISP0_DAT11__IPU1_DISP0_DATA11  0x38
>> +			MX6QDL_PAD_DISP0_DAT12__IPU1_DISP0_DATA12  0x38
>> +			MX6QDL_PAD_DISP0_DAT13__IPU1_DISP0_DATA13  0x38
>> +			MX6QDL_PAD_DISP0_DAT14__IPU1_DISP0_DATA14  0x38
>> +			MX6QDL_PAD_DISP0_DAT15__IPU1_DISP0_DATA15  0x38
>> +			MX6QDL_PAD_DISP0_DAT16__IPU1_DISP0_DATA16  0x38
>> +			MX6QDL_PAD_DISP0_DAT17__IPU1_DISP0_DATA17  0x38
>> +			MX6QDL_PAD_DISP0_DAT18__IPU1_DISP0_DATA18  0x38
>> +			MX6QDL_PAD_DISP0_DAT19__IPU1_DISP0_DATA19  0x38
>> +			MX6QDL_PAD_DISP0_DAT20__IPU1_DISP0_DATA20  0x38
>> +			MX6QDL_PAD_DISP0_DAT21__IPU1_DISP0_DATA21  0x38
>> +			MX6QDL_PAD_DISP0_DAT22__IPU1_DISP0_DATA22  0x38
>> +			MX6QDL_PAD_DISP0_DAT23__IPU1_DISP0_DATA23  0x38
>> +		>;
>> +	};
>> +
>>  	pinctrl_uart2: uart2grp {
>>  		fsl,pins = <
>>  			MX6QDL_PAD_GPIO_7__UART2_TX_DATA 0x1b0b1
>> @@ -194,6 +305,10 @@
>>  	};
>>  };
>>
>> +&ipu1_di0_disp0 {
>> +	remote-endpoint = <&parallel_display_in>;
>> +};
>> +
>>  &pcie {
>>  	pcie@0,0 {
>>  		reg = <0x000000 0 0 0 0>;
>> --
>> 2.10.2
>>

Re: [PATCH] ARM: dts: imx6q-utilite-pro: enable 2nd display pipeline

[Christopher Spinrath]

Hi Philipp,

ping? It would be very nice if you comment on this patch before it's too 
late for v4.11 (which is soon, I think).

Cheers,
Christopher

On 12/30/2016 03:27 PM, Shawn Guo wrote:
> On Fri, Dec 02, 2016 at 03:37:22PM +0100, christopher.spinrath@rwth-aachen.de wrote:
>> From: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
>>
>> Apart from the already enabled Designware HDMI port, the Utilite Pro
>> has a second display pipeline which has the following shape:
>>
>>   IPU1 DI0 --> Parallel display --> tfp410 rgb24 to DVI encoder
>>                                 --> HDMI connector.
>> Enable support for it.
>>
>> In addition, since this pipeline is hardwired to IPU1, sever the link
>> between IPU1 and the SoC-internal Designware HDMI encoder forcing the
>> latter to be connected to IPU2 instead of IPU1. Otherwise, it is not
>> possible to drive both displays at high resolution due to the bandwidth
>> limitations of a single IPU.
>>
>> Signed-off-by: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
>
> @Philipp, can you help review the changes?
>
>> ---
>>
>> Hi all,
>>
>> the removal of the link between IPU1 and the Designware HDMI encoder is the
>> result of a discussion I had with Philipp Zabel:
>>
>>   https://lists.freedesktop.org/archives/dri-devel/2016-November/125399.html .
>>
>> Altough it is not possible to connect anything else to IPU1 on the Utilite, this
>> approach has at least one disadvantage: if the resolution is low enough such
>> that a single IPU can handle both displays then muxing both displays to IPU1
>> would reduce the power consumption.
>>
>> However, IMHO omitting the link IPU1 <--> DW HDMI is still the preferrable
>> solution since I'm not aware of any OS/driver that is capable of switching IPUs
>> or can handle the bandwidth limitation in a sane way. In particular, Linux is
>> unusable when both displays are supposed to be driven at high resolution and
>> both muxing options for the DW HDMI are available (this is not a userspace
>> issue; the system becomes almost unresponsive as soon as the kernel sets the
>> initial resolution).
>>
>> Cheers,
>> Christopher
>>
>> P.S.: this patch depends on the tfp410 bridge driver which has recently been
>> merged into drm-next.
>
> v4.10-rc1 has the driver, so the dependency is gone now, I guess.
>
>>
>>  arch/arm/boot/dts/imx6q-utilite-pro.dts | 115 ++++++++++++++++++++++++++++++++
>>  1 file changed, 115 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/imx6q-utilite-pro.dts b/arch/arm/boot/dts/imx6q-utilite-pro.dts
>> index 2200994..69bdd82 100644
>> --- a/arch/arm/boot/dts/imx6q-utilite-pro.dts
>> +++ b/arch/arm/boot/dts/imx6q-utilite-pro.dts
>> @@ -59,6 +59,33 @@
>>  		rtc1 = &snvs_rtc;
>>  	};
>>
>> +	encoder {
>> +		compatible = "ti,tfp410";
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		ports {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +
>> +			port@0 {
>> +				reg = <0>;
>> +
>> +				tfp410_in: endpoint {
>> +					remote-endpoint = <&parallel_display_out>;
>> +				};
>> +			};
>> +
>> +			port@1 {
>> +				reg = <1>;
>> +
>> +				tfp410_out: endpoint {
>> +					remote-endpoint = <&hdmi_connector_in>;
>> +				};
>> +			};
>> +		};
>> +	};
>> +
>>  	gpio-keys {
>>  		compatible = "gpio-keys";
>>  		pinctrl-names = "default";
>> @@ -72,6 +99,19 @@
>>  		};
>>  	};
>>
>> +	hdmi-connector {
>> +		compatible = "hdmi-connector";
>> +
>
> The newline is unnecessary.
>
>> +		type = "a";
>> +		ddc-i2c-bus = <&i2c_dvi_ddc>;
>> +
>> +		port {
>> +			hdmi_connector_in: endpoint {
>> +				remote-endpoint = <&tfp410_out>;
>> +			};
>> +		};
>> +	};
>> +
>>  	i2cmux {
>>  		compatible = "i2c-mux-gpio";
>>  		pinctrl-names = "default";
>> @@ -105,8 +145,46 @@
>>  			#size-cells = <0>;
>>  		};
>>  	};
>> +
>> +	parallel-display {
>> +		compatible = "fsl,imx-parallel-display";
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_ipu1>;
>> +
>
> Ditto
>
> I can fix them up if I get a Reviewed-by tag from Philipp on this
> version.
>
> Shawn
>
>> +		interface-pix-fmt = "rgb24";
>> +
>> +		port@0 {
>> +			reg = <0>;
>> +
>> +			parallel_display_in: endpoint {
>> +				remote-endpoint = <&ipu1_di0_disp0>;
>> +			};
>> +		};
>> +
>> +		port@1 {
>> +			reg = <1>;
>> +
>> +			parallel_display_out: endpoint {
>> +				remote-endpoint = <&tfp410_in>;
>> +			};
>> +		};
>> +	};
>>  };
>>
>> +/*
>> + * A single IPU is not able to drive both display interfaces available on the
>> + * Utilite Pro at high resolution due to its bandwidth limitation. Since the
>> + * tfp410 encoder is wired up to IPU1, sever the link between IPU1 and the
>> + * SoC-internal Designware HDMI encoder forcing the latter to be connected to
>> + * IPU2 instead of IPU1.
>> + */
>> +/delete-node/&ipu1_di0_hdmi;
>> +/delete-node/&hdmi_mux_0;
>> +/delete-node/&ipu1_di1_hdmi;
>> +/delete-node/&hdmi_mux_1;
>> +
>>  &hdmi {
>>  	ddc-i2c-bus = <&i2c2>;
>>  	status = "okay";
>> @@ -151,6 +229,39 @@
>>  		>;
>>  	};
>>
>> +	pinctrl_ipu1: ipu1grp {
>> +		fsl,pins = <
>> +			MX6QDL_PAD_DI0_DISP_CLK__IPU1_DI0_DISP_CLK 0x38
>> +			MX6QDL_PAD_DI0_PIN15__IPU1_DI0_PIN15       0x38
>> +			MX6QDL_PAD_DI0_PIN2__IPU1_DI0_PIN02        0x38
>> +			MX6QDL_PAD_DI0_PIN3__IPU1_DI0_PIN03        0x38
>> +			MX6QDL_PAD_DISP0_DAT0__IPU1_DISP0_DATA00   0x38
>> +			MX6QDL_PAD_DISP0_DAT1__IPU1_DISP0_DATA01   0x38
>> +			MX6QDL_PAD_DISP0_DAT2__IPU1_DISP0_DATA02   0x38
>> +			MX6QDL_PAD_DISP0_DAT3__IPU1_DISP0_DATA03   0x38
>> +			MX6QDL_PAD_DISP0_DAT4__IPU1_DISP0_DATA04   0x38
>> +			MX6QDL_PAD_DISP0_DAT5__IPU1_DISP0_DATA05   0x38
>> +			MX6QDL_PAD_DISP0_DAT6__IPU1_DISP0_DATA06   0x38
>> +			MX6QDL_PAD_DISP0_DAT7__IPU1_DISP0_DATA07   0x38
>> +			MX6QDL_PAD_DISP0_DAT8__IPU1_DISP0_DATA08   0x38
>> +			MX6QDL_PAD_DISP0_DAT9__IPU1_DISP0_DATA09   0x38
>> +			MX6QDL_PAD_DISP0_DAT10__IPU1_DISP0_DATA10  0x38
>> +			MX6QDL_PAD_DISP0_DAT11__IPU1_DISP0_DATA11  0x38
>> +			MX6QDL_PAD_DISP0_DAT12__IPU1_DISP0_DATA12  0x38
>> +			MX6QDL_PAD_DISP0_DAT13__IPU1_DISP0_DATA13  0x38
>> +			MX6QDL_PAD_DISP0_DAT14__IPU1_DISP0_DATA14  0x38
>> +			MX6QDL_PAD_DISP0_DAT15__IPU1_DISP0_DATA15  0x38
>> +			MX6QDL_PAD_DISP0_DAT16__IPU1_DISP0_DATA16  0x38
>> +			MX6QDL_PAD_DISP0_DAT17__IPU1_DISP0_DATA17  0x38
>> +			MX6QDL_PAD_DISP0_DAT18__IPU1_DISP0_DATA18  0x38
>> +			MX6QDL_PAD_DISP0_DAT19__IPU1_DISP0_DATA19  0x38
>> +			MX6QDL_PAD_DISP0_DAT20__IPU1_DISP0_DATA20  0x38
>> +			MX6QDL_PAD_DISP0_DAT21__IPU1_DISP0_DATA21  0x38
>> +			MX6QDL_PAD_DISP0_DAT22__IPU1_DISP0_DATA22  0x38
>> +			MX6QDL_PAD_DISP0_DAT23__IPU1_DISP0_DATA23  0x38
>> +		>;
>> +	};
>> +
>>  	pinctrl_uart2: uart2grp {
>>  		fsl,pins = <
>>  			MX6QDL_PAD_GPIO_7__UART2_TX_DATA 0x1b0b1
>> @@ -194,6 +305,10 @@
>>  	};
>>  };
>>
>> +&ipu1_di0_disp0 {
>> +	remote-endpoint = <&parallel_display_in>;
>> +};
>> +
>>  &pcie {
>>  	pcie@0,0 {
>>  		reg = <0x000000 0 0 0 0>;
>> --
>> 2.10.2
>>

Re: [PATCH] ARM: dts: imx6q-utilite-pro: enable 2nd display pipeline

[Philipp Zabel]

Hi Christopher,

On Fri, 2016-12-02 at 15:37 +0100, christopher.spinrath@rwth-aachen.de
wrote:
> From: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
> 
> Apart from the already enabled Designware HDMI port, the Utilite Pro
> has a second display pipeline which has the following shape:
> 
>   IPU1 DI0 --> Parallel display --> tfp410 rgb24 to DVI encoder
>                                 --> HDMI connector.
> Enable support for it.
> 
> In addition, since this pipeline is hardwired to IPU1, sever the link
> between IPU1 and the SoC-internal Designware HDMI encoder forcing the
> latter to be connected to IPU2 instead of IPU1. Otherwise, it is not
> possible to drive both displays at high resolution due to the bandwidth
> limitations of a single IPU.
> 
> Signed-off-by: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
>
> ---
> 
> Hi all,
> 
> the removal of the link between IPU1 and the Designware HDMI encoder is the
> result of a discussion I had with Philipp Zabel:
> 
>   https://lists.freedesktop.org/archives/dri-devel/2016-November/125399.html .
> 
> Altough it is not possible to connect anything else to IPU1 on the Utilite, this
> approach has at least one disadvantage: if the resolution is low enough such 
> that a single IPU can handle both displays then muxing both displays to IPU1
> would reduce the power consumption.
> 
> However, IMHO omitting the link IPU1 <--> DW HDMI is still the preferrable
> solution since I'm not aware of any OS/driver that is capable of switching IPUs
> or can handle the bandwidth limitation in a sane way. In particular, Linux is
> unusable when both displays are supposed to be driven at high resolution and
> both muxing options for the DW HDMI are available (this is not a userspace
> issue; the system becomes almost unresponsive as soon as the kernel sets the
> initial resolution).
> 
> Cheers,
> Christopher
> 
> P.S.: this patch depends on the tfp410 bridge driver which has recently been
> merged into drm-next.
> 
>  arch/arm/boot/dts/imx6q-utilite-pro.dts | 115 ++++++++++++++++++++++++++++++++
>  1 file changed, 115 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx6q-utilite-pro.dts b/arch/arm/boot/dts/imx6q-utilite-pro.dts
> index 2200994..69bdd82 100644
> --- a/arch/arm/boot/dts/imx6q-utilite-pro.dts
> +++ b/arch/arm/boot/dts/imx6q-utilite-pro.dts
> @@ -59,6 +59,33 @@
>  		rtc1 = &snvs_rtc;
>  	};
>  
> +	encoder {
> +		compatible = "ti,tfp410";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
> +				reg = <0>;
> +
> +				tfp410_in: endpoint {
> +					remote-endpoint = <&parallel_display_out>;
> +				};
> +			};
> +
> +			port@1 {
> +				reg = <1>;
> +
> +				tfp410_out: endpoint {
> +					remote-endpoint = <&hdmi_connector_in>;
> +				};
> +			};
> +		};
> +	};
> +
>  	gpio-keys {
>  		compatible = "gpio-keys";
>  		pinctrl-names = "default";
> @@ -72,6 +99,19 @@
>  		};
>  	};
>  
> +	hdmi-connector {
> +		compatible = "hdmi-connector";
> +
> +		type = "a";
> +		ddc-i2c-bus = <&i2c_dvi_ddc>;
> +
> +		port {
> +			hdmi_connector_in: endpoint {
> +				remote-endpoint = <&tfp410_out>;
> +			};
> +		};
> +	};
> +
>  	i2cmux {
>  		compatible = "i2c-mux-gpio";
>  		pinctrl-names = "default";
> @@ -105,8 +145,46 @@
>  			#size-cells = <0>;
>  		};
>  	};
> +
> +	parallel-display {
> +		compatible = "fsl,imx-parallel-display";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_ipu1>;
> +
> +		interface-pix-fmt = "rgb24";

This is not necessary if the connector created by the tpf410 has the
correct media bus format set in its display_info structure. This can be
done in tfp410_attach, before calling drm_mode_connector_attach_encoder:

        u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;

	drm_display_info_set_bus_formats(&dvi->connector.display_info,                                                                         
					 &bus_format, 1);

After this is done, the above line should be removed in a follow-up
patch.

> +		port@0 {
> +			reg = <0>;
> +
> +			parallel_display_in: endpoint {
> +				remote-endpoint = <&ipu1_di0_disp0>;
> +			};
> +		};
> +
> +		port@1 {
> +			reg = <1>;
> +
> +			parallel_display_out: endpoint {
> +				remote-endpoint = <&tfp410_in>;
> +			};
> +		};
> +	};
>  };
>  
> +/*
> + * A single IPU is not able to drive both display interfaces available on the
> + * Utilite Pro at high resolution due to its bandwidth limitation. Since the
> + * tfp410 encoder is wired up to IPU1, sever the link between IPU1 and the
> + * SoC-internal Designware HDMI encoder forcing the latter to be connected to
> + * IPU2 instead of IPU1.
> + */
> +/delete-node/&ipu1_di0_hdmi;
> +/delete-node/&hdmi_mux_0;
> +/delete-node/&ipu1_di1_hdmi;
> +/delete-node/&hdmi_mux_1;
> +
>  &hdmi {
>  	ddc-i2c-bus = <&i2c2>;
>  	status = "okay";
> @@ -151,6 +229,39 @@
>  		>;
>  	};
>  
> +	pinctrl_ipu1: ipu1grp {
> +		fsl,pins = <
> +			MX6QDL_PAD_DI0_DISP_CLK__IPU1_DI0_DISP_CLK 0x38
> +			MX6QDL_PAD_DI0_PIN15__IPU1_DI0_PIN15       0x38
> +			MX6QDL_PAD_DI0_PIN2__IPU1_DI0_PIN02        0x38
> +			MX6QDL_PAD_DI0_PIN3__IPU1_DI0_PIN03        0x38
> +			MX6QDL_PAD_DISP0_DAT0__IPU1_DISP0_DATA00   0x38
> +			MX6QDL_PAD_DISP0_DAT1__IPU1_DISP0_DATA01   0x38
> +			MX6QDL_PAD_DISP0_DAT2__IPU1_DISP0_DATA02   0x38
> +			MX6QDL_PAD_DISP0_DAT3__IPU1_DISP0_DATA03   0x38
> +			MX6QDL_PAD_DISP0_DAT4__IPU1_DISP0_DATA04   0x38
> +			MX6QDL_PAD_DISP0_DAT5__IPU1_DISP0_DATA05   0x38
> +			MX6QDL_PAD_DISP0_DAT6__IPU1_DISP0_DATA06   0x38
> +			MX6QDL_PAD_DISP0_DAT7__IPU1_DISP0_DATA07   0x38
> +			MX6QDL_PAD_DISP0_DAT8__IPU1_DISP0_DATA08   0x38
> +			MX6QDL_PAD_DISP0_DAT9__IPU1_DISP0_DATA09   0x38
> +			MX6QDL_PAD_DISP0_DAT10__IPU1_DISP0_DATA10  0x38
> +			MX6QDL_PAD_DISP0_DAT11__IPU1_DISP0_DATA11  0x38
> +			MX6QDL_PAD_DISP0_DAT12__IPU1_DISP0_DATA12  0x38
> +			MX6QDL_PAD_DISP0_DAT13__IPU1_DISP0_DATA13  0x38
> +			MX6QDL_PAD_DISP0_DAT14__IPU1_DISP0_DATA14  0x38
> +			MX6QDL_PAD_DISP0_DAT15__IPU1_DISP0_DATA15  0x38
> +			MX6QDL_PAD_DISP0_DAT16__IPU1_DISP0_DATA16  0x38
> +			MX6QDL_PAD_DISP0_DAT17__IPU1_DISP0_DATA17  0x38
> +			MX6QDL_PAD_DISP0_DAT18__IPU1_DISP0_DATA18  0x38
> +			MX6QDL_PAD_DISP0_DAT19__IPU1_DISP0_DATA19  0x38
> +			MX6QDL_PAD_DISP0_DAT20__IPU1_DISP0_DATA20  0x38
> +			MX6QDL_PAD_DISP0_DAT21__IPU1_DISP0_DATA21  0x38
> +			MX6QDL_PAD_DISP0_DAT22__IPU1_DISP0_DATA22  0x38
> +			MX6QDL_PAD_DISP0_DAT23__IPU1_DISP0_DATA23  0x38
> +		>;
> +	};
> +
>  	pinctrl_uart2: uart2grp {
>  		fsl,pins = <
>  			MX6QDL_PAD_GPIO_7__UART2_TX_DATA 0x1b0b1
> @@ -194,6 +305,10 @@
>  	};
>  };
>  
> +&ipu1_di0_disp0 {
> +	remote-endpoint = <&parallel_display_in>;
> +};
> +
>  &pcie {
>  	pcie@0,0 {
>  		reg = <0x000000 0 0 0 0>;

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Re: [PATCH] ARM: dts: imx6q-utilite-pro: enable 2nd display pipeline

[Christopher Spinrath]

Hi Philipp,

thanks for the review!

On 01/17/2017 09:57 AM, Philipp Zabel wrote:
> [...]
>> +
>> +	parallel-display {
>> +		compatible = "fsl,imx-parallel-display";
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_ipu1>;
>> +
>> +		interface-pix-fmt = "rgb24";
>
> This is not necessary if the connector created by the tpf410 has the
> correct media bus format set in its display_info structure. This can be
> done in tfp410_attach, before calling drm_mode_connector_attach_encoder:
>
>         u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>
> 	drm_display_info_set_bus_formats(&dvi->connector.display_info,
> 					 &bus_format, 1);
>
> After this is done, the above line should be removed in a follow-up
> patch.

Ok, I will send a mini follow-up series doing that with your 
Suggested-by (unless you object) in the next few days.

Cheers,
Christopher

>> +		port@0 {
>> +			reg = <0>;
>> +
>> +			parallel_display_in: endpoint {
>> +				remote-endpoint = <&ipu1_di0_disp0>;
>> +			};
>> +		};
>> +
>> +		port@1 {
>> +			reg = <1>;
>> +
>> +			parallel_display_out: endpoint {
>> +				remote-endpoint = <&tfp410_in>;
>> +			};
>> +		};
>> +	};
>>  };
>>  [...]
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: dts: imx6q-utilite-pro: enable 2nd display pipeline

[Christopher Spinrath]

Hi Philipp,

turns out I have a question on your comment after all:

On 01/17/2017 07:35 PM, Christopher Spinrath wrote:
> Hi Philipp,
>
> thanks for the review!
>
> On 01/17/2017 09:57 AM, Philipp Zabel wrote:
>> [...]
>>> +
>>> +    parallel-display {
>>> +        compatible = "fsl,imx-parallel-display";
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +        pinctrl-names = "default";
>>> +        pinctrl-0 = <&pinctrl_ipu1>;
>>> +
>>> +        interface-pix-fmt = "rgb24";
>>
>> This is not necessary if the connector created by the tpf410 has the
>> correct media bus format set in its display_info structure. This can be
>> done in tfp410_attach, before calling drm_mode_connector_attach_encoder:
>>
>>         u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>>
>>     drm_display_info_set_bus_formats(&dvi->connector.display_info,
>>                      &bus_format, 1);
>>
>> After this is done, the above line should be removed in a follow-up
>> patch.

On closer inspection the tfp410 can handle rgb12, rgb24, and DVI 
formats. Considering this it feels wrong to hardcode the bus format to 
rgb24 (isn't it?).

So a solution might be to add a property specifying the bus format to 
the tfp410 binding. But then we would effectively just move this 
property from one node to another. I wonder if this is still desireable...?

Cheers,
Christopher

> Ok, I will send a mini follow-up series doing that with your
> Suggested-by (unless you object) in the next few days.
>
> Cheers,
> Christopher
>
>>> +        port@0 {
>>> +            reg = <0>;
>>> +
>>> +            parallel_display_in: endpoint {
>>> +                remote-endpoint = <&ipu1_di0_disp0>;
>>> +            };
>>> +        };
>>> +
>>> +        port@1 {
>>> +            reg = <1>;
>>> +
>>> +            parallel_display_out: endpoint {
>>> +                remote-endpoint = <&tfp410_in>;
>>> +            };
>>> +        };
>>> +    };
>>>  };
>>>  [...]

Re: [PATCH] ARM: dts: imx6q-utilite-pro: enable 2nd display pipeline

[Philipp Zabel]

Hi Christopher,

On Wed, 2017-01-18 at 23:20 +0100, Christopher Spinrath wrote:
> Hi Philipp,
> 
> turns out I have a question on your comment after all:
> 
> On 01/17/2017 07:35 PM, Christopher Spinrath wrote:
> > Hi Philipp,
> >
> > thanks for the review!
> >
> > On 01/17/2017 09:57 AM, Philipp Zabel wrote:
> >> [...]
> >>> +
> >>> +    parallel-display {
> >>> +        compatible = "fsl,imx-parallel-display";
> >>> +        #address-cells = <1>;
> >>> +        #size-cells = <0>;
> >>> +        pinctrl-names = "default";
> >>> +        pinctrl-0 = <&pinctrl_ipu1>;
> >>> +
> >>> +        interface-pix-fmt = "rgb24";
> >>
> >> This is not necessary if the connector created by the tpf410 has the
> >> correct media bus format set in its display_info structure. This can be
> >> done in tfp410_attach, before calling drm_mode_connector_attach_encoder:
> >>
> >>         u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> >>
> >>     drm_display_info_set_bus_formats(&dvi->connector.display_info,
> >>                      &bus_format, 1);
> >>
> >> After this is done, the above line should be removed in a follow-up
> >> patch.
> 
> On closer inspection the tfp410 can handle rgb12, rgb24, and DVI 
> formats. Considering this it feels wrong to hardcode the bus format to 
> rgb24 (isn't it?).

That is a good point, I agree. I have some thoughts on this:

> So a solution might be to add a property specifying the bus format to 
> the tfp410 binding. But then we would effectively just move this 
> property from one node to another. I wonder if this is still desireable...?

If this is configurable on both ends, the necessary setting is neither a
hardware property of the bridge, nor of the display interface, but
rather one of the board specific wiring between the two (if at all).

Ideally all possible settings should be known to the drivers and the
best format should be negotiated. Note that display_info.bus_formats
already is an array of possible bus formats, even though the parallel
display driver currently only looks at the first element.

If there is no limitation imposed by the wiring, presenting best format
first in that array seems reasonable for now.

Otherwise I think the links should describe the parallel bus layout,
which is specified for the (input) media video interfaces:

Documentation/devicetree/bindings/media/video-interfaces.txt

Using the
	bus-width = <24>;
or
	bus-width = <12>;
property the tfp510 driver could choose which bus_formats to add to the
display_info.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: dts: imx6q-utilite-pro: enable 2nd display pipeline

[Christopher Spinrath]

Hi Philipp,

sorry for the late answer; I'm quite busy with other stuff these days.

On 01/19/2017 03:34 PM, Philipp Zabel wrote:
> Hi Christopher,
>
> On Wed, 2017-01-18 at 23:20 +0100, Christopher Spinrath wrote:
>> Hi Philipp,
>>
>> turns out I have a question on your comment after all:
>>
>> On 01/17/2017 07:35 PM, Christopher Spinrath wrote:
>>> Hi Philipp,
>>>
>>> thanks for the review!
>>>
>>> On 01/17/2017 09:57 AM, Philipp Zabel wrote:
>>>> [...]
>>>>> +
>>>>> +    parallel-display {
>>>>> +        compatible = "fsl,imx-parallel-display";
>>>>> +        #address-cells = <1>;
>>>>> +        #size-cells = <0>;
>>>>> +        pinctrl-names = "default";
>>>>> +        pinctrl-0 = <&pinctrl_ipu1>;
>>>>> +
>>>>> +        interface-pix-fmt = "rgb24";
>>>>
>>>> This is not necessary if the connector created by the tpf410 has the
>>>> correct media bus format set in its display_info structure. This can be
>>>> done in tfp410_attach, before calling drm_mode_connector_attach_encoder:
>>>>
>>>>         u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>>>>
>>>>     drm_display_info_set_bus_formats(&dvi->connector.display_info,
>>>>                      &bus_format, 1);
>>>>
>>>> After this is done, the above line should be removed in a follow-up
>>>> patch.
>>
>> On closer inspection the tfp410 can handle rgb12, rgb24, and DVI
>> formats. Considering this it feels wrong to hardcode the bus format to
>> rgb24 (isn't it?).
>
> That is a good point, I agree. I have some thoughts on this:
>
>> So a solution might be to add a property specifying the bus format to
>> the tfp410 binding. But then we would effectively just move this
>> property from one node to another. I wonder if this is still desireable...?
>
> If this is configurable on both ends, the necessary setting is neither a
> hardware property of the bridge, nor of the display interface, but
> rather one of the board specific wiring between the two (if at all).
>
> Ideally all possible settings should be known to the drivers and the
> best format should be negotiated. Note that display_info.bus_formats
> already is an array of possible bus formats, even though the parallel
> display driver currently only looks at the first element.
>
> If there is no limitation imposed by the wiring, presenting best format
> first in that array seems reasonable for now.

The tfp410 can be operated either in i2c mode or in dump mode (i2c is 
not wired up). The Linux driver does only support dump mode so far. (In 
essence the driver does nothing but the neccesary boilerplate stuff.) 
While the format can be read and changed neither is possible in the dumb 
mode. The format is configured by connecting some pins of the chip to 
ground. So we have to deal with the "otherwise" case below.

> Otherwise I think the links should describe the parallel bus layout,
> which is specified for the (input) media video interfaces:
>
> Documentation/devicetree/bindings/media/video-interfaces.txt
>
> Using the
> 	bus-width = <24>;
> or
> 	bus-width = <12>;
> property the tfp510 driver could choose which bus_formats to add to the
> display_info.

I see your point that the format is a property of the wiring and, thus, 
it is better to utilizie the bus-width property in the of graph remote 
nodes from a modelling point of view.

On the other hand, the parallel-display already has some data on the 
wiring: the pinctrl settings. This weakens the above reasoning because 
the information is splitted up. Moreover, a quick grep session revealed 
that on all arm platforms the format is obtained from the "SoC-side" 
(read parallel-display) node, if it is dynamic. Is there some effort to 
change that? (essentially, I'm trying to understand here the 
justification for the change we are discussing).

Cheers,
Christopher

> regards
> Philipp
>