Hi Jacopo,

On Sun, Aug 25, 2019 at 3:50 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 R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).

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

Thanks for your patch!

--- /dev/null +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt @@ -0,0 +1,33 @@ +* 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 or more of the following:
• • "renesas,cmm-r8a7795": for R8A7795 (R-Car H3) compatible CMM.
• • "renesas,cmm-r8a7796": for R8A7796 (R-Car M3-W) compatible CMM.
• • "renesas,cmm-r8a77965": for R8A77965 (R-Car M3-N) compatible CMM.
• • "renesas,cmm-r8a77990": for R8A77990 (R-Car E3) compatible CMM.
• • "renesas,cmm-r8a77995": for R8A77995 (R-Car D3) compatible CMM.

Please use "renesas,<socype->-cmm" instead of "renesas,cmm-<soctype>".

• • "renesas,rcar-gen3-cmm": for a generic R-Car Gen3 compatible CMM.
• • "renesas,rcar-gen2-cmm": for a generic R-Car Gen2 compatible CMM.
• When the generic compatible string is specified, the SoC-specific
• version corresponding to the platform should be listed first.
• • 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.

Thinking about yaml validation:

power-domains? resets?

+Example: +--------

•   cmm0: cmm@fea40000 {
  

•           compatible = "renesas,cmm-r8a7796";
  

•           reg = <0 0xfea40000 0 0x1000>;
  

•           power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
  

•           clocks = <&cpg CPG_MOD 711>;
  

•           resets = <&cpg 711>;
  

•   };
  

Gr{oetje,eeting}s,

Geert

Hi Jacopo,

On Mon, Aug 26, 2019 at 9:58 AM Jacopo Mondi jacopo@jmondi.org wrote:

On Mon, Aug 26, 2019 at 09:34:41AM +0200, Geert Uytterhoeven wrote:

...

On Sun, Aug 25, 2019 at 3:50 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 R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).

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

Thanks for your patch!

...

--- /dev/null +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt @@ -0,0 +1,33 @@ +* 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 or more of the following:
• • "renesas,cmm-r8a7795": for R8A7795 (R-Car H3) compatible CMM.
• • "renesas,cmm-r8a7796": for R8A7796 (R-Car M3-W) compatible CMM.
• • "renesas,cmm-r8a77965": for R8A77965 (R-Car M3-N) compatible CMM.
• • "renesas,cmm-r8a77990": for R8A77990 (R-Car E3) compatible CMM.
• • "renesas,cmm-r8a77995": for R8A77995 (R-Car D3) compatible CMM.

Please use "renesas,<socype->-cmm" instead of "renesas,cmm-<soctype>".

I actually copied it from the r-car gpio bindings, and I liked cmm-<soctype> better. If you prefer I can change it though.

We switched from "renesas,cmm-<soctype>" to "renesas,<socype->-cmm" a few years ago, upon request from the DT people:

vendor -> family/SoC -> IP core

Unfortunately we cannot change the old style in existing bindings, without great effort and backwards compatibility ramifications.

Gr{oetje,eeting}s,

Geert

Hi Laurent,

On Mon, Aug 26, 2019 at 01:15:50PM +0300, Laurent Pinchart wrote:

Hi Jacopo,

On Mon, Aug 26, 2019 at 09:59:43AM +0200, Jacopo Mondi wrote:

...

On Mon, Aug 26, 2019 at 09:34:41AM +0200, Geert Uytterhoeven wrote:

...

On Sun, Aug 25, 2019 at 3:50 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 R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).

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

Thanks for your patch!

...

--- /dev/null +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt @@ -0,0 +1,33 @@ +* 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 or more of the following:
• • "renesas,cmm-r8a7795": for R8A7795 (R-Car H3) compatible CMM.
• • "renesas,cmm-r8a7796": for R8A7796 (R-Car M3-W) compatible CMM.
• • "renesas,cmm-r8a77965": for R8A77965 (R-Car M3-N) compatible CMM.
• • "renesas,cmm-r8a77990": for R8A77990 (R-Car E3) compatible CMM.
• • "renesas,cmm-r8a77995": for R8A77995 (R-Car D3) compatible CMM.

Please use "renesas,<socype->-cmm" instead of "renesas,cmm-<soctype>".

I actually copied it from the r-car gpio bindings, and I liked cmm-<soctype> better. If you prefer I can change it though.

...

...

• • "renesas,rcar-gen3-cmm": for a generic R-Car Gen3 compatible CMM.
• • "renesas,rcar-gen2-cmm": for a generic R-Car Gen2 compatible CMM.
• When the generic compatible string is specified, the SoC-specific
• version corresponding to the platform should be listed first.
• • 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.

Thinking about yaml validation:

power-domains? resets?

They should indeed be documented.

How about converting this binding to yaml alreay ? It should be fairly simple.

I'm trying to, and I'm having my portion of fun time at it.

The definition of the schema itself seems good, but I wonder, is this the first renesas schema we have? Because it seems to me the schema validator is having an hard time to digest the examplea 'clocks' and 'power-domains' properties, which have 1 phandle and 2 specifiers and 1 phandle and 1 specifier respectively for Rensas SoCs.

In other words, if in the example I have:

examples: - | cmm0: cmm@fea40000 { compatible = "renesas,r8a7796-cmm"; reg = <0 0xfea40000 0 0x1000>; clocks = <&cpg 711> <---- 1 phandle + 1 specifier resets = <&cpg 711>; power-domains = <&sysc>; <---- 1 phandle };

The schema validation is good.

While if I use an actual example - | cmm0: cmm@fea40000 { compatible = "renesas,r8a7796-cmm"; reg = <0 0xfea40000 0 0x1000>; clocks = <&cpg CPG_MOD 711> <---- 1 phandle + 2 specifier resets = <&cpg 711>; power-domains = <&sysc R8A7796_PD_ALWAYS_ON>; <---- 1 phandle }; + 1 specfier

The schema validation fails... Error: Documentation/devicetree/bindings/display/renesas,cmm.example.dts:20.29-30 syntax error FATAL ERROR: Unable to parse input tree

Are clocks properties with > 2 entries and power-domains properties with

1 entries supported?

Because from what I read here: https://github.com/robherring/yaml-bindings/blob/master/schemas/clock/clock.... "The length of a clock specifier is defined by the value of a #clock-cells property in the clock provider node."

And that's expected, but is the examples actually validated against the clock provider pointed by the phandle? Because in that case, if we had a yaml schema for the cpg-mssr provider, it would indeed specify clock-cells=2.

Do we need a schema for cpg-mssr first, or am I doing something else wrong?

Thanks j

...

...

...

+Example: +--------

•   cmm0: cmm@fea40000 {
  

•           compatible = "renesas,cmm-r8a7796";
  

•           reg = <0 0xfea40000 0 0x1000>;
  

•           power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
  

•           clocks = <&cpg CPG_MOD 711>;
  

•           resets = <&cpg 711>;
  

•   };
  

-- Regards,

Laurent Pinchart

Hi Laurent, Geert,

On Thu, Sep 05, 2019 at 03:20:59PM +0300, Laurent Pinchart wrote:

Hi Geert,

On Thu, Sep 05, 2019 at 02:05:34PM +0200, Geert Uytterhoeven wrote:

...

On Thu, Sep 5, 2019 at 1:50 PM Laurent Pinchart wrote:

...

On Fri, Aug 30, 2019 at 08:01:09PM +0200, Jacopo Mondi wrote:

...

On Mon, Aug 26, 2019 at 01:15:50PM +0300, Laurent Pinchart wrote:

...

How about converting this binding to yaml alreay ? It should be fairly simple.

I'm trying to, and I'm having my portion of fun time at it.

The definition of the schema itself seems good, but I wonder, is this the first renesas schema we have? Because it seems to me the schema validator is having an hard time to digest the examplea 'clocks' and 'power-domains' properties, which have 1 phandle and 2 specifiers and 1 phandle and 1 specifier respectively for Rensas SoCs.

In other words, if in the example I have:

examples:

• | cmm0: cmm@fea40000 { compatible = "renesas,r8a7796-cmm"; reg = <0 0xfea40000 0 0x1000>; clocks = <&cpg 711> <---- 1 phandle + 1 specifier resets = <&cpg 711>; power-domains = <&sysc>; <---- 1 phandle };

The schema validation is good.

While if I use an actual example

• | cmm0: cmm@fea40000 { compatible = "renesas,r8a7796-cmm"; reg = <0 0xfea40000 0 0x1000>; clocks = <&cpg CPG_MOD 711> <---- 1 phandle + 2 specifier resets = <&cpg 711>; power-domains = <&sysc R8A7796_PD_ALWAYS_ON>; <---- 1 phandle }; + 1 specfier

The schema validation fails... Error: Documentation/devicetree/bindings/display/renesas,cmm.example.dts:20.29-30 syntax error FATAL ERROR: Unable to parse input tree

Are clocks properties with > 2 entries and power-domains properties with

...

1 entries supported?

Because from what I read here: https://github.com/robherring/yaml-bindings/blob/master/schemas/clock/clock.... "The length of a clock specifier is defined by the value of a #clock-cells property in the clock provider node."

And that's expected, but is the examples actually validated against the clock provider pointed by the phandle? Because in that case, if we had a yaml schema for the cpg-mssr provider, it would indeed specify clock-cells=2.

Do we need a schema for cpg-mssr first, or am I doing something else wrong?

I think you just need to #include the headers that define CPG_MOD and R8A7796_PD_ALWAYS_ON.

If that helps, you might want to replace the symbols in the examples by their actual values (1 resp. 32).

And perhaps keep the symbols as comments?

    clocks = <&cpg 1 /* CPG_MOD */ 711>;
    power-domains = <&sysc 32 /* R8A7796_PD_ALWAYS_ON */>;

I think adding the required #include at the beginning of the example is a better solution.

I didn't realize that, but it actually makes sense, as the example is extracted and actually compiled from the yaml schema.. brilliant!

With a simple:

--- a/Documentation/devicetree/bindings/display/renesas,cmm.yaml +++ b/Documentation/devicetree/bindings/display/renesas,cmm.yaml @@ -51,6 +51,9 @@ additionalProperties: false

examples: - | + #include <dt-bindings/clock/r8a7796-cpg-mssr.h> + #include <dt-bindings/power/r8a7796-sysc.h>

The example now compiles.

Thanks, I will submit the bindings in yaml format in next iteration.

-- Regards,

Laurent Pinchart

On Sun, Aug 25, 2019 at 03:51:42PM +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 Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

---

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 c97dfacad281..c2265e2a1af2 100644 --- a/Documentation/devicetree/bindings/display/renesas,du.txt +++ b/Documentation/devicetree/bindings/display/renesas,du.txt @@ -45,6 +45,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 do not provide any CMM (such as V3M and V3H).

renesas,cmms

Perhaps define what CMM is.

Required nodes:

The connections to the DU output video ports are modeled using the OF graph @@ -91,6 +95,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.22.0

On Wed, Aug 28, 2019 at 09:32:23AM +0200, Geert Uytterhoeven wrote:

On Tue, Aug 27, 2019 at 10:29 PM Rob Herring robh@kernel.org wrote:

...

On Sun, Aug 25, 2019 at 03:51:42PM +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 Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

---

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 c97dfacad281..c2265e2a1af2 100644 --- a/Documentation/devicetree/bindings/display/renesas,du.txt +++ b/Documentation/devicetree/bindings/display/renesas,du.txt @@ -45,6 +45,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 do not provide any CMM (such as V3M and V3H).

renesas,cmms

So I guess we really wanted to have the prefix for the vsps property, too?

Yes, we should have :-( My bad.

Hi Jacopo,

On Sun, Aug 25, 2019 at 3:50 PM Jacopo Mondi jacopo+renesas@jmondi.org 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

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 3dc9d73f589a..8d78e1f98a23 100644 --- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi @@ -2613,6 +2613,30 @@ renesas,fcp = <&fcpvi0>; };

•           cmm0: cmm@fea40000 {
  

•                   compatible = "renesas,cmm-r8a7796";
  

Shouldn't you add "renesas,rcar-gen3-cmm" as a fallback? (for all CMM nodes in all DT patches)

Gr{oetje,eeting}s,

Geert

Hi Jacopo,

Thank you for the patch.

Should you squash this with the patches that add CMM units to the other SoCs ?

On Sun, Aug 25, 2019 at 03:51:43PM +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

Apart from the issue pointed out by Geert,

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 3dc9d73f589a..8d78e1f98a23 100644 --- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi @@ -2613,6 +2613,30 @@ renesas,fcp = <&fcpvi0>; };

• cmm0: cmm@fea40000 {
  

• 	compatible = "renesas,cmm-r8a7796";
  

• 	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-r8a7796";
  

• 	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-r8a7796";
  

• 	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>;

@@ -2766,6 +2790,7 @@ status = "disabled";

	vsps = <&vspd0 &vspd1 &vspd2>;

• 	cmms = <&cmm0 &cmm1 &cmm2>;
  
  ports {
  	#address-cells = <1>;
  

Hi Jacopo,

Thank you for the patch.

On Sun, Aug 25, 2019 at 03:51:44PM +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

Apart from the issue with compatible string as pointed out for patch 03/14,

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 95deff66eeb6..21b4069f07e7 100644 --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi @@ -2909,6 +2909,38 @@ iommus = <&ipmmu_vi1 10>; };

• cmm0: cmm@fea40000 {
  

• 	compatible = "renesas,cmm-r8a7795";
  

• 	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-r8a7795";
  

• 	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-r8a7795";
  

• 	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-r8a7795";
  

• 	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>;

@@ -3112,9 +3144,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>;
  

Hi Jacopo,

Thank you for the patch.

On Sun, Aug 25, 2019 at 03:51:45PM +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

Apart from the issue with compatible string as pointed out for patch 03/14,

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 4ae163220f60..8cf0d049203d 100644 --- a/arch/arm64/boot/dts/renesas/r8a77965.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi @@ -2320,6 +2320,30 @@ resets = <&cpg 611>; };

• cmm0: cmm@fea40000 {
  

• 	compatible = "renesas,cmm-r8a77965";
  

• 	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-r8a77965";
  

• 	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-r8a77965";
  

• 	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>;

@@ -2470,6 +2494,7 @@ status = "disabled";

	vsps = <&vspd0 0>, <&vspd1 0>, <&vspd0 1>;

• 	cmms = <&cmm0 &cmm1 &cmm3>;
  
  ports {
  	#address-cells = <1>;
  

Hi Jacopo,

Thank you for the patch.

On Sun, Aug 25, 2019 at 03:51:46PM +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

Apart from the issue with compatible string as pointed out for patch 03/14,

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 455954c3d98e..89ebe6f565af 100644 --- a/arch/arm64/boot/dts/renesas/r8a77990.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a77990.dtsi @@ -1727,6 +1727,22 @@ iommus = <&ipmmu_vi0 9>; };

• cmm0: cmm@fea40000 {
  

• 	compatible = "renesas,cmm-r8a77990";
  

• 	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-r8a77990";
  

• 	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"; reg = <0 0xfeaa0000 0 0x10000>;

@@ -1768,9 +1784,11 @@ clock-names = "du.0", "du.1"; resets = <&cpg 724>; reset-names = "du.0";

• 	vsps = <&vspd0 0>, <&vspd1 0>;
  status = "disabled";
  
  

• 	vsps = <&vspd0 0>, <&vspd1 0>;
  

• 	cmms = <&cmm0 &cmm1>;
  

• ports {
  	#address-cells = <1>;
  	#size-cells = <0>;
  

Hi Jacopo,

Thank you for the patch.

On Sun, Aug 25, 2019 at 03:51:47PM +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

Apart from the issue with compatible string as pointed out for patch 03/14,

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 183fef86cf7c..b91a20fbbbc6 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-r8a77995";
  

• 	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-r8a77995";
  

• 	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 0x40000>;

@@ -1003,9 +1019,11 @@ clock-names = "du.0", "du.1"; resets = <&cpg 724>; reset-names = "du.0";

• 	vsps = <&vspd0 0>, <&vspd1 0>;
  status = "disabled";
  
  

• 	vsps = <&vspd0 0>, <&vspd1 0>;
  

• 	cmms = <&cmm0 &cmm1>;
  

• ports {
  	#address-cells = <1>;
  	#size-cells = <0>;
  

Hi Jacopo,

Thank you for the patch.

On Sun, Aug 25, 2019 at 03:51:48PM +0200, Jacopo Mondi wrote:

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

In most of Gen3 SoCs, 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 will 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 | 262 +++++++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 38 +++++ 4 files changed, 308 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..55361f5701e8 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -0,0 +1,262 @@ +// 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/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h>

+#include <drm/drm_color_mgmt.h>

+#include "rcar_cmm.h"

+#define CM2_LUT_CTRL 0x0000 +#define CM2_LUT_CTRL_LUT_EN BIT(0) +#define CM2_LUT_TBL_BASE 0x0600 +#define CM2_LUT_TBL(__i) (CM2_LUT_TBL_BASE + (__i) * 4)

+struct rcar_cmm {

• void __iomem *base;
• bool enabled;
• /*

• * @lut:		1D-LUT status
  

• * @lut.enabled:	1D-LUT enabled flag
  

• * @lut.size:		Number of entries in the LUT table
  

Please see my review of patch 13/14, I wonder if we could drop this field.

• * @lut.table:		Table of 1D-LUT entries scaled to HW support
  

• *			precision (8-bits per color component)
  

• */
  

• struct {

• bool enabled;
  

• unsigned int size;
  

• u32 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);

+}

+/*

• • rcar_cmm_lut_extract() - Scale down to hw precision the DRM LUT table

s/hw/hardware/ (and below too)

• • 	    entries and store them.
    

• • @rcmm: Pointer to the CMM device
• • @size: Number of entries in the table
• • @drm_lut: DRM LUT table
• */

+static void rcar_cmm_lut_extract(struct rcar_cmm *rcmm, size_t size,

• 		 const struct drm_color_lut *drm_lut)
  

+{

• unsigned int i;
• for (i = 0; i < size; ++i) {

• const struct drm_color_lut *lut = &drm_lut[i];
  

• rcmm->lut.table[i] = drm_color_lut_extract(lut->red, 8) << 16
  

• 		   | drm_color_lut_extract(lut->green, 8) << 8
  

• 		   | drm_color_lut_extract(lut->blue, 8);
  

• }
• rcmm->lut.size = size;

+}

+/*

• • rcar_cmm_lut_load() - Write to hw the LUT table entries from the local table.

No need for a blank line

• • @rcmm: Pointer to the CMM device
• */

+static void rcar_cmm_lut_load(struct rcar_cmm *rcmm)

I would name this rcar_cmm_lut_write().

+{

• unsigned int i;
• for (i = 0; i < rcmm->lut.size; ++i) {

• u32 entry = rcmm->lut.table[i];
  

• rcar_cmm_write(rcmm, CM2_LUT_TBL(i), entry);
  

You don't need the local entry variable.

• }

+}

+/**

• • rcar_cmm_setup() - configure the CMM unit

s/configure/Configure/ and s/$/./, or the other way around for the other functions (I don't mine which one, but let's stay consistent).

No need for a blank line (same for the functions below).

• • @pdev: The platform device associated with the CMM instance
• • @config: The CRTC-provided configuration.
• • Configure the CMM unit with the CRTC-provided configuration.
• • Currently enabling, disabling and programming of the 1-D LUT unit is
• • supported.
• */

+int rcar_cmm_setup(struct platform_device *pdev,

•    const struct rcar_cmm_config *config)
  

+{

• struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
• if (config->lut.size > CMM_GAMMA_LUT_SIZE)

• return -EINVAL;
  

• /*

• * As rcar_cmm_setup() is called by atomic commit tail helper, it might
  

• * be called when the CMM is disabled. As we can't program the hardware
  

• * in that case, store the configuration internally and apply it when
  

• * the CMM will be enabled by the CRTC through rcar_cmm_enable().
  

• */
  

• if (!rcmm->enabled) {

• if (!config->lut.enable)
  

• 	return 0;
  

• rcar_cmm_lut_extract(rcmm, config->lut.size, config->lut.table);
  

• rcmm->lut.enabled = true;
  

• return 0;
  

• }
• /* Stop LUT operations if requested. */
• if (!config->lut.enable) {

• if (rcmm->lut.enabled) {
  

• 	rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
  

• 	rcmm->lut.enabled = false;
  

• 	rcmm->lut.size = 0;
  

• }
  

• return 0;
  

• }
• /*

• * Enable LUT and program the new gamma table values.
  

• *
  

• * FIXME: In order to have stable operations it is required to first
  

• * enable the 1D-LUT and then program its table entries. This seems to
  

• * contradict what the chip manual reports, and will have to be
  

• * reconsidered when implementing support for double buffering.
  

• */
  

• if (!rcmm->lut.enabled) {

• rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);
  

• rcmm->lut.enabled = true;
  

• }
• rcar_cmm_lut_extract(rcmm, config->lut.size, config->lut.table);
• rcar_cmm_lut_load(rcmm);
• return 0;

+} +EXPORT_SYMBOL_GPL(rcar_cmm_setup);

+/**

• • rcar_cmm_enable() - enable the CMM unit
• • @pdev: The platform device associated with the CMM instance
• • Enable the CMM unit by enabling the parent clock and enabling the CMM
• • components, such as 1-D LUT, if requested.
• */

+int rcar_cmm_enable(struct platform_device *pdev) +{

• struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
• int ret;
• if (!rcmm)

• return -EPROBE_DEFER;
  

This function is called in rcar_du_crtc_atomic_enable(), so that's not the right error code. It seems we need another function for the CMM API to defer probing :-/ I would call it rcar_cmm_init(). This check would then be removed.

• ret = pm_runtime_get_sync(&pdev->dev);
• if (ret < 0)

• return ret;
  

• /* Apply the LUT table values saved at rcar_cmm_setup() time. */
• if (rcmm->lut.enabled) {

• rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);
  

• rcar_cmm_lut_load(rcmm);
  

You will not like this, but I just realised that we're now reprogramming the LUT contents every time the CMM is enabled. Do you think that's something we should optimise ? And yes, that would require introducing back an update flag in rcmm->lut :-S Sorry for not realising this when I proposed dropping it.

• }
• rcmm->enabled = true;
• return 0;

+} +EXPORT_SYMBOL_GPL(rcar_cmm_enable);

+/**

• • rcar_cmm_disable() - disable the CMM unit
• • @pdev: The platform device associated with the CMM instance
• • Disable the CMM unit by stopping the parent clock.
• */

+void rcar_cmm_disable(struct platform_device *pdev) +{

• struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
• rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
• pm_runtime_put(&pdev->dev);
• rcmm->lut.enabled = false;
• rcmm->lut.size = 0;
• rcmm->enabled = false;

+} +EXPORT_SYMBOL_GPL(rcar_cmm_disable);

+static int rcar_cmm_probe(struct platform_device *pdev) +{

• struct rcar_cmm *rcmm;
• struct resource *res;
• 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);
• rcmm->base = devm_ioremap_resource(&pdev->dev, res);
• if (IS_ERR(rcmm->base))

• return PTR_ERR(rcmm->base);
  

You really don't like combining those two calls, do you ? :-)

• pm_runtime_enable(&pdev->dev);
• return 0;

+}

+static int rcar_cmm_remove(struct platform_device *pdev) +{

• pm_runtime_disable(&pdev->dev);
• return 0;

+}

+static const struct of_device_id rcar_cmm_of_table[] = {

• { .compatible = "renesas,cmm-r8a7795", },
• { .compatible = "renesas,cmm-r8a7796", },
• { .compatible = "renesas,cmm-r8a77965", },
• { .compatible = "renesas,cmm-r8a77990", },
• { .compatible = "renesas,cmm-r8a77995", },

As Geert pointed out, I would drop those entries.

• { .compatible = "renesas,rcar-gen3-cmm", },
• { .compatible = "renesas,rcar-gen2-cmm", },
• { },

+}; +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);

+static struct platform_driver rcar_cmm_platform_driver = {

• .probe = rcar_cmm_probe,
• .remove = rcar_cmm_remove,
• .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..b0bb7349ebaa --- /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__

+#define CMM_GAMMA_LUT_SIZE 256

+struct drm_color_lut; +struct platform_device;

+/**

• • struct rcar_cmm_config - CMM configuration
• • @lut: 1D-LUT configuration
• • @lut.enable: 1D-LUT enable flag
• • @lut.table: 1D-LUT table entries
• • @lut.size: Number of 1D-LUT (max 256)

s/1D-LUT/1D-LUT entries/

• */

+struct rcar_cmm_config {

• struct {

• bool enable;
  

• struct drm_color_lut *table;
  

• unsigned int size;
  

• } lut;

+};

+int rcar_cmm_enable(struct platform_device *pdev); +void rcar_cmm_disable(struct platform_device *pdev);

+int rcar_cmm_setup(struct platform_device *pdev,

•    const struct rcar_cmm_config *config);
  

+#endif /* __RCAR_CMM_H__ */

On Tue, Aug 27, 2019 at 04:56:19PM +0200, Jacopo Mondi wrote:

Hi Laurent,

On Tue, Aug 27, 2019 at 03:24:22AM +0300, Laurent Pinchart wrote:

...

Hi Jacopo,

Thank you for the patch.

On Sun, Aug 25, 2019 at 03:51:48PM +0200, Jacopo Mondi wrote:

...

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

In most of Gen3 SoCs, 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 will 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 | 262 +++++++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 38 +++++ 4 files changed, 308 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..55361f5701e8 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -0,0 +1,262 @@ +// 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/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h>

+#include <drm/drm_color_mgmt.h>

+#include "rcar_cmm.h"

+#define CM2_LUT_CTRL 0x0000 +#define CM2_LUT_CTRL_LUT_EN BIT(0) +#define CM2_LUT_TBL_BASE 0x0600 +#define CM2_LUT_TBL(__i) (CM2_LUT_TBL_BASE + (__i) * 4)

+struct rcar_cmm {

• void __iomem *base;
• bool enabled;
• /*

• * @lut:		1D-LUT status
  

• * @lut.enabled:	1D-LUT enabled flag
  

• * @lut.size:		Number of entries in the LUT table
  

Please see my review of patch 13/14, I wonder if we could drop this field.

...

• * @lut.table:		Table of 1D-LUT entries scaled to HW support
  

• *			precision (8-bits per color component)
  

• */
  

• struct {

• bool enabled;
  

• unsigned int size;
  

• u32 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);

+}

+/*

• • rcar_cmm_lut_extract() - Scale down to hw precision the DRM LUT table

s/hw/hardware/ (and below too)

...

• • 	    entries and store them.
    

• • @rcmm: Pointer to the CMM device
• • @size: Number of entries in the table
• • @drm_lut: DRM LUT table
• */

+static void rcar_cmm_lut_extract(struct rcar_cmm *rcmm, size_t size,

• 		 const struct drm_color_lut *drm_lut)
  

+{

• unsigned int i;
• for (i = 0; i < size; ++i) {

• const struct drm_color_lut *lut = &drm_lut[i];
  

• rcmm->lut.table[i] = drm_color_lut_extract(lut->red, 8) << 16
  

• 		   | drm_color_lut_extract(lut->green, 8) << 8
  

• 		   | drm_color_lut_extract(lut->blue, 8);
  

• }
• rcmm->lut.size = size;

+}

+/*

• • rcar_cmm_lut_load() - Write to hw the LUT table entries from the local table.

No need for a blank line

...

• • @rcmm: Pointer to the CMM device
• */

+static void rcar_cmm_lut_load(struct rcar_cmm *rcmm)

I would name this rcar_cmm_lut_write().

I won't, as I would like to convey the LUT tables is loaded from the

Sorry, I meant "I wouldn't"... "I won't" is really harsh, sorry about it :p

Hi Laurent,

On Tue, Aug 27, 2019 at 07:34:23PM +0300, Laurent Pinchart wrote:

Hi Laurent,

On Tue, Aug 27, 2019 at 04:56:19PM +0200, Jacopo Mondi wrote:

...

On Tue, Aug 27, 2019 at 03:24:22AM +0300, Laurent Pinchart wrote:

...

On Sun, Aug 25, 2019 at 03:51:48PM +0200, Jacopo Mondi wrote:

...

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

In most of Gen3 SoCs, 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 will 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 | 262 +++++++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 38 +++++ 4 files changed, 308 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..55361f5701e8 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -0,0 +1,262 @@ +// 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/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h>

+#include <drm/drm_color_mgmt.h>

+#include "rcar_cmm.h"

+#define CM2_LUT_CTRL 0x0000 +#define CM2_LUT_CTRL_LUT_EN BIT(0) +#define CM2_LUT_TBL_BASE 0x0600 +#define CM2_LUT_TBL(__i) (CM2_LUT_TBL_BASE + (__i) * 4)

+struct rcar_cmm {

• void __iomem *base;
• bool enabled;
• /*

• * @lut:		1D-LUT status
  

• * @lut.enabled:	1D-LUT enabled flag
  

• * @lut.size:		Number of entries in the LUT table
  

Please see my review of patch 13/14, I wonder if we could drop this field.

...

• * @lut.table:		Table of 1D-LUT entries scaled to HW support
  

• *			precision (8-bits per color component)
  

• */
  

• struct {

• bool enabled;
  

• unsigned int size;
  

• u32 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);

+}

+/*

• • rcar_cmm_lut_extract() - Scale down to hw precision the DRM LUT table

s/hw/hardware/ (and below too)

...

• • 	    entries and store them.
    

• • @rcmm: Pointer to the CMM device
• • @size: Number of entries in the table
• • @drm_lut: DRM LUT table
• */

+static void rcar_cmm_lut_extract(struct rcar_cmm *rcmm, size_t size,

• 		 const struct drm_color_lut *drm_lut)
  

+{

• unsigned int i;
• for (i = 0; i < size; ++i) {

• const struct drm_color_lut *lut = &drm_lut[i];
  

• rcmm->lut.table[i] = drm_color_lut_extract(lut->red, 8) << 16
  

• 		   | drm_color_lut_extract(lut->green, 8) << 8
  

• 		   | drm_color_lut_extract(lut->blue, 8);
  

• }
• rcmm->lut.size = size;

+}

+/*

• • rcar_cmm_lut_load() - Write to hw the LUT table entries from the local table.

No need for a blank line

...

• • @rcmm: Pointer to the CMM device
• */

+static void rcar_cmm_lut_load(struct rcar_cmm *rcmm)

I would name this rcar_cmm_lut_write().

I won't, as I would like to convey the LUT tables is loaded from the local cache after it has been scaled down to the hardware supported precision.

"load" hints a read though, and here you write the LUT to the hardware. Without reading the comments I would have thought this function would read the LUT back from the hardware.

...

...

...

+{

• unsigned int i;
• for (i = 0; i < rcmm->lut.size; ++i) {

• u32 entry = rcmm->lut.table[i];
  

• rcar_cmm_write(rcmm, CM2_LUT_TBL(i), entry);
  

You don't need the local entry variable.

True, but the code is nicer to read and the compiler should be smart enough to optimize it away

I'm not sure about nicer to read, I find the opposite personally, but it's your code :-)

...

...

...

• }

+}

+/**

• • rcar_cmm_setup() - configure the CMM unit

s/configure/Configure/ and s/$/./, or the other way around for the other functions (I don't mine which one, but let's stay consistent).

Oh right, sorry for the confusion

It's just my OCD kicking in :-)

...

...

...

No need for a blank line (same for the functions below).

...

• • @pdev: The platform device associated with the CMM instance
• • @config: The CRTC-provided configuration.
• • Configure the CMM unit with the CRTC-provided configuration.
• • Currently enabling, disabling and programming of the 1-D LUT unit is
• • supported.
• */

+int rcar_cmm_setup(struct platform_device *pdev,

•    const struct rcar_cmm_config *config)
  

+{

• struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
• if (config->lut.size > CMM_GAMMA_LUT_SIZE)

• return -EINVAL;
  

• /*

• * As rcar_cmm_setup() is called by atomic commit tail helper, it might
  

• * be called when the CMM is disabled. As we can't program the hardware
  

• * in that case, store the configuration internally and apply it when
  

• * the CMM will be enabled by the CRTC through rcar_cmm_enable().
  

• */
  

• if (!rcmm->enabled) {

• if (!config->lut.enable)
  

• 	return 0;
  

• rcar_cmm_lut_extract(rcmm, config->lut.size, config->lut.table);
  

• rcmm->lut.enabled = true;
  

• return 0;
  

• }
• /* Stop LUT operations if requested. */
• if (!config->lut.enable) {

• if (rcmm->lut.enabled) {
  

• 	rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
  

• 	rcmm->lut.enabled = false;
  

• 	rcmm->lut.size = 0;
  

• }
  

• return 0;
  

• }
• /*

• * Enable LUT and program the new gamma table values.
  

• *
  

• * FIXME: In order to have stable operations it is required to first
  

• * enable the 1D-LUT and then program its table entries. This seems to
  

• * contradict what the chip manual reports, and will have to be
  

• * reconsidered when implementing support for double buffering.
  

• */
  

• if (!rcmm->lut.enabled) {

• rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);
  

• rcmm->lut.enabled = true;
  

• }
• rcar_cmm_lut_extract(rcmm, config->lut.size, config->lut.table);
• rcar_cmm_lut_load(rcmm);
• return 0;

+} +EXPORT_SYMBOL_GPL(rcar_cmm_setup);

+/**

• • rcar_cmm_enable() - enable the CMM unit
• • @pdev: The platform device associated with the CMM instance
• • Enable the CMM unit by enabling the parent clock and enabling the CMM
• • components, such as 1-D LUT, if requested.
• */

+int rcar_cmm_enable(struct platform_device *pdev) +{

• struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
• int ret;
• if (!rcmm)

• return -EPROBE_DEFER;
  

This function is called in rcar_du_crtc_atomic_enable(), so that's not the right error code. It seems we need another function for the CMM API to defer probing :-/ I would call it rcar_cmm_init(). This check would then be removed.

I agree about the return code, but not the name, as this function actually enables the CMM.

I meant creating a new rcar_cmm_init() function that would just have the !rcmm check.

...

PROBE_DEFER does not make any sense here, I wonder where it come from, as the probing of CMM and DU has long happened once we get here (at least, I assume so, if we receive a gamma_table, userspace has already been running, and both DU and CMM should have probed. Otherwise, we can exploit the newly created device link, and make sure DU probes after the CMM).

I would just change the return value here, and possibly use the device link to ensure the correct probing sequence.

How does device link help here ?

Currently it doesn't, as we are creating a stateless link.

But if we go for a managed device link (which is the default, by the way, you have to opt-out from it) we can guarantee the CMM has probed before the DU probes, so that we have a guarantee when we get here !rcmm cannot happen.

https://www.kernel.org/doc/html/v5.2-rc7/driver-api/device_link.html "The consumer devices are not probed before the supplier is bound to a driver, and they’re unbound before the supplier is unbound."

As we create the link, the CMM is the supplier of DU, so we could just drop the DL_FLAG_STATELESS flag in device_link_add() in 10/14.

Does this match your understanding ?

...

...

...

• ret = pm_runtime_get_sync(&pdev->dev);
• if (ret < 0)

• return ret;
  

• /* Apply the LUT table values saved at rcar_cmm_setup() time. */
• if (rcmm->lut.enabled) {

• rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);
  

• rcar_cmm_lut_load(rcmm);
  

You will not like this, but I just realised that we're now reprogramming the LUT contents every time the CMM is enabled. Do you think that's something we should optimise ? And yes, that would require introducing

Why so? If we receive an enable after a disable which stops the CMM clock and we have no guarantees the table entries have been kept, or what we receive from userspace has changed or not. Why is this an issue in your opinion?

I thought the hardware preserved the LUT ? Skipping the LUT write is an optimisation, so we could do without it in the initial version. I think it would become more important with the CLU though, as we'll have more data entries there. Maybe we should first check how much time the LUT and CLU writes take before deciding to optimise them.

Yeah, let's post-pone optimizations...

...

...

back an update flag in rcmm->lut :-S Sorry for not realising this when I proposed dropping it.

...

• }
• rcmm->enabled = true;
• return 0;

+} +EXPORT_SYMBOL_GPL(rcar_cmm_enable);

+/**

• • rcar_cmm_disable() - disable the CMM unit
• • @pdev: The platform device associated with the CMM instance
• • Disable the CMM unit by stopping the parent clock.
• */

+void rcar_cmm_disable(struct platform_device *pdev) +{

• struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
• rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
• pm_runtime_put(&pdev->dev);
• rcmm->lut.enabled = false;
• rcmm->lut.size = 0;
• rcmm->enabled = false;

+} +EXPORT_SYMBOL_GPL(rcar_cmm_disable);

+static int rcar_cmm_probe(struct platform_device *pdev) +{

• struct rcar_cmm *rcmm;
• struct resource *res;
• 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);
• rcmm->base = devm_ioremap_resource(&pdev->dev, res);
• if (IS_ERR(rcmm->base))

• return PTR_ERR(rcmm->base);
  

You really don't like combining those two calls, do you ? :-)

devm_of_iomap() ?

devm_platform_ioremap_resource()

Oh stupid, thanks!

Thanks j

...

...

...

• pm_runtime_enable(&pdev->dev);
• return 0;

+}

+static int rcar_cmm_remove(struct platform_device *pdev) +{

• pm_runtime_disable(&pdev->dev);
• return 0;

+}

+static const struct of_device_id rcar_cmm_of_table[] = {

• { .compatible = "renesas,cmm-r8a7795", },
• { .compatible = "renesas,cmm-r8a7796", },
• { .compatible = "renesas,cmm-r8a77965", },
• { .compatible = "renesas,cmm-r8a77990", },
• { .compatible = "renesas,cmm-r8a77995", },

As Geert pointed out, I would drop those entries.

yes

...

...

• { .compatible = "renesas,rcar-gen3-cmm", },
• { .compatible = "renesas,rcar-gen2-cmm", },
• { },

+}; +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);

+static struct platform_driver rcar_cmm_platform_driver = {

• .probe = rcar_cmm_probe,
• .remove = rcar_cmm_remove,
• .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..b0bb7349ebaa --- /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__

+#define CMM_GAMMA_LUT_SIZE 256

+struct drm_color_lut; +struct platform_device;

+/**

• • struct rcar_cmm_config - CMM configuration
• • @lut: 1D-LUT configuration
• • @lut.enable: 1D-LUT enable flag
• • @lut.table: 1D-LUT table entries
• • @lut.size: Number of 1D-LUT (max 256)

s/1D-LUT/1D-LUT entries/

ack, I'll change this.

...

...

• */

+struct rcar_cmm_config {

• struct {

• bool enable;
  

• struct drm_color_lut *table;
  

• unsigned int size;
  

• } lut;

+};

+int rcar_cmm_enable(struct platform_device *pdev); +void rcar_cmm_disable(struct platform_device *pdev);

+int rcar_cmm_setup(struct platform_device *pdev,

•    const struct rcar_cmm_config *config);
  

+#endif /* __RCAR_CMM_H__ */

-- Regards,

Laurent Pinchart

Hi Jacopo,

Thank you for the patch.

On Sun, Aug 25, 2019 at 03:51:50PM +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.

Enforce the suspend/resume ordering of DU and CMM by creating a stateless device link between the two to make sure the CMM supplier device suspends after and resumes before the DU consumer.

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

drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 6 +++ 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 | 63 +++++++++++++++++++++++++ 5 files changed, 76 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..23f1d6cc1719 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -1194,6 +1194,12 @@ 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 (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

Maybe alphabetically sorted ?

#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];

As there's one CMM per CRTC, should we use RCAR_DU_MAX_CRTCS here ? It's not very useful to have two different macros that are required to have the same value :-)

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..e9906609c635 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 available 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 2dc9caee8767..61ca1d3c379a 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -17,7 +17,9 @@ #include <drm/drm_probe_helper.h> #include <drm/drm_vblank.h>

+#include <linux/device.h> #include <linux/of_graph.h> +#include <linux/of_platform.h> #include <linux/wait.h>

#include "rcar_du_crtc.h" @@ -614,6 +616,62 @@ 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 i;
• int cells;
• cells = of_property_count_u32_elems(np, "cmms");
• if (cells == -EINVAL)

• return 0;
  

• if (cells > RCAR_DU_MAX_CMMS || cells > rcdu->num_crtcs) {

And if we remove RCAR_DU_MAX_CMMS as proposed above, I would just remove the first part of the condition, as rcdu->num_crtcs is guaranteed to be <= RCAR_DU_MAX_CRTCS.

With this addressed,

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

• dev_err(rcdu->dev,
  

• 	"Invalid number of entries in 'cmms' property\n");
  

• return -EINVAL;
  

• }
• for (i = 0; i < cells; ++i) {

• struct platform_device *pdev;
  

• struct device_link *link;
  

• 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);
  

• }
  

• if (!of_device_is_available(cmm)) {
  

• 	/* It's fine to have a phandle to a non-enabled CMM. */
  

• 	of_node_put(cmm);
  

• 	continue;
  

• }
  

• pdev = of_find_device_by_node(cmm);
  

• if (IS_ERR(pdev)) {
  

• 	dev_err(rcdu->dev, "No device found for CMM%u\n", i);
  

• 	of_node_put(cmm);
  

• 	return PTR_ERR(pdev);
  

• }
  

• of_node_put(cmm);
  

• link = device_link_add(rcdu->dev, &pdev->dev,
  

• 		       DL_FLAG_STATELESS);
  

• if (!link) {
  

• 	dev_err(rcdu->dev,
  

• 		"Failed to create device link to CMM%u\n", i);
  

• 	return -EINVAL;
  

• }
  

• rcdu->cmms[i] = pdev;
  

• }
• return 0;

+}

int rcar_du_modeset_init(struct rcar_du_device *rcdu) { static const unsigned int mmio_offsets[] = { @@ -704,6 +762,11 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu) return ret; }

• /* Initialize the Color Management Modules. */
• ret = rcar_du_cmm_init(rcdu);
• if (ret)

• return ret;
  

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

Hi Jacopo,

Thank you for the patch.

On Sun, Aug 25, 2019 at 03:51:53PM +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.

Reviewed-by: Ulrich Hecht uli+renesas@fpond.eu Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org

---

drivers/gpu/drm/rcar-du/rcar_du_kms.c | 35 +++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 61ca1d3c379a..047fdb982a11 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -22,6 +22,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" @@ -368,6 +369,37 @@ 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]);
  

It has just occurred to me that the hardware only support LUTs of exactly 256 entries. Should we remove cmm_config.lut.size (simplifying the code in the CMM driver), and add a check to the CRTC .atomic_check() handler to reject invalid LUTs ? Sorry for not having caught this earlier.

• 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) { @@ -410,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,

HI Laurent,

On Tue, Aug 27, 2019 at 03:19:27AM +0300, Laurent Pinchart wrote:

On Tue, Aug 27, 2019 at 03:00:17AM +0300, Laurent Pinchart wrote:

...

Hi Jacopo,

Thank you for the patch.

On Sun, Aug 25, 2019 at 03:51:53PM +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.

Reviewed-by: Ulrich Hecht uli+renesas@fpond.eu Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org

---

drivers/gpu/drm/rcar-du/rcar_du_kms.c | 35 +++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 61ca1d3c379a..047fdb982a11 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -22,6 +22,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" @@ -368,6 +369,37 @@ 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]);
  

It has just occurred to me that the hardware only support LUTs of

Where did you find this strict requirement ? I have tried programming less than 256 entries in the 1-D LUT table, and it seems to me things are working fine (from a visual inspection of the output image, I don't see much differences from when I program the full table, maybe that's an indication something is bad?)

Thanks j

...

exactly 256 entries. Should we remove cmm_config.lut.size (simplifying the code in the CMM driver), and add a check to the CRTC .atomic_check() handler to reject invalid LUTs ? Sorry for not having caught this earlier.

Just an additional comment, if we drop the size field, then the cmm_config.lut.table pointer should be set to NULL when the LUT contents don't need to be updated.

...

...

• 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) { @@ -410,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

-- Regards,

Laurent Pinchart