[PATCH 0/4] DRM driver for ST-Ericsson MCDE

[PATCH 0/4] DRM driver for ST-Ericsson MCDE

[Linus Walleij]

This adds a driver for the ST-Ericsson MCDE.

I had to come up with some way to support passing an external
encoder to the simple KMS helper to make DSI work with the
simple KMS helper.

This work was motivated by the ongoing work on the LIMA driver,
as Ux500 has the MALI400 so once that driver is in place
as well, there will be a full graphic stack for Ux500 with
this display driver, which is pretty neat.

Linus Walleij (4):
  drm/simple_kms_helper: enable use of external encoder
  drm/mcde: Add device tree bindings
  drm/mcde: Add new driver for ST-Ericsson MCDE
  ARM: dts: Ux500: Add MCDE and Samsung display

 .../devicetree/bindings/display/ste,mcde.txt  |  110 ++
 Documentation/gpu/drivers.rst                 |    1 +
 Documentation/gpu/mcde.rst                    |    6 +
 arch/arm/boot/dts/ste-dbx5x0.dtsi             |   36 +-
 arch/arm/boot/dts/ste-href-stuib.dtsi         |   25 +
 arch/arm/boot/dts/ste-href-tvk1281618.dtsi    |   25 +
 drivers/gpu/drm/Kconfig                       |    2 +
 drivers/gpu/drm/Makefile                      |    1 +
 drivers/gpu/drm/drm_simple_kms_helper.c       |   23 +-
 drivers/gpu/drm/mcde/Kconfig                  |   18 +
 drivers/gpu/drm/mcde/Makefile                 |    3 +
 drivers/gpu/drm/mcde/mcde_display.c           | 1285 +++++++++++++++
 drivers/gpu/drm/mcde/mcde_drm.h               |   52 +
 drivers/gpu/drm/mcde/mcde_drv.c               |  540 +++++++
 drivers/gpu/drm/mcde/mcde_dsi.c               | 1376 +++++++++++++++++
 15 files changed, 3493 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/ste,mcde.txt
 create mode 100644 Documentation/gpu/mcde.rst
 create mode 100644 drivers/gpu/drm/mcde/Kconfig
 create mode 100644 drivers/gpu/drm/mcde/Makefile
 create mode 100644 drivers/gpu/drm/mcde/mcde_display.c
 create mode 100644 drivers/gpu/drm/mcde/mcde_drm.h
 create mode 100644 drivers/gpu/drm/mcde/mcde_drv.c
 create mode 100644 drivers/gpu/drm/mcde/mcde_dsi.c

-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/4] drm/simple_kms_helper: enable use of external encoder

[Linus Walleij]

This makes it possible to pass a connector with an already
attached external encoder into the simple KMS helper.

This is helpful for my MCDE drivers, as it is pretty simple
but uses DSI to communicate with the displays and bridges.
DSI requires the use of the DSI bus which in turn requires
us to set up a custom connector from the display driver.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpu/drm/drm_simple_kms_helper.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
index 917812448d1b..e7499b939235 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -266,7 +266,7 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
 			const uint64_t *format_modifiers,
 			struct drm_connector *connector)
 {
-	struct drm_encoder *encoder = &pipe->encoder;
+	struct drm_encoder *encoder;
 	struct drm_plane *plane = &pipe->plane;
 	struct drm_crtc *crtc = &pipe->crtc;
 	int ret;
@@ -289,10 +289,23 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
 	if (ret)
 		return ret;
 
-	encoder->possible_crtcs = drm_crtc_mask(crtc);
-	ret = drm_encoder_init(dev, encoder, &drm_simple_kms_encoder_funcs,
-			       DRM_MODE_ENCODER_NONE, NULL);
-	if (ret || !connector)
+	/* Other encoder already attached to the connector */
+	if (connector->encoder_ids[0] != 0) {
+		encoder = drm_encoder_find(connector->dev, NULL,
+					   connector->encoder_ids[0]);
+		encoder->possible_crtcs = drm_crtc_mask(crtc);
+		DRM_INFO("an encoder is already attached to the connector\n");
+	} else {
+		encoder = &pipe->encoder;
+		encoder->possible_crtcs = drm_crtc_mask(crtc);
+		ret = drm_encoder_init(dev, encoder,
+				       &drm_simple_kms_encoder_funcs,
+				       DRM_MODE_ENCODER_NONE, NULL);
+		if (ret)
+			return ret;
+	}
+
+	if (!connector)
 		return ret;
 
 	return drm_connector_attach_encoder(connector, encoder);
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/4] drm/mcde: Add device tree bindings

[Linus Walleij]

This adds the device tree bindings for the ST-Ericsson
Multi Channel Display Engine MCDE as found in the U8500
SoCs.

Cc: devicetree@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 .../devicetree/bindings/display/ste,mcde.txt  | 110 ++++++++++++++++++
 1 file changed, 110 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/ste,mcde.txt

diff --git a/Documentation/devicetree/bindings/display/ste,mcde.txt b/Documentation/devicetree/bindings/display/ste,mcde.txt
new file mode 100644
index 000000000000..fc58aa5effb5
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/ste,mcde.txt
@@ -0,0 +1,110 @@
+ST-Ericsson Multi Channel Display Engine MCDE
+
+The ST-Ericsson MCDE is a display controller with support for compositing
+and displaying several channels memory resident graphics data on DSI or
+LCD displays or bridges. It is used in the ST-Ericsson U8500 platform.
+
+Required properties:
+
+- compatible: must be:
+  "ste,mcde"
+- reg: register base for the main MCDE control registers, should be
+  0x1000 in size
+- interrupts: the interrupt line for the MCDE
+- epod-supply: a phandle to the EPOD regulator
+- vana-supply: a phandle to the analog voltage regulator
+- clocks: an array of the MCDE clocks in this strict order:
+  MCDECLK (main MCDE clock), LCDCLK (LCD clock), PLLDSI
+  (HDMI clock), DSI0ESCLK (DSI0 energy save clock),
+  DSI1ESCLK (DSI1 energy save clock), DSI2ESCLK (DSI2 energy
+  save clock)
+- clock-names: must be the following array:
+  "mcde", "lcd", "hdmi", "dsi0", "dsi1", "dsi0es", "dsi1es", "dsi2es"
+  to match the required clock inputs above.
+- #address-cells: should be <1> (for the DSI hosts that will be children)
+- #size-cells: should be <1> (for the DSI hosts that will be children)
+- ranges: this should always be stated
+
+Required subnodes:
+
+The devicetree must specify subnodes for the DSI host adapters.
+These must have the following characteristics:
+
+- compatible: must be:
+  "ste,mcde-dsi"
+- reg: must specify the register range for the DSI host
+- vana-supply: phandle to the VANA voltage regulator
+- #address-cells: should be <1>
+- #size-cells: should be <0>
+
+Display panels and bridges will appear as children on the DSI hosts, and
+the displays are connected to the DSI hosts using the common binding
+for video transmitter interfaces; see
+Documentation/devicetree/bindings/media/video-interfaces.txt
+
+If a DSI host is unused (not connected) it will have no children or ports
+defined.
+
+Example:
+
+mcde@a0350000 {
+	compatible = "ste,mcde";
+	reg = <0xa0350000 0x1000>;
+	interrupts = <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>;
+	epod-supply = <&db8500_b2r2_mcde_reg>;
+	vana-supply = <&ab8500_ldo_ana_reg>;
+	clocks = <&prcmu_clk PRCMU_MCDECLK>, /* Main MCDE clock */
+		 <&prcmu_clk PRCMU_LCDCLK>, /* LCD clock */
+		 <&prcmu_clk PRCMU_PLLDSI>, /* HDMI clock */
+		 <&prcmu_clk PRCMU_DSI0CLK>, /* DSI 0 */
+		 <&prcmu_clk PRCMU_DSI1CLK>, /* DSI 1 */
+		 <&prcmu_clk PRCMU_DSI0ESCCLK>, /* TVout clock 0 */
+		 <&prcmu_clk PRCMU_DSI1ESCCLK>, /* TVout clock 1 */
+		 <&prcmu_clk PRCMU_DSI2ESCCLK>; /* TVout clock 2 */
+	clock-names = "mcde", "lcd", "hdmi", "dsi0", "dsi1",
+		    "dsi0es", "dsi1es", "dsi2es";
+	#address-cells = <1>;
+	#size-cells = <1>;
+	ranges;
+
+	dsi0: dsi@a0351000 {
+		compatible = "ste,mcde-dsi";
+		reg = <0xa0351000 0x1000>;
+		vana-supply = <&ab8500_ldo_ana_reg>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		port {
+			dsi0_out: endpoint {
+				remote-endpoint = <&panel_in>;
+			};
+		};
+
+		panel: display {
+			compatible = "samsung,s6d16d0";
+			reg = <0>;
+			vdd1-supply = <&ab8500_ldo_aux1_reg>;
+			reset-gpios = <&gpio2 1 GPIO_ACTIVE_LOW>;
+
+			port {
+				panel_in: endpoint {
+					remote-endpoint = <&dsi0_out>;
+				};
+			};
+		};
+
+	};
+	dsi1: dsi@a0352000 {
+		compatible = "ste,mcde-dsi";
+		reg = <0xa0352000 0x1000>;
+		vana-supply = <&ab8500_ldo_ana_reg>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+	};
+	dsi2: dsi@a0353000 {
+		compatible = "ste,mcde-dsi";
+		reg = <0xa0353000 0x1000>;
+		vana-supply = <&ab8500_ldo_ana_reg>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+	};
+};
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/4] ARM: dts: Ux500: Add MCDE and Samsung display

[Linus Walleij]

This adds and updates the device tree nodes for the MCDE
display controller and connects the Samsung display to
the TVK1281618 user interface board (UIB) so we get
nicely working graphics on this reference design.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 arch/arm/boot/dts/ste-dbx5x0.dtsi          | 36 +++++++++++++++++++---
 arch/arm/boot/dts/ste-href-stuib.dtsi      | 25 +++++++++++++++
 arch/arm/boot/dts/ste-href-tvk1281618.dtsi | 25 +++++++++++++++
 3 files changed, 81 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/ste-dbx5x0.dtsi b/arch/arm/boot/dts/ste-dbx5x0.dtsi
index e6ed7c0354a2..a33b36a8c879 100644
--- a/arch/arm/boot/dts/ste-dbx5x0.dtsi
+++ b/arch/arm/boot/dts/ste-dbx5x0.dtsi
@@ -1197,12 +1197,11 @@
 		};
 
 		mcde@a0350000 {
-			compatible = "stericsson,mcde";
-			reg = <0xa0350000 0x1000>, /* MCDE */
-			      <0xa0351000 0x1000>, /* DSI link 1 */
-			      <0xa0352000 0x1000>, /* DSI link 2 */
-			      <0xa0353000 0x1000>; /* DSI link 3 */
+			compatible = "ste,mcde";
+			reg = <0xa0350000 0x1000>;
 			interrupts = <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>;
+			epod-supply = <&db8500_b2r2_mcde_reg>;
+			vana-supply = <&ab8500_ldo_ana_reg>;
 			clocks = <&prcmu_clk PRCMU_MCDECLK>, /* Main MCDE clock */
 				 <&prcmu_clk PRCMU_LCDCLK>, /* LCD clock */
 				 <&prcmu_clk PRCMU_PLLDSI>, /* HDMI clock */
@@ -1211,6 +1210,33 @@
 				 <&prcmu_clk PRCMU_DSI0ESCCLK>, /* TVout clock 0 */
 				 <&prcmu_clk PRCMU_DSI1ESCCLK>, /* TVout clock 1 */
 				 <&prcmu_clk PRCMU_DSI2ESCCLK>; /* TVout clock 2 */
+			clock-names = "mcde", "lcd", "hdmi", "dsi0", "dsi1", "dsi0es", "dsi1es", "dsi2es";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+			status = "disabled";
+
+			dsi0: dsi@a0351000 {
+				compatible = "ste,mcde-dsi";
+				reg = <0xa0351000 0x1000>;
+				vana-supply = <&ab8500_ldo_ana_reg>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+			dsi1: dsi@a0352000 {
+				compatible = "ste,mcde-dsi";
+				reg = <0xa0352000 0x1000>;
+				vana-supply = <&ab8500_ldo_ana_reg>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+			dsi2: dsi@a0353000 {
+				compatible = "ste,mcde-dsi";
+				reg = <0xa0353000 0x1000>;
+				vana-supply = <&ab8500_ldo_ana_reg>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
 		};
 
 		cryp@a03cb000 {
diff --git a/arch/arm/boot/dts/ste-href-stuib.dtsi b/arch/arm/boot/dts/ste-href-stuib.dtsi
index 35e944d8b5c4..714c2fb758d4 100644
--- a/arch/arm/boot/dts/ste-href-stuib.dtsi
+++ b/arch/arm/boot/dts/ste-href-stuib.dtsi
@@ -190,5 +190,30 @@
 				};
 			};
 		};
+
+		mcde@a0350000 {
+			status = "okay";
+
+			dsi@a0351000 {
+				port {
+					dsi0_out: endpoint {
+						remote-endpoint = <&panel_in>;
+					};
+				};
+
+				panel: display {
+					compatible = "samsung,s6d16d0";
+					reg = <0>;
+					vdd1-supply = <&ab8500_ldo_aux1_reg>;
+					reset-gpios = <&gpio2 1 GPIO_ACTIVE_LOW>;
+
+					port {
+						panel_in: endpoint {
+							remote-endpoint = <&dsi0_out>;
+						};
+					};
+				};
+			};
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/ste-href-tvk1281618.dtsi b/arch/arm/boot/dts/ste-href-tvk1281618.dtsi
index 0e7d77d719d7..84bb1e79168b 100644
--- a/arch/arm/boot/dts/ste-href-tvk1281618.dtsi
+++ b/arch/arm/boot/dts/ste-href-tvk1281618.dtsi
@@ -274,5 +274,30 @@
 				};
 			};
 		};
+
+		mcde@a0350000 {
+			status = "okay";
+
+			dsi@a0351000 {
+				port {
+					dsi0_out: endpoint {
+						remote-endpoint = <&panel_in>;
+					};
+				};
+
+				panel: display {
+					compatible = "samsung,s6d16d0";
+					reg = <0>;
+					vdd1-supply = <&ab8500_ldo_aux1_reg>;
+					reset-gpios = <&gpio2 1 GPIO_ACTIVE_LOW>;
+
+					port {
+						panel_in: endpoint {
+							remote-endpoint = <&dsi0_out>;
+						};
+					};
+				};
+			};
+		};
 	};
 };
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/4] drm/simple_kms_helper: enable use of external encoder

[Daniel Vetter]

On Thu, Feb 07, 2019 at 09:36:44AM +0100, Linus Walleij wrote:
> This makes it possible to pass a connector with an already
> attached external encoder into the simple KMS helper.
> 
> This is helpful for my MCDE drivers, as it is pretty simple
> but uses DSI to communicate with the displays and bridges.
> DSI requires the use of the DSI bus which in turn requires
> us to set up a custom connector from the display driver.

So the idea was that you'd just use a bridge for this, if you need more
than a dummy encoder. I'm a bit worried about mission creep for the simple
helpers, sooner or later we'll add extensions and then we're back to full
kms, except it's a hairball of classic midlayer mistake ...

See drm_simple_display_pipe_attach_bridge().

Cheers, Daniel

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpu/drm/drm_simple_kms_helper.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> index 917812448d1b..e7499b939235 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -266,7 +266,7 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
>  			const uint64_t *format_modifiers,
>  			struct drm_connector *connector)
>  {
> -	struct drm_encoder *encoder = &pipe->encoder;
> +	struct drm_encoder *encoder;
>  	struct drm_plane *plane = &pipe->plane;
>  	struct drm_crtc *crtc = &pipe->crtc;
>  	int ret;
> @@ -289,10 +289,23 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
>  	if (ret)
>  		return ret;
>  
> -	encoder->possible_crtcs = drm_crtc_mask(crtc);
> -	ret = drm_encoder_init(dev, encoder, &drm_simple_kms_encoder_funcs,
> -			       DRM_MODE_ENCODER_NONE, NULL);
> -	if (ret || !connector)
> +	/* Other encoder already attached to the connector */
> +	if (connector->encoder_ids[0] != 0) {
> +		encoder = drm_encoder_find(connector->dev, NULL,
> +					   connector->encoder_ids[0]);
> +		encoder->possible_crtcs = drm_crtc_mask(crtc);
> +		DRM_INFO("an encoder is already attached to the connector\n");
> +	} else {
> +		encoder = &pipe->encoder;
> +		encoder->possible_crtcs = drm_crtc_mask(crtc);
> +		ret = drm_encoder_init(dev, encoder,
> +				       &drm_simple_kms_encoder_funcs,
> +				       DRM_MODE_ENCODER_NONE, NULL);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (!connector)
>  		return ret;
>  
>  	return drm_connector_attach_encoder(connector, encoder);
> -- 
> 2.20.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/4] drm/simple_kms_helper: enable use of external encoder

[Linus Walleij]

On Thu, Feb 7, 2019 at 10:17 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Feb 07, 2019 at 09:36:44AM +0100, Linus Walleij wrote:

> > This makes it possible to pass a connector with an already
> > attached external encoder into the simple KMS helper.
> >
> > This is helpful for my MCDE drivers, as it is pretty simple
> > but uses DSI to communicate with the displays and bridges.
> > DSI requires the use of the DSI bus which in turn requires
> > us to set up a custom connector from the display driver.
>
> So the idea was that you'd just use a bridge for this, if you need more
> than a dummy encoder. I'm a bit worried about mission creep for the simple
> helpers, sooner or later we'll add extensions and then we're back to full
> kms, except it's a hairball of classic midlayer mistake ...
>
> See drm_simple_display_pipe_attach_bridge().

Attaching bridges usually work fine for simple KMS drivers,
like the panel bridge, or the dumb VGA bridge or even
the HDMI encoder bridges so I'm on board with that.

The thing is that the DSI driver is using the panel bridge
(and has a placeholder for using other bridges) already.
So we end up wrapping the DSI host device(s) with a
"my DSI bridge" using another
bridge (panel) IIUC.

display driver -> custom DSI bridge -> panel bridge -> panel

The endpoint bridge (the panel) has all information on
resolution etc. So this means we will need to just pass this
information through to the next step in the pipe I
suppose.

I just want to confirm that I'm on the right track here before
I code another thousand lines of code for this :)

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/4] drm/simple_kms_helper: enable use of external encoder

[Daniel Vetter]

On Thu, Feb 07, 2019 at 10:02:17PM +0100, Linus Walleij wrote:
> On Thu, Feb 7, 2019 at 10:17 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Feb 07, 2019 at 09:36:44AM +0100, Linus Walleij wrote:
> 
> > > This makes it possible to pass a connector with an already
> > > attached external encoder into the simple KMS helper.
> > >
> > > This is helpful for my MCDE drivers, as it is pretty simple
> > > but uses DSI to communicate with the displays and bridges.
> > > DSI requires the use of the DSI bus which in turn requires
> > > us to set up a custom connector from the display driver.
> >
> > So the idea was that you'd just use a bridge for this, if you need more
> > than a dummy encoder. I'm a bit worried about mission creep for the simple
> > helpers, sooner or later we'll add extensions and then we're back to full
> > kms, except it's a hairball of classic midlayer mistake ...
> >
> > See drm_simple_display_pipe_attach_bridge().
> 
> Attaching bridges usually work fine for simple KMS drivers,
> like the panel bridge, or the dumb VGA bridge or even
> the HDMI encoder bridges so I'm on board with that.
> 
> The thing is that the DSI driver is using the panel bridge
> (and has a placeholder for using other bridges) already.
> So we end up wrapping the DSI host device(s) with a
> "my DSI bridge" using another
> bridge (panel) IIUC.
> 
> display driver -> custom DSI bridge -> panel bridge -> panel
> 
> The endpoint bridge (the panel) has all information on
> resolution etc. So this means we will need to just pass this
> information through to the next step in the pipe I
> suppose.

Hm, if you have you're mode_check functions working correctly (i.e. make
sure the mode userspace asks for is the one the panel ones), then the mode
will be in the crtc_state->requested_mode that the atomic helpers already
hand to all the bridges. Or well would, if bridge wouldn't have predated
atomic, so this is all a bit a more a mess than strictly needed. But you
should be getting the right mode already.

> I just want to confirm that I'm on the right track here before
> I code another thousand lines of code for this :)

bridges are supposed to be stackable, but I think you're the first to find
out whether that actually works. Afaiui the real stacking fun is if you
need to have some state between the bridges, because you e.g. have a
scaler or transcoder or something like that in-between.

Looking at your patch, converting the encoder into a bridge, and chaining
up the bridges in the right order, is all that should be needed really.
Not retyping the 1k lines of code that actually does stuff.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/4] drm/mcde: Add new driver for ST-Ericsson MCDE

[Daniel Vetter]

On Thu, Feb 07, 2019 at 11:22:39PM +0100, Daniel Vetter wrote:
> On Thu, Feb 07, 2019 at 09:36:46AM +0100, Linus Walleij wrote:
> > +static const struct drm_mode_config_funcs mode_config_funcs = {
> > +	.fb_create = drm_gem_fb_create,
> 
> You need drm_gem_fb_create_with_dirty here because you have a manual
> upload screen. Also might head the advice from the kerneldoc and check
> your buffer alignment constraints here instead of in the check callback.
> But I guess either works, best would be if dumb_create would align stuff
> correctly (which it should), but since this is super theoretical (usually
> modes are even enough), who cares :-)

Forgot to mention: You might want to wire up the damage rect stuff and
only upload the part of the screen that userspace/fbdev tells you has
actually changed. Doesn't apply to all (or even most) userspace just yet,
since brand new stuff.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/4] drm/mcde: Add new driver for ST-Ericsson MCDE

[Sam Ravnborg]

Hi Linus.

Good looking driver. A few nits in the following.
I did not try to follow the code, so no proper review done, sorry.

	Sam

> +++ b/drivers/gpu/drm/mcde/mcde_display.c
> @@ -0,0 +1,1284 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018 Linus Walleij <linus.walleij@linaro.org>
> + * Parts of this file were based on the MCDE driver by Marcus Lorentzon
> + * (C) ST-Ericsson SA 2013
> + */
> +#include <linux/clk.h>
> +#include <linux/version.h>
> +#include <linux/dma-buf.h>
> +#include <linux/of_graph.h>
> +
> +#include <drm/drmP.h>

Please do not use drmP.h in new drivers.
drmP.h is deprecated and we are working on gettting rid of it.

> +
> +static void mcde_display_enable(struct drm_simple_display_pipe *pipe,
> +				struct drm_crtc_state *cstate,
> +				struct drm_plane_state *plane_state)
> +{
> +	struct drm_crtc *crtc = &pipe->crtc;
> +	struct drm_plane *plane = &pipe->plane;
> +	struct drm_device *drm = crtc->dev;
> +	struct mcde *mcde = drm->dev_private;
> +	const struct drm_display_mode *mode = &cstate->mode;
> +	struct drm_framebuffer *fb = plane->state->fb;
> +	u32 format = fb->format->format;
> +	u32 formatter_ppl = mode->hdisplay; /* pixels per line */
> +	u32 formatter_lpf = mode->vdisplay; /* lines per frame */
> +	int pkt_size, fifo_wtrmrk;
> +	int cpp = drm_format_plane_cpp(format, 0);
> +	int formatter_cpp;
> +	struct drm_format_name_buf tmp;
> +	u32 formatter_frame;
> +	u32 pkt_div;
> +	u32 val;

...
This is a very long function. Please consider splitting it up in a
a smaller more readable set of functions.


> +	default:
> +		dev_err(drm->dev, "Unknown pixel format 0x%08x\n",
> +			fb->format->format);
> +		break;
> +	}
Despite and unknow pixel format the following code is executed.
Looks wrong, if it is OK then maybe add a comment in the default: case
to say so.

> diff --git a/drivers/gpu/drm/mcde/mcde_drm.h b/drivers/gpu/drm/mcde/mcde_drm.h
> new file mode 100644
> index 000000000000..eea6dc23436a
> --- /dev/null
> +++ b/drivers/gpu/drm/mcde/mcde_drm.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (C) 2018 Linus Walleij <linus.walleij@linaro.org>
> + * Parts of this file were based on the MCDE driver by Marcus Lorentzon
> + * (C) ST-Ericsson SA 2013
> + */
> +#include <drm/drm_simple_kms_helper.h>
> +
> +#ifndef _MCDE_DRM_H_
> +#define _MCDE_DRM_H_
> +
It is considered good practice (at least by me) that a header
file includes all necessary files, so users do not need to care.
It looks like a few is missing here.

Also expand the include guards to cover the incldue files
so they are not included twice.
This file is likely only included once, so this is only to make it look
like any other heder file.


> diff --git a/drivers/gpu/drm/mcde/mcde_drv.c b/drivers/gpu/drm/mcde/mcde_drv.c
> new file mode 100644
> index 000000000000..cb65609ac812
> --- /dev/null
> +++ b/drivers/gpu/drm/mcde/mcde_drv.c
> @@ -0,0 +1,540 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018 Linus Walleij <linus.walleij@linaro.org>
> + * Parts of this file were based on the MCDE driver by Marcus Lorentzon
> + * (C) ST-Ericsson SA 2013
> + */
> +
> +/**
> + * DOC: ST-Ericsson MCDE Driver
> + *
> + * The MCDE (short for Multi-channel display engine) is a graphics
> + * controller found in the Ux500 chipsets, such as NovaThor U8500.
> + * It was initially conceptualized by ST Microelectronics for the
> + * successor of the Nomadik line, STn8500 but productified in the
> + * ST-Ericsson U8500 where is was used for mass-market deployments
> + * in Android phones from Samsung and Sony Ericsson.
> + *
> + * It can do 1080p30 on SDTV CCIR656, DPI-2, DBI-2 or DSI for
> + * panels with or without frame buffering and can convert most
> + * input formats including most variants of RGB and YUV.
> + *
> + * The hardware has four display pipes, and the layout is a little
> + * bit like this:
> + *
> + * Memory     -> 6 channels -> 5 formatters -> DSI/DPI -> LCD/HDMI
> + * 10 sources    (overlays)                    3 x DSI
> + *
> + * The memory has 5 input channels (memory ports):
> + * 2 channel A (LCD/TV)
> + * 2 channel B (LCD/TV)
> + * 1 channel CO/C1 (Panel with embedded buffer)
> + *
> + * 3 of the formatters are for DSI
> + * 2 of the formatters are for DPI
> + *
> + * Behind the formatters are the DSI or DPI ports, that route to
> + * the external pins of the chip. As there are 3 DSI ports and one
> + * DPI port, it is possible to configure up to 4 display pipelines.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/dma-buf.h>
> +#include <linux/irq.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/slab.h>
> +#include <linux/component.h>

Please sort include files alphabatically.

> +
> +#include <drm/drmP.h>

And like before, get rid of drmP.h

> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_gem.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_fb_cma_helper.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_bridge.h>
Also alphabetically sort needed again.

> +
> +#define MCDE_CR 0x00000000
> +#define MCDE_CR_IFIFOEMPTYLINECOUNT_V422_SHIFT 0
> +#define MCDE_CR_IFIFOEMPTYLINECOUNT_V422_MASK 0x0000003F
> +#define MCDE_CR_IFIFOCTRLEN BIT(15)
> +#define MCDE_CR_UFRECOVERY_MODE_V422 BIT(16)
> +#define MCDE_CR_WRAP_MODE_V422_SHIFT BIT(17)
> +#define MCDE_CR_AUTOCLKG_EN BIT(30)
> +#define MCDE_CR_MCDEEN BIT(31)

The register definitions are spread over several .c files.
And that is also nice so it is easy to navigate to the register
definitions in the same file.
But if refactoring then this can be annoying as you may end up with two
files that require the same register definitions.
Consider an own heder file.

> +	if (ret) {
> +		dev_err(dev, "faule to add component master\n");
Spelling error. faule => failed?

> +++ b/drivers/gpu/drm/mcde/mcde_dsi.c
> @@ -0,0 +1,1374 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/clk.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +#include <linux/component.h>
> +#include <linux/of.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +#include <video/mipi_display.h>
Alphabetic order.

> +
> +#include <drm/drm_device.h>
> +#include <drm/drm_encoder.h>
> +#include <drm/drm_modeset_helper_vtables.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_of.h>
Alphabetic order.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] drm/mcde: Add device tree bindings

[Rob Herring]

On Thu, Feb 07, 2019 at 09:36:45AM +0100, Linus Walleij wrote:
> This adds the device tree bindings for the ST-Ericsson
> Multi Channel Display Engine MCDE as found in the U8500
> SoCs.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  .../devicetree/bindings/display/ste,mcde.txt  | 110 ++++++++++++++++++
>  1 file changed, 110 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/ste,mcde.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/ste,mcde.txt b/Documentation/devicetree/bindings/display/ste,mcde.txt
> new file mode 100644
> index 000000000000..fc58aa5effb5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/ste,mcde.txt
> @@ -0,0 +1,110 @@
> +ST-Ericsson Multi Channel Display Engine MCDE
> +
> +The ST-Ericsson MCDE is a display controller with support for compositing
> +and displaying several channels memory resident graphics data on DSI or
> +LCD displays or bridges. It is used in the ST-Ericsson U8500 platform.
> +
> +Required properties:
> +
> +- compatible: must be:
> +  "ste,mcde"

Only one version? This too is old enough, I'm not too worried about how 
specific the compatibles are here.

> +- reg: register base for the main MCDE control registers, should be
> +  0x1000 in size
> +- interrupts: the interrupt line for the MCDE
> +- epod-supply: a phandle to the EPOD regulator
> +- vana-supply: a phandle to the analog voltage regulator
> +- clocks: an array of the MCDE clocks in this strict order:
> +  MCDECLK (main MCDE clock), LCDCLK (LCD clock), PLLDSI
> +  (HDMI clock), DSI0ESCLK (DSI0 energy save clock),
> +  DSI1ESCLK (DSI1 energy save clock), DSI2ESCLK (DSI2 energy
> +  save clock)
> +- clock-names: must be the following array:
> +  "mcde", "lcd", "hdmi", "dsi0", "dsi1", "dsi0es", "dsi1es", "dsi2es"

dsi2 clock?

Should the dsi clocks be in the child nodes?

> +  to match the required clock inputs above.
> +- #address-cells: should be <1> (for the DSI hosts that will be children)
> +- #size-cells: should be <1> (for the DSI hosts that will be children)
> +- ranges: this should always be stated
> +
> +Required subnodes:
> +
> +The devicetree must specify subnodes for the DSI host adapters.
> +These must have the following characteristics:
> +
> +- compatible: must be:
> +  "ste,mcde-dsi"
> +- reg: must specify the register range for the DSI host
> +- vana-supply: phandle to the VANA voltage regulator
> +- #address-cells: should be <1>
> +- #size-cells: should be <0>
> +
> +Display panels and bridges will appear as children on the DSI hosts, and
> +the displays are connected to the DSI hosts using the common binding
> +for video transmitter interfaces; see
> +Documentation/devicetree/bindings/media/video-interfaces.txt
> +
> +If a DSI host is unused (not connected) it will have no children or ports
> +defined.
> +
> +Example:
> +
> +mcde@a0350000 {
> +	compatible = "ste,mcde";
> +	reg = <0xa0350000 0x1000>;
> +	interrupts = <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>;
> +	epod-supply = <&db8500_b2r2_mcde_reg>;
> +	vana-supply = <&ab8500_ldo_ana_reg>;
> +	clocks = <&prcmu_clk PRCMU_MCDECLK>, /* Main MCDE clock */
> +		 <&prcmu_clk PRCMU_LCDCLK>, /* LCD clock */
> +		 <&prcmu_clk PRCMU_PLLDSI>, /* HDMI clock */
> +		 <&prcmu_clk PRCMU_DSI0CLK>, /* DSI 0 */
> +		 <&prcmu_clk PRCMU_DSI1CLK>, /* DSI 1 */
> +		 <&prcmu_clk PRCMU_DSI0ESCCLK>, /* TVout clock 0 */
> +		 <&prcmu_clk PRCMU_DSI1ESCCLK>, /* TVout clock 1 */
> +		 <&prcmu_clk PRCMU_DSI2ESCCLK>; /* TVout clock 2 */
> +	clock-names = "mcde", "lcd", "hdmi", "dsi0", "dsi1",
> +		    "dsi0es", "dsi1es", "dsi2es";
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	ranges;

A non-empty ranges is preferred, then you can just do offsets below.

> +
> +	dsi0: dsi@a0351000 {
> +		compatible = "ste,mcde-dsi";
> +		reg = <0xa0351000 0x1000>;
> +		vana-supply = <&ab8500_ldo_ana_reg>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		port {
> +			dsi0_out: endpoint {
> +				remote-endpoint = <&panel_in>;
> +			};
> +		};
> +
> +		panel: display {

'panel' would be the somewhat more standard node name.

> +			compatible = "samsung,s6d16d0";
> +			reg = <0>;
> +			vdd1-supply = <&ab8500_ldo_aux1_reg>;
> +			reset-gpios = <&gpio2 1 GPIO_ACTIVE_LOW>;
> +
> +			port {
> +				panel_in: endpoint {
> +					remote-endpoint = <&dsi0_out>;

You don't really need the graph here as it is already a child. Generally 
it is either or. The driver can support both ways though.

> +				};
> +			};
> +		};
> +
> +	};
> +	dsi1: dsi@a0352000 {
> +		compatible = "ste,mcde-dsi";
> +		reg = <0xa0352000 0x1000>;
> +		vana-supply = <&ab8500_ldo_ana_reg>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +	};
> +	dsi2: dsi@a0353000 {
> +		compatible = "ste,mcde-dsi";
> +		reg = <0xa0353000 0x1000>;
> +		vana-supply = <&ab8500_ldo_ana_reg>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +	};
> +};
> -- 
> 2.20.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel