[PATCH 00/20] drm: rcar-du: Add Color Management Module (CMM)

[PATCH 00/20] drm: rcar-du: Add Color Management Module (CMM)

[Jacopo Mondi]

Hello,
   this series add support for the Color Management Module (CMM) found on
the R-Car Display Unit output channels. CMM is present in most of the Gen3
SoCs (V3-H and V3-M excluded) and in Gen2 (which is not supported by the
series).

The CMM allows setting 1-D and 3-D gamma tables and generates histograms on
images before they are directed to the DU's output pads.

The current implementation only supports configuration of the 1-D gamma table,
by using the GAMMA_LUT KMS property. On top of this current version, more
advanced features such as 3-D LUT support and histogram generation can be
later implemented.

The series starts by adding the bindings definitions and by registering the
clock providers for the various SoCs that support the CMM. It then registers
CMMs in the SoC device tree sources and finally adds a driver for the CMM
itself and integrates it in the rcar-du driver infrastructure to register,
enable and update the CMM units.

Tested on M3-W Salvator-x on HDMI connector and on E3 Ebisu on VGA connector
by injecting a 'color inversion' gamma table using a patched version of
the KMS++ kmstest utility[1], and observing the CMM applies the correction
matrix properly.

Series based on Laurent's drm/du/next branch at
git://linuxtv.org/pinchartl/media.git

Thanks
   j

[1] https://github.com/tomba/kmsxx/

Jacopo Mondi (20):
  dt-bindings: display: renesas,cmm: Add R-Car CMM documentation
  dt-bindings: display, renesas,du: Document cmms property
  dt-bindings: display, renesas,du: Update 'vsps' in example
  clk: renesas: r8a7796: Add CMM clocks
  clk: renesas: r8a7795: Add CMM clocks
  clk: renesas: r8a77965: Add CMM clocks
  clk: renesas: r8a77990: Add CMM clocks
  clk: renesas: r8a77995: Add CMM clocks
  arm64: dts: renesas: r8a7796: Add CMM units
  arm64: dts: renesas: r8a7795: Add CMM units
  arm64: dts: renesas: r8a77965: Add CMM units
  arm64: dts: renesas: r8a77990: Add CMM units
  arm64: dts: renesas: r8a77995: Add CMM units
  drm: rcar-du: Add support for CMM
  drm: rcar-du: Claim CMM support for Gen3 SoCs
  drm: rcar-du: kms: Collect CMM instances
  drm: rcar-du: crtc: Enable and disable CMMs
  drm: rcar-du: group: Enable DU's CMM extension
  drm: rcar-du: crtc: Register GAMMA_LUT properties
  drm: rcar-du: kms: Update CMM in atomic commit tail

 .../bindings/display/renesas,cmm.txt          |  25 +++
 .../bindings/display/renesas,du.txt           |   7 +-
 arch/arm64/boot/dts/renesas/r8a7795.dtsi      |  36 +++-
 arch/arm64/boot/dts/renesas/r8a7796.dtsi      |  25 +++
 arch/arm64/boot/dts/renesas/r8a77965.dtsi     |  25 +++
 arch/arm64/boot/dts/renesas/r8a77990.dtsi     |  20 +-
 arch/arm64/boot/dts/renesas/r8a77995.dtsi     |  20 +-
 drivers/clk/renesas/r8a7795-cpg-mssr.c        |   4 +
 drivers/clk/renesas/r8a7796-cpg-mssr.c        |   3 +
 drivers/clk/renesas/r8a77965-cpg-mssr.c       |   3 +
 drivers/clk/renesas/r8a77990-cpg-mssr.c       |   2 +
 drivers/clk/renesas/r8a77995-cpg-mssr.c       |   2 +
 drivers/gpu/drm/rcar-du/Kconfig               |   7 +
 drivers/gpu/drm/rcar-du/Makefile              |   1 +
 drivers/gpu/drm/rcar-du/rcar_cmm.c            | 197 ++++++++++++++++++
 drivers/gpu/drm/rcar-du/rcar_cmm.h            |  38 ++++
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c        |  18 ++
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h        |   2 +
 drivers/gpu/drm/rcar-du/rcar_du_drv.c         |  12 +-
 drivers/gpu/drm/rcar-du/rcar_du_drv.h         |   4 +
 drivers/gpu/drm/rcar-du/rcar_du_group.c       |   8 +
 drivers/gpu/drm/rcar-du/rcar_du_group.h       |   2 +
 drivers/gpu/drm/rcar-du/rcar_du_kms.c         |  86 ++++++++
 drivers/gpu/drm/rcar-du/rcar_du_regs.h        |   5 +
 24 files changed, 544 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/renesas,cmm.txt
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.c
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.h

--
2.21.0

[PATCH 01/20] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation

[Jacopo Mondi]

Add device tree bindings documentation for the Renesas R-Car Display
Unit Color Management Module.

CMM is the image enhancement module available on each R-Car DU video
channel on Gen2 and Gen3 SoCs (V3H and V3M excluded).

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../bindings/display/renesas,cmm.txt          | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/renesas,cmm.txt

diff --git a/Documentation/devicetree/bindings/display/renesas,cmm.txt b/Documentation/devicetree/bindings/display/renesas,cmm.txt
new file mode 100644
index 000000000000..d8d3cf9ce2ce
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
@@ -0,0 +1,25 @@
+* Renesas R-Car Color Management Module (CMM)
+
+Renesas R-Car image enhancement module connected to R-Car DU video channels.
+
+Required properties:
+ - compatible: shall be one of:
+   - "renesas,cmm-gen3"
+   - "renesas,cmm-gen2"
+
+ - reg: the address base and length of the memory area where CMM control
+   registers are mapped to.
+
+ - clocks: phandle and clock-specifier pair to the CMM functional clock
+   supplier.
+
+Example:
+--------
+
+	cmm0: cmm@fea40000 {
+		compatible = "renesas,cmm";
+		reg = <0 0xfea40000 0 0x1000>;
+		power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
+		clocks = <&cpg CPG_MOD 711>;
+		resets = <&cpg 711>;
+	};
-- 
2.21.0

[PATCH 02/20] dt-bindings: display, renesas,du: Document cmms property

[Jacopo Mondi]

Document the newly added 'cmms' property which accepts a list of phandle
and channel index pairs that point to the CMM units available for each
Display Unit output video channel.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 Documentation/devicetree/bindings/display/renesas,du.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt b/Documentation/devicetree/bindings/display/renesas,du.txt
index aedb22b4d161..100114ef11d5 100644
--- a/Documentation/devicetree/bindings/display/renesas,du.txt
+++ b/Documentation/devicetree/bindings/display/renesas,du.txt
@@ -44,6 +44,10 @@ Required Properties:
     instance that serves the DU channel, and the channel index identifies the
     LIF instance in that VSP.
 
+  - cmms: A list of phandles to the CMM instances present in the SoC, one
+    for each available DU channel. The property shall not be specified for
+    SoCs that does not provide any CMM (such as V3M and V3H).
+
 Required nodes:
 
 The connections to the DU output video ports are modeled using the OF graph
@@ -89,6 +93,7 @@ Example: R8A7795 (R-Car H3) ES2.0 DU
 			 <&cpg CPG_MOD 721>;
 		clock-names = "du.0", "du.1", "du.2", "du.3";
 		vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd0 1>;
+		cmms = <&cmm0 &cmm1 &cmm2 &cmm3>;
 
 		ports {
 			#address-cells = <1>;
-- 
2.21.0

[PATCH 03/20] dt-bindings: display, renesas,du: Update 'vsps' in example

[Jacopo Mondi]

Update the 'vsps' property structure in the documentation example to
reflect what's actually implemented in the device tree sources.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 Documentation/devicetree/bindings/display/renesas,du.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt b/Documentation/devicetree/bindings/display/renesas,du.txt
index 100114ef11d5..262047053d31 100644
--- a/Documentation/devicetree/bindings/display/renesas,du.txt
+++ b/Documentation/devicetree/bindings/display/renesas,du.txt
@@ -92,7 +92,7 @@ Example: R8A7795 (R-Car H3) ES2.0 DU
 			 <&cpg CPG_MOD 722>,
 			 <&cpg CPG_MOD 721>;
 		clock-names = "du.0", "du.1", "du.2", "du.3";
-		vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd0 1>;
+		vsps = <&vspd0 0 &vspd1 0 &vspd2  &vspd0 1>;
 		cmms = <&cmm0 &cmm1 &cmm2 &cmm3>;
 
 		ports {
-- 
2.21.0

[PATCH 04/20] clk: renesas: r8a7796: Add CMM clocks

[Jacopo Mondi]

Add clock definitions for CMM units on Renesas R-Car Gen3 M3-W.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/clk/renesas/r8a7796-cpg-mssr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/renesas/r8a7796-cpg-mssr.c b/drivers/clk/renesas/r8a7796-cpg-mssr.c
index 12c455859f2c..6044aeda0f83 100644
--- a/drivers/clk/renesas/r8a7796-cpg-mssr.c
+++ b/drivers/clk/renesas/r8a7796-cpg-mssr.c
@@ -179,6 +179,9 @@ static const struct mssr_mod_clk r8a7796_mod_clks[] __initconst = {
 	DEF_MOD("ehci1",		 702,	R8A7796_CLK_S3D4),
 	DEF_MOD("ehci0",		 703,	R8A7796_CLK_S3D4),
 	DEF_MOD("hsusb",		 704,	R8A7796_CLK_S3D4),
+	DEF_MOD("cmm2",			 709,	R8A7796_CLK_S2D1),
+	DEF_MOD("cmm1",			 710,	R8A7796_CLK_S2D1),
+	DEF_MOD("cmm0",			 711,	R8A7796_CLK_S2D1),
 	DEF_MOD("csi20",		 714,	R8A7796_CLK_CSI0),
 	DEF_MOD("csi40",		 716,	R8A7796_CLK_CSI0),
 	DEF_MOD("du2",			 722,	R8A7796_CLK_S2D1),
-- 
2.21.0

[PATCH 05/20] clk: renesas: r8a7795: Add CMM clocks

[Jacopo Mondi]

Add clock definitions for CMM units on Renesas R-Car Gen3 H3.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/clk/renesas/r8a7795-cpg-mssr.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/renesas/r8a7795-cpg-mssr.c b/drivers/clk/renesas/r8a7795-cpg-mssr.c
index 86842c9fd314..e8f1d5ec0455 100644
--- a/drivers/clk/renesas/r8a7795-cpg-mssr.c
+++ b/drivers/clk/renesas/r8a7795-cpg-mssr.c
@@ -200,6 +200,10 @@ static struct mssr_mod_clk r8a7795_mod_clks[] __initdata = {
 	DEF_MOD("ehci0",		 703,	R8A7795_CLK_S3D4),
 	DEF_MOD("hsusb",		 704,	R8A7795_CLK_S3D4),
 	DEF_MOD("hsusb3",		 705,	R8A7795_CLK_S3D4),
+	DEF_MOD("cmm3",			 708,	R8A7795_CLK_S2D1),
+	DEF_MOD("cmm2",			 709,	R8A7795_CLK_S2D1),
+	DEF_MOD("cmm1",			 710,	R8A7795_CLK_S2D1),
+	DEF_MOD("cmm0",			 711,	R8A7795_CLK_S2D1),
 	DEF_MOD("csi21",		 713,	R8A7795_CLK_CSI0), /* ES1.x */
 	DEF_MOD("csi20",		 714,	R8A7795_CLK_CSI0),
 	DEF_MOD("csi41",		 715,	R8A7795_CLK_CSI0),
-- 
2.21.0

[PATCH 06/20] clk: renesas: r8a77965: Add CMM clocks

[Jacopo Mondi]

Add clock definitions for CMM units on Renesas R-Car Gen3 M3-N.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/clk/renesas/r8a77965-cpg-mssr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/renesas/r8a77965-cpg-mssr.c b/drivers/clk/renesas/r8a77965-cpg-mssr.c
index eb1cca58a1e1..58f75b03c8bb 100644
--- a/drivers/clk/renesas/r8a77965-cpg-mssr.c
+++ b/drivers/clk/renesas/r8a77965-cpg-mssr.c
@@ -178,6 +178,9 @@ static const struct mssr_mod_clk r8a77965_mod_clks[] __initconst = {
 	DEF_MOD("ehci1",		702,	R8A77965_CLK_S3D4),
 	DEF_MOD("ehci0",		703,	R8A77965_CLK_S3D4),
 	DEF_MOD("hsusb",		704,	R8A77965_CLK_S3D4),
+	DEF_MOD("cmm3",			708,	R8A77965_CLK_S2D1),
+	DEF_MOD("cmm1",			710,	R8A77965_CLK_S2D1),
+	DEF_MOD("cmm0",			711,	R8A77965_CLK_S2D1),
 	DEF_MOD("csi20",		714,	R8A77965_CLK_CSI0),
 	DEF_MOD("csi40",		716,	R8A77965_CLK_CSI0),
 	DEF_MOD("du3",			721,	R8A77965_CLK_S2D1),
-- 
2.21.0

[PATCH 07/20] clk: renesas: r8a77990: Add CMM clocks

[Jacopo Mondi]

Add clock definitions for CMM units on Renesas R-Car Gen3 E3.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/clk/renesas/r8a77990-cpg-mssr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/renesas/r8a77990-cpg-mssr.c b/drivers/clk/renesas/r8a77990-cpg-mssr.c
index 9a278c75c918..8cdd0e6fb74f 100644
--- a/drivers/clk/renesas/r8a77990-cpg-mssr.c
+++ b/drivers/clk/renesas/r8a77990-cpg-mssr.c
@@ -182,6 +182,8 @@ static const struct mssr_mod_clk r8a77990_mod_clks[] __initconst = {
 
 	DEF_MOD("ehci0",		 703,	R8A77990_CLK_S3D4),
 	DEF_MOD("hsusb",		 704,	R8A77990_CLK_S3D4),
+	DEF_MOD("cmm1",			 710,	R8A77990_CLK_S1D1),
+	DEF_MOD("cmm0",			 711,	R8A77990_CLK_S1D1),
 	DEF_MOD("csi40",		 716,	R8A77990_CLK_CSI0),
 	DEF_MOD("du1",			 723,	R8A77990_CLK_S1D1),
 	DEF_MOD("du0",			 724,	R8A77990_CLK_S1D1),
-- 
2.21.0

[PATCH 08/20] clk: renesas: r8a77995: Add CMM clocks

[Jacopo Mondi]

Add clock definitions for CMM units on Renesas R-Car Gen3 D3.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/clk/renesas/r8a77995-cpg-mssr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/renesas/r8a77995-cpg-mssr.c b/drivers/clk/renesas/r8a77995-cpg-mssr.c
index eee3874865a9..acd5fabb785a 100644
--- a/drivers/clk/renesas/r8a77995-cpg-mssr.c
+++ b/drivers/clk/renesas/r8a77995-cpg-mssr.c
@@ -146,6 +146,8 @@ static const struct mssr_mod_clk r8a77995_mod_clks[] __initconst = {
 	DEF_MOD("vspbs",		 627,	R8A77995_CLK_S0D1),
 	DEF_MOD("ehci0",		 703,	R8A77995_CLK_S3D2),
 	DEF_MOD("hsusb",		 704,	R8A77995_CLK_S3D2),
+	DEF_MOD("cmm1",			 710,	R8A77995_CLK_S1D1),
+	DEF_MOD("cmm0",			 711,	R8A77995_CLK_S1D1),
 	DEF_MOD("du1",			 723,	R8A77995_CLK_S1D1),
 	DEF_MOD("du0",			 724,	R8A77995_CLK_S1D1),
 	DEF_MOD("lvds",			 727,	R8A77995_CLK_S2D1),
-- 
2.21.0

[PATCH 09/20] arm64: dts: renesas: r8a7796: Add CMM units

[Jacopo Mondi]

Add CMM units to Renesas R-Car M3-W device tree and reference them from
the Display Unit they are connected to.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm64/boot/dts/renesas/r8a7796.dtsi | 25 ++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
index cdf784899cf8..2e1891d9d322 100644
--- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
@@ -2610,6 +2610,30 @@
 			renesas,fcp = <&fcpvi0>;
 		};
 
+		cmm0: cmm@fea40000 {
+			compatible = "renesas,cmm-gen3";
+			reg = <0 0xfea40000 0 0x1000>;
+			clocks = <&cpg CPG_MOD 711>;
+			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
+			resets = <&cpg 711>;
+		};
+
+		cmm1: cmm@fea50000 {
+			compatible = "renesas,cmm-gen3";
+			reg = <0 0xfea50000 0 0x1000>;
+			clocks = <&cpg CPG_MOD 710>;
+			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
+			resets = <&cpg 710>;
+		};
+
+		cmm2: cmm@fea60000 {
+			compatible = "renesas,cmm-gen3";
+			reg = <0 0xfea60000 0 0x1000>;
+			clocks = <&cpg CPG_MOD 709>;
+			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
+			resets = <&cpg 709>;
+		};
+
 		csi20: csi2@fea80000 {
 			compatible = "renesas,r8a7796-csi2";
 			reg = <0 0xfea80000 0 0x10000>;
@@ -2763,6 +2787,7 @@
 			status = "disabled";
 
 			vsps = <&vspd0 &vspd1 &vspd2>;
+			cmms = <&cmm0 &cmm1 &cmm2>;
 
 			ports {
 				#address-cells = <1>;
-- 
2.21.0

[PATCH 10/20] arm64: dts: renesas: r8a7795: Add CMM units

[Jacopo Mondi]

Add CMM units to Renesas R-Car H3 device tree and reference them from
the Display Unit they are connected to.

While at it, re-sort the du device node properties to match the ordering
found in other SoCs.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm64/boot/dts/renesas/r8a7795.dtsi | 36 +++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
index abeac3059383..83edd9f12caa 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
@@ -2825,6 +2825,38 @@
 			renesas,fcp = <&fcpvi1>;
 		};
 
+		cmm0: cmm@fea40000 {
+			compatible = "renesas,cmm-gen3";
+			reg = <0 0xfea40000 0 0x1000>;
+			clocks = <&cpg CPG_MOD 711>;
+			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
+			resets = <&cpg 711>;
+		};
+
+		cmm1: cmm@fea50000 {
+			compatible = "renesas,cmm-gen3";
+			reg = <0 0xfea50000 0 0x1000>;
+			clocks = <&cpg CPG_MOD 710>;
+			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
+			resets = <&cpg 710>;
+		};
+
+		cmm2: cmm@fea60000 {
+			compatible = "renesas,cmm-gen3";
+			reg = <0 0xfea60000 0 0x1000>;
+			clocks = <&cpg CPG_MOD 709>;
+			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
+			resets = <&cpg 709>;
+		};
+
+		cmm3: cmm@fea70000 {
+			compatible = "renesas,cmm-gen3";
+			reg = <0 0xfea70000 0 0x1000>;
+			clocks = <&cpg CPG_MOD 708>;
+			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
+			resets = <&cpg 708>;
+		};
+
 		csi20: csi2@fea80000 {
 			compatible = "renesas,r8a7795-csi2";
 			reg = <0 0xfea80000 0 0x10000>;
@@ -3028,9 +3060,11 @@
 				 <&cpg CPG_MOD 722>,
 				 <&cpg CPG_MOD 721>;
 			clock-names = "du.0", "du.1", "du.2", "du.3";
-			vsps = <&vspd0 0 &vspd1 0 &vspd2 0 &vspd0 1>;
 			status = "disabled";
 
+			vsps = <&vspd0 0 &vspd1 0 &vspd2 0 &vspd0 1>;
+			cmms = <&cmm0 &cmm1 &cmm2 &cmm3>;
+
 			ports {
 				#address-cells = <1>;
 				#size-cells = <0>;
-- 
2.21.0

[PATCH 11/20] arm64: dts: renesas: r8a77965: Add CMM units

[Jacopo Mondi]

Add CMM units to Renesas R-Car M3-N device tree and reference them from
the Display Unit they are connected to.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm64/boot/dts/renesas/r8a77965.dtsi | 25 +++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a77965.dtsi b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
index 9763d108e183..a90854685b33 100644
--- a/arch/arm64/boot/dts/renesas/r8a77965.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
@@ -2026,6 +2026,30 @@
 			resets = <&cpg 602>;
 		};
 
+		cmm0: cmm@fea40000 {
+			compatible = "renesas,cmm-gen3";
+			reg = <0 0xfea40000 0 0x1000>;
+			clocks = <&cpg CPG_MOD 711>;
+			power-domains = <&sysc R8A77965_PD_ALWAYS_ON>;
+			resets = <&cpg 711>;
+		};
+
+		cmm1: cmm@fea50000 {
+			compatible = "renesas,cmm-gen3";
+			reg = <0 0xfea50000 0 0x1000>;
+			clocks = <&cpg CPG_MOD 710>;
+			power-domains = <&sysc R8A77965_PD_ALWAYS_ON>;
+			resets = <&cpg 710>;
+		};
+
+		cmm3: cmm@fea70000 {
+			compatible = "renesas,cmm-gen3";
+			reg = <0 0xfea70000 0 0x1000>;
+			clocks = <&cpg CPG_MOD 708>;
+			power-domains = <&sysc R8A77965_PD_ALWAYS_ON>;
+			resets = <&cpg 708>;
+		};
+
 		csi20: csi2@fea80000 {
 			compatible = "renesas,r8a77965-csi2";
 			reg = <0 0xfea80000 0 0x10000>;
@@ -2177,6 +2201,7 @@
 			status = "disabled";
 
 			vsps = <&vspd0 0 &vspd1 0 &vspd0 1>;
+			cmms = <&cmm0 &cmm1 &cmm3>;
 
 			ports {
 				#address-cells = <1>;
-- 
2.21.0

[PATCH 12/20] arm64: dts: renesas: r8a77990: Add CMM units

[Jacopo Mondi]

Add CMM units to Renesas R-Car E3 device tree and reference them from
the Display Unit they are connected to.

While at it, re-sort the du device node properties to match the ordering
found in other SoCs.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm64/boot/dts/renesas/r8a77990.dtsi | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/renesas/r8a77990.dtsi b/arch/arm64/boot/dts/renesas/r8a77990.dtsi
index a69faa60ea4d..87453ddbc7e3 100644
--- a/arch/arm64/boot/dts/renesas/r8a77990.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a77990.dtsi
@@ -1656,6 +1656,22 @@
 			iommus = <&ipmmu_vi0 9>;
 		};
 
+		cmm0: cmm@fea40000 {
+			compatible = "renesas,cmm-gen3";
+			reg = <0 0xfea40000 0 0x1000>;
+			clocks = <&cpg CPG_MOD 711>;
+			power-domains = <&sysc R8A77990_PD_ALWAYS_ON>;
+			resets = <&cpg 711>;
+		};
+
+		cmm1: cmm@fea50000 {
+			compatible = "renesas,cmm-gen3";
+			reg = <0 0xfea50000 0 0x1000>;
+			clocks = <&cpg CPG_MOD 710>;
+			power-domains = <&sysc R8A77990_PD_ALWAYS_ON>;
+			resets = <&cpg 710>;
+		};
+
 		csi40: csi2@feaa0000 {
 			compatible = "renesas,r8a77990-csi2", "renesas,rcar-gen3-csi2";
 			reg = <0 0xfeaa0000 0 0x10000>;
@@ -1695,9 +1711,11 @@
 			clocks = <&cpg CPG_MOD 724>,
 				 <&cpg CPG_MOD 723>;
 			clock-names = "du.0", "du.1";
-			vsps = <&vspd0 0 &vspd1 0>;
 			status = "disabled";
 
+			vsps = <&vspd0 0 &vspd1 0>;
+			cmms = <&cmm0 &cmm1>;
+
 			ports {
 				#address-cells = <1>;
 				#size-cells = <0>;
-- 
2.21.0

[PATCH 13/20] arm64: dts: renesas: r8a77995: Add CMM units

[Jacopo Mondi]

Add CMM units to Renesas R-Car D3 device tree and reference them from
the Display Unit they are connected to.

While at it, re-sort the du device node properties to match the ordering
found in other SoCs.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm64/boot/dts/renesas/r8a77995.dtsi | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/renesas/r8a77995.dtsi b/arch/arm64/boot/dts/renesas/r8a77995.dtsi
index 5bf3af246e14..c52d73068304 100644
--- a/arch/arm64/boot/dts/renesas/r8a77995.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a77995.dtsi
@@ -993,6 +993,22 @@
 			iommus = <&ipmmu_vi0 9>;
 		};
 
+		cmm0: cmm@fea40000 {
+			compatible = "renesas,cmm-gen3";
+			reg = <0 0xfea40000 0 0x1000>;
+			clocks = <&cpg CPG_MOD 711>;
+			power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
+			resets = <&cpg 711>;
+		};
+
+		cmm1: cmm@fea50000 {
+			compatible = "renesas,cmm-gen3";
+			reg = <0 0xfea50000 0 0x1000>;
+			clocks = <&cpg CPG_MOD 710>;
+			power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
+			resets = <&cpg 710>;
+		};
+
 		du: display@feb00000 {
 			compatible = "renesas,du-r8a77995";
 			reg = <0 0xfeb00000 0 0x80000>;
@@ -1001,9 +1017,11 @@
 			clocks = <&cpg CPG_MOD 724>,
 				 <&cpg CPG_MOD 723>;
 			clock-names = "du.0", "du.1";
-			vsps = <&vspd0 0 &vspd1 0>;
 			status = "disabled";
 
+			vsps = <&vspd0 0 &vspd1 0>;
+			cmms = <&cmm0 &cmm1>;
+
 			ports {
 				#address-cells = <1>;
 				#size-cells = <0>;
-- 
2.21.0

[PATCH 14/20] drm: rcar-du: Add support for CMM

[Jacopo Mondi]

Add a driver for the R-Car Display Unit Color Correction Module.

Each DU output channel is provided with a CMM unit to perform image
enhancement and color correction.

Add support for CMM through a driver that supports configuration of
the 1-dimensional LUT table. More advanced CMM feature could be
implemented on top of this basic one.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/rcar-du/Kconfig    |   7 +
 drivers/gpu/drm/rcar-du/Makefile   |   1 +
 drivers/gpu/drm/rcar-du/rcar_cmm.c | 197 +++++++++++++++++++++++++++++
 drivers/gpu/drm/rcar-du/rcar_cmm.h |  38 ++++++
 4 files changed, 243 insertions(+)
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.c
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.h

diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
index 1529849e217e..539d232790d1 100644
--- a/drivers/gpu/drm/rcar-du/Kconfig
+++ b/drivers/gpu/drm/rcar-du/Kconfig
@@ -13,6 +13,13 @@ config DRM_RCAR_DU
 	  Choose this option if you have an R-Car chipset.
 	  If M is selected the module will be called rcar-du-drm.
 
+config DRM_RCAR_CMM
+	bool "R-Car DU Color Management Module (CMM) Support"
+	depends on DRM && OF
+	depends on DRM_RCAR_DU
+	help
+	  Enable support for R-Car Color Management Module (CMM).
+
 config DRM_RCAR_DW_HDMI
 	tristate "R-Car DU Gen3 HDMI Encoder Support"
 	depends on DRM && OF
diff --git a/drivers/gpu/drm/rcar-du/Makefile b/drivers/gpu/drm/rcar-du/Makefile
index 6c2ed9c46467..4d1187ccc3e5 100644
--- a/drivers/gpu/drm/rcar-du/Makefile
+++ b/drivers/gpu/drm/rcar-du/Makefile
@@ -15,6 +15,7 @@ rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS)	+= rcar_du_of.o \
 rcar-du-drm-$(CONFIG_DRM_RCAR_VSP)	+= rcar_du_vsp.o
 rcar-du-drm-$(CONFIG_DRM_RCAR_WRITEBACK) += rcar_du_writeback.o
 
+obj-$(CONFIG_DRM_RCAR_CMM)		+= rcar_cmm.o
 obj-$(CONFIG_DRM_RCAR_DU)		+= rcar-du-drm.o
 obj-$(CONFIG_DRM_RCAR_DW_HDMI)		+= rcar_dw_hdmi.o
 obj-$(CONFIG_DRM_RCAR_LVDS)		+= rcar_lvds.o
diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c
new file mode 100644
index 000000000000..5d9d917b91f4
--- /dev/null
+++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c
@@ -0,0 +1,197 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * rcar_cmm.c -- R-Car Display Unit Color Management Module
+ *
+ * Copyright (C) 2019 Jacopo Mondi <jacopo+renesas@jmondi.org>
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#include <drm/drm_atomic.h>
+
+#include "rcar_cmm.h"
+
+#define CM2_LUT_CTRL		0x00
+#define CM2_LUT_CTRL_EN		BIT(0)
+#define CM2_LUT_TBLA		0x600
+
+struct rcar_cmm {
+	struct clk *clk;
+	void __iomem *base;
+	bool enabled;
+
+	/* LUT table scratch buffer. */
+	struct {
+		bool restore;
+		unsigned int size;
+		uint32_t table[CMM_GAMMA_LUT_SIZE];
+	} lut;
+};
+
+static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg)
+{
+	return ioread32(rcmm->base + reg);
+}
+
+static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data)
+{
+	iowrite32(data, rcmm->base + reg);
+}
+
+int rcar_cmm_setup(struct platform_device *pdev, struct rcar_cmm_config *config)
+{
+	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
+	unsigned int i;
+
+	if (config->lut.size > CMM_GAMMA_LUT_SIZE)
+		return -EINVAL;
+
+	/*
+	 * As cmm_setup is called by atomic commit tail helper, it might be
+	 * called before the enabling the CRTC (which calls cmm_enable()).
+	 *
+	 * Store the LUT table entries in the scratch buffer to be later
+	 * programmed at enable time.
+	 */
+	if (!rcmm->enabled) {
+		if (!config->lut.enable)
+			return 0;
+
+		for (i = 0; i < config->lut.size; ++i) {
+			struct drm_color_lut *lut = &config->lut.table[i];
+
+			rcmm->lut.table[i] = (lut->red & 0xff) << 16 |
+					     (lut->green & 0xff) << 8 |
+					     (lut->blue & 0xff);
+		}
+
+		rcmm->lut.restore = true;
+		rcmm->lut.size = config->lut.size;
+
+		return 0;
+	}
+
+	if (rcar_cmm_read(rcmm, CM2_LUT_CTRL) & CM2_LUT_CTRL_EN &&
+	    !config->lut.enable) {
+		rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
+		return 0;
+	}
+
+	/* Enable LUT and program the new gamma table values. */
+	rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
+	for (i = 0; i < config->lut.size; ++i) {
+		struct drm_color_lut *lut = &config->lut.table[i];
+		u32 val = (lut->red & 0xff) << 16 | (lut->green & 0xff) << 8 |
+			  (lut->blue & 0xff);
+
+		rcar_cmm_write(rcmm, CM2_LUT_TBLA + i * 4, val);
+	}
+
+	return 0;
+}
+
+int rcar_cmm_enable(struct platform_device *pdev)
+{
+	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
+	unsigned int i;
+	int ret;
+
+	if (rcmm->enabled)
+		return 0;
+
+	ret = clk_prepare_enable(rcmm->clk);
+	if (ret)
+		return ret;
+
+	/* Apply the LUT table values saved at cmm_setup time. */
+	if (rcmm->lut.restore) {
+		rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
+		for (i = 0; i < rcmm->lut.size; ++i)
+			rcar_cmm_write(rcmm, CM2_LUT_TBLA + i * 4,
+				       rcmm->lut.table[i]);
+
+		rcmm->lut.restore = false;
+		rcmm->lut.size = 0;
+	}
+
+	rcmm->enabled = true;
+
+	return 0;
+}
+
+void rcar_cmm_disable(struct platform_device *pdev)
+{
+	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
+
+	rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
+
+	clk_disable_unprepare(rcmm->clk);
+
+	rcmm->lut.restore = false;
+	rcmm->lut.size = 0;
+	rcmm->enabled = false;
+}
+
+static int rcar_cmm_probe(struct platform_device *pdev)
+{
+	struct rcar_cmm *rcmm;
+	struct resource *res;
+	resource_size_t size;
+
+	rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
+	if (!rcmm)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, rcmm);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	size = resource_size(res);
+	if (!devm_request_mem_region(&pdev->dev, res->start, size,
+				     dev_name(&pdev->dev))) {
+		dev_err(&pdev->dev,
+			"can't request region for resource %pR\n", res);
+		return -EBUSY;
+	}
+
+	rcmm->base = devm_ioremap_nocache(&pdev->dev, res->start, size);
+	if (IS_ERR(rcmm->base))
+		return PTR_ERR(rcmm->base);
+
+	rcmm->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(rcmm->clk)) {
+		dev_err(&pdev->dev, "Failed to get CMM clock");
+		return PTR_ERR(rcmm->clk);
+	}
+
+	rcmm->lut.restore = false;
+	rcmm->lut.size = 0;
+	rcmm->enabled = false;
+
+	return 0;
+}
+
+static const struct of_device_id rcar_cmm_of_table[] = {
+	{ .compatible = "renesas,cmm-gen3" },
+	{ .compatible = "renesas,cmm-gen2" },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
+
+static struct platform_driver rcar_cmm_platform_driver = {
+	.probe		= rcar_cmm_probe,
+	.driver		= {
+		.name	= "rcar-cmm",
+		.of_match_table = rcar_cmm_of_table,
+	},
+};
+
+module_platform_driver(rcar_cmm_platform_driver);
+
+MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org");
+MODULE_DESCRIPTION("Renesas R-Car CMM Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h
new file mode 100644
index 000000000000..da61a145dc5c
--- /dev/null
+++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * rcar_cmm.h -- R-Car Display Unit Color Management Module
+ *
+ * Copyright (C) 2019 Jacopo Mondi <jacopo+renesas@jmondi.org>
+ */
+
+#ifndef __RCAR_CMM_H__
+#define __RCAR_CMM_H__
+
+#include <linux/platform_device.h>
+
+#define CMM_GAMMA_LUT_SIZE		256
+
+struct drm_color_lut;
+
+/**
+ * struct rcar_cmm_config - CMM configuration
+ *
+ * @lut:	1D-LUT configuration
+ * @lut.enable:	1D-LUT enable flag
+ * @lut.table:	1D-LUT table entries.
+ * @lut.size	1D-LUT number of entries. Max is 256
+ */
+struct rcar_cmm_config {
+	struct {
+		bool enable;
+		struct drm_color_lut *table;
+		unsigned int size;
+	} lut;
+};
+
+int rcar_cmm_enable(struct platform_device *);
+void rcar_cmm_disable(struct platform_device *);
+
+int rcar_cmm_setup(struct platform_device *, struct rcar_cmm_config *);
+
+#endif /* __RCAR_CMM_H__ */
-- 
2.21.0

[PATCH 15/20] drm: rcar-du: Claim CMM support for Gen3 SoCs

[Jacopo Mondi]

Add CMM to the list of supported features for Gen3 SoCs that provide it:
- R8A7795
- R8A7796
- R8A77965
- R8A7799x

Leave R8A77970 out as V3M and V3H are the only Gen3 SoCs that do not
support CMM.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/rcar-du/rcar_du_drv.c | 12 ++++++++----
 drivers/gpu/drm/rcar-du/rcar_du_drv.h |  1 +
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index 75ab17af13a9..1e69cfa11798 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -247,7 +247,8 @@ static const struct rcar_du_device_info rcar_du_r8a7795_info = {
 	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
 		  | RCAR_DU_FEATURE_VSP1_SOURCE
 		  | RCAR_DU_FEATURE_INTERLACED
-		  | RCAR_DU_FEATURE_TVM_SYNC,
+		  | RCAR_DU_FEATURE_TVM_SYNC
+		  | RCAR_DU_FEATURE_CMM,
 	.channels_mask = BIT(3) | BIT(2) | BIT(1) | BIT(0),
 	.routes = {
 		/*
@@ -280,7 +281,8 @@ static const struct rcar_du_device_info rcar_du_r8a7796_info = {
 	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
 		  | RCAR_DU_FEATURE_VSP1_SOURCE
 		  | RCAR_DU_FEATURE_INTERLACED
-		  | RCAR_DU_FEATURE_TVM_SYNC,
+		  | RCAR_DU_FEATURE_TVM_SYNC
+		  | RCAR_DU_FEATURE_CMM,
 	.channels_mask = BIT(2) | BIT(1) | BIT(0),
 	.routes = {
 		/*
@@ -309,7 +311,8 @@ static const struct rcar_du_device_info rcar_du_r8a77965_info = {
 	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
 		  | RCAR_DU_FEATURE_VSP1_SOURCE
 		  | RCAR_DU_FEATURE_INTERLACED
-		  | RCAR_DU_FEATURE_TVM_SYNC,
+		  | RCAR_DU_FEATURE_TVM_SYNC
+		  | RCAR_DU_FEATURE_CMM,
 	.channels_mask = BIT(3) | BIT(1) | BIT(0),
 	.routes = {
 		/*
@@ -357,7 +360,8 @@ static const struct rcar_du_device_info rcar_du_r8a77970_info = {
 static const struct rcar_du_device_info rcar_du_r8a7799x_info = {
 	.gen = 3,
 	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
-		  | RCAR_DU_FEATURE_VSP1_SOURCE,
+		  | RCAR_DU_FEATURE_VSP1_SOURCE
+		  | RCAR_DU_FEATURE_CMM,
 	.channels_mask = BIT(1) | BIT(0),
 	.routes = {
 		/*
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
index 1327cd0df90a..a00dccc447aa 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
@@ -28,6 +28,7 @@ struct rcar_du_encoder;
 #define RCAR_DU_FEATURE_VSP1_SOURCE	BIT(1)	/* Has inputs from VSP1 */
 #define RCAR_DU_FEATURE_INTERLACED	BIT(2)	/* HW supports interlaced */
 #define RCAR_DU_FEATURE_TVM_SYNC	BIT(3)	/* Has TV switch/sync modes */
+#define RCAR_DU_FEATURE_CMM		BIT(4)	/* Has CMM */
 
 #define RCAR_DU_QUIRK_ALIGN_128B	BIT(0)	/* Align pitches to 128 bytes */
 
-- 
2.21.0

[PATCH 16/20] drm: rcar-du: kms: Collect CMM instances

[Jacopo Mondi]

Implement device tree parsing to collect the available CMM instances
described by the 'cmms' property. Associate CMMs with CRTCs and store a
mask of active CMMs in the DU group for later enablement.

Also define a new feature to let each SoC claim support for CMM.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c  |  7 ++++
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h  |  2 +
 drivers/gpu/drm/rcar-du/rcar_du_drv.h   |  3 ++
 drivers/gpu/drm/rcar-du/rcar_du_group.h |  2 +
 drivers/gpu/drm/rcar-du/rcar_du_kms.c   | 50 +++++++++++++++++++++++++
 5 files changed, 64 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 2da46e3dc4ae..9f270a54b164 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -1194,6 +1194,13 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
 	if (ret < 0)
 		return ret;
 
+	/* CMM might be disabled for this CRTC. */
+	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM) &&
+	    rcdu->cmms[swindex]) {
+		rcrtc->cmm = rcdu->cmms[swindex];
+		rgrp->cmms_mask |= BIT(hwindex % 2);
+	}
+
 	drm_crtc_helper_add(crtc, &crtc_helper_funcs);
 
 	/* Start with vertical blanking interrupt reporting disabled. */
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
index 3b7fc668996f..5f2940c42225 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -39,6 +39,7 @@ struct rcar_du_vsp;
  * @vblank_wait: wait queue used to signal vertical blanking
  * @vblank_count: number of vertical blanking interrupts to wait for
  * @group: CRTC group this CRTC belongs to
+ * @cmm: CMM associated with this CRTC
  * @vsp: VSP feeding video to this CRTC
  * @vsp_pipe: index of the VSP pipeline feeding video to this CRTC
  * @writeback: the writeback connector
@@ -64,6 +65,7 @@ struct rcar_du_crtc {
 	unsigned int vblank_count;
 
 	struct rcar_du_group *group;
+	struct platform_device *cmm;
 	struct rcar_du_vsp *vsp;
 	unsigned int vsp_pipe;
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
index a00dccc447aa..300ec60ba31b 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
@@ -13,6 +13,7 @@
 #include <linux/kernel.h>
 #include <linux/wait.h>
 
+#include "rcar_cmm.h"
 #include "rcar_du_crtc.h"
 #include "rcar_du_group.h"
 #include "rcar_du_vsp.h"
@@ -70,6 +71,7 @@ struct rcar_du_device_info {
 
 #define RCAR_DU_MAX_CRTCS		4
 #define RCAR_DU_MAX_GROUPS		DIV_ROUND_UP(RCAR_DU_MAX_CRTCS, 2)
+#define RCAR_DU_MAX_CMMS		4
 #define RCAR_DU_MAX_VSPS		4
 
 struct rcar_du_device {
@@ -86,6 +88,7 @@ struct rcar_du_device {
 	struct rcar_du_encoder *encoders[RCAR_DU_OUTPUT_MAX];
 
 	struct rcar_du_group groups[RCAR_DU_MAX_GROUPS];
+	struct platform_device *cmms[RCAR_DU_MAX_CMMS];
 	struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS];
 
 	struct {
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h b/drivers/gpu/drm/rcar-du/rcar_du_group.h
index 87950c1f6a52..b0c1466593a3 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h
@@ -22,6 +22,7 @@ struct rcar_du_device;
  * @mmio_offset: registers offset in the device memory map
  * @index: group index
  * @channels_mask: bitmask of populated DU channels in this group
+ * @cmms_mask: bitmask of enabled CMMs in this group
  * @num_crtcs: number of CRTCs in this group (1 or 2)
  * @use_count: number of users of the group (rcar_du_group_(get|put))
  * @used_crtcs: number of CRTCs currently in use
@@ -37,6 +38,7 @@ struct rcar_du_group {
 	unsigned int index;
 
 	unsigned int channels_mask;
+	unsigned int cmms_mask;
 	unsigned int num_crtcs;
 	unsigned int use_count;
 	unsigned int used_crtcs;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index adbc4f5d8fc5..5a910a04e1d9 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -18,6 +18,7 @@
 #include <drm/drm_vblank.h>
 
 #include <linux/of_graph.h>
+#include <linux/of_platform.h>
 #include <linux/wait.h>
 
 #include "rcar_du_crtc.h"
@@ -614,6 +615,48 @@ static int rcar_du_vsps_init(struct rcar_du_device *rcdu)
 	return ret;
 }
 
+static int rcar_du_cmm_init(struct rcar_du_device *rcdu)
+{
+	const struct device_node *np = rcdu->dev->of_node;
+	unsigned int cells;
+	unsigned int i;
+
+	cells = of_property_count_u32_elems(np, "cmms");
+	if (cells > RCAR_DU_MAX_CMMS || cells > rcdu->num_crtcs) {
+		dev_err(rcdu->dev, "invalid 'cmms' property format\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < cells; ++i) {
+		struct platform_device *pdev;
+		struct device_node *cmm;
+
+		cmm = of_parse_phandle(np, "cmms", i);
+		if (IS_ERR(cmm)) {
+			dev_err(rcdu->dev, "failed to parse 'cmms' property\n");
+			return PTR_ERR(cmm);
+		}
+
+		pdev = of_find_device_by_node(cmm);
+		if (IS_ERR(pdev)) {
+			dev_err(rcdu->dev, "invalid property 'cmms'[%u]\n", i);
+			of_node_put(cmm);
+			return PTR_ERR(pdev);
+		}
+
+		if (!of_device_is_available(cmm)) {
+			/* It's fine to have a phandle to a non-enabled CMM. */
+			of_node_put(cmm);
+			continue;
+		}
+
+		of_node_put(cmm);
+		rcdu->cmms[i] = pdev;
+	}
+
+	return 0;
+}
+
 int rcar_du_modeset_init(struct rcar_du_device *rcdu)
 {
 	static const unsigned int mmio_offsets[] = {
@@ -704,6 +747,13 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
 			return ret;
 	}
 
+	/* Initialize the Color Management Modules. */
+	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM)) {
+		ret = rcar_du_cmm_init(rcdu);
+		if (ret < 0)
+			return ret;
+	}
+
 	/* Create the CRTCs. */
 	for (swindex = 0, hwindex = 0; swindex < rcdu->num_crtcs; ++hwindex) {
 		struct rcar_du_group *rgrp;
-- 
2.21.0

[PATCH 17/20] drm: rcar-du: crtc: Enable and disable CMMs

[Jacopo Mondi]

Enable/disable the CMM associated with a CRTC at
atomic_enable()/atomic_disable() time.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 9f270a54b164..e6d3df37c827 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -21,6 +21,7 @@
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_vblank.h>
 
+#include "rcar_cmm.h"
 #include "rcar_du_crtc.h"
 #include "rcar_du_drv.h"
 #include "rcar_du_encoder.h"
@@ -523,6 +524,7 @@ static int rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
 		goto error_group;
 
 	rcar_du_crtc_setup(rcrtc);
+
 	rcrtc->initialized = true;
 
 	return 0;
@@ -619,6 +621,9 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
 	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
 		rcar_du_vsp_disable(rcrtc);
 
+	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_CMM) && rcrtc->cmm)
+		rcar_cmm_disable(rcrtc->cmm);
+
 	/*
 	 * Select switch sync mode. This stops display operation and configures
 	 * the HSYNC and VSYNC signals as inputs.
@@ -686,6 +691,9 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
 	}
 
 	rcar_du_crtc_start(rcrtc);
+
+	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_CMM) && rcrtc->cmm)
+		rcar_cmm_enable(rcrtc->cmm);
 }
 
 static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,
-- 
2.21.0

[PATCH 18/20] drm: rcar-du: group: Enable DU's CMM extension

[Jacopo Mondi]

Enable the CMM units through the display unit extensional function control
group register DEFR7.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/rcar-du/rcar_du_group.c | 8 ++++++++
 drivers/gpu/drm/rcar-du/rcar_du_regs.h  | 5 +++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
index 9eee47969e77..d252c9bb9809 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
@@ -147,6 +147,14 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
 
 	rcar_du_group_setup_pins(rgrp);
 
+	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM)) {
+		u32 defr7 = DEFR7_CODE |
+			    (rgrp->cmms_mask & BIT(1) ? DEFR7_CMME1 : 0) |
+			    (rgrp->cmms_mask & BIT(0) ? DEFR7_CMME0 : 0);
+
+		rcar_du_group_write(rgrp, DEFR7, defr7);
+	}
+
 	if (rcdu->info->gen >= 2) {
 		rcar_du_group_setup_defr8(rgrp);
 		rcar_du_group_setup_didsr(rgrp);
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
index bc87f080b170..fb9964949368 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
@@ -197,6 +197,11 @@
 #define DEFR6_MLOS1		(1 << 2)
 #define DEFR6_DEFAULT		(DEFR6_CODE | DEFR6_TCNE1)
 
+#define DEFR7			0x000ec
+#define DEFR7_CODE		(0x7779 << 16)
+#define DEFR7_CMME1		BIT(6)
+#define DEFR7_CMME0		BIT(4)
+
 /* -----------------------------------------------------------------------------
  * R8A7790-only Control Registers
  */
-- 
2.21.0

[PATCH 19/20] drm: rcar-du: crtc: Register GAMMA_LUT properties

[Jacopo Mondi]

Enable the GAMMA_LUT KMS property using the framework helpers to
register the proeprty and the associated gamma table size maximum size.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index e6d3df37c827..c920fb5dba65 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -1207,6 +1207,9 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
 	    rcdu->cmms[swindex]) {
 		rcrtc->cmm = rcdu->cmms[swindex];
 		rgrp->cmms_mask |= BIT(hwindex % 2);
+
+		drm_mode_crtc_set_gamma_size(crtc, CMM_GAMMA_LUT_SIZE);
+		drm_crtc_enable_color_mgmt(crtc, 0, false, CMM_GAMMA_LUT_SIZE);
 	}
 
 	drm_crtc_helper_add(crtc, &crtc_helper_funcs);
-- 
2.21.0

[PATCH 20/20] drm: rcar-du: kms: Update CMM in atomic commit tail

[Jacopo Mondi]

Update CMM settings at in the atomic commit tail helper method.

The CMM is updated with new gamma values provided to the driver
in the GAMMA_LUT blob property.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/rcar-du/rcar_du_kms.c | 36 +++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index 5a910a04e1d9..29a2020a46b5 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -21,6 +21,7 @@
 #include <linux/of_platform.h>
 #include <linux/wait.h>
 
+#include "rcar_cmm.h"
 #include "rcar_du_crtc.h"
 #include "rcar_du_drv.h"
 #include "rcar_du_encoder.h"
@@ -367,6 +368,38 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv,
  * Atomic Check and Update
  */
 
+static void rcar_du_atomic_commit_update_cmm(struct drm_crtc *crtc,
+					     struct drm_crtc_state *old_state)
+{
+	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
+	struct rcar_cmm_config cmm_config = {};
+
+	if (!rcrtc->cmm || !crtc->state->color_mgmt_changed)
+		return;
+
+	if (!crtc->state->gamma_lut) {
+		cmm_config.lut.enable = false;
+		rcar_cmm_setup(rcrtc->cmm, &cmm_config);
+
+		return;
+	}
+
+	cmm_config.lut.enable = true;
+	cmm_config.lut.table = (struct drm_color_lut *)
+			       crtc->state->gamma_lut->data;
+
+	/* Set LUT table size to 0 if entries should not be updated. */
+	if (!old_state->gamma_lut ||
+	    (old_state->gamma_lut->base.id !=
+	    crtc->state->gamma_lut->base.id))
+		cmm_config.lut.size = crtc->state->gamma_lut->length
+				    / sizeof(cmm_config.lut.table[0]);
+	else
+		cmm_config.lut.size = 0;
+
+	rcar_cmm_setup(rcrtc->cmm, &cmm_config);
+}
+
 static int rcar_du_atomic_check(struct drm_device *dev,
 				struct drm_atomic_state *state)
 {
@@ -409,6 +442,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
 			rcdu->dpad1_source = rcrtc->index;
 	}
 
+	for_each_old_crtc_in_state(old_state, crtc, crtc_state, i)
+		rcar_du_atomic_commit_update_cmm(crtc, crtc_state);
+
 	/* Apply the atomic update. */
 	drm_atomic_helper_commit_modeset_disables(dev, old_state);
 	drm_atomic_helper_commit_planes(dev, old_state,
-- 
2.21.0

Re: [PATCH 01/20] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:01PM +0200, Jacopo Mondi wrote:
> Add device tree bindings documentation for the Renesas R-Car Display
> Unit Color Management Module.
> 
> CMM is the image enhancement module available on each R-Car DU video
> channel on Gen2 and Gen3 SoCs (V3H and V3M excluded).
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  .../bindings/display/renesas,cmm.txt          | 25 +++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/renesas,cmm.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/renesas,cmm.txt b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> new file mode 100644
> index 000000000000..d8d3cf9ce2ce
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> @@ -0,0 +1,25 @@
> +* Renesas R-Car Color Management Module (CMM)
> +
> +Renesas R-Car image enhancement module connected to R-Car DU video channels.
> +
> +Required properties:
> + - compatible: shall be one of:
> +   - "renesas,cmm-gen3"
> +   - "renesas,cmm-gen2"
> +
> + - reg: the address base and length of the memory area where CMM control
> +   registers are mapped to.
> +
> + - clocks: phandle and clock-specifier pair to the CMM functional clock
> +   supplier.
> +
> +Example:
> +--------
> +
> +	cmm0: cmm@fea40000 {
> +		compatible = "renesas,cmm";
> +		reg = <0 0xfea40000 0 0x1000>;
> +		power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
> +		clocks = <&cpg CPG_MOD 711>;
> +		resets = <&cpg 711>;
> +	};

-- 
Regards,

Laurent Pinchart

Re: [PATCH 02/20] dt-bindings: display, renesas,du: Document cmms property

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:02PM +0200, Jacopo Mondi wrote:
> Document the newly added 'cmms' property which accepts a list of phandle
> and channel index pairs that point to the CMM units available for each
> Display Unit output video channel.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  Documentation/devicetree/bindings/display/renesas,du.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt b/Documentation/devicetree/bindings/display/renesas,du.txt
> index aedb22b4d161..100114ef11d5 100644
> --- a/Documentation/devicetree/bindings/display/renesas,du.txt
> +++ b/Documentation/devicetree/bindings/display/renesas,du.txt
> @@ -44,6 +44,10 @@ Required Properties:
>      instance that serves the DU channel, and the channel index identifies the
>      LIF instance in that VSP.
>  
> +  - cmms: A list of phandles to the CMM instances present in the SoC, one
> +    for each available DU channel. The property shall not be specified for
> +    SoCs that does not provide any CMM (such as V3M and V3H).

s/does/do/

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
>  Required nodes:
>  
>  The connections to the DU output video ports are modeled using the OF graph
> @@ -89,6 +93,7 @@ Example: R8A7795 (R-Car H3) ES2.0 DU
>  			 <&cpg CPG_MOD 721>;
>  		clock-names = "du.0", "du.1", "du.2", "du.3";
>  		vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd0 1>;
> +		cmms = <&cmm0 &cmm1 &cmm2 &cmm3>;
>  
>  		ports {
>  			#address-cells = <1>;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 03/20] dt-bindings: display, renesas,du: Update 'vsps' in example

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:03PM +0200, Jacopo Mondi wrote:
> Update the 'vsps' property structure in the documentation example to
> reflect what's actually implemented in the device tree sources.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  Documentation/devicetree/bindings/display/renesas,du.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt b/Documentation/devicetree/bindings/display/renesas,du.txt
> index 100114ef11d5..262047053d31 100644
> --- a/Documentation/devicetree/bindings/display/renesas,du.txt
> +++ b/Documentation/devicetree/bindings/display/renesas,du.txt
> @@ -92,7 +92,7 @@ Example: R8A7795 (R-Car H3) ES2.0 DU
>  			 <&cpg CPG_MOD 722>,
>  			 <&cpg CPG_MOD 721>;
>  		clock-names = "du.0", "du.1", "du.2", "du.3";
> -		vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd0 1>;
> +		vsps = <&vspd0 0 &vspd1 0 &vspd2  &vspd0 1>;

The former is simpler to read than the latter in my opinion. Shouldn't
we update the .dtsi files instead ?

>  		cmms = <&cmm0 &cmm1 &cmm2 &cmm3>;
>  
>  		ports {

-- 
Regards,

Laurent Pinchart

Re: [PATCH 05/20] clk: renesas: r8a7795: Add CMM clocks

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:05PM +0200, Jacopo Mondi wrote:
> Add clock definitions for CMM units on Renesas R-Car Gen3 H3.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/clk/renesas/r8a7795-cpg-mssr.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/clk/renesas/r8a7795-cpg-mssr.c b/drivers/clk/renesas/r8a7795-cpg-mssr.c
> index 86842c9fd314..e8f1d5ec0455 100644
> --- a/drivers/clk/renesas/r8a7795-cpg-mssr.c
> +++ b/drivers/clk/renesas/r8a7795-cpg-mssr.c
> @@ -200,6 +200,10 @@ static struct mssr_mod_clk r8a7795_mod_clks[] __initdata = {
>  	DEF_MOD("ehci0",		 703,	R8A7795_CLK_S3D4),
>  	DEF_MOD("hsusb",		 704,	R8A7795_CLK_S3D4),
>  	DEF_MOD("hsusb3",		 705,	R8A7795_CLK_S3D4),
> +	DEF_MOD("cmm3",			 708,	R8A7795_CLK_S2D1),
> +	DEF_MOD("cmm2",			 709,	R8A7795_CLK_S2D1),
> +	DEF_MOD("cmm1",			 710,	R8A7795_CLK_S2D1),
> +	DEF_MOD("cmm0",			 711,	R8A7795_CLK_S2D1),

Could you try to get confirmation that S2D1 is the right parent (for all
SoCs) ? Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	DEF_MOD("csi21",		 713,	R8A7795_CLK_CSI0), /* ES1.x */
>  	DEF_MOD("csi20",		 714,	R8A7795_CLK_CSI0),
>  	DEF_MOD("csi41",		 715,	R8A7795_CLK_CSI0),

-- 
Regards,

Laurent Pinchart

Re: [PATCH 06/20] clk: renesas: r8a77965: Add CMM clocks

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:06PM +0200, Jacopo Mondi wrote:
> Add clock definitions for CMM units on Renesas R-Car Gen3 M3-N.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/clk/renesas/r8a77965-cpg-mssr.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/clk/renesas/r8a77965-cpg-mssr.c b/drivers/clk/renesas/r8a77965-cpg-mssr.c
> index eb1cca58a1e1..58f75b03c8bb 100644
> --- a/drivers/clk/renesas/r8a77965-cpg-mssr.c
> +++ b/drivers/clk/renesas/r8a77965-cpg-mssr.c
> @@ -178,6 +178,9 @@ static const struct mssr_mod_clk r8a77965_mod_clks[] __initconst = {
>  	DEF_MOD("ehci1",		702,	R8A77965_CLK_S3D4),
>  	DEF_MOD("ehci0",		703,	R8A77965_CLK_S3D4),
>  	DEF_MOD("hsusb",		704,	R8A77965_CLK_S3D4),
> +	DEF_MOD("cmm3",			708,	R8A77965_CLK_S2D1),
> +	DEF_MOD("cmm1",			710,	R8A77965_CLK_S2D1),
> +	DEF_MOD("cmm0",			711,	R8A77965_CLK_S2D1),
>  	DEF_MOD("csi20",		714,	R8A77965_CLK_CSI0),
>  	DEF_MOD("csi40",		716,	R8A77965_CLK_CSI0),
>  	DEF_MOD("du3",			721,	R8A77965_CLK_S2D1),

-- 
Regards,

Laurent Pinchart

Re: [PATCH 07/20] clk: renesas: r8a77990: Add CMM clocks

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:07PM +0200, Jacopo Mondi wrote:
> Add clock definitions for CMM units on Renesas R-Car Gen3 E3.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/clk/renesas/r8a77990-cpg-mssr.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/clk/renesas/r8a77990-cpg-mssr.c b/drivers/clk/renesas/r8a77990-cpg-mssr.c
> index 9a278c75c918..8cdd0e6fb74f 100644
> --- a/drivers/clk/renesas/r8a77990-cpg-mssr.c
> +++ b/drivers/clk/renesas/r8a77990-cpg-mssr.c
> @@ -182,6 +182,8 @@ static const struct mssr_mod_clk r8a77990_mod_clks[] __initconst = {
>  
>  	DEF_MOD("ehci0",		 703,	R8A77990_CLK_S3D4),
>  	DEF_MOD("hsusb",		 704,	R8A77990_CLK_S3D4),
> +	DEF_MOD("cmm1",			 710,	R8A77990_CLK_S1D1),
> +	DEF_MOD("cmm0",			 711,	R8A77990_CLK_S1D1),
>  	DEF_MOD("csi40",		 716,	R8A77990_CLK_CSI0),
>  	DEF_MOD("du1",			 723,	R8A77990_CLK_S1D1),
>  	DEF_MOD("du0",			 724,	R8A77990_CLK_S1D1),

-- 
Regards,

Laurent Pinchart

Re: [PATCH 08/20] clk: renesas: r8a77995: Add CMM clocks

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:08PM +0200, Jacopo Mondi wrote:
> Add clock definitions for CMM units on Renesas R-Car Gen3 D3.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/clk/renesas/r8a77995-cpg-mssr.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/clk/renesas/r8a77995-cpg-mssr.c b/drivers/clk/renesas/r8a77995-cpg-mssr.c
> index eee3874865a9..acd5fabb785a 100644
> --- a/drivers/clk/renesas/r8a77995-cpg-mssr.c
> +++ b/drivers/clk/renesas/r8a77995-cpg-mssr.c
> @@ -146,6 +146,8 @@ static const struct mssr_mod_clk r8a77995_mod_clks[] __initconst = {
>  	DEF_MOD("vspbs",		 627,	R8A77995_CLK_S0D1),
>  	DEF_MOD("ehci0",		 703,	R8A77995_CLK_S3D2),
>  	DEF_MOD("hsusb",		 704,	R8A77995_CLK_S3D2),
> +	DEF_MOD("cmm1",			 710,	R8A77995_CLK_S1D1),
> +	DEF_MOD("cmm0",			 711,	R8A77995_CLK_S1D1),
>  	DEF_MOD("du1",			 723,	R8A77995_CLK_S1D1),
>  	DEF_MOD("du0",			 724,	R8A77995_CLK_S1D1),
>  	DEF_MOD("lvds",			 727,	R8A77995_CLK_S2D1),

-- 
Regards,

Laurent Pinchart

Re: [PATCH 09/20] arm64: dts: renesas: r8a7796: Add CMM units

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:09PM +0200, Jacopo Mondi wrote:
> Add CMM units to Renesas R-Car M3-W device tree and reference them from
> the Display Unit they are connected to.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  arch/arm64/boot/dts/renesas/r8a7796.dtsi | 25 ++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> index cdf784899cf8..2e1891d9d322 100644
> --- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> @@ -2610,6 +2610,30 @@
>  			renesas,fcp = <&fcpvi0>;
>  		};
>  
> +		cmm0: cmm@fea40000 {
> +			compatible = "renesas,cmm-gen3";
> +			reg = <0 0xfea40000 0 0x1000>;
> +			clocks = <&cpg CPG_MOD 711>;
> +			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
> +			resets = <&cpg 711>;
> +		};
> +
> +		cmm1: cmm@fea50000 {
> +			compatible = "renesas,cmm-gen3";
> +			reg = <0 0xfea50000 0 0x1000>;
> +			clocks = <&cpg CPG_MOD 710>;
> +			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
> +			resets = <&cpg 710>;
> +		};
> +
> +		cmm2: cmm@fea60000 {
> +			compatible = "renesas,cmm-gen3";
> +			reg = <0 0xfea60000 0 0x1000>;
> +			clocks = <&cpg CPG_MOD 709>;
> +			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
> +			resets = <&cpg 709>;
> +		};
> +
>  		csi20: csi2@fea80000 {
>  			compatible = "renesas,r8a7796-csi2";
>  			reg = <0 0xfea80000 0 0x10000>;
> @@ -2763,6 +2787,7 @@
>  			status = "disabled";
>  
>  			vsps = <&vspd0 &vspd1 &vspd2>;
> +			cmms = <&cmm0 &cmm1 &cmm2>;
>  
>  			ports {
>  				#address-cells = <1>;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 10/20] arm64: dts: renesas: r8a7795: Add CMM units

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:10PM +0200, Jacopo Mondi wrote:
> Add CMM units to Renesas R-Car H3 device tree and reference them from
> the Display Unit they are connected to.
> 
> While at it, re-sort the du device node properties to match the ordering
> found in other SoCs.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  arch/arm64/boot/dts/renesas/r8a7795.dtsi | 36 +++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> index abeac3059383..83edd9f12caa 100644
> --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> @@ -2825,6 +2825,38 @@
>  			renesas,fcp = <&fcpvi1>;
>  		};
>  
> +		cmm0: cmm@fea40000 {
> +			compatible = "renesas,cmm-gen3";
> +			reg = <0 0xfea40000 0 0x1000>;
> +			clocks = <&cpg CPG_MOD 711>;
> +			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
> +			resets = <&cpg 711>;
> +		};
> +
> +		cmm1: cmm@fea50000 {
> +			compatible = "renesas,cmm-gen3";
> +			reg = <0 0xfea50000 0 0x1000>;
> +			clocks = <&cpg CPG_MOD 710>;
> +			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
> +			resets = <&cpg 710>;
> +		};
> +
> +		cmm2: cmm@fea60000 {
> +			compatible = "renesas,cmm-gen3";
> +			reg = <0 0xfea60000 0 0x1000>;
> +			clocks = <&cpg CPG_MOD 709>;
> +			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
> +			resets = <&cpg 709>;
> +		};
> +
> +		cmm3: cmm@fea70000 {
> +			compatible = "renesas,cmm-gen3";
> +			reg = <0 0xfea70000 0 0x1000>;
> +			clocks = <&cpg CPG_MOD 708>;
> +			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
> +			resets = <&cpg 708>;
> +		};
> +
>  		csi20: csi2@fea80000 {
>  			compatible = "renesas,r8a7795-csi2";
>  			reg = <0 0xfea80000 0 0x10000>;
> @@ -3028,9 +3060,11 @@
>  				 <&cpg CPG_MOD 722>,
>  				 <&cpg CPG_MOD 721>;
>  			clock-names = "du.0", "du.1", "du.2", "du.3";
> -			vsps = <&vspd0 0 &vspd1 0 &vspd2 0 &vspd0 1>;
>  			status = "disabled";
>  
> +			vsps = <&vspd0 0 &vspd1 0 &vspd2 0 &vspd0 1>;
> +			cmms = <&cmm0 &cmm1 &cmm2 &cmm3>;
> +
>  			ports {
>  				#address-cells = <1>;
>  				#size-cells = <0>;
> -- 
> 2.21.0
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 11/20] arm64: dts: renesas: r8a77965: Add CMM units

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:11PM +0200, Jacopo Mondi wrote:
> Add CMM units to Renesas R-Car M3-N device tree and reference them from
> the Display Unit they are connected to.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  arch/arm64/boot/dts/renesas/r8a77965.dtsi | 25 +++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77965.dtsi b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> index 9763d108e183..a90854685b33 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
> @@ -2026,6 +2026,30 @@
>  			resets = <&cpg 602>;
>  		};
>  
> +		cmm0: cmm@fea40000 {
> +			compatible = "renesas,cmm-gen3";
> +			reg = <0 0xfea40000 0 0x1000>;
> +			clocks = <&cpg CPG_MOD 711>;
> +			power-domains = <&sysc R8A77965_PD_ALWAYS_ON>;
> +			resets = <&cpg 711>;
> +		};
> +
> +		cmm1: cmm@fea50000 {
> +			compatible = "renesas,cmm-gen3";
> +			reg = <0 0xfea50000 0 0x1000>;
> +			clocks = <&cpg CPG_MOD 710>;
> +			power-domains = <&sysc R8A77965_PD_ALWAYS_ON>;
> +			resets = <&cpg 710>;
> +		};
> +
> +		cmm3: cmm@fea70000 {
> +			compatible = "renesas,cmm-gen3";
> +			reg = <0 0xfea70000 0 0x1000>;
> +			clocks = <&cpg CPG_MOD 708>;
> +			power-domains = <&sysc R8A77965_PD_ALWAYS_ON>;
> +			resets = <&cpg 708>;
> +		};
> +
>  		csi20: csi2@fea80000 {
>  			compatible = "renesas,r8a77965-csi2";
>  			reg = <0 0xfea80000 0 0x10000>;
> @@ -2177,6 +2201,7 @@
>  			status = "disabled";
>  
>  			vsps = <&vspd0 0 &vspd1 0 &vspd0 1>;
> +			cmms = <&cmm0 &cmm1 &cmm3>;
>  
>  			ports {
>  				#address-cells = <1>;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 12/20] arm64: dts: renesas: r8a77990: Add CMM units

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:12PM +0200, Jacopo Mondi wrote:
> Add CMM units to Renesas R-Car E3 device tree and reference them from
> the Display Unit they are connected to.
> 
> While at it, re-sort the du device node properties to match the ordering
> found in other SoCs.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  arch/arm64/boot/dts/renesas/r8a77990.dtsi | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77990.dtsi b/arch/arm64/boot/dts/renesas/r8a77990.dtsi
> index a69faa60ea4d..87453ddbc7e3 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77990.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a77990.dtsi
> @@ -1656,6 +1656,22 @@
>  			iommus = <&ipmmu_vi0 9>;
>  		};
>  
> +		cmm0: cmm@fea40000 {
> +			compatible = "renesas,cmm-gen3";
> +			reg = <0 0xfea40000 0 0x1000>;
> +			clocks = <&cpg CPG_MOD 711>;
> +			power-domains = <&sysc R8A77990_PD_ALWAYS_ON>;
> +			resets = <&cpg 711>;
> +		};
> +
> +		cmm1: cmm@fea50000 {
> +			compatible = "renesas,cmm-gen3";
> +			reg = <0 0xfea50000 0 0x1000>;
> +			clocks = <&cpg CPG_MOD 710>;
> +			power-domains = <&sysc R8A77990_PD_ALWAYS_ON>;
> +			resets = <&cpg 710>;
> +		};
> +
>  		csi40: csi2@feaa0000 {
>  			compatible = "renesas,r8a77990-csi2", "renesas,rcar-gen3-csi2";
>  			reg = <0 0xfeaa0000 0 0x10000>;
> @@ -1695,9 +1711,11 @@
>  			clocks = <&cpg CPG_MOD 724>,
>  				 <&cpg CPG_MOD 723>;
>  			clock-names = "du.0", "du.1";
> -			vsps = <&vspd0 0 &vspd1 0>;
>  			status = "disabled";
>  
> +			vsps = <&vspd0 0 &vspd1 0>;
> +			cmms = <&cmm0 &cmm1>;
> +
>  			ports {
>  				#address-cells = <1>;
>  				#size-cells = <0>;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 13/20] arm64: dts: renesas: r8a77995: Add CMM units

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:13PM +0200, Jacopo Mondi wrote:
> Add CMM units to Renesas R-Car D3 device tree and reference them from
> the Display Unit they are connected to.
> 
> While at it, re-sort the du device node properties to match the ordering
> found in other SoCs.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  arch/arm64/boot/dts/renesas/r8a77995.dtsi | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77995.dtsi b/arch/arm64/boot/dts/renesas/r8a77995.dtsi
> index 5bf3af246e14..c52d73068304 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77995.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a77995.dtsi
> @@ -993,6 +993,22 @@
>  			iommus = <&ipmmu_vi0 9>;
>  		};
>  
> +		cmm0: cmm@fea40000 {
> +			compatible = "renesas,cmm-gen3";
> +			reg = <0 0xfea40000 0 0x1000>;
> +			clocks = <&cpg CPG_MOD 711>;
> +			power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
> +			resets = <&cpg 711>;
> +		};
> +
> +		cmm1: cmm@fea50000 {
> +			compatible = "renesas,cmm-gen3";
> +			reg = <0 0xfea50000 0 0x1000>;
> +			clocks = <&cpg CPG_MOD 710>;
> +			power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
> +			resets = <&cpg 710>;
> +		};
> +
>  		du: display@feb00000 {
>  			compatible = "renesas,du-r8a77995";
>  			reg = <0 0xfeb00000 0 0x80000>;
> @@ -1001,9 +1017,11 @@
>  			clocks = <&cpg CPG_MOD 724>,
>  				 <&cpg CPG_MOD 723>;
>  			clock-names = "du.0", "du.1";
> -			vsps = <&vspd0 0 &vspd1 0>;
>  			status = "disabled";
>  
> +			vsps = <&vspd0 0 &vspd1 0>;
> +			cmms = <&cmm0 &cmm1>;
> +
>  			ports {
>  				#address-cells = <1>;
>  				#size-cells = <0>;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 01/20] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation

[Geert Uytterhoeven]

Hi Jacopo,

On Thu, Jun 6, 2019 at 4:21 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> Add device tree bindings documentation for the Renesas R-Car Display
> Unit Color Management Module.
>
> CMM is the image enhancement module available on each R-Car DU video
> channel on Gen2 and Gen3 SoCs (V3H and V3M excluded).

R-Car Gen2 ...

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks for your patch!

> index 000000000000..d8d3cf9ce2ce
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> @@ -0,0 +1,25 @@
> +* Renesas R-Car Color Management Module (CMM)
> +
> +Renesas R-Car image enhancement module connected to R-Car DU video channels.
> +
> +Required properties:
> + - compatible: shall be one of:
> +   - "renesas,cmm-gen3"
> +   - "renesas,cmm-gen2"

"gen3" and "gen2" don't carry much meaning on their own (SH2 is gen2 of
SuperH?). Furthermore, revision info should immediately follow the comma.

"renesas,rcar-gen3-cmm" and "renesas,rcar-gen2-cmm"?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 05/20] clk: renesas: r8a7795: Add CMM clocks

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 1650 bytes --]

Hi Laurent,

On Thu, Jun 06, 2019 at 07:58:57PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Jun 06, 2019 at 04:22:05PM +0200, Jacopo Mondi wrote:
> > Add clock definitions for CMM units on Renesas R-Car Gen3 H3.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/clk/renesas/r8a7795-cpg-mssr.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/clk/renesas/r8a7795-cpg-mssr.c b/drivers/clk/renesas/r8a7795-cpg-mssr.c
> > index 86842c9fd314..e8f1d5ec0455 100644
> > --- a/drivers/clk/renesas/r8a7795-cpg-mssr.c
> > +++ b/drivers/clk/renesas/r8a7795-cpg-mssr.c
> > @@ -200,6 +200,10 @@ static struct mssr_mod_clk r8a7795_mod_clks[] __initdata = {
> >  	DEF_MOD("ehci0",		 703,	R8A7795_CLK_S3D4),
> >  	DEF_MOD("hsusb",		 704,	R8A7795_CLK_S3D4),
> >  	DEF_MOD("hsusb3",		 705,	R8A7795_CLK_S3D4),
> > +	DEF_MOD("cmm3",			 708,	R8A7795_CLK_S2D1),
> > +	DEF_MOD("cmm2",			 709,	R8A7795_CLK_S2D1),
> > +	DEF_MOD("cmm1",			 710,	R8A7795_CLK_S2D1),
> > +	DEF_MOD("cmm0",			 711,	R8A7795_CLK_S2D1),
>
> Could you try to get confirmation that S2D1 is the right parent (for all
> SoCs) ? Apart from that,

It's not.. for r8a7799x it's S1D1, the same parent as the DU clock.
The patches in the BSP use the same clocks I have used here, so I
assume at least that part is correct.

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks
   j

>
> >  	DEF_MOD("csi21",		 713,	R8A7795_CLK_CSI0), /* ES1.x */
> >  	DEF_MOD("csi20",		 714,	R8A7795_CLK_CSI0),
> >  	DEF_MOD("csi41",		 715,	R8A7795_CLK_CSI0),
>
> --
> Regards,
>
> Laurent Pinchart

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 03/20] dt-bindings: display, renesas,du: Update 'vsps' in example

[Geert Uytterhoeven]

Hi Laurent, Jacopo,

On Thu, Jun 6, 2019 at 8:50 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Thu, Jun 06, 2019 at 04:22:03PM +0200, Jacopo Mondi wrote:
> > Update the 'vsps' property structure in the documentation example to
> > reflect what's actually implemented in the device tree sources.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

> > --- a/Documentation/devicetree/bindings/display/renesas,du.txt
> > +++ b/Documentation/devicetree/bindings/display/renesas,du.txt
> > @@ -92,7 +92,7 @@ Example: R8A7795 (R-Car H3) ES2.0 DU
> >                        <&cpg CPG_MOD 722>,
> >                        <&cpg CPG_MOD 721>;
> >               clock-names = "du.0", "du.1", "du.2", "du.3";
> > -             vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd0 1>;
> > +             vsps = <&vspd0 0 &vspd1 0 &vspd2  &vspd0 1>;
>
> The former is simpler to read than the latter in my opinion. Shouldn't
> we update the .dtsi files instead ?

Yes, it is easier to read (for humans).

> >               cmms = <&cmm0 &cmm1 &cmm2 &cmm3>;

Perhaps we want grouping here, too?

> >
> >               ports {

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 01/20] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation

[Sergei Shtylyov]

On 06/06/2019 05:22 PM, Jacopo Mondi wrote:

> Add device tree bindings documentation for the Renesas R-Car Display
> Unit Color Management Module.
> 
> CMM is the image enhancement module available on each R-Car DU video
> channel on Gen2 and Gen3 SoCs (V3H and V3M excluded).
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../bindings/display/renesas,cmm.txt          | 25 +++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/renesas,cmm.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/renesas,cmm.txt b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> new file mode 100644
> index 000000000000..d8d3cf9ce2ce
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> @@ -0,0 +1,25 @@
> +* Renesas R-Car Color Management Module (CMM)
> +
> +Renesas R-Car image enhancement module connected to R-Car DU video channels.
> +
> +Required properties:
> + - compatible: shall be one of:
> +   - "renesas,cmm-gen3"
> +   - "renesas,cmm-gen2"
> +
> + - reg: the address base and length of the memory area where CMM control
> +   registers are mapped to.
> +
> + - clocks: phandle and clock-specifier pair to the CMM functional clock
> +   supplier.
> +
> +Example:
> +--------
> +
> +	cmm0: cmm@fea40000 {
> +		compatible = "renesas,cmm";

   Dosent' match the description above.

> +		reg = <0 0xfea40000 0 0x1000>;
> +		power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
> +		clocks = <&cpg CPG_MOD 711>;
> +		resets = <&cpg 711>;
> +	};
> 

MBR, Sergei

Re: [PATCH 14/20] drm: rcar-du: Add support for CMM

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:14PM +0200, Jacopo Mondi wrote:
> Add a driver for the R-Car Display Unit Color Correction Module.
> 
> Each DU output channel is provided with a CMM unit to perform image
> enhancement and color correction.

I would say "On most Gen3 SoCs, each DU ..." as V3* SoCs have no CMM.

> 
> Add support for CMM through a driver that supports configuration of
> the 1-dimensional LUT table. More advanced CMM feature could be
> implemented on top of this basic one.

s/could be/will be/ ? :-)

> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/Kconfig    |   7 +
>  drivers/gpu/drm/rcar-du/Makefile   |   1 +
>  drivers/gpu/drm/rcar-du/rcar_cmm.c | 197 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/rcar-du/rcar_cmm.h |  38 ++++++
>  4 files changed, 243 insertions(+)
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.c
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.h
> 
> diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
> index 1529849e217e..539d232790d1 100644
> --- a/drivers/gpu/drm/rcar-du/Kconfig
> +++ b/drivers/gpu/drm/rcar-du/Kconfig
> @@ -13,6 +13,13 @@ config DRM_RCAR_DU
>  	  Choose this option if you have an R-Car chipset.
>  	  If M is selected the module will be called rcar-du-drm.
>  
> +config DRM_RCAR_CMM
> +	bool "R-Car DU Color Management Module (CMM) Support"
> +	depends on DRM && OF
> +	depends on DRM_RCAR_DU
> +	help
> +	  Enable support for R-Car Color Management Module (CMM).
> +
>  config DRM_RCAR_DW_HDMI
>  	tristate "R-Car DU Gen3 HDMI Encoder Support"
>  	depends on DRM && OF
> diff --git a/drivers/gpu/drm/rcar-du/Makefile b/drivers/gpu/drm/rcar-du/Makefile
> index 6c2ed9c46467..4d1187ccc3e5 100644
> --- a/drivers/gpu/drm/rcar-du/Makefile
> +++ b/drivers/gpu/drm/rcar-du/Makefile
> @@ -15,6 +15,7 @@ rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS)	+= rcar_du_of.o \
>  rcar-du-drm-$(CONFIG_DRM_RCAR_VSP)	+= rcar_du_vsp.o
>  rcar-du-drm-$(CONFIG_DRM_RCAR_WRITEBACK) += rcar_du_writeback.o
>  
> +obj-$(CONFIG_DRM_RCAR_CMM)		+= rcar_cmm.o
>  obj-$(CONFIG_DRM_RCAR_DU)		+= rcar-du-drm.o
>  obj-$(CONFIG_DRM_RCAR_DW_HDMI)		+= rcar_dw_hdmi.o
>  obj-$(CONFIG_DRM_RCAR_LVDS)		+= rcar_lvds.o
> diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> new file mode 100644
> index 000000000000..5d9d917b91f4
> --- /dev/null
> +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> @@ -0,0 +1,197 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * rcar_cmm.c -- R-Car Display Unit Color Management Module
> + *
> + * Copyright (C) 2019 Jacopo Mondi <jacopo+renesas@jmondi.org>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +#include <drm/drm_atomic.h>
> +
> +#include "rcar_cmm.h"
> +
> +#define CM2_LUT_CTRL		0x00

I would write all register addresses with 3 (or 4) digits.

> +#define CM2_LUT_CTRL_EN		BIT(0)
> +#define CM2_LUT_TBLA		0x600

I would define this as

#define CM2_LUT_TBLA(n)		(0x600 + (n) * 4)

> +
> +struct rcar_cmm {
> +	struct clk *clk;
> +	void __iomem *base;
> +	bool enabled;
> +
> +	/* LUT table scratch buffer. */
> +	struct {
> +		bool restore;
> +		unsigned int size;
> +		uint32_t table[CMM_GAMMA_LUT_SIZE];
> +	} lut;
> +};
> +
> +static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg)
> +{
> +	return ioread32(rcmm->base + reg);
> +}
> +
> +static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data)
> +{
> +	iowrite32(data, rcmm->base + reg);
> +}
> +
> +int rcar_cmm_setup(struct platform_device *pdev, struct rcar_cmm_config *config)

Please document the functions exposed to the DU driver. It's hard to
understand the setup vs. enable/disable split by just reading this
driver.

> +{
> +	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> +	unsigned int i;
> +
> +	if (config->lut.size > CMM_GAMMA_LUT_SIZE)
> +		return -EINVAL;
> +
> +	/*
> +	 * As cmm_setup is called by atomic commit tail helper, it might be
> +	 * called before the enabling the CRTC (which calls cmm_enable()).
> +	 *
> +	 * Store the LUT table entries in the scratch buffer to be later
> +	 * programmed at enable time.
> +	 */
> +	if (!rcmm->enabled) {
> +		if (!config->lut.enable)
> +			return 0;
> +
> +		for (i = 0; i < config->lut.size; ++i) {
> +			struct drm_color_lut *lut = &config->lut.table[i];
> +
> +			rcmm->lut.table[i] = (lut->red & 0xff) << 16 |
> +					     (lut->green & 0xff) << 8 |
> +					     (lut->blue & 0xff);
> +		}
> +
> +		rcmm->lut.restore = true;
> +		rcmm->lut.size = config->lut.size;
> +
> +		return 0;
> +	}
> +
> +	if (rcar_cmm_read(rcmm, CM2_LUT_CTRL) & CM2_LUT_CTRL_EN &&
> +	    !config->lut.enable) {
> +		rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> +		return 0;
> +	}
> +
> +	/* Enable LUT and program the new gamma table values. */
> +	rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);

Shouldn't you write the LUT contents before enabling it (same below) ?

> +	for (i = 0; i < config->lut.size; ++i) {
> +		struct drm_color_lut *lut = &config->lut.table[i];
> +		u32 val = (lut->red & 0xff) << 16 | (lut->green & 0xff) << 8 |
> +			  (lut->blue & 0xff);

Do you need to recompute the value, can't you use rcmm->lut.table ?

> +
> +		rcar_cmm_write(rcmm, CM2_LUT_TBLA + i * 4, val);
> +	}
> +
> +	return 0;
> +}

You need to export this and the next two functions.

> +
> +int rcar_cmm_enable(struct platform_device *pdev)
> +{
> +	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> +	unsigned int i;
> +	int ret;
> +
> +	if (rcmm->enabled)
> +		return 0;

Can this happen without a bug in the caller ? If it can, and assuming
the caller balances the enable and disable calls, you will have
unbalanced clk_prepare_enable() and clk_disable_unprepare() calls.

> +
> +	ret = clk_prepare_enable(rcmm->clk);
> +	if (ret)
> +		return ret;

Could you use pm_runtime_get_sync() instead ?

> +
> +	/* Apply the LUT table values saved at cmm_setup time. */
> +	if (rcmm->lut.restore) {
> +		rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> +		for (i = 0; i < rcmm->lut.size; ++i)
> +			rcar_cmm_write(rcmm, CM2_LUT_TBLA + i * 4,
> +				       rcmm->lut.table[i]);
> +
> +		rcmm->lut.restore = false;
> +		rcmm->lut.size = 0;
> +	}
> +
> +	rcmm->enabled = true;
> +
> +	return 0;
> +}
> +
> +void rcar_cmm_disable(struct platform_device *pdev)
> +{
> +	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> +
> +	rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> +
> +	clk_disable_unprepare(rcmm->clk);
> +
> +	rcmm->lut.restore = false;
> +	rcmm->lut.size = 0;
> +	rcmm->enabled = false;
> +}
> +
> +static int rcar_cmm_probe(struct platform_device *pdev)
> +{
> +	struct rcar_cmm *rcmm;
> +	struct resource *res;
> +	resource_size_t size;
> +
> +	rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
> +	if (!rcmm)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, rcmm);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	size = resource_size(res);
> +	if (!devm_request_mem_region(&pdev->dev, res->start, size,
> +				     dev_name(&pdev->dev))) {
> +		dev_err(&pdev->dev,
> +			"can't request region for resource %pR\n", res);
> +		return -EBUSY;
> +	}
> +
> +	rcmm->base = devm_ioremap_nocache(&pdev->dev, res->start, size);
> +	if (IS_ERR(rcmm->base))
> +		return PTR_ERR(rcmm->base);

Anything wrong with devm_ioremap_resource() ?

> +
> +	rcmm->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(rcmm->clk)) {
> +		dev_err(&pdev->dev, "Failed to get CMM clock");
> +		return PTR_ERR(rcmm->clk);
> +	}
> +
> +	rcmm->lut.restore = false;
> +	rcmm->lut.size = 0;
> +	rcmm->enabled = false;

As you allocate memory with kzalloc() you could skip this.

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id rcar_cmm_of_table[] = {
> +	{ .compatible = "renesas,cmm-gen3" },
> +	{ .compatible = "renesas,cmm-gen2" },
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
> +
> +static struct platform_driver rcar_cmm_platform_driver = {
> +	.probe		= rcar_cmm_probe,
> +	.driver		= {
> +		.name	= "rcar-cmm",
> +		.of_match_table = rcar_cmm_of_table,
> +	},

No need for suspend/resume support ? The DU driver should disable/enable
the CMM in its suspend/resume paths, so this should be fine, but won't
the LUT contents be lost and need to be restored ?

> +};
> +
> +module_platform_driver(rcar_cmm_platform_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org");

Missing >.

> +MODULE_DESCRIPTION("Renesas R-Car CMM Driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h
> new file mode 100644
> index 000000000000..da61a145dc5c
> --- /dev/null
> +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */

The .c and .h file licenses don't match.

> +/*
> + * rcar_cmm.h -- R-Car Display Unit Color Management Module
> + *
> + * Copyright (C) 2019 Jacopo Mondi <jacopo+renesas@jmondi.org>
> + */
> +
> +#ifndef __RCAR_CMM_H__
> +#define __RCAR_CMM_H__
> +
> +#include <linux/platform_device.h>

You can forward-declare struct platform_device instead.

> +
> +#define CMM_GAMMA_LUT_SIZE		256
> +
> +struct drm_color_lut;
> +
> +/**
> + * struct rcar_cmm_config - CMM configuration
> + *
> + * @lut:	1D-LUT configuration
> + * @lut.enable:	1D-LUT enable flag
> + * @lut.table:	1D-LUT table entries.
> + * @lut.size	1D-LUT number of entries. Max is 256

The last line is missing a colon.

> + */
> +struct rcar_cmm_config {
> +	struct {
> +		bool enable;
> +		struct drm_color_lut *table;
> +		unsigned int size;
> +	} lut;
> +};
> +
> +int rcar_cmm_enable(struct platform_device *);

As the OF API looks up a struct device from a device_node, should we use
struct device here ?

> +void rcar_cmm_disable(struct platform_device *);

I find headers more readable when function arguments are named. In this
case the types probably provide enough information, but good luck trying
to read a function such as

int foo(int, int, bool);

> +
> +int rcar_cmm_setup(struct platform_device *, struct rcar_cmm_config *);

Can the second argument be const ?

> +
> +#endif /* __RCAR_CMM_H__ */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 03/20] dt-bindings: display, renesas,du: Update 'vsps' in example

[Laurent Pinchart]

Hi Geert,

On Thu, Jun 06, 2019 at 10:00:23PM +0200, Geert Uytterhoeven wrote:
> On Thu, Jun 6, 2019 at 8:50 PM Laurent Pinchart wrote:
> > On Thu, Jun 06, 2019 at 04:22:03PM +0200, Jacopo Mondi wrote:
> >> Update the 'vsps' property structure in the documentation example to
> >> reflect what's actually implemented in the device tree sources.
> >>
> >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> >> --- a/Documentation/devicetree/bindings/display/renesas,du.txt
> >> +++ b/Documentation/devicetree/bindings/display/renesas,du.txt
> >> @@ -92,7 +92,7 @@ Example: R8A7795 (R-Car H3) ES2.0 DU
> >>                        <&cpg CPG_MOD 722>,
> >>                        <&cpg CPG_MOD 721>;
> >>               clock-names = "du.0", "du.1", "du.2", "du.3";
> >> -             vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd0 1>;
> >> +             vsps = <&vspd0 0 &vspd1 0 &vspd2  &vspd0 1>;
> >
> > The former is simpler to read than the latter in my opinion. Shouldn't
> > we update the .dtsi files instead ?
> 
> Yes, it is easier to read (for humans).
> 
> >>               cmms = <&cmm0 &cmm1 &cmm2 &cmm3>;
> 
> Perhaps we want grouping here, too?

As there's a single entry per CMM it matters less in my opinion. I'm
fine with either options.

> 
> >>
> >>               ports {

-- 
Regards,

Laurent Pinchart

Re: [PATCH 15/20] drm: rcar-du: Claim CMM support for Gen3 SoCs

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:15PM +0200, Jacopo Mondi wrote:
> Add CMM to the list of supported features for Gen3 SoCs that provide it:
> - R8A7795
> - R8A7796
> - R8A77965
> - R8A7799x
> 
> Leave R8A77970 out as V3M and V3H are the only Gen3 SoCs that do not
> support CMM.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Do we actually need this ? Could we just skip CMM handling if the cmms
property isn't set in DT ?

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 12 ++++++++----
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h |  1 +
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> index 75ab17af13a9..1e69cfa11798 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -247,7 +247,8 @@ static const struct rcar_du_device_info rcar_du_r8a7795_info = {
>  	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>  		  | RCAR_DU_FEATURE_VSP1_SOURCE
>  		  | RCAR_DU_FEATURE_INTERLACED
> -		  | RCAR_DU_FEATURE_TVM_SYNC,
> +		  | RCAR_DU_FEATURE_TVM_SYNC
> +		  | RCAR_DU_FEATURE_CMM,
>  	.channels_mask = BIT(3) | BIT(2) | BIT(1) | BIT(0),
>  	.routes = {
>  		/*
> @@ -280,7 +281,8 @@ static const struct rcar_du_device_info rcar_du_r8a7796_info = {
>  	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>  		  | RCAR_DU_FEATURE_VSP1_SOURCE
>  		  | RCAR_DU_FEATURE_INTERLACED
> -		  | RCAR_DU_FEATURE_TVM_SYNC,
> +		  | RCAR_DU_FEATURE_TVM_SYNC
> +		  | RCAR_DU_FEATURE_CMM,
>  	.channels_mask = BIT(2) | BIT(1) | BIT(0),
>  	.routes = {
>  		/*
> @@ -309,7 +311,8 @@ static const struct rcar_du_device_info rcar_du_r8a77965_info = {
>  	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>  		  | RCAR_DU_FEATURE_VSP1_SOURCE
>  		  | RCAR_DU_FEATURE_INTERLACED
> -		  | RCAR_DU_FEATURE_TVM_SYNC,
> +		  | RCAR_DU_FEATURE_TVM_SYNC
> +		  | RCAR_DU_FEATURE_CMM,
>  	.channels_mask = BIT(3) | BIT(1) | BIT(0),
>  	.routes = {
>  		/*
> @@ -357,7 +360,8 @@ static const struct rcar_du_device_info rcar_du_r8a77970_info = {
>  static const struct rcar_du_device_info rcar_du_r8a7799x_info = {
>  	.gen = 3,
>  	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
> -		  | RCAR_DU_FEATURE_VSP1_SOURCE,
> +		  | RCAR_DU_FEATURE_VSP1_SOURCE
> +		  | RCAR_DU_FEATURE_CMM,
>  	.channels_mask = BIT(1) | BIT(0),
>  	.routes = {
>  		/*
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> index 1327cd0df90a..a00dccc447aa 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> @@ -28,6 +28,7 @@ struct rcar_du_encoder;
>  #define RCAR_DU_FEATURE_VSP1_SOURCE	BIT(1)	/* Has inputs from VSP1 */
>  #define RCAR_DU_FEATURE_INTERLACED	BIT(2)	/* HW supports interlaced */
>  #define RCAR_DU_FEATURE_TVM_SYNC	BIT(3)	/* Has TV switch/sync modes */
> +#define RCAR_DU_FEATURE_CMM		BIT(4)	/* Has CMM */
>  
>  #define RCAR_DU_QUIRK_ALIGN_128B	BIT(0)	/* Align pitches to 128 bytes */
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 14/20] drm: rcar-du: Add support for CMM

[Laurent Pinchart]

Hi Jacopo,

On Fri, Jun 07, 2019 at 02:35:40PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Thu, Jun 06, 2019 at 04:22:14PM +0200, Jacopo Mondi wrote:
> > Add a driver for the R-Car Display Unit Color Correction Module.
> > 
> > Each DU output channel is provided with a CMM unit to perform image
> > enhancement and color correction.
> 
> I would say "On most Gen3 SoCs, each DU ..." as V3* SoCs have no CMM.
> 
> > 
> > Add support for CMM through a driver that supports configuration of
> > the 1-dimensional LUT table. More advanced CMM feature could be
> > implemented on top of this basic one.
> 
> s/could be/will be/ ? :-)
> 
> > 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/gpu/drm/rcar-du/Kconfig    |   7 +
> >  drivers/gpu/drm/rcar-du/Makefile   |   1 +
> >  drivers/gpu/drm/rcar-du/rcar_cmm.c | 197 +++++++++++++++++++++++++++++
> >  drivers/gpu/drm/rcar-du/rcar_cmm.h |  38 ++++++
> >  4 files changed, 243 insertions(+)
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.c
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.h
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
> > index 1529849e217e..539d232790d1 100644
> > --- a/drivers/gpu/drm/rcar-du/Kconfig
> > +++ b/drivers/gpu/drm/rcar-du/Kconfig
> > @@ -13,6 +13,13 @@ config DRM_RCAR_DU
> >  	  Choose this option if you have an R-Car chipset.
> >  	  If M is selected the module will be called rcar-du-drm.
> >  
> > +config DRM_RCAR_CMM
> > +	bool "R-Car DU Color Management Module (CMM) Support"
> > +	depends on DRM && OF
> > +	depends on DRM_RCAR_DU
> > +	help
> > +	  Enable support for R-Car Color Management Module (CMM).
> > +
> >  config DRM_RCAR_DW_HDMI
> >  	tristate "R-Car DU Gen3 HDMI Encoder Support"
> >  	depends on DRM && OF
> > diff --git a/drivers/gpu/drm/rcar-du/Makefile b/drivers/gpu/drm/rcar-du/Makefile
> > index 6c2ed9c46467..4d1187ccc3e5 100644
> > --- a/drivers/gpu/drm/rcar-du/Makefile
> > +++ b/drivers/gpu/drm/rcar-du/Makefile
> > @@ -15,6 +15,7 @@ rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS)	+= rcar_du_of.o \
> >  rcar-du-drm-$(CONFIG_DRM_RCAR_VSP)	+= rcar_du_vsp.o
> >  rcar-du-drm-$(CONFIG_DRM_RCAR_WRITEBACK) += rcar_du_writeback.o
> >  
> > +obj-$(CONFIG_DRM_RCAR_CMM)		+= rcar_cmm.o
> >  obj-$(CONFIG_DRM_RCAR_DU)		+= rcar-du-drm.o
> >  obj-$(CONFIG_DRM_RCAR_DW_HDMI)		+= rcar_dw_hdmi.o
> >  obj-$(CONFIG_DRM_RCAR_LVDS)		+= rcar_lvds.o
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> > new file mode 100644
> > index 000000000000..5d9d917b91f4
> > --- /dev/null
> > +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> > @@ -0,0 +1,197 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * rcar_cmm.c -- R-Car Display Unit Color Management Module
> > + *
> > + * Copyright (C) 2019 Jacopo Mondi <jacopo+renesas@jmondi.org>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <drm/drm_atomic.h>
> > +
> > +#include "rcar_cmm.h"
> > +
> > +#define CM2_LUT_CTRL		0x00
> 
> I would write all register addresses with 3 (or 4) digits.
> 
> > +#define CM2_LUT_CTRL_EN		BIT(0)
> > +#define CM2_LUT_TBLA		0x600
> 
> I would define this as
> 
> #define CM2_LUT_TBLA(n)		(0x600 + (n) * 4)
> 
> > +
> > +struct rcar_cmm {
> > +	struct clk *clk;
> > +	void __iomem *base;
> > +	bool enabled;
> > +
> > +	/* LUT table scratch buffer. */
> > +	struct {
> > +		bool restore;
> > +		unsigned int size;
> > +		uint32_t table[CMM_GAMMA_LUT_SIZE];
> > +	} lut;
> > +};
> > +
> > +static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg)
> > +{
> > +	return ioread32(rcmm->base + reg);
> > +}
> > +
> > +static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data)
> > +{
> > +	iowrite32(data, rcmm->base + reg);
> > +}
> > +
> > +int rcar_cmm_setup(struct platform_device *pdev, struct rcar_cmm_config *config)
> 
> Please document the functions exposed to the DU driver. It's hard to
> understand the setup vs. enable/disable split by just reading this
> driver.
> 
> > +{
> > +	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> > +	unsigned int i;
> > +
> > +	if (config->lut.size > CMM_GAMMA_LUT_SIZE)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * As cmm_setup is called by atomic commit tail helper, it might be
> > +	 * called before the enabling the CRTC (which calls cmm_enable()).
> > +	 *
> > +	 * Store the LUT table entries in the scratch buffer to be later
> > +	 * programmed at enable time.
> > +	 */
> > +	if (!rcmm->enabled) {
> > +		if (!config->lut.enable)
> > +			return 0;
> > +
> > +		for (i = 0; i < config->lut.size; ++i) {
> > +			struct drm_color_lut *lut = &config->lut.table[i];
> > +
> > +			rcmm->lut.table[i] = (lut->red & 0xff) << 16 |
> > +					     (lut->green & 0xff) << 8 |
> > +					     (lut->blue & 0xff);
> > +		}
> > +
> > +		rcmm->lut.restore = true;
> > +		rcmm->lut.size = config->lut.size;
> > +
> > +		return 0;
> > +	}
> > +
> > +	if (rcar_cmm_read(rcmm, CM2_LUT_CTRL) & CM2_LUT_CTRL_EN &&
> > +	    !config->lut.enable) {
> > +		rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> > +		return 0;
> > +	}
> > +
> > +	/* Enable LUT and program the new gamma table values. */
> > +	rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> 
> Shouldn't you write the LUT contents before enabling it (same below) ?
> 
> > +	for (i = 0; i < config->lut.size; ++i) {
> > +		struct drm_color_lut *lut = &config->lut.table[i];
> > +		u32 val = (lut->red & 0xff) << 16 | (lut->green & 0xff) << 8 |
> > +			  (lut->blue & 0xff);
> 
> Do you need to recompute the value, can't you use rcmm->lut.table ?
> 
> > +
> > +		rcar_cmm_write(rcmm, CM2_LUT_TBLA + i * 4, val);
> > +	}
> > +
> > +	return 0;
> > +}
> 
> You need to export this and the next two functions.
> 
> > +
> > +int rcar_cmm_enable(struct platform_device *pdev)
> > +{
> > +	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	if (rcmm->enabled)
> > +		return 0;
> 
> Can this happen without a bug in the caller ? If it can, and assuming
> the caller balances the enable and disable calls, you will have
> unbalanced clk_prepare_enable() and clk_disable_unprepare() calls.
> 
> > +
> > +	ret = clk_prepare_enable(rcmm->clk);
> > +	if (ret)
> > +		return ret;
> 
> Could you use pm_runtime_get_sync() instead ?
> 
> > +
> > +	/* Apply the LUT table values saved at cmm_setup time. */
> > +	if (rcmm->lut.restore) {
> > +		rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> > +		for (i = 0; i < rcmm->lut.size; ++i)
> > +			rcar_cmm_write(rcmm, CM2_LUT_TBLA + i * 4,
> > +				       rcmm->lut.table[i]);
> > +
> > +		rcmm->lut.restore = false;
> > +		rcmm->lut.size = 0;
> > +	}
> > +
> > +	rcmm->enabled = true;
> > +
> > +	return 0;
> > +}
> > +
> > +void rcar_cmm_disable(struct platform_device *pdev)
> > +{
> > +	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> > +
> > +	rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> > +
> > +	clk_disable_unprepare(rcmm->clk);
> > +
> > +	rcmm->lut.restore = false;
> > +	rcmm->lut.size = 0;
> > +	rcmm->enabled = false;
> > +}
> > +
> > +static int rcar_cmm_probe(struct platform_device *pdev)
> > +{
> > +	struct rcar_cmm *rcmm;
> > +	struct resource *res;
> > +	resource_size_t size;
> > +
> > +	rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
> > +	if (!rcmm)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, rcmm);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	size = resource_size(res);
> > +	if (!devm_request_mem_region(&pdev->dev, res->start, size,
> > +				     dev_name(&pdev->dev))) {
> > +		dev_err(&pdev->dev,
> > +			"can't request region for resource %pR\n", res);
> > +		return -EBUSY;
> > +	}
> > +
> > +	rcmm->base = devm_ioremap_nocache(&pdev->dev, res->start, size);
> > +	if (IS_ERR(rcmm->base))
> > +		return PTR_ERR(rcmm->base);
> 
> Anything wrong with devm_ioremap_resource() ?
> 
> > +
> > +	rcmm->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(rcmm->clk)) {
> > +		dev_err(&pdev->dev, "Failed to get CMM clock");
> > +		return PTR_ERR(rcmm->clk);
> > +	}
> > +
> > +	rcmm->lut.restore = false;
> > +	rcmm->lut.size = 0;
> > +	rcmm->enabled = false;
> 
> As you allocate memory with kzalloc() you could skip this.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id rcar_cmm_of_table[] = {
> > +	{ .compatible = "renesas,cmm-gen3" },
> > +	{ .compatible = "renesas,cmm-gen2" },
> > +	{ },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
> > +
> > +static struct platform_driver rcar_cmm_platform_driver = {
> > +	.probe		= rcar_cmm_probe,
> > +	.driver		= {
> > +		.name	= "rcar-cmm",
> > +		.of_match_table = rcar_cmm_of_table,
> > +	},
> 
> No need for suspend/resume support ? The DU driver should disable/enable
> the CMM in its suspend/resume paths, so this should be fine, but won't
> the LUT contents be lost and need to be restored ?
> 
> > +};
> > +
> > +module_platform_driver(rcar_cmm_platform_driver);
> > +
> > +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org");
> 
> Missing >.
> 
> > +MODULE_DESCRIPTION("Renesas R-Car CMM Driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h
> > new file mode 100644
> > index 000000000000..da61a145dc5c
> > --- /dev/null
> > +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h
> > @@ -0,0 +1,38 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> 
> The .c and .h file licenses don't match.
> 
> > +/*
> > + * rcar_cmm.h -- R-Car Display Unit Color Management Module
> > + *
> > + * Copyright (C) 2019 Jacopo Mondi <jacopo+renesas@jmondi.org>
> > + */
> > +
> > +#ifndef __RCAR_CMM_H__
> > +#define __RCAR_CMM_H__
> > +
> > +#include <linux/platform_device.h>
> 
> You can forward-declare struct platform_device instead.
> 
> > +
> > +#define CMM_GAMMA_LUT_SIZE		256
> > +
> > +struct drm_color_lut;
> > +
> > +/**
> > + * struct rcar_cmm_config - CMM configuration
> > + *
> > + * @lut:	1D-LUT configuration
> > + * @lut.enable:	1D-LUT enable flag
> > + * @lut.table:	1D-LUT table entries.
> > + * @lut.size	1D-LUT number of entries. Max is 256
> 
> The last line is missing a colon.
> 
> > + */
> > +struct rcar_cmm_config {
> > +	struct {
> > +		bool enable;
> > +		struct drm_color_lut *table;
> > +		unsigned int size;
> > +	} lut;
> > +};
> > +
> > +int rcar_cmm_enable(struct platform_device *);
> 
> As the OF API looks up a struct device from a device_node, should we use
> struct device here ?

My bad, the API actually returns a struct platform_device. I would still
prefer using struct device, but it then becomes more of a personal
preference.

> > +void rcar_cmm_disable(struct platform_device *);
> 
> I find headers more readable when function arguments are named. In this
> case the types probably provide enough information, but good luck trying
> to read a function such as
> 
> int foo(int, int, bool);
> 
> > +
> > +int rcar_cmm_setup(struct platform_device *, struct rcar_cmm_config *);
> 
> Can the second argument be const ?
> 
> > +
> > +#endif /* __RCAR_CMM_H__ */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 16/20] drm: rcar-du: kms: Collect CMM instances

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:16PM +0200, Jacopo Mondi wrote:
> Implement device tree parsing to collect the available CMM instances
> described by the 'cmms' property. Associate CMMs with CRTCs and store a
> mask of active CMMs in the DU group for later enablement.
> 
> Also define a new feature to let each SoC claim support for CMM.

The feature is added in the previous patch.

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c  |  7 ++++
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h  |  2 +
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h   |  3 ++
>  drivers/gpu/drm/rcar-du/rcar_du_group.h |  2 +
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c   | 50 +++++++++++++++++++++++++
>  5 files changed, 64 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 2da46e3dc4ae..9f270a54b164 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -1194,6 +1194,13 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
>  	if (ret < 0)
>  		return ret;
>  
> +	/* CMM might be disabled for this CRTC. */
> +	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM) &&
> +	    rcdu->cmms[swindex]) {

You can skip testing RCAR_DU_FEATURE_CMM here as rcdu->cmms[swindex]
can't be set if the feature isn't available.

> +		rcrtc->cmm = rcdu->cmms[swindex];
> +		rgrp->cmms_mask |= BIT(hwindex % 2);
> +	}
> +
>  	drm_crtc_helper_add(crtc, &crtc_helper_funcs);
>  
>  	/* Start with vertical blanking interrupt reporting disabled. */
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> index 3b7fc668996f..5f2940c42225 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -39,6 +39,7 @@ struct rcar_du_vsp;
>   * @vblank_wait: wait queue used to signal vertical blanking
>   * @vblank_count: number of vertical blanking interrupts to wait for
>   * @group: CRTC group this CRTC belongs to
> + * @cmm: CMM associated with this CRTC
>   * @vsp: VSP feeding video to this CRTC
>   * @vsp_pipe: index of the VSP pipeline feeding video to this CRTC
>   * @writeback: the writeback connector
> @@ -64,6 +65,7 @@ struct rcar_du_crtc {
>  	unsigned int vblank_count;
>  
>  	struct rcar_du_group *group;
> +	struct platform_device *cmm;
>  	struct rcar_du_vsp *vsp;
>  	unsigned int vsp_pipe;
>  
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> index a00dccc447aa..300ec60ba31b 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> @@ -13,6 +13,7 @@
>  #include <linux/kernel.h>
>  #include <linux/wait.h>
>  
> +#include "rcar_cmm.h"
>  #include "rcar_du_crtc.h"
>  #include "rcar_du_group.h"
>  #include "rcar_du_vsp.h"
> @@ -70,6 +71,7 @@ struct rcar_du_device_info {
>  
>  #define RCAR_DU_MAX_CRTCS		4
>  #define RCAR_DU_MAX_GROUPS		DIV_ROUND_UP(RCAR_DU_MAX_CRTCS, 2)
> +#define RCAR_DU_MAX_CMMS		4
>  #define RCAR_DU_MAX_VSPS		4
>  
>  struct rcar_du_device {
> @@ -86,6 +88,7 @@ struct rcar_du_device {
>  	struct rcar_du_encoder *encoders[RCAR_DU_OUTPUT_MAX];
>  
>  	struct rcar_du_group groups[RCAR_DU_MAX_GROUPS];
> +	struct platform_device *cmms[RCAR_DU_MAX_CMMS];
>  	struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS];
>  
>  	struct {
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> index 87950c1f6a52..b0c1466593a3 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> @@ -22,6 +22,7 @@ struct rcar_du_device;
>   * @mmio_offset: registers offset in the device memory map
>   * @index: group index
>   * @channels_mask: bitmask of populated DU channels in this group
> + * @cmms_mask: bitmask of enabled CMMs in this group
>   * @num_crtcs: number of CRTCs in this group (1 or 2)
>   * @use_count: number of users of the group (rcar_du_group_(get|put))
>   * @used_crtcs: number of CRTCs currently in use
> @@ -37,6 +38,7 @@ struct rcar_du_group {
>  	unsigned int index;
>  
>  	unsigned int channels_mask;
> +	unsigned int cmms_mask;
>  	unsigned int num_crtcs;
>  	unsigned int use_count;
>  	unsigned int used_crtcs;
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index adbc4f5d8fc5..5a910a04e1d9 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -18,6 +18,7 @@
>  #include <drm/drm_vblank.h>
>  
>  #include <linux/of_graph.h>
> +#include <linux/of_platform.h>
>  #include <linux/wait.h>
>  
>  #include "rcar_du_crtc.h"
> @@ -614,6 +615,48 @@ static int rcar_du_vsps_init(struct rcar_du_device *rcdu)
>  	return ret;
>  }
>  
> +static int rcar_du_cmm_init(struct rcar_du_device *rcdu)
> +{
> +	const struct device_node *np = rcdu->dev->of_node;
> +	unsigned int cells;
> +	unsigned int i;
> +
> +	cells = of_property_count_u32_elems(np, "cmms");
> +	if (cells > RCAR_DU_MAX_CMMS || cells > rcdu->num_crtcs) {
> +		dev_err(rcdu->dev, "invalid 'cmms' property format\n");
> +		return -EINVAL;
> +	}

I don't think you need to count the elements first, you can loop over
the number of CRTCs below.

> +
> +	for (i = 0; i < cells; ++i) {
> +		struct platform_device *pdev;
> +		struct device_node *cmm;
> +
> +		cmm = of_parse_phandle(np, "cmms", i);
> +		if (IS_ERR(cmm)) {
> +			dev_err(rcdu->dev, "failed to parse 'cmms' property\n");
> +			return PTR_ERR(cmm);
> +		}
> +
> +		pdev = of_find_device_by_node(cmm);
> +		if (IS_ERR(pdev)) {
> +			dev_err(rcdu->dev, "invalid property 'cmms'[%u]\n", i);

How about "No device found for cmms[%u]\n" ?

> +			of_node_put(cmm);
> +			return PTR_ERR(pdev);
> +		}
> +
> +		if (!of_device_is_available(cmm)) {
> +			/* It's fine to have a phandle to a non-enabled CMM. */
> +			of_node_put(cmm);
> +			continue;
> +		}
> +
> +		of_node_put(cmm);
> +		rcdu->cmms[i] = pdev;
> +	}
> +
> +	return 0;
> +}
> +
>  int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>  {
>  	static const unsigned int mmio_offsets[] = {
> @@ -704,6 +747,13 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>  			return ret;
>  	}
>  
> +	/* Initialize the Color Management Modules. */
> +	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM)) {

Instead of using a feature, could we simply return 0 in
rcar_du_cmm_init() if the cmms property doesn't exist ? This will
preserve compatibility with older DTs.

> +		ret = rcar_du_cmm_init(rcdu);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	/* Create the CRTCs. */
>  	for (swindex = 0, hwindex = 0; swindex < rcdu->num_crtcs; ++hwindex) {
>  		struct rcar_du_group *rgrp;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 17/20] drm: rcar-du: crtc: Enable and disable CMMs

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:17PM +0200, Jacopo Mondi wrote:
> Enable/disable the CMM associated with a CRTC at
> atomic_enable()/atomic_disable() time.

I would squash this patch with the next 3. It's hard to review them
independently.

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 9f270a54b164..e6d3df37c827 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -21,6 +21,7 @@
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_vblank.h>
>  
> +#include "rcar_cmm.h"
>  #include "rcar_du_crtc.h"
>  #include "rcar_du_drv.h"
>  #include "rcar_du_encoder.h"
> @@ -523,6 +524,7 @@ static int rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
>  		goto error_group;
>  
>  	rcar_du_crtc_setup(rcrtc);
> +

Unrelated change ?

>  	rcrtc->initialized = true;
>  
>  	return 0;
> @@ -619,6 +621,9 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
>  	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
>  		rcar_du_vsp_disable(rcrtc);
>  
> +	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_CMM) && rcrtc->cmm)

You can just test rcrtc->cmm. Same below.

> +		rcar_cmm_disable(rcrtc->cmm);
> +
>  	/*
>  	 * Select switch sync mode. This stops display operation and configures
>  	 * the HSYNC and VSYNC signals as inputs.
> @@ -686,6 +691,9 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
>  	}
>  
>  	rcar_du_crtc_start(rcrtc);
> +
> +	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_CMM) && rcrtc->cmm)
> +		rcar_cmm_enable(rcrtc->cmm);
>  }
>  
>  static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 18/20] drm: rcar-du: group: Enable DU's CMM extension

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:18PM +0200, Jacopo Mondi wrote:
> Enable the CMM units through the display unit extensional function control
> group register DEFR7.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_group.c | 8 ++++++++
>  drivers/gpu/drm/rcar-du/rcar_du_regs.h  | 5 +++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> index 9eee47969e77..d252c9bb9809 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> @@ -147,6 +147,14 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
>  
>  	rcar_du_group_setup_pins(rgrp);
>  
> +	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM)) {

This is a good enough reason to keep RCAR_DU_FEATURE_CMM :-/

> +		u32 defr7 = DEFR7_CODE |
> +			    (rgrp->cmms_mask & BIT(1) ? DEFR7_CMME1 : 0) |
> +			    (rgrp->cmms_mask & BIT(0) ? DEFR7_CMME0 : 0);
> +
> +		rcar_du_group_write(rgrp, DEFR7, defr7);

It would be nice to disable the CMM when the LUT isn't used, but that
would be difficult at the moment. We can revisit this when Kieran's DU
group handling series will land.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	}
> +
>  	if (rcdu->info->gen >= 2) {
>  		rcar_du_group_setup_defr8(rgrp);
>  		rcar_du_group_setup_didsr(rgrp);
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> index bc87f080b170..fb9964949368 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> @@ -197,6 +197,11 @@
>  #define DEFR6_MLOS1		(1 << 2)
>  #define DEFR6_DEFAULT		(DEFR6_CODE | DEFR6_TCNE1)
>  
> +#define DEFR7			0x000ec
> +#define DEFR7_CODE		(0x7779 << 16)
> +#define DEFR7_CMME1		BIT(6)
> +#define DEFR7_CMME0		BIT(4)
> +
>  /* -----------------------------------------------------------------------------
>   * R8A7790-only Control Registers
>   */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 19/20] drm: rcar-du: crtc: Register GAMMA_LUT properties

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:19PM +0200, Jacopo Mondi wrote:
> Enable the GAMMA_LUT KMS property using the framework helpers to
> register the proeprty and the associated gamma table size maximum size.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index e6d3df37c827..c920fb5dba65 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -1207,6 +1207,9 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
>  	    rcdu->cmms[swindex]) {
>  		rcrtc->cmm = rcdu->cmms[swindex];
>  		rgrp->cmms_mask |= BIT(hwindex % 2);
> +
> +		drm_mode_crtc_set_gamma_size(crtc, CMM_GAMMA_LUT_SIZE);
> +		drm_crtc_enable_color_mgmt(crtc, 0, false, CMM_GAMMA_LUT_SIZE);

This change looks good, but you also need to add support for legacy API.
According to the function's documentation,

 * Drivers should use drm_atomic_helper_legacy_gamma_set() to implement the
 * legacy &drm_crtc_funcs.gamma_set callback.

>  	}
>  
>  	drm_crtc_helper_add(crtc, &crtc_helper_funcs);
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 20/20] drm: rcar-du: kms: Update CMM in atomic commit tail

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:20PM +0200, Jacopo Mondi wrote:
> Update CMM settings at in the atomic commit tail helper method.
> 
> The CMM is updated with new gamma values provided to the driver
> in the GAMMA_LUT blob property.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c | 36 +++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index 5a910a04e1d9..29a2020a46b5 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -21,6 +21,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/wait.h>
>  
> +#include "rcar_cmm.h"
>  #include "rcar_du_crtc.h"
>  #include "rcar_du_drv.h"
>  #include "rcar_du_encoder.h"
> @@ -367,6 +368,38 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv,
>   * Atomic Check and Update
>   */
>  
> +static void rcar_du_atomic_commit_update_cmm(struct drm_crtc *crtc,
> +					     struct drm_crtc_state *old_state)
> +{
> +	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> +	struct rcar_cmm_config cmm_config = {};
> +
> +	if (!rcrtc->cmm || !crtc->state->color_mgmt_changed)
> +		return;
> +
> +	if (!crtc->state->gamma_lut) {
> +		cmm_config.lut.enable = false;
> +		rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> +
> +		return;
> +	}
> +
> +	cmm_config.lut.enable = true;
> +	cmm_config.lut.table = (struct drm_color_lut *)
> +			       crtc->state->gamma_lut->data;
> +
> +	/* Set LUT table size to 0 if entries should not be updated. */
> +	if (!old_state->gamma_lut ||
> +	    (old_state->gamma_lut->base.id !=
> +	    crtc->state->gamma_lut->base.id))
> +		cmm_config.lut.size = crtc->state->gamma_lut->length
> +				    / sizeof(cmm_config.lut.table[0]);

Do you need to call rcar_cmm_setup() at all in this case ?

> +	else
> +		cmm_config.lut.size = 0;
> +
> +	rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> +}
> +
>  static int rcar_du_atomic_check(struct drm_device *dev,
>  				struct drm_atomic_state *state)
>  {
> @@ -409,6 +442,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
>  			rcdu->dpad1_source = rcrtc->index;
>  	}
>  
> +	for_each_old_crtc_in_state(old_state, crtc, crtc_state, i)
> +		rcar_du_atomic_commit_update_cmm(crtc, crtc_state);
> +
>  	/* Apply the atomic update. */
>  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
>  	drm_atomic_helper_commit_planes(dev, old_state,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 14/20] drm: rcar-du: Add support for CMM

[Laurent Pinchart]

Hi Jacopo,

Now that I've read the whole series, here are a few comments.

On Fri, Jun 07, 2019 at 02:35:40PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Thu, Jun 06, 2019 at 04:22:14PM +0200, Jacopo Mondi wrote:
> > Add a driver for the R-Car Display Unit Color Correction Module.
> > 
> > Each DU output channel is provided with a CMM unit to perform image
> > enhancement and color correction.
> 
> I would say "On most Gen3 SoCs, each DU ..." as V3* SoCs have no CMM.
> 
> > 
> > Add support for CMM through a driver that supports configuration of
> > the 1-dimensional LUT table. More advanced CMM feature could be
> > implemented on top of this basic one.
> 
> s/could be/will be/ ? :-)
> 
> > 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/gpu/drm/rcar-du/Kconfig    |   7 +
> >  drivers/gpu/drm/rcar-du/Makefile   |   1 +
> >  drivers/gpu/drm/rcar-du/rcar_cmm.c | 197 +++++++++++++++++++++++++++++
> >  drivers/gpu/drm/rcar-du/rcar_cmm.h |  38 ++++++
> >  4 files changed, 243 insertions(+)
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.c
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.h
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
> > index 1529849e217e..539d232790d1 100644
> > --- a/drivers/gpu/drm/rcar-du/Kconfig
> > +++ b/drivers/gpu/drm/rcar-du/Kconfig
> > @@ -13,6 +13,13 @@ config DRM_RCAR_DU
> >  	  Choose this option if you have an R-Car chipset.
> >  	  If M is selected the module will be called rcar-du-drm.
> >  
> > +config DRM_RCAR_CMM
> > +	bool "R-Car DU Color Management Module (CMM) Support"
> > +	depends on DRM && OF
> > +	depends on DRM_RCAR_DU
> > +	help
> > +	  Enable support for R-Car Color Management Module (CMM).
> > +
> >  config DRM_RCAR_DW_HDMI
> >  	tristate "R-Car DU Gen3 HDMI Encoder Support"
> >  	depends on DRM && OF
> > diff --git a/drivers/gpu/drm/rcar-du/Makefile b/drivers/gpu/drm/rcar-du/Makefile
> > index 6c2ed9c46467..4d1187ccc3e5 100644
> > --- a/drivers/gpu/drm/rcar-du/Makefile
> > +++ b/drivers/gpu/drm/rcar-du/Makefile
> > @@ -15,6 +15,7 @@ rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS)	+= rcar_du_of.o \
> >  rcar-du-drm-$(CONFIG_DRM_RCAR_VSP)	+= rcar_du_vsp.o
> >  rcar-du-drm-$(CONFIG_DRM_RCAR_WRITEBACK) += rcar_du_writeback.o
> >  
> > +obj-$(CONFIG_DRM_RCAR_CMM)		+= rcar_cmm.o
> >  obj-$(CONFIG_DRM_RCAR_DU)		+= rcar-du-drm.o
> >  obj-$(CONFIG_DRM_RCAR_DW_HDMI)		+= rcar_dw_hdmi.o
> >  obj-$(CONFIG_DRM_RCAR_LVDS)		+= rcar_lvds.o
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> > new file mode 100644
> > index 000000000000..5d9d917b91f4
> > --- /dev/null
> > +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> > @@ -0,0 +1,197 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * rcar_cmm.c -- R-Car Display Unit Color Management Module
> > + *
> > + * Copyright (C) 2019 Jacopo Mondi <jacopo+renesas@jmondi.org>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <drm/drm_atomic.h>
> > +
> > +#include "rcar_cmm.h"
> > +
> > +#define CM2_LUT_CTRL		0x00
> 
> I would write all register addresses with 3 (or 4) digits.
> 
> > +#define CM2_LUT_CTRL_EN		BIT(0)
> > +#define CM2_LUT_TBLA		0x600
> 
> I would define this as
> 
> #define CM2_LUT_TBLA(n)		(0x600 + (n) * 4)
> 
> > +
> > +struct rcar_cmm {
> > +	struct clk *clk;
> > +	void __iomem *base;
> > +	bool enabled;
> > +
> > +	/* LUT table scratch buffer. */
> > +	struct {
> > +		bool restore;
> > +		unsigned int size;
> > +		uint32_t table[CMM_GAMMA_LUT_SIZE];
> > +	} lut;
> > +};
> > +
> > +static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg)
> > +{
> > +	return ioread32(rcmm->base + reg);
> > +}
> > +
> > +static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data)
> > +{
> > +	iowrite32(data, rcmm->base + reg);
> > +}
> > +
> > +int rcar_cmm_setup(struct platform_device *pdev, struct rcar_cmm_config *config)
> 
> Please document the functions exposed to the DU driver. It's hard to
> understand the setup vs. enable/disable split by just reading this
> driver.
> 
> > +{
> > +	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> > +	unsigned int i;
> > +
> > +	if (config->lut.size > CMM_GAMMA_LUT_SIZE)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * As cmm_setup is called by atomic commit tail helper, it might be
> > +	 * called before the enabling the CRTC (which calls cmm_enable()).
> > +	 *
> > +	 * Store the LUT table entries in the scratch buffer to be later
> > +	 * programmed at enable time.
> > +	 */

I think we'll be able to simplify this once Kieran's DU group handling
patches land. Let's see which series gets merged first :-)

> > +	if (!rcmm->enabled) {
> > +		if (!config->lut.enable)
> > +			return 0;
> > +
> > +		for (i = 0; i < config->lut.size; ++i) {
> > +			struct drm_color_lut *lut = &config->lut.table[i];
> > +
> > +			rcmm->lut.table[i] = (lut->red & 0xff) << 16 |
> > +					     (lut->green & 0xff) << 8 |
> > +					     (lut->blue & 0xff);
> > +		}
> > +
> > +		rcmm->lut.restore = true;
> > +		rcmm->lut.size = config->lut.size;
> > +
> > +		return 0;
> > +	}
> > +
> > +	if (rcar_cmm_read(rcmm, CM2_LUT_CTRL) & CM2_LUT_CTRL_EN &&
> > +	    !config->lut.enable) {
> > +		rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> > +		return 0;
> > +	}
> > +
> > +	/* Enable LUT and program the new gamma table values. */
> > +	rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> 
> Shouldn't you write the LUT contents before enabling it (same below) ?
> 
> > +	for (i = 0; i < config->lut.size; ++i) {
> > +		struct drm_color_lut *lut = &config->lut.table[i];
> > +		u32 val = (lut->red & 0xff) << 16 | (lut->green & 0xff) << 8 |
> > +			  (lut->blue & 0xff);
> 
> Do you need to recompute the value, can't you use rcmm->lut.table ?
> 
> > +
> > +		rcar_cmm_write(rcmm, CM2_LUT_TBLA + i * 4, val);
> > +	}
> > +
> > +	return 0;
> > +}
> 
> You need to export this and the next two functions.
> 
> > +
> > +int rcar_cmm_enable(struct platform_device *pdev)
> > +{
> > +	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);

What if this function gets called before the CMM is probed ? I think you
need an equivalent to vsp1_du_init() to handle this.

> > +	unsigned int i;
> > +	int ret;
> > +
> > +	if (rcmm->enabled)
> > +		return 0;
> 
> Can this happen without a bug in the caller ? If it can, and assuming
> the caller balances the enable and disable calls, you will have
> unbalanced clk_prepare_enable() and clk_disable_unprepare() calls.
> 
> > +
> > +	ret = clk_prepare_enable(rcmm->clk);
> > +	if (ret)
> > +		return ret;
> 
> Could you use pm_runtime_get_sync() instead ?
> 
> > +
> > +	/* Apply the LUT table values saved at cmm_setup time. */
> > +	if (rcmm->lut.restore) {
> > +		rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> > +		for (i = 0; i < rcmm->lut.size; ++i)
> > +			rcar_cmm_write(rcmm, CM2_LUT_TBLA + i * 4,
> > +				       rcmm->lut.table[i]);
> > +
> > +		rcmm->lut.restore = false;
> > +		rcmm->lut.size = 0;
> > +	}

I would move this code to a separate function called by both
rcar_cmm_setup() and rcar_cmm_enable().

> > +
> > +	rcmm->enabled = true;
> > +
> > +	return 0;
> > +}
> > +
> > +void rcar_cmm_disable(struct platform_device *pdev)
> > +{
> > +	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> > +
> > +	rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> > +
> > +	clk_disable_unprepare(rcmm->clk);
> > +
> > +	rcmm->lut.restore = false;
> > +	rcmm->lut.size = 0;
> > +	rcmm->enabled = false;
> > +}
> > +
> > +static int rcar_cmm_probe(struct platform_device *pdev)
> > +{
> > +	struct rcar_cmm *rcmm;
> > +	struct resource *res;
> > +	resource_size_t size;
> > +
> > +	rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
> > +	if (!rcmm)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, rcmm);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	size = resource_size(res);
> > +	if (!devm_request_mem_region(&pdev->dev, res->start, size,
> > +				     dev_name(&pdev->dev))) {
> > +		dev_err(&pdev->dev,
> > +			"can't request region for resource %pR\n", res);
> > +		return -EBUSY;
> > +	}
> > +
> > +	rcmm->base = devm_ioremap_nocache(&pdev->dev, res->start, size);
> > +	if (IS_ERR(rcmm->base))
> > +		return PTR_ERR(rcmm->base);
> 
> Anything wrong with devm_ioremap_resource() ?
> 
> > +
> > +	rcmm->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(rcmm->clk)) {
> > +		dev_err(&pdev->dev, "Failed to get CMM clock");
> > +		return PTR_ERR(rcmm->clk);
> > +	}
> > +
> > +	rcmm->lut.restore = false;
> > +	rcmm->lut.size = 0;
> > +	rcmm->enabled = false;
> 
> As you allocate memory with kzalloc() you could skip this.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id rcar_cmm_of_table[] = {
> > +	{ .compatible = "renesas,cmm-gen3" },
> > +	{ .compatible = "renesas,cmm-gen2" },
> > +	{ },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
> > +
> > +static struct platform_driver rcar_cmm_platform_driver = {
> > +	.probe		= rcar_cmm_probe,
> > +	.driver		= {
> > +		.name	= "rcar-cmm",
> > +		.of_match_table = rcar_cmm_of_table,
> > +	},
> 
> No need for suspend/resume support ? The DU driver should disable/enable
> the CMM in its suspend/resume paths, so this should be fine, but won't
> the LUT contents be lost and need to be restored ?
> 
> > +};
> > +
> > +module_platform_driver(rcar_cmm_platform_driver);
> > +
> > +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org");
> 
> Missing >.
> 
> > +MODULE_DESCRIPTION("Renesas R-Car CMM Driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h
> > new file mode 100644
> > index 000000000000..da61a145dc5c
> > --- /dev/null
> > +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h
> > @@ -0,0 +1,38 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> 
> The .c and .h file licenses don't match.
> 
> > +/*
> > + * rcar_cmm.h -- R-Car Display Unit Color Management Module
> > + *
> > + * Copyright (C) 2019 Jacopo Mondi <jacopo+renesas@jmondi.org>
> > + */
> > +
> > +#ifndef __RCAR_CMM_H__
> > +#define __RCAR_CMM_H__
> > +
> > +#include <linux/platform_device.h>
> 
> You can forward-declare struct platform_device instead.
> 
> > +
> > +#define CMM_GAMMA_LUT_SIZE		256
> > +
> > +struct drm_color_lut;
> > +
> > +/**
> > + * struct rcar_cmm_config - CMM configuration
> > + *
> > + * @lut:	1D-LUT configuration
> > + * @lut.enable:	1D-LUT enable flag
> > + * @lut.table:	1D-LUT table entries.
> > + * @lut.size	1D-LUT number of entries. Max is 256
> 
> The last line is missing a colon.
> 
> > + */
> > +struct rcar_cmm_config {
> > +	struct {
> > +		bool enable;
> > +		struct drm_color_lut *table;
> > +		unsigned int size;
> > +	} lut;
> > +};
> > +
> > +int rcar_cmm_enable(struct platform_device *);
> 
> As the OF API looks up a struct device from a device_node, should we use
> struct device here ?
> 
> > +void rcar_cmm_disable(struct platform_device *);
> 
> I find headers more readable when function arguments are named. In this
> case the types probably provide enough information, but good luck trying
> to read a function such as
> 
> int foo(int, int, bool);
> 
> > +
> > +int rcar_cmm_setup(struct platform_device *, struct rcar_cmm_config *);
> 
> Can the second argument be const ?
> 
> > +
> > +#endif /* __RCAR_CMM_H__ */
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,

Laurent Pinchart

Re: [PATCH 05/20] clk: renesas: r8a7795: Add CMM clocks

[Geert Uytterhoeven]

On Thu, Jun 6, 2019 at 4:22 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> Add clock definitions for CMM units on Renesas R-Car Gen3 H3.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in clk-renesas-for-v5.3.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 06/20] clk: renesas: r8a77965: Add CMM clocks

[Geert Uytterhoeven]

On Thu, Jun 6, 2019 at 4:22 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> Add clock definitions for CMM units on Renesas R-Car Gen3 M3-N.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in clk-renesas-for-v5.3.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 07/20] clk: renesas: r8a77990: Add CMM clocks

[Geert Uytterhoeven]

On Thu, Jun 6, 2019 at 4:25 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> Add clock definitions for CMM units on Renesas R-Car Gen3 E3.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in clk-renesas-for-v5.3.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 08/20] clk: renesas: r8a77995: Add CMM clocks

[Geert Uytterhoeven]

On Thu, Jun 6, 2019 at 4:22 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> Add clock definitions for CMM units on Renesas R-Car Gen3 D3.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in clk-renesas-for-v5.3.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 19/20] drm: rcar-du: crtc: Register GAMMA_LUT properties

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 1600 bytes --]

Hi Laurent,
   thanks for review

On Fri, Jun 07, 2019 at 03:03:04PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Jun 06, 2019 at 04:22:19PM +0200, Jacopo Mondi wrote:
> > Enable the GAMMA_LUT KMS property using the framework helpers to
> > register the proeprty and the associated gamma table size maximum size.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > index e6d3df37c827..c920fb5dba65 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > @@ -1207,6 +1207,9 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
> >  	    rcdu->cmms[swindex]) {
> >  		rcrtc->cmm = rcdu->cmms[swindex];
> >  		rgrp->cmms_mask |= BIT(hwindex % 2);
> > +
> > +		drm_mode_crtc_set_gamma_size(crtc, CMM_GAMMA_LUT_SIZE);
> > +		drm_crtc_enable_color_mgmt(crtc, 0, false, CMM_GAMMA_LUT_SIZE);
>
> This change looks good, but you also need to add support for legacy API.
> According to the function's documentation,
>
>  * Drivers should use drm_atomic_helper_legacy_gamma_set() to implement the
>  * legacy &drm_crtc_funcs.gamma_set callback.
>

Drivers 'shuld' or drivers 'shall' ?
Isn't this required only to support the 'legacy APIs' ? Do we want that?

Thanks
   j

> >  	}
> >
> >  	drm_crtc_helper_add(crtc, &crtc_helper_funcs);
> >
>
> --
> Regards,
>
> Laurent Pinchart

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 20/20] drm: rcar-du: kms: Update CMM in atomic commit tail

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 3021 bytes --]

Hi Laurent,

On Fri, Jun 07, 2019 at 03:06:33PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Jun 06, 2019 at 04:22:20PM +0200, Jacopo Mondi wrote:
> > Update CMM settings at in the atomic commit tail helper method.
> >
> > The CMM is updated with new gamma values provided to the driver
> > in the GAMMA_LUT blob property.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_du_kms.c | 36 +++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > index 5a910a04e1d9..29a2020a46b5 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > @@ -21,6 +21,7 @@
> >  #include <linux/of_platform.h>
> >  #include <linux/wait.h>
> >
> > +#include "rcar_cmm.h"
> >  #include "rcar_du_crtc.h"
> >  #include "rcar_du_drv.h"
> >  #include "rcar_du_encoder.h"
> > @@ -367,6 +368,38 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv,
> >   * Atomic Check and Update
> >   */
> >
> > +static void rcar_du_atomic_commit_update_cmm(struct drm_crtc *crtc,
> > +					     struct drm_crtc_state *old_state)
> > +{
> > +	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> > +	struct rcar_cmm_config cmm_config = {};
> > +
> > +	if (!rcrtc->cmm || !crtc->state->color_mgmt_changed)
> > +		return;
> > +
> > +	if (!crtc->state->gamma_lut) {
> > +		cmm_config.lut.enable = false;
> > +		rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> > +
> > +		return;
> > +	}
> > +
> > +	cmm_config.lut.enable = true;
> > +	cmm_config.lut.table = (struct drm_color_lut *)
> > +			       crtc->state->gamma_lut->data;
> > +
> > +	/* Set LUT table size to 0 if entries should not be updated. */
> > +	if (!old_state->gamma_lut ||
> > +	    (old_state->gamma_lut->base.id !=
> > +	    crtc->state->gamma_lut->base.id))
> > +		cmm_config.lut.size = crtc->state->gamma_lut->length
> > +				    / sizeof(cmm_config.lut.table[0]);
>
> Do you need to call rcar_cmm_setup() at all in this case ?
>

Do you mean in case the lut.size field is set to 0 ?
I considered it useful when the CMM has to be re-enabled without
updateing the LUT table entries? It is not required in your opinion?

Thanks
   j

> > +	else
> > +		cmm_config.lut.size = 0;
> > +
> > +	rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> > +}
> > +
> >  static int rcar_du_atomic_check(struct drm_device *dev,
> >  				struct drm_atomic_state *state)
> >  {
> > @@ -409,6 +442,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
> >  			rcdu->dpad1_source = rcrtc->index;
> >  	}
> >
> > +	for_each_old_crtc_in_state(old_state, crtc, crtc_state, i)
> > +		rcar_du_atomic_commit_update_cmm(crtc, crtc_state);
> > +
> >  	/* Apply the atomic update. */
> >  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
> >  	drm_atomic_helper_commit_planes(dev, old_state,
>
> --
> Regards,
>
> Laurent Pinchart

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 19/20] drm: rcar-du: crtc: Register GAMMA_LUT properties

[Daniel Vetter]

On Fri, Jun 14, 2019 at 10:15:52AM +0200, Jacopo Mondi wrote:
> Hi Laurent,
>    thanks for review
> 
> On Fri, Jun 07, 2019 at 03:03:04PM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Thu, Jun 06, 2019 at 04:22:19PM +0200, Jacopo Mondi wrote:
> > > Enable the GAMMA_LUT KMS property using the framework helpers to
> > > register the proeprty and the associated gamma table size maximum size.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > > index e6d3df37c827..c920fb5dba65 100644
> > > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > > @@ -1207,6 +1207,9 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
> > >  	    rcdu->cmms[swindex]) {
> > >  		rcrtc->cmm = rcdu->cmms[swindex];
> > >  		rgrp->cmms_mask |= BIT(hwindex % 2);
> > > +
> > > +		drm_mode_crtc_set_gamma_size(crtc, CMM_GAMMA_LUT_SIZE);
> > > +		drm_crtc_enable_color_mgmt(crtc, 0, false, CMM_GAMMA_LUT_SIZE);
> >
> > This change looks good, but you also need to add support for legacy API.
> > According to the function's documentation,
> >
> >  * Drivers should use drm_atomic_helper_legacy_gamma_set() to implement the
> >  * legacy &drm_crtc_funcs.gamma_set callback.
> >
> 
> Drivers 'shuld' or drivers 'shall' ?
> Isn't this required only to support the 'legacy APIs' ? Do we want that?

You're calling drm_mode_crtc_set_gamma_size, which is only useful for the
legacy ioctls. should here = assuming your hw supports something that
legacy gamma ioctl can use. Feel free to patch up the docs.
-Daniel

> 
> Thanks
>    j
> 
> > >  	}
> > >
> > >  	drm_crtc_helper_add(crtc, &crtc_helper_funcs);
> > >
> >
> > --
> > Regards,
> >
> > Laurent Pinchart



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

Re: [PATCH 14/20] drm: rcar-du: Add support for CMM

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 12862 bytes --]

Hi Laurent,
   thanks for the review

On Fri, Jun 07, 2019 at 02:35:40PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Jun 06, 2019 at 04:22:14PM +0200, Jacopo Mondi wrote:
> > Add a driver for the R-Car Display Unit Color Correction Module.
> >
> > Each DU output channel is provided with a CMM unit to perform image
> > enhancement and color correction.
>
> I would say "On most Gen3 SoCs, each DU ..." as V3* SoCs have no CMM.
>
> >
> > Add support for CMM through a driver that supports configuration of
> > the 1-dimensional LUT table. More advanced CMM feature could be
> > implemented on top of this basic one.
>
> s/could be/will be/ ? :-)
>

Hopefully :)

> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/gpu/drm/rcar-du/Kconfig    |   7 +
> >  drivers/gpu/drm/rcar-du/Makefile   |   1 +
> >  drivers/gpu/drm/rcar-du/rcar_cmm.c | 197 +++++++++++++++++++++++++++++
> >  drivers/gpu/drm/rcar-du/rcar_cmm.h |  38 ++++++
> >  4 files changed, 243 insertions(+)
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.c
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.h
> >
> > diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
> > index 1529849e217e..539d232790d1 100644
> > --- a/drivers/gpu/drm/rcar-du/Kconfig
> > +++ b/drivers/gpu/drm/rcar-du/Kconfig
> > @@ -13,6 +13,13 @@ config DRM_RCAR_DU
> >  	  Choose this option if you have an R-Car chipset.
> >  	  If M is selected the module will be called rcar-du-drm.
> >
> > +config DRM_RCAR_CMM
> > +	bool "R-Car DU Color Management Module (CMM) Support"
> > +	depends on DRM && OF
> > +	depends on DRM_RCAR_DU
> > +	help
> > +	  Enable support for R-Car Color Management Module (CMM).
> > +
> >  config DRM_RCAR_DW_HDMI
> >  	tristate "R-Car DU Gen3 HDMI Encoder Support"
> >  	depends on DRM && OF
> > diff --git a/drivers/gpu/drm/rcar-du/Makefile b/drivers/gpu/drm/rcar-du/Makefile
> > index 6c2ed9c46467..4d1187ccc3e5 100644
> > --- a/drivers/gpu/drm/rcar-du/Makefile
> > +++ b/drivers/gpu/drm/rcar-du/Makefile
> > @@ -15,6 +15,7 @@ rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS)	+= rcar_du_of.o \
> >  rcar-du-drm-$(CONFIG_DRM_RCAR_VSP)	+= rcar_du_vsp.o
> >  rcar-du-drm-$(CONFIG_DRM_RCAR_WRITEBACK) += rcar_du_writeback.o
> >
> > +obj-$(CONFIG_DRM_RCAR_CMM)		+= rcar_cmm.o
> >  obj-$(CONFIG_DRM_RCAR_DU)		+= rcar-du-drm.o
> >  obj-$(CONFIG_DRM_RCAR_DW_HDMI)		+= rcar_dw_hdmi.o
> >  obj-$(CONFIG_DRM_RCAR_LVDS)		+= rcar_lvds.o
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> > new file mode 100644
> > index 000000000000..5d9d917b91f4
> > --- /dev/null
> > +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> > @@ -0,0 +1,197 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * rcar_cmm.c -- R-Car Display Unit Color Management Module
> > + *
> > + * Copyright (C) 2019 Jacopo Mondi <jacopo+renesas@jmondi.org>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <drm/drm_atomic.h>
> > +
> > +#include "rcar_cmm.h"
> > +
> > +#define CM2_LUT_CTRL		0x00
>
> I would write all register addresses with 3 (or 4) digits.
>
> > +#define CM2_LUT_CTRL_EN		BIT(0)
> > +#define CM2_LUT_TBLA		0x600
>
> I would define this as
>
> #define CM2_LUT_TBLA(n)		(0x600 + (n) * 4)
>

Ack to both

> > +
> > +struct rcar_cmm {
> > +	struct clk *clk;
> > +	void __iomem *base;
> > +	bool enabled;
> > +
> > +	/* LUT table scratch buffer. */
> > +	struct {
> > +		bool restore;
> > +		unsigned int size;
> > +		uint32_t table[CMM_GAMMA_LUT_SIZE];
> > +	} lut;
> > +};
> > +
> > +static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg)
> > +{
> > +	return ioread32(rcmm->base + reg);
> > +}
> > +
> > +static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data)
> > +{
> > +	iowrite32(data, rcmm->base + reg);
> > +}
> > +
> > +int rcar_cmm_setup(struct platform_device *pdev, struct rcar_cmm_config *config)
>
> Please document the functions exposed to the DU driver. It's hard to
> understand the setup vs. enable/disable split by just reading this
> driver.
>

Ack. Document it in the header or here in the implementation?

> > +{
> > +	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> > +	unsigned int i;
> > +
> > +	if (config->lut.size > CMM_GAMMA_LUT_SIZE)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * As cmm_setup is called by atomic commit tail helper, it might be
> > +	 * called before the enabling the CRTC (which calls cmm_enable()).
> > +	 *
> > +	 * Store the LUT table entries in the scratch buffer to be later
> > +	 * programmed at enable time.
> > +	 */
> > +	if (!rcmm->enabled) {
> > +		if (!config->lut.enable)
> > +			return 0;
> > +
> > +		for (i = 0; i < config->lut.size; ++i) {
> > +			struct drm_color_lut *lut = &config->lut.table[i];
> > +
> > +			rcmm->lut.table[i] = (lut->red & 0xff) << 16 |
> > +					     (lut->green & 0xff) << 8 |
> > +					     (lut->blue & 0xff);
> > +		}
> > +
> > +		rcmm->lut.restore = true;
> > +		rcmm->lut.size = config->lut.size;
> > +
> > +		return 0;
> > +	}
> > +
> > +	if (rcar_cmm_read(rcmm, CM2_LUT_CTRL) & CM2_LUT_CTRL_EN &&
> > +	    !config->lut.enable) {
> > +		rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> > +		return 0;
> > +	}
> > +
> > +	/* Enable LUT and program the new gamma table values. */
> > +	rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
>
> Shouldn't you write the LUT contents before enabling it (same below) ?
>

Surprisingly, no. I lost quite some time trying to fix a bug that was
due to the fact I wrote the table content first, then enabled the CMM.

But, in facts, section 35A.3.2, which lists the CMM activation steps,
prescribes to enable the CMM first, then update the LUT table entries.

> > +	for (i = 0; i < config->lut.size; ++i) {
> > +		struct drm_color_lut *lut = &config->lut.table[i];
> > +		u32 val = (lut->red & 0xff) << 16 | (lut->green & 0xff) << 8 |
> > +			  (lut->blue & 0xff);
>
> Do you need to recompute the value, can't you use rcmm->lut.table ?
>

rcmm->lut.table stores the already computed table entries to be later
restored at 'enable()' time. Here I'm writing the values to the HW
directly, so I don't need to cache them in rcmm->lut.table first.

> > +
> > +		rcar_cmm_write(rcmm, CM2_LUT_TBLA + i * 4, val);
> > +	}
> > +
> > +	return 0;
> > +}
>
> You need to export this and the next two functions.
>

Ah! Correct, I wonder however if it makes any difference, as the CMM
and the DU will always be compiled together.

> > +
> > +int rcar_cmm_enable(struct platform_device *pdev)
> > +{
> > +	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	if (rcmm->enabled)
> > +		return 0;
>
> Can this happen without a bug in the caller ? If it can, and assuming
> the caller balances the enable and disable calls, you will have
> unbalanced clk_prepare_enable() and clk_disable_unprepare() calls.
>

It shouldn't, but if I check here, I should check below as well.

> > +
> > +	ret = clk_prepare_enable(rcmm->clk);
> > +	if (ret)
> > +		return ret;
>
> Could you use pm_runtime_get_sync() instead ?
>

I'll move to use pm_runtime in next version

> > +
> > +	/* Apply the LUT table values saved at cmm_setup time. */
> > +	if (rcmm->lut.restore) {
> > +		rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> > +		for (i = 0; i < rcmm->lut.size; ++i)
> > +			rcar_cmm_write(rcmm, CM2_LUT_TBLA + i * 4,
> > +				       rcmm->lut.table[i]);
> > +
> > +		rcmm->lut.restore = false;
> > +		rcmm->lut.size = 0;
> > +	}
> > +
> > +	rcmm->enabled = true;
> > +
> > +	return 0;
> > +}
> > +
> > +void rcar_cmm_disable(struct platform_device *pdev)
> > +{
> > +	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> > +
> > +	rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> > +
> > +	clk_disable_unprepare(rcmm->clk);
> > +
> > +	rcmm->lut.restore = false;
> > +	rcmm->lut.size = 0;
> > +	rcmm->enabled = false;
> > +}
> > +
> > +static int rcar_cmm_probe(struct platform_device *pdev)
> > +{
> > +	struct rcar_cmm *rcmm;
> > +	struct resource *res;
> > +	resource_size_t size;
> > +
> > +	rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
> > +	if (!rcmm)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, rcmm);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	size = resource_size(res);
> > +	if (!devm_request_mem_region(&pdev->dev, res->start, size,
> > +				     dev_name(&pdev->dev))) {
> > +		dev_err(&pdev->dev,
> > +			"can't request region for resource %pR\n", res);
> > +		return -EBUSY;
> > +	}
> > +
> > +	rcmm->base = devm_ioremap_nocache(&pdev->dev, res->start, size);
> > +	if (IS_ERR(rcmm->base))
> > +		return PTR_ERR(rcmm->base);
>
> Anything wrong with devm_ioremap_resource() ?
>

The manual prescribes to map the CMM register address space to a
non-cachable memory region (probably due to histograms tables,
which the CMM generates?). I admit I have been 'inspired' by the BSP
code, which maps the memory resources using dev_ioremap_nocache() (but
doesn't request_mem_region() first...)

> > +
> > +	rcmm->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(rcmm->clk)) {
> > +		dev_err(&pdev->dev, "Failed to get CMM clock");
> > +		return PTR_ERR(rcmm->clk);
> > +	}
> > +
> > +	rcmm->lut.restore = false;
> > +	rcmm->lut.size = 0;
> > +	rcmm->enabled = false;
>
> As you allocate memory with kzalloc() you could skip this.
>

Yes, I should, even if I like explict initialization of driver global
flags.

> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id rcar_cmm_of_table[] = {
> > +	{ .compatible = "renesas,cmm-gen3" },
> > +	{ .compatible = "renesas,cmm-gen2" },
> > +	{ },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
> > +
> > +static struct platform_driver rcar_cmm_platform_driver = {
> > +	.probe		= rcar_cmm_probe,
> > +	.driver		= {
> > +		.name	= "rcar-cmm",
> > +		.of_match_table = rcar_cmm_of_table,
> > +	},
>
> No need for suspend/resume support ? The DU driver should disable/enable
> the CMM in its suspend/resume paths, so this should be fine, but won't
> the LUT contents be lost and need to be restored ?
>

I should test this, you're right. In case, I'll cache the LUT content
at suspend, and verify if I need to restore it or not.

> > +};
> > +
> > +module_platform_driver(rcar_cmm_platform_driver);
> > +
> > +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org");
>
> Missing >.
>
> > +MODULE_DESCRIPTION("Renesas R-Car CMM Driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h
> > new file mode 100644
> > index 000000000000..da61a145dc5c
> > --- /dev/null
> > +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h
> > @@ -0,0 +1,38 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
>
> The .c and .h file licenses don't match.
>

Ah ups, that'a typo, but as the DU is licensed with GPL-2.0+ I should
probably change the SPDX header in the .c file.

> > +/*
> > + * rcar_cmm.h -- R-Car Display Unit Color Management Module
> > + *
> > + * Copyright (C) 2019 Jacopo Mondi <jacopo+renesas@jmondi.org>
> > + */
> > +
> > +#ifndef __RCAR_CMM_H__
> > +#define __RCAR_CMM_H__
> > +
> > +#include <linux/platform_device.h>
>
> You can forward-declare struct platform_device instead.
>
> > +
> > +#define CMM_GAMMA_LUT_SIZE		256
> > +
> > +struct drm_color_lut;
> > +
> > +/**
> > + * struct rcar_cmm_config - CMM configuration
> > + *
> > + * @lut:	1D-LUT configuration
> > + * @lut.enable:	1D-LUT enable flag
> > + * @lut.table:	1D-LUT table entries.
> > + * @lut.size	1D-LUT number of entries. Max is 256
>
> The last line is missing a colon.
>
> > + */
> > +struct rcar_cmm_config {
> > +	struct {
> > +		bool enable;
> > +		struct drm_color_lut *table;
> > +		unsigned int size;
> > +	} lut;
> > +};
> > +
> > +int rcar_cmm_enable(struct platform_device *);
>
> As the OF API looks up a struct device from a device_node, should we use
> struct device here ?

of_find_device_by_node() returns a "struct platform_device *".. I
could easily get to the struct device if you think it's better. I have
no strong preferences...

>
> > +void rcar_cmm_disable(struct platform_device *);
>
> I find headers more readable when function arguments are named. In this
> case the types probably provide enough information, but good luck trying
> to read a function such as
>
> int foo(int, int, bool);
>
> > +
> > +int rcar_cmm_setup(struct platform_device *, struct rcar_cmm_config *);
>
> Can the second argument be const ?
>
It probably could right now. Not sure if this will hold when we'll
have to read histograms from the CMM, if they ever will go through
this structure.

Thanks
   j

> > +
> > +#endif /* __RCAR_CMM_H__ */
>
> --
> Regards,
>
> Laurent Pinchart

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 19/20] drm: rcar-du: crtc: Register GAMMA_LUT properties

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 3030 bytes --]

Hi Daniel,

On Fri, Jun 14, 2019 at 10:42:51AM +0200, Daniel Vetter wrote:
> On Fri, Jun 14, 2019 at 10:15:52AM +0200, Jacopo Mondi wrote:
> > Hi Laurent,
> >    thanks for review
> >
> > On Fri, Jun 07, 2019 at 03:03:04PM +0300, Laurent Pinchart wrote:
> > > Hi Jacopo,
> > >
> > > Thank you for the patch.
> > >
> > > On Thu, Jun 06, 2019 at 04:22:19PM +0200, Jacopo Mondi wrote:
> > > > Enable the GAMMA_LUT KMS property using the framework helpers to
> > > > register the proeprty and the associated gamma table size maximum size.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > ---
> > > >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > > > index e6d3df37c827..c920fb5dba65 100644
> > > > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > > > @@ -1207,6 +1207,9 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
> > > >  	    rcdu->cmms[swindex]) {
> > > >  		rcrtc->cmm = rcdu->cmms[swindex];
> > > >  		rgrp->cmms_mask |= BIT(hwindex % 2);
> > > > +
> > > > +		drm_mode_crtc_set_gamma_size(crtc, CMM_GAMMA_LUT_SIZE);
> > > > +		drm_crtc_enable_color_mgmt(crtc, 0, false, CMM_GAMMA_LUT_SIZE);
> > >
> > > This change looks good, but you also need to add support for legacy API.
> > > According to the function's documentation,
> > >
> > >  * Drivers should use drm_atomic_helper_legacy_gamma_set() to implement the
> > >  * legacy &drm_crtc_funcs.gamma_set callback.
> > >
> >
> > Drivers 'shuld' or drivers 'shall' ?
> > Isn't this required only to support the 'legacy APIs' ? Do we want that?
>
> You're calling drm_mode_crtc_set_gamma_size, which is only useful for the
> legacy ioctls. should here = assuming your hw supports something that
> legacy gamma ioctl can use. Feel free to patch up the docs.

Oh, I see. I should either leave the old API alone without calling
drm_mode_crtc_set_gamma_size(), or set the .gamma_set callback to
point to drm_atomic_helper_legacy_gamma_set(), which translates the
old gamma table interface to a blob property and attach it to a crtc
state which is then commited and applied through the atomic helpers.

So I would change the doc to prescribe that if the driver intends to
support the legacy SETGAMMA/GETGAMMA IOCTLs it should declare the
gamma table size with drm_mode_crtc_set_gamma_size() first, and set
the .gamma_set crtc callback to drm_atomic_helper_legacy_gamma_set(),
which translates the legacy interface to a GAMMA_LUT property blob
and commit it.

If that works, I'll make a small patch to the documentation in v2.

Thanks
  j


> -Daniel
>
> >
> > Thanks
> >    j
> >
> > > >  	}
> > > >
> > > >  	drm_crtc_helper_add(crtc, &crtc_helper_funcs);
> > > >
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 19/20] drm: rcar-du: crtc: Register GAMMA_LUT properties

[Daniel Vetter]

On Fri, Jun 14, 2019 at 11:27:45AM +0200, Jacopo Mondi wrote:
> Hi Daniel,
> 
> On Fri, Jun 14, 2019 at 10:42:51AM +0200, Daniel Vetter wrote:
> > On Fri, Jun 14, 2019 at 10:15:52AM +0200, Jacopo Mondi wrote:
> > > Hi Laurent,
> > >    thanks for review
> > >
> > > On Fri, Jun 07, 2019 at 03:03:04PM +0300, Laurent Pinchart wrote:
> > > > Hi Jacopo,
> > > >
> > > > Thank you for the patch.
> > > >
> > > > On Thu, Jun 06, 2019 at 04:22:19PM +0200, Jacopo Mondi wrote:
> > > > > Enable the GAMMA_LUT KMS property using the framework helpers to
> > > > > register the proeprty and the associated gamma table size maximum size.
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > > ---
> > > > >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > > > > index e6d3df37c827..c920fb5dba65 100644
> > > > > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > > > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > > > > @@ -1207,6 +1207,9 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
> > > > >  	    rcdu->cmms[swindex]) {
> > > > >  		rcrtc->cmm = rcdu->cmms[swindex];
> > > > >  		rgrp->cmms_mask |= BIT(hwindex % 2);
> > > > > +
> > > > > +		drm_mode_crtc_set_gamma_size(crtc, CMM_GAMMA_LUT_SIZE);
> > > > > +		drm_crtc_enable_color_mgmt(crtc, 0, false, CMM_GAMMA_LUT_SIZE);
> > > >
> > > > This change looks good, but you also need to add support for legacy API.
> > > > According to the function's documentation,
> > > >
> > > >  * Drivers should use drm_atomic_helper_legacy_gamma_set() to implement the
> > > >  * legacy &drm_crtc_funcs.gamma_set callback.
> > > >
> > >
> > > Drivers 'shuld' or drivers 'shall' ?
> > > Isn't this required only to support the 'legacy APIs' ? Do we want that?
> >
> > You're calling drm_mode_crtc_set_gamma_size, which is only useful for the
> > legacy ioctls. should here = assuming your hw supports something that
> > legacy gamma ioctl can use. Feel free to patch up the docs.
> 
> Oh, I see. I should either leave the old API alone without calling
> drm_mode_crtc_set_gamma_size(), or set the .gamma_set callback to
> point to drm_atomic_helper_legacy_gamma_set(), which translates the
> old gamma table interface to a blob property and attach it to a crtc
> state which is then commited and applied through the atomic helpers.
> 
> So I would change the doc to prescribe that if the driver intends to
> support the legacy SETGAMMA/GETGAMMA IOCTLs it should declare the
> gamma table size with drm_mode_crtc_set_gamma_size() first, and set
> the .gamma_set crtc callback to drm_atomic_helper_legacy_gamma_set(),
> which translates the legacy interface to a GAMMA_LUT property blob
> and commit it.
> 
> If that works, I'll make a small patch to the documentation in v2.

Not quite what I meant: You should support the legacy gamma ioctl, if your
gamma ramp can be squared into support that. Which generally means yes.
We've been thinking about just wiring this all up by default, but there's
some drivers who use a 256 entry legacy gamma ramp (for backwards compat
with old X11 userspace), but advertise much bigger tables through the new
ioctl. So it's not quite as simple.

But except if you have some really strange hw there's just no good reason
not to support the old legacy ioctl. We also don't just support the new
atomic ioctl on new drivers, they all still support the older interfaces.
This is the same.

That's what I meant should be clarified if it's not yet clear in docs,
plus maybe a new todo entry in Documentation/gpu/todo.rst.
-Daniel

> 
> Thanks
>   j
> 
> 
> > -Daniel
> >
> > >
> > > Thanks
> > >    j
> > >
> > > > >  	}
> > > > >
> > > > >  	drm_crtc_helper_add(crtc, &crtc_helper_funcs);
> > > > >
> > > >
> > > > --
> > > > Regards,
> > > >
> > > > Laurent Pinchart
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch



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


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

Re: [PATCH 20/20] drm: rcar-du: kms: Update CMM in atomic commit tail

[Laurent Pinchart]

Hi Jacopo,

On Fri, Jun 14, 2019 at 10:19:13AM +0200, Jacopo Mondi wrote:
> On Fri, Jun 07, 2019 at 03:06:33PM +0300, Laurent Pinchart wrote:
> > On Thu, Jun 06, 2019 at 04:22:20PM +0200, Jacopo Mondi wrote:
> >> Update CMM settings at in the atomic commit tail helper method.
> >>
> >> The CMM is updated with new gamma values provided to the driver
> >> in the GAMMA_LUT blob property.
> >>
> >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >> ---
> >>  drivers/gpu/drm/rcar-du/rcar_du_kms.c | 36 +++++++++++++++++++++++++++
> >>  1 file changed, 36 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> >> index 5a910a04e1d9..29a2020a46b5 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> >> @@ -21,6 +21,7 @@
> >>  #include <linux/of_platform.h>
> >>  #include <linux/wait.h>
> >>
> >> +#include "rcar_cmm.h"
> >>  #include "rcar_du_crtc.h"
> >>  #include "rcar_du_drv.h"
> >>  #include "rcar_du_encoder.h"
> >> @@ -367,6 +368,38 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv,
> >>   * Atomic Check and Update
> >>   */
> >>
> >> +static void rcar_du_atomic_commit_update_cmm(struct drm_crtc *crtc,
> >> +					     struct drm_crtc_state *old_state)
> >> +{
> >> +	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> >> +	struct rcar_cmm_config cmm_config = {};
> >> +
> >> +	if (!rcrtc->cmm || !crtc->state->color_mgmt_changed)
> >> +		return;
> >> +
> >> +	if (!crtc->state->gamma_lut) {
> >> +		cmm_config.lut.enable = false;
> >> +		rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> >> +
> >> +		return;
> >> +	}
> >> +
> >> +	cmm_config.lut.enable = true;
> >> +	cmm_config.lut.table = (struct drm_color_lut *)
> >> +			       crtc->state->gamma_lut->data;
> >> +
> >> +	/* Set LUT table size to 0 if entries should not be updated. */
> >> +	if (!old_state->gamma_lut ||
> >> +	    (old_state->gamma_lut->base.id !=
> >> +	    crtc->state->gamma_lut->base.id))
> >> +		cmm_config.lut.size = crtc->state->gamma_lut->length
> >> +				    / sizeof(cmm_config.lut.table[0]);
> >
> > Do you need to call rcar_cmm_setup() at all in this case ?
> 
> Do you mean in case the lut.size field is set to 0 ?
> I considered it useful when the CMM has to be re-enabled without
> updateing the LUT table entries? It is not required in your opinion?

You're right, I thought userspace couldn't do this, but it actually can.

> >> +	else
> >> +		cmm_config.lut.size = 0;
> >> +
> >> +	rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> >> +}
> >> +
> >>  static int rcar_du_atomic_check(struct drm_device *dev,
> >>  				struct drm_atomic_state *state)
> >>  {
> >> @@ -409,6 +442,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
> >>  			rcdu->dpad1_source = rcrtc->index;
> >>  	}
> >>
> >> +	for_each_old_crtc_in_state(old_state, crtc, crtc_state, i)
> >> +		rcar_du_atomic_commit_update_cmm(crtc, crtc_state);
> >> +
> >>  	/* Apply the atomic update. */
> >>  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
> >>  	drm_atomic_helper_commit_planes(dev, old_state,

-- 
Regards,

Laurent Pinchart