Hi Jacopo,

On Sat, Jul 6, 2019 at 4:07 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 Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

Thanks for your patch!

--- /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,rcar-gen3-cmm"
• • "renesas,rcar-gen2-cmm"

Why do you think you do not need SoC-specific compatible values? What if you discover a different across the R-Car Gen3 line tomorrow? Does the IP block have a version register?

Gr{oetje,eeting}s,

Geert

Hi Geert, sorry for the delayed response..

On Mon, Aug 19, 2019 at 03:45:54PM +0200, Geert Uytterhoeven wrote:

Hi Jacopo,

On Mon, Jul 8, 2019 at 9:58 AM Geert Uytterhoeven geert@linux-m68k.org wrote:

...

On Sat, Jul 6, 2019 at 4:07 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 Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

Thanks for your patch!

...

--- /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,rcar-gen3-cmm"
• • "renesas,rcar-gen2-cmm"

Why do you think you do not need SoC-specific compatible values? What if you discover a different across the R-Car Gen3 line tomorrow? Does the IP block have a version register?

Do you have an answer to these questions?

It does not seem to me that CMM has any version register, nor there are differences between the different Gen3 SoCs..

However, even if we now define a single compatible property for gen3/gen2 and we later find out one of the SoC needs a soc-specific property we can safely add it and keep the generic gen3/gen2 one as fallback.. Does it work for you?

Thanks j

Thanks!

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

Hi Geert,

On Tue, Aug 20, 2019 at 09:53:44AM +0200, Geert Uytterhoeven wrote:

Hi Jacopo,

On Tue, Aug 20, 2019 at 9:47 AM Jacopo Mondi jacopo@jmondi.org wrote:

...

On Mon, Aug 19, 2019 at 03:45:54PM +0200, Geert Uytterhoeven wrote:

...

On Mon, Jul 8, 2019 at 9:58 AM Geert Uytterhoeven geert@linux-m68k.org wrote:

...

On Sat, Jul 6, 2019 at 4:07 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 Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

Thanks for your patch!

...

--- /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,rcar-gen3-cmm"
• • "renesas,rcar-gen2-cmm"

Why do you think you do not need SoC-specific compatible values? What if you discover a different across the R-Car Gen3 line tomorrow? Does the IP block have a version register?

Do you have an answer to these questions?

It does not seem to me that CMM has any version register, nor there are differences between the different Gen3 SoCs..

However, even if we now define a single compatible property for gen3/gen2 and we later find out one of the SoC needs a soc-specific property we can safely add it and keep the generic gen3/gen2 one as fallback.. Does it work for you?

Unfortunately that won't work, as the existing DTBs won't have the soc-specific compatible value.

Correct, existing dtbs won't have the soc-specific value... However, there are functional differences between different SoCs according to the datasheet, but if it's good practice to provide soc-specific compatibles "just in case" I'm fine doing that..

You could still resort to soc_device_match(), but it is better to avoid that.

I see... Also that function's documentation prescribes to go through DT first, so I guess it's our last resort...

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

Hi Laurent,

On Tue, Aug 20, 2019 at 7:41 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:

On Tue, Aug 20, 2019 at 09:53:44AM +0200, Geert Uytterhoeven wrote:

...

On Tue, Aug 20, 2019 at 9:47 AM Jacopo Mondi jacopo@jmondi.org wrote:

...

On Mon, Aug 19, 2019 at 03:45:54PM +0200, Geert Uytterhoeven wrote:

...

On Mon, Jul 8, 2019 at 9:58 AM Geert Uytterhoeven geert@linux-m68k.org wrote:

...

On Sat, Jul 6, 2019 at 4:07 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 Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

Thanks for your patch!

...

--- /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,rcar-gen3-cmm"
• • "renesas,rcar-gen2-cmm"

Why do you think you do not need SoC-specific compatible values? What if you discover a different across the R-Car Gen3 line tomorrow? Does the IP block have a version register?

Do you have an answer to these questions?

It does not seem to me that CMM has any version register, nor there are differences between the different Gen3 SoCs..

However, even if we now define a single compatible property for gen3/gen2 and we later find out one of the SoC needs a soc-specific property we can safely add it and keep the generic gen3/gen2 one as fallback.. Does it work for you?

Unfortunately that won't work, as the existing DTBs won't have the soc-specific compatible value. You could still resort to soc_device_match(), but it is better to avoid that.

We've had the same discussion over and over for quite a long time :-) I wonder, now that we have implemented SoC-specific compatible values for many IP cores, how many of them have actually benefited from it ? I'm not considering IP cores where we knew from the start that each SoC was different (such as pinctrl or clocks for instance), but IP cores where we though all SoCs would be handled in the same way. I also wouldn't count ES-specific differences, as those are handled by soc_device_match() anyway.

Thank you for making me dive into this ;-)

For R-Car Gen3 only:

DRIF? The driver still matches against "renesas,rcar-gen3-drif", but we found a difference on M3-N (didn't check if that was documented initially or not).

PCIe? IIRC, there is still a special PHY register on one of the V3x SoCs that the driver doesn't handle yet, and wasn't documented initially.

RPC-IF? Lots of small differences (you can claim they were documented, but they were unexpected), and non-documented different magic values in the (not yet upstreamed) driver.

SOUND? R-Car E3 special handling was bolted on later.

Thermal? M3-W turned out to have a different Tj than the other SoCs using the same thermal module (yeah, thermal is a bad example, as some Gen3 SoCs use the Gen2 thermal module, so we needed to differentiate anyway).

USBHS? Initially we thought Gen3 was identical to Gen2. Later it turned out that (1) wasn't true, and (2) E3/D3 used a different PLL than the other Gen3 SoCs.

VIN? IIRC, we initialize thought a family-specific compatible value would work for Gen3, like it did for Gen2, but had to reconsider.

I may have missed some...

Convinced?

Gr{oetje,eeting}s,

Geert

Hi Jacopo,

On 08/07/2019 04:11, Laurent Pinchart wrote:

Hi Jacopo,

Thank you for the patch.

On Sat, Jul 06, 2019 at 04:07:30PM +0200, Jacopo Mondi wrote:

...

Update the 'vsps' property in the R-Car Gen3 SoC device tree files to match what's in in the documentation example.

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

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

+1 from me too. This certainly improves readability/clarity IMO.

Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

...

---

arch/arm64/boot/dts/renesas/r8a774c0.dtsi | 2 +- arch/arm64/boot/dts/renesas/r8a7795.dtsi | 2 +- arch/arm64/boot/dts/renesas/r8a77965.dtsi | 2 +- arch/arm64/boot/dts/renesas/r8a77990.dtsi | 2 +- arch/arm64/boot/dts/renesas/r8a77995.dtsi | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/boot/dts/renesas/r8a774c0.dtsi b/arch/arm64/boot/dts/renesas/r8a774c0.dtsi index 3f86db199dbf..e643f9d3c102 100644 --- a/arch/arm64/boot/dts/renesas/r8a774c0.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a774c0.dtsi @@ -1807,7 +1807,7 @@ clocks = <&cpg CPG_MOD 724>, <&cpg CPG_MOD 723>; clock-names = "du.0", "du.1";

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

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

diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi index 097538cc4b1f..432f4036a8a8 100644 --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi @@ -3098,7 +3098,7 @@ <&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 0>, <&vspd0 1>;
  status = "disabled";
  
  ports {
  

diff --git a/arch/arm64/boot/dts/renesas/r8a77965.dtsi b/arch/arm64/boot/dts/renesas/r8a77965.dtsi index 2554b1742dbf..b701aeb4f438 100644 --- a/arch/arm64/boot/dts/renesas/r8a77965.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi @@ -2456,7 +2456,7 @@ clock-names = "du.0", "du.1", "du.3"; status = "disabled";

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

• 	vsps = <&vspd0 0>, <&vspd1 0>, <&vspd0 1>;
  
  ports {
  	#address-cells = <1>;
  

diff --git a/arch/arm64/boot/dts/renesas/r8a77990.dtsi b/arch/arm64/boot/dts/renesas/r8a77990.dtsi index 56cb566ffa09..79db5441b7e7 100644 --- a/arch/arm64/boot/dts/renesas/r8a77990.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a77990.dtsi @@ -1764,7 +1764,7 @@ clocks = <&cpg CPG_MOD 724>, <&cpg CPG_MOD 723>; clock-names = "du.0", "du.1";

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

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

diff --git a/arch/arm64/boot/dts/renesas/r8a77995.dtsi b/arch/arm64/boot/dts/renesas/r8a77995.dtsi index 5bf3af246e14..49a11b4f55bd 100644 --- a/arch/arm64/boot/dts/renesas/r8a77995.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a77995.dtsi @@ -1001,7 +1001,7 @@ clocks = <&cpg CPG_MOD 724>, <&cpg CPG_MOD 723>; clock-names = "du.0", "du.1";

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

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

Hi Jacopo,

Thank you for the patch.

On Sat, Jul 06, 2019 at 04:07:38PM +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 b701aeb4f438..aad9ea67c379 100644 --- a/arch/arm64/boot/dts/renesas/r8a77965.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi @@ -2307,6 +2307,30 @@ resets = <&cpg 602>; };

• cmm0: cmm@fea40000 {
  

• 	compatible = "renesas,rcar-gen3-cmm";
  

• 	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,rcar-gen3-cmm";
  

• 	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,rcar-gen3-cmm";
  

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

@@ -2457,6 +2481,7 @@ status = "disabled";

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

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

Thanks for your patch!

On July 6, 2019 at 4:07 PM Jacopo Mondi jacopo+renesas@jmondi.org 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 | 292 +++++++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 38 ++++ 4 files changed, 338 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..76ed3fce2b33 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -0,0 +1,292 @@ +// 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 <linux/pm.h>

+#include <drm/drm_atomic.h>

+#include "rcar_cmm.h"

+#define CM2_LUT_CTRL 0x0000 +#define CM2_LUT_CTRL_EN BIT(0) +#define CM2_LUT_TBLA_BASE 0x0600 +#define CM2_LUT_TBLA(__i) (CM2_LUT_TBLA_BASE + (__i) * 4)

+struct rcar_cmm {

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

• * restore: LUT restore flag
  

• * running: LUT operating flag
  

• * size: Number of programmed entries in the LUT table
  

• * table: Scratch buffer where to store the LUT table entries to be
  

• *	  later restored.
  

• */
  

• struct {

• bool restore;
  

• bool running;
  

• unsigned int size;
  

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

+}

+static void rcar_cmm_lut_write(struct rcar_cmm *rcmm, unsigned int size,

• 	       struct drm_color_lut *lut)
  

+{

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

• struct drm_color_lut *entry = &lut[i];
  

• u32 val = (entry->red & 0xff) << 16 |
  

• 	  (entry->green & 0xff) << 8 |
  

• 	  (entry->blue & 0xff);
  

I don't think it's correct to cut off the high bits here. There is a function "drm_color_lut_extract()" for converting drm_color_lut values to hardware precision.

• rcar_cmm_write(rcmm, CM2_LUT_TBLA(i), val);
  

• }

+}

+/**

• • rcar_cmm_setup() - configure the CMM unit
• • @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);
• 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 enabling the CRTC (which calls cmm_enable()).
  

• */
  

• if (!rcmm->enabled) {

• if (!config->lut.enable)
  

• 	return 0;
  

• /*
  

•  * Store the LUT table entries in the scratch buffer to be later
  

•  * programmed at enable time.
  

•  */
  

• for (i = 0; i < config->lut.size; ++i)
  

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

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

• rcmm->lut.restore = true;
  

• return 0;
  

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

• rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
  

• rcmm->lut.running = 0;
  

• rcmm->lut.size = 0;
  

• return 0;
  

• }
• /* Enable LUT and program the new gamma table values. */
• if (!rcmm->lut.running) {

• rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
  

• rcmm->lut.running = true;
  

• }
• rcar_cmm_lut_write(rcmm, config->lut.size, config->lut.table);
• rcmm->lut.size = config->lut.size;
• 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;
  

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

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

• rcmm->lut.restore = false;
  

• rcmm->lut.running = true;
  

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

+} +EXPORT_SYMBOL_GPL(rcar_cmm_enable);

+/**

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

+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.running = false;
• rcmm->lut.size = 0;
• rcmm->enabled = false;

+} +EXPORT_SYMBOL_GPL(rcar_cmm_disable);

+#ifdef CONFIG_PM_SLEEP +static int rcar_cmm_pm_suspend(struct device *dev) +{

• struct rcar_cmm *rcmm = dev_get_drvdata(dev);
• unsigned int i;
• if (!(rcmm->lut.running || rcmm->lut.restore))

• return 0;
  

• /* Save the LUT table entries in the scratch buffer table. */
• for (i = 0; i < rcmm->lut.size; ++i) {

• int entry = rcar_cmm_read(rcmm, CM2_LUT_TBLA(i));
  

• struct drm_color_lut *lut = &rcmm->lut.table[i];
  

• lut->blue = entry & 0xff;
  

• lut->green = (entry >> 8) & 0xff;
  

• lut->red = (entry >> 16) & 0xff;
  

Need to convert the values back to drm_color_lut scale here. I could not find a counterpart to drm_color_lut_extract(), though...

• }
• rcmm->lut.restore = true;
• rcmm->lut.running = false;
• rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
• return 0;

+}

+static int rcar_cmm_pm_resume(struct device *dev) +{

• struct rcar_cmm *rcmm = dev_get_drvdata(dev);
• if (!rcmm->lut.restore)

• return 0;
  

• /* Program the LUT entries saved at suspend time. */
• rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
• rcar_cmm_lut_write(rcmm, rcmm->lut.size, rcmm->lut.table);
• rcmm->lut.running = true;
• rcmm->lut.restore = false;
• return 0;

+} +#endif

+static const struct dev_pm_ops rcar_cmm_pm_ops = {

• SET_SYSTEM_SLEEP_PM_OPS(rcar_cmm_pm_suspend, rcar_cmm_pm_resume)

+};

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

• }
• return 0;

+}

+static const struct of_device_id rcar_cmm_of_table[] = {

• { .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,
• .driver = {

• .name	= "rcar-cmm",
  

• .pm	= &rcar_cmm_pm_ops,
  

• .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..8744e72f32cd --- /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 platform_device; +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 *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__ */

2.21.0

CU Uli

Hi Ulrich,

On Tue, Aug 20, 2019 at 08:37:44PM +0300, Laurent Pinchart wrote:

Hello,

On Tue, Jul 16, 2019 at 03:17:04PM +0200, Ulrich Hecht wrote:

...

...

On July 6, 2019 at 4:07 PM Jacopo Mondi jacopo+renesas@jmondi.org 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 | 292 +++++++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 38 ++++ 4 files changed, 338 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..76ed3fce2b33 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -0,0 +1,292 @@ +// 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 <linux/pm.h>

+#include <drm/drm_atomic.h>

+#include "rcar_cmm.h"

+#define CM2_LUT_CTRL 0x0000 +#define CM2_LUT_CTRL_EN BIT(0) +#define CM2_LUT_TBLA_BASE 0x0600 +#define CM2_LUT_TBLA(__i) (CM2_LUT_TBLA_BASE + (__i) * 4)

+struct rcar_cmm {

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

• * restore: LUT restore flag
  

• * running: LUT operating flag
  

• * size: Number of programmed entries in the LUT table
  

• * table: Scratch buffer where to store the LUT table entries to be
  

• *	  later restored.
  

• */
  

• struct {

• bool restore;
  

• bool running;
  

• unsigned int size;
  

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

+}

+static void rcar_cmm_lut_write(struct rcar_cmm *rcmm, unsigned int size,

• 	       struct drm_color_lut *lut)
  

+{

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

• struct drm_color_lut *entry = &lut[i];
  

• u32 val = (entry->red & 0xff) << 16 |
  

• 	  (entry->green & 0xff) << 8 |
  

• 	  (entry->blue & 0xff);
  

I don't think it's correct to cut off the high bits here. There is a function "drm_color_lut_extract()" for converting drm_color_lut values to hardware precision.

Oh, I see! the value received from userspace in 16-bits precision needs to be rescaled to the hw supported one! Thanks, I totally missed this!

...

...

• rcar_cmm_write(rcmm, CM2_LUT_TBLA(i), val);
  

• }

+}

+/**

• • rcar_cmm_setup() - configure the CMM unit
• • @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);
• 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 enabling the CRTC (which calls cmm_enable()).
  

• */
  

• if (!rcmm->enabled) {

• if (!config->lut.enable)
  

• 	return 0;
  

• /*
  

•  * Store the LUT table entries in the scratch buffer to be later
  

•  * programmed at enable time.
  

•  */
  

• for (i = 0; i < config->lut.size; ++i)
  

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

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

• rcmm->lut.restore = true;
  

• return 0;
  

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

• rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
  

• rcmm->lut.running = 0;
  

• rcmm->lut.size = 0;
  

• return 0;
  

• }
• /* Enable LUT and program the new gamma table values. */
• if (!rcmm->lut.running) {

• rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
  

• rcmm->lut.running = true;
  

• }
• rcar_cmm_lut_write(rcmm, config->lut.size, config->lut.table);
• rcmm->lut.size = config->lut.size;
• 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;
  

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

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

• rcmm->lut.restore = false;
  

• rcmm->lut.running = true;
  

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

+} +EXPORT_SYMBOL_GPL(rcar_cmm_enable);

+/**

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

+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.running = false;
• rcmm->lut.size = 0;
• rcmm->enabled = false;

+} +EXPORT_SYMBOL_GPL(rcar_cmm_disable);

+#ifdef CONFIG_PM_SLEEP +static int rcar_cmm_pm_suspend(struct device *dev) +{

• struct rcar_cmm *rcmm = dev_get_drvdata(dev);
• unsigned int i;
• if (!(rcmm->lut.running || rcmm->lut.restore))

• return 0;
  

• /* Save the LUT table entries in the scratch buffer table. */
• for (i = 0; i < rcmm->lut.size; ++i) {

• int entry = rcar_cmm_read(rcmm, CM2_LUT_TBLA(i));
  

• struct drm_color_lut *lut = &rcmm->lut.table[i];
  

• lut->blue = entry & 0xff;
  

• lut->green = (entry >> 8) & 0xff;
  

• lut->red = (entry >> 16) & 0xff;
  

Need to convert the values back to drm_color_lut scale here. I could not find a counterpart to drm_color_lut_extract(), though...

How about storing the LUT in hardware format internally ? I wonder if we should always update the cached copy, in which case rcar_cmm_lut_write() could just write the LUT values from the cache.

Probably that's the easier, so we always write stuff already scaled down to the 8-bit precision the CMM supports.

Thanks both j

...

...

• }
• rcmm->lut.restore = true;
• rcmm->lut.running = false;
• rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
• return 0;

+}

+static int rcar_cmm_pm_resume(struct device *dev) +{

• struct rcar_cmm *rcmm = dev_get_drvdata(dev);
• if (!rcmm->lut.restore)

• return 0;
  

• /* Program the LUT entries saved at suspend time. */
• rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
• rcar_cmm_lut_write(rcmm, rcmm->lut.size, rcmm->lut.table);
• rcmm->lut.running = true;
• rcmm->lut.restore = false;
• return 0;

+} +#endif

+static const struct dev_pm_ops rcar_cmm_pm_ops = {

• SET_SYSTEM_SLEEP_PM_OPS(rcar_cmm_pm_suspend, rcar_cmm_pm_resume)

+};

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

• }
• return 0;

+}

+static const struct of_device_id rcar_cmm_of_table[] = {

• { .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,
• .driver = {

• .name	= "rcar-cmm",
  

• .pm	= &rcar_cmm_pm_ops,
  

• .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..8744e72f32cd --- /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 platform_device; +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 *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 Laurent, thanks for review

On Tue, Aug 20, 2019 at 08:34:44PM +0300, Laurent Pinchart wrote:

Hi Jacopo,

Thank you for the patch.

On Sat, Jul 06, 2019 at 04:07:41PM +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 | 292 +++++++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 38 ++++ 4 files changed, 338 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..76ed3fce2b33 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -0,0 +1,292 @@ +// 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 <linux/pm.h>

+#include <drm/drm_atomic.h>

The only thing you need from DRM is the drm_color_lut structure, so I would include drm/drm_mode.h instead.

...

+#include "rcar_cmm.h"

+#define CM2_LUT_CTRL 0x0000 +#define CM2_LUT_CTRL_EN BIT(0)

The datasheet names the bit LUT_EN, I would thus rename the macro to CM2_LUT_CTRL_LUT_EN.

...

+#define CM2_LUT_TBLA_BASE 0x0600 +#define CM2_LUT_TBLA(__i) (CM2_LUT_TBLA_BASE + (__i) * 4)

Similarly, the datasheet names the register CM2_LUT_TBL (and the LUT table B is named CM2_LUT_TBL2), would it make sense to stick to those names ?

Ack on all of these

...

+struct rcar_cmm {

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

• * restore: LUT restore flag
  

I'm none the wiser after reading this comment :-)

...

• * running: LUT operating flag
  

• * size: Number of programmed entries in the LUT table
  

• * table: Scratch buffer where to store the LUT table entries to be
  

• *	  later restored.
  

If you want to document individual fields I propose using kerneldoc syntax.

• @lut.restore: ...

...

Yeah, might be a bit of over-documentation here. I don't mind it to be honest, but I'm fine if someone wants this to be dropped.

...

• */
  

• struct {

• bool restore;
  

• bool running;
  

• unsigned int size;
  

• struct drm_color_lut table[CMM_GAMMA_LUT_SIZE];
  

• } lut;

I think the lut.running field name is a bit confusing, as you modify it based on the lut.enable field in the cmm config structure. I would name it lut.enabled. That could then possibly be confused with cmm.enabled (although in my opinion that's fine), if you're concerned by that I would rename that field to running. It would be more logical to consider the CMM as a whole as running or stopped, with the LUT (and later the CLU) enabled or disabled.

I'm a bit bothered that we would have rcmm.enabled rcmm.lut.enabled rcmm_config.lut.enable

I'll see how it looks. enabled is probably the right name for all of these fields, but it might get confusing...

...

+};

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

+}

+static void rcar_cmm_lut_write(struct rcar_cmm *rcmm, unsigned int size,

s/unsigned int/size_t/ ?

...

• 	       struct drm_color_lut *lut)
  

You can make this pointer const.

Ack

...

+{

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

• struct drm_color_lut *entry = &lut[i];
  

• u32 val = (entry->red & 0xff) << 16 |
  

• 	  (entry->green & 0xff) << 8 |
  

• 	  (entry->blue & 0xff);
  

• rcar_cmm_write(rcmm, CM2_LUT_TBLA(i), val);
  

• }

+}

+/**

• • rcar_cmm_setup() - configure the CMM unit
• • @pdev: The platform device associated with the CMM instance
• • @config: The CRTC provided configuration.
• • Configure the CMM unit with the CRTC provided configuration.

s/CRTC provided/CRTC-provided/

"CRTC-provided" is a compound adjective, qualifying the noun "configuration". It thus needs a hyphen. If you had written "The process uses the CRTC provided to this function", then no hyphen would be needed, as "provided" then qualifies the noun "CRTC", without the combination being used as an adjective.

...

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

s/cmm_setup/rcar_cmm_setup()/ (or just "this function").

...

• * called before enabling the CRTC (which calls cmm_enable()).
  

I would phrase this as "... it might be called when the CMM is disabled. 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 by rcar_cmm_enable()." and remove the next comment.

Ack

...

• */
  

• if (!rcmm->enabled) {

• if (!config->lut.enable)
  

• 	return 0;
  

• /*
  

•  * Store the LUT table entries in the scratch buffer to be later
  

•  * programmed at enable time.
  

•  */
  

• for (i = 0; i < config->lut.size; ++i)
  

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

Can you do a memcpy() over the whole table ?

memcpy(rcmm->lut.table, config->lut.table,
       sizeof(*rcmm->lut.table) * config.lut.size);

Yeah, probably better

...

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

• rcmm->lut.restore = true;
  

• return 0;
  

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

• rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
  

• rcmm->lut.running = 0;
  

• rcmm->lut.size = 0;
  

• return 0;
  

• }
• /* Enable LUT and program the new gamma table values. */
• if (!rcmm->lut.running) {

Should this be !rcmm->lut.running && config->lut.enable ? Or do you rely on the caller to not try to disable the LUT when it's not currently enabled ? If you rely on the caller than I think you should rely on it for the enabling case above too, and write is if (!config->lut.enabled). Otherwise I think you're mishandling the !running && !enable, it will end up enabling the LUT.

I think it's fine, as if (!rcmm->enable) then (!rcmm->lut.running) so the (!running && !enable) you have pointed out gets caught by the very first condition check here above (!rcmm->enabled).

I'll try to restructure this to be more readable and as you suggested get rid of the restore field.

...

• rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
  

• rcmm->lut.running = true;
  

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

I'm still very puzzled by the fact that you have to write the LUT contents after enabling the LUT. The datasheet states

I know -_(;.;)_/- (is this the crying cthulhu emoticon ?)

"Note that if the module that references that space is operating, read and write accesses to the relevant space are prohibited. In case of double buffer mode, referenced side of LUT is distinguished by CM2_CTL1.BFS."

It looks to me like you may have to implement double-buffering, but even without that,

I think you have left out the end of the sentence, but I agree that what the manual reports suggests the table should be programmed when it is not operating, but it also mentions double buffering, so in case we're using single buffer mode maybe this does not apply?

with double buffering this is going to change anyway, but so far, that's the only reliable operation sequence I have found...

...

• rcmm->lut.size = config->lut.size;
• 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;
  

• ret = clk_prepare_enable(rcmm->clk);
• if (ret)

• return ret;
  

Didn't you say this version would use runtime PM ? :-)

Ups. I do for suspend/resume not for runtime_suspend/resume. It will be trivial to add.

...

• /* Apply the LUT table values saved at cmm_setup time. */

rcar_cmm_setup() here too.

...

• if (rcmm->lut.restore) {

• rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
  

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

• rcmm->lut.restore = false;
  

• rcmm->lut.running = true;
  

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

+} +EXPORT_SYMBOL_GPL(rcar_cmm_enable);

+/**

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

Parameters usually go before the description test.

Indeed, sorry about this.

...

• */

+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.running = false;
• rcmm->lut.size = 0;
• rcmm->enabled = false;

+} +EXPORT_SYMBOL_GPL(rcar_cmm_disable);

+#ifdef CONFIG_PM_SLEEP +static int rcar_cmm_pm_suspend(struct device *dev) +{

• struct rcar_cmm *rcmm = dev_get_drvdata(dev);
• unsigned int i;
• if (!(rcmm->lut.running || rcmm->lut.restore))

Do you need the second part of this condition ? If a cached copy of the LUT data has been stored but not applied yet because the CMM is disabled, why would you need to overwrite that cached copy with values from the hardware ?

You are right, the second part of the condition is not needed (if not wrong).

I think you should have a first check on rcmm->enabled :

if (!rcmm->enabled) return 0;

as in that case you can't access the hardware. Then, you can save the LUT content only if it's running :

if (rcmm->lut.running) { /* Save the content */ ... rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0); }

I wouldn't clear rcmm->lut.running here, as from a software point of view I think we still want to record that it's enabled. There's also no need to touch the restore flag, see below.

...

• return 0;
  

• /* Save the LUT table entries in the scratch buffer table. */

Should we call this a cache instead of a scratch buffer ?

...

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

• int entry = rcar_cmm_read(rcmm, CM2_LUT_TBLA(i));
  

• struct drm_color_lut *lut = &rcmm->lut.table[i];
  

• lut->blue = entry & 0xff;
  

• lut->green = (entry >> 8) & 0xff;
  

• lut->red = (entry >> 16) & 0xff;
  

• }
• rcmm->lut.restore = true;
• rcmm->lut.running = false;
• rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
• return 0;

+}

+static int rcar_cmm_pm_resume(struct device *dev) +{

• struct rcar_cmm *rcmm = dev_get_drvdata(dev);
• if (!rcmm->lut.restore)

• return 0;
  

• /* Program the LUT entries saved at suspend time. */
• rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
• rcar_cmm_lut_write(rcmm, rcmm->lut.size, rcmm->lut.table);
• rcmm->lut.running = true;
• rcmm->lut.restore = false;

To be kept in sync with the modifications proposed above,

if (!rcmm->enabled) return 0;

if (rcmm->lut.running) { /* Program the LUT entries saved at suspend time. */ rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN); rcar_cmm_lut_write(rcmm, rcmm->lut.size, rcmm->lut.table); }

I think you can remove the restore field completely, as it's the only used in rcar_cmm_enable(), and there you can check rcmm->lut.running instead if you set rcmm->lut.running to true in rcar_cmm_setup() when the CMM is disabled and the config requests the LUT to be enabled. The overall logic should become simpler, with less corner cases.

Thanks, very good suggestion, I probably don't need any restore flag at all. I'll see how it looks like, thanks.

...

• return 0;

+} +#endif

+static const struct dev_pm_ops rcar_cmm_pm_ops = {

• SET_SYSTEM_SLEEP_PM_OPS(rcar_cmm_pm_suspend, rcar_cmm_pm_resume)

+};

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

I still think you can use devm_ioremap_resource(). I agree it doesn't explicitly request an uncached mapping, but I think the magic happens behind the scene with the IO accessors to ensure it will work fine.

Probably, but does using the _nocache version hurt somehow ?

...

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

• }
• return 0;

+}

+static const struct of_device_id rcar_cmm_of_table[] = {

• { .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,
• .driver = {

• .name	= "rcar-cmm",
  

• .pm	= &rcar_cmm_pm_ops,
  

• .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..8744e72f32cd --- /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 platform_device; +struct drm_color_lut;

Could you please sort the forward declarations alphabetically ?

...

+/**

• • struct rcar_cmm_config - CMM configuration
• • @lut: 1D-LUT configuration
• • @lut.enable: 1D-LUT enable flag
• • @lut.table: 1D-LUT table entries.

s/.$//

...

• • @lut.size 1D-LUT number of entries. Max is 256.

"Number of 1D-LUT entries (max 256)"

And missing : after @lut.size

Thanks for review j

...

• */

+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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

Hi Jacopo,

Thank you for the patch.

On Sat, Jul 06, 2019 at 04:07:42PM +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

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

---

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 */

On July 6, 2019 at 4:07 PM Jacopo Mondi jacopo+renesas@jmondi.org 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.

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 | 50 +++++++++++++++++++++++++ 5 files changed, 63 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 #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 f8f7fff34dff..b79cda2f5531 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" @@ -534,6 +535,51 @@ 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) {

• 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, "No device found for 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[] = { @@ -624,6 +670,10 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu) return ret; }

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

• return ret;
  

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

-- 2.21.0

Reviewed-by: Ulrich Hecht uli+renesas@fpond.eu

CU Uli

Hi Laurent,

On Tue, Aug 20, 2019 at 08:54:57PM +0300, Laurent Pinchart wrote:

Hi Jacopo,

Thank you for the patch.

On Sat, Jul 06, 2019 at 04:07:43PM +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.

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 | 50 +++++++++++++++++++++++++ 5 files changed, 63 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 #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

enabled or available ?

I considered having a 'cmm' entry in DT as enabling it, but it is actually just available.

...

• @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 f8f7fff34dff..b79cda2f5531 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" @@ -534,6 +535,51 @@ 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) {

Should this be

if (cells != rcdu->num_crtcs)

or do we want to support cases where not all DU channels have a CMM in DT ?

That was my idea yes, but I'm not sure it makes sense, as ideally CMM should be specified in DT for all SoC that provides it.

...

• dev_err(rcdu->dev, "Invalid 'cmms' property format\n");
  

How about "Invalid number of entries in 'cmms' property" ?

Ok

...

• 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, "No device found for cmms[%u]\n", i);
  

s/cmms[%u]/CMM%u/ ?

...

• 	of_node_put(cmm);
  

• 	return PTR_ERR(pdev);
  

• }
  

• if (!of_device_is_available(cmm)) {
  

Should this come before the pdev check, as there will be no pdev in that case ?

No pdev if the device is not enabled in DT ? Anyway, yes, I could move it up and save retrieving pdev in case the device is not available.

...

• 	/* 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[] = { @@ -624,6 +670,10 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu) return ret; }

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

• return ret;
  

ret = rcar_du_cmm_init(rcdu); if (ret < 0) return ret;

Ups, this was probably returning void in some earlier version and I failed to assign it properly, thanks for spotting this!

Thanks j

...

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

-- Regards,

Laurent Pinchart

On July 6, 2019 at 4:07 PM Jacopo Mondi jacopo+renesas@jmondi.org wrote:

Enable/disable the CMM associated with a CRTC at crtc start and stop time and enable the CMM unit through the Display Extensional Functions register at group setup time.

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_group.c | 8 ++++++++ drivers/gpu/drm/rcar-du/rcar_du_regs.h | 5 +++++ 3 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 23f1d6cc1719..3dac605c3a67 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" @@ -619,6 +620,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 (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 +690,9 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, }

rcar_du_crtc_start(rcrtc);

• if (rcrtc->cmm)

• rcar_cmm_enable(rcrtc->cmm);
  

}

static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc, 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)

Reviewed-by: Ulrich Hecht uli+renesas@fpond.eu

CU Uli

Hi Jacopo,

Thank you for the patch.

On Sat, Jul 06, 2019 at 04:07:45PM +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.

s/proeprty/property/ "and set the associated gamme table maximum size" ?

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

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

---

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

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 3dac605c3a67..222ccc20d6d8 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -1082,6 +1082,7 @@ static const struct drm_crtc_funcs crtc_funcs_gen3 = { .set_crc_source = rcar_du_crtc_set_crc_source, .verify_crc_source = rcar_du_crtc_verify_crc_source, .get_crc_sources = rcar_du_crtc_get_crc_sources,

• .gamma_set = drm_atomic_helper_legacy_gamma_set,

};

/* ----------------------------------------------------------------------------- @@ -1205,6 +1206,9 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex, if (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);

On July 6, 2019 at 4:07 PM Jacopo Mondi jacopo+renesas@jmondi.org 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 | 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 b79cda2f5531..f9aece78ca5f 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" @@ -287,6 +288,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]);
  

• 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) { @@ -329,6 +361,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

Reviewed-by: Ulrich Hecht uli+renesas@fpond.eu

CU Uli

Hi Laurent,

On Tue, Aug 20, 2019 at 09:42:15PM +0300, Laurent Pinchart wrote:

Hi Jacopo,

Thank you for the patch.

On Sat, Jul 06, 2019 at 04:07:46PM +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 | 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 b79cda2f5531..f9aece78ca5f 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" @@ -287,6 +288,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]);
  

• 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) { @@ -329,6 +361,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);
  

I think this looks good overall, but I wonder if we couldn't simplify the CMM driver suspend/resume and LUT caching due to config while not enabled by handling it on the DU side. I have a rework on the commit tail handler in progress, I'll think how this could be done. For now I think you can leave it as is.