Hi Jacopo,

On Thu, Jun 6, 2019 at 4:21 PM Jacopo Mondi jacopo+renesas@jmondi.org wrote:

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

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

R-Car Gen2 ...

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

Thanks for your patch!

index 000000000000..d8d3cf9ce2ce --- /dev/null +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt @@ -0,0 +1,25 @@ +* Renesas R-Car Color Management Module (CMM)

+Renesas R-Car image enhancement module connected to R-Car DU video channels.

+Required properties:

• • compatible: shall be one of:
• • "renesas,cmm-gen3"
• • "renesas,cmm-gen2"

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

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

Gr{oetje,eeting}s,

Geert

Hi Laurent, Jacopo,

On Thu, Jun 6, 2019 at 8:50 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:

On Thu, Jun 06, 2019 at 04:22:03PM +0200, Jacopo Mondi wrote:

...

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

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

...

--- a/Documentation/devicetree/bindings/display/renesas,du.txt +++ b/Documentation/devicetree/bindings/display/renesas,du.txt @@ -92,7 +92,7 @@ Example: R8A7795 (R-Car H3) ES2.0 DU <&cpg CPG_MOD 722>, <&cpg CPG_MOD 721>; clock-names = "du.0", "du.1", "du.2", "du.3";

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

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

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

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

...

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

Perhaps we want grouping here, too?

...

          ports {

Gr{oetje,eeting}s,

Geert

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:05PM +0200, Jacopo Mondi wrote:

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

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

drivers/clk/renesas/r8a7795-cpg-mssr.c | 4 ++++ 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/renesas/r8a7795-cpg-mssr.c b/drivers/clk/renesas/r8a7795-cpg-mssr.c index 86842c9fd314..e8f1d5ec0455 100644 --- a/drivers/clk/renesas/r8a7795-cpg-mssr.c +++ b/drivers/clk/renesas/r8a7795-cpg-mssr.c @@ -200,6 +200,10 @@ static struct mssr_mod_clk r8a7795_mod_clks[] __initdata = { DEF_MOD("ehci0", 703, R8A7795_CLK_S3D4), DEF_MOD("hsusb", 704, R8A7795_CLK_S3D4), DEF_MOD("hsusb3", 705, R8A7795_CLK_S3D4),

• DEF_MOD("cmm3", 708, R8A7795_CLK_S2D1),
• DEF_MOD("cmm2", 709, R8A7795_CLK_S2D1),
• DEF_MOD("cmm1", 710, R8A7795_CLK_S2D1),
• DEF_MOD("cmm0", 711, R8A7795_CLK_S2D1),

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

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

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

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:06PM +0200, Jacopo Mondi wrote:

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

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

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

---

drivers/clk/renesas/r8a77965-cpg-mssr.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/renesas/r8a77965-cpg-mssr.c b/drivers/clk/renesas/r8a77965-cpg-mssr.c index eb1cca58a1e1..58f75b03c8bb 100644 --- a/drivers/clk/renesas/r8a77965-cpg-mssr.c +++ b/drivers/clk/renesas/r8a77965-cpg-mssr.c @@ -178,6 +178,9 @@ static const struct mssr_mod_clk r8a77965_mod_clks[] __initconst = { DEF_MOD("ehci1", 702, R8A77965_CLK_S3D4), DEF_MOD("ehci0", 703, R8A77965_CLK_S3D4), DEF_MOD("hsusb", 704, R8A77965_CLK_S3D4),

• DEF_MOD("cmm3", 708, R8A77965_CLK_S2D1),
• DEF_MOD("cmm1", 710, R8A77965_CLK_S2D1),
• DEF_MOD("cmm0", 711, R8A77965_CLK_S2D1), DEF_MOD("csi20", 714, R8A77965_CLK_CSI0), DEF_MOD("csi40", 716, R8A77965_CLK_CSI0), DEF_MOD("du3", 721, R8A77965_CLK_S2D1),

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:08PM +0200, Jacopo Mondi wrote:

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

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

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

---

drivers/clk/renesas/r8a77995-cpg-mssr.c | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/renesas/r8a77995-cpg-mssr.c b/drivers/clk/renesas/r8a77995-cpg-mssr.c index eee3874865a9..acd5fabb785a 100644 --- a/drivers/clk/renesas/r8a77995-cpg-mssr.c +++ b/drivers/clk/renesas/r8a77995-cpg-mssr.c @@ -146,6 +146,8 @@ static const struct mssr_mod_clk r8a77995_mod_clks[] __initconst = { DEF_MOD("vspbs", 627, R8A77995_CLK_S0D1), DEF_MOD("ehci0", 703, R8A77995_CLK_S3D2), DEF_MOD("hsusb", 704, R8A77995_CLK_S3D2),

• DEF_MOD("cmm1", 710, R8A77995_CLK_S1D1),
• DEF_MOD("cmm0", 711, R8A77995_CLK_S1D1), DEF_MOD("du1", 723, R8A77995_CLK_S1D1), DEF_MOD("du0", 724, R8A77995_CLK_S1D1), DEF_MOD("lvds", 727, R8A77995_CLK_S2D1),

Hi Jacopo,

On Fri, Jun 07, 2019 at 02:35:40PM +0300, Laurent Pinchart wrote:

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:14PM +0200, Jacopo Mondi wrote:

...

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

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

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

...

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

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

...

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

drivers/gpu/drm/rcar-du/Kconfig | 7 + drivers/gpu/drm/rcar-du/Makefile | 1 + drivers/gpu/drm/rcar-du/rcar_cmm.c | 197 +++++++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 38 ++++++ 4 files changed, 243 insertions(+) create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.c create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.h

diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig index 1529849e217e..539d232790d1 100644 --- a/drivers/gpu/drm/rcar-du/Kconfig +++ b/drivers/gpu/drm/rcar-du/Kconfig @@ -13,6 +13,13 @@ config DRM_RCAR_DU Choose this option if you have an R-Car chipset. If M is selected the module will be called rcar-du-drm.

+config DRM_RCAR_CMM

• bool "R-Car DU Color Management Module (CMM) Support"
• depends on DRM && OF
• depends on DRM_RCAR_DU
• help

•  Enable support for R-Car Color Management Module (CMM).
  

config DRM_RCAR_DW_HDMI tristate "R-Car DU Gen3 HDMI Encoder Support" depends on DRM && OF diff --git a/drivers/gpu/drm/rcar-du/Makefile b/drivers/gpu/drm/rcar-du/Makefile index 6c2ed9c46467..4d1187ccc3e5 100644 --- a/drivers/gpu/drm/rcar-du/Makefile +++ b/drivers/gpu/drm/rcar-du/Makefile @@ -15,6 +15,7 @@ rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS) += rcar_du_of.o \ rcar-du-drm-$(CONFIG_DRM_RCAR_VSP) += rcar_du_vsp.o rcar-du-drm-$(CONFIG_DRM_RCAR_WRITEBACK) += rcar_du_writeback.o

+obj-$(CONFIG_DRM_RCAR_CMM) += rcar_cmm.o obj-$(CONFIG_DRM_RCAR_DU) += rcar-du-drm.o obj-$(CONFIG_DRM_RCAR_DW_HDMI) += rcar_dw_hdmi.o obj-$(CONFIG_DRM_RCAR_LVDS) += rcar_lvds.o diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c new file mode 100644 index 000000000000..5d9d917b91f4 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -0,0 +1,197 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • rcar_cmm.c -- R-Car Display Unit Color Management Module
• • Copyright (C) 2019 Jacopo Mondi jacopo+renesas@jmondi.org
• */

+#include <linux/clk.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h>

+#include <drm/drm_atomic.h>

+#include "rcar_cmm.h"

+#define CM2_LUT_CTRL 0x00

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

...

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

I would define this as

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

...

+struct rcar_cmm {

• struct clk *clk;
• void __iomem *base;
• bool enabled;
• /* LUT table scratch buffer. */
• struct {

• bool restore;
  

• unsigned int size;
  

• uint32_t table[CMM_GAMMA_LUT_SIZE];
  

• } lut;

+};

+static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg) +{

• return ioread32(rcmm->base + reg);

+}

+static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data) +{

• iowrite32(data, rcmm->base + reg);

+}

+int rcar_cmm_setup(struct platform_device *pdev, struct rcar_cmm_config *config)

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

...

+{

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

• return -EINVAL;
  

• /*

• * As cmm_setup is called by atomic commit tail helper, it might be
  

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

• *
  

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

• * programmed at enable time.
  

• */
  

• if (!rcmm->enabled) {

• if (!config->lut.enable)
  

• 	return 0;
  

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

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

• 	rcmm->lut.table[i] = (lut->red & 0xff) << 16 |
  

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

• 			     (lut->blue & 0xff);
  

• }
  

• rcmm->lut.restore = true;
  

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

• return 0;
  

• }
• if (rcar_cmm_read(rcmm, CM2_LUT_CTRL) & CM2_LUT_CTRL_EN &&

•    !config->lut.enable) {
  

• rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
  

• return 0;
  

• }
• /* Enable LUT and program the new gamma table values. */
• rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);

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

...

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

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

• u32 val = (lut->red & 0xff) << 16 | (lut->green & 0xff) << 8 |
  

• 	  (lut->blue & 0xff);
  

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

...

• rcar_cmm_write(rcmm, CM2_LUT_TBLA + i * 4, val);
  

• }
• return 0;

+}

You need to export this and the next two functions.

...

+int rcar_cmm_enable(struct platform_device *pdev) +{

• struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
• unsigned int i;
• int ret;
• if (rcmm->enabled)

• return 0;
  

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

...

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

• return ret;
  

Could you use pm_runtime_get_sync() instead ?

...

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

• rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
  

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

• 	rcar_cmm_write(rcmm, CM2_LUT_TBLA + i * 4,
  

• 		       rcmm->lut.table[i]);
  

• rcmm->lut.restore = false;
  

• rcmm->lut.size = 0;
  

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

+}

+void rcar_cmm_disable(struct platform_device *pdev) +{

• struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
• rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
• clk_disable_unprepare(rcmm->clk);
• rcmm->lut.restore = false;
• rcmm->lut.size = 0;
• rcmm->enabled = false;

+}

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

• struct rcar_cmm *rcmm;
• struct resource *res;
• resource_size_t size;
• rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
• if (!rcmm)

• return -ENOMEM;
  

• platform_set_drvdata(pdev, rcmm);
• res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
• size = resource_size(res);
• if (!devm_request_mem_region(&pdev->dev, res->start, size,

• 		     dev_name(&pdev->dev))) {
  

• dev_err(&pdev->dev,
  

• 	"can't request region for resource %pR\n", res);
  

• return -EBUSY;
  

• }
• rcmm->base = devm_ioremap_nocache(&pdev->dev, res->start, size);
• if (IS_ERR(rcmm->base))

• return PTR_ERR(rcmm->base);
  

Anything wrong with devm_ioremap_resource() ?

...

• rcmm->clk = devm_clk_get(&pdev->dev, NULL);
• if (IS_ERR(rcmm->clk)) {

• dev_err(&pdev->dev, "Failed to get CMM clock");
  

• return PTR_ERR(rcmm->clk);
  

• }
• rcmm->lut.restore = false;
• rcmm->lut.size = 0;
• rcmm->enabled = false;

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

...

• return 0;

+}

+static const struct of_device_id rcar_cmm_of_table[] = {

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

+};

+MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);

+static struct platform_driver rcar_cmm_platform_driver = {

• .probe = rcar_cmm_probe,
• .driver = {

• .name	= "rcar-cmm",
  

• .of_match_table = rcar_cmm_of_table,
  

• },

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

...

+};

+module_platform_driver(rcar_cmm_platform_driver);

+MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org");

Missing >.

...

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

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

...

+/*

• • rcar_cmm.h -- R-Car Display Unit Color Management Module
• • Copyright (C) 2019 Jacopo Mondi jacopo+renesas@jmondi.org
• */

+#ifndef __RCAR_CMM_H__ +#define __RCAR_CMM_H__

+#include <linux/platform_device.h>

You can forward-declare struct platform_device instead.

...

+#define CMM_GAMMA_LUT_SIZE 256

+struct drm_color_lut;

+/**

• • struct rcar_cmm_config - CMM configuration
• • @lut: 1D-LUT configuration
• • @lut.enable: 1D-LUT enable flag
• • @lut.table: 1D-LUT table entries.
• • @lut.size 1D-LUT number of entries. Max is 256

The last line is missing a colon.

...

• */

+struct rcar_cmm_config {

• struct {

• bool enable;
  

• struct drm_color_lut *table;
  

• unsigned int size;
  

• } lut;

+};

+int rcar_cmm_enable(struct platform_device *);

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

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

...

+void rcar_cmm_disable(struct platform_device *);

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

int foo(int, int, bool);

...

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

Can the second argument be const ?

...

+#endif /* __RCAR_CMM_H__ */

Hi Laurent, thanks for the review

On Fri, Jun 07, 2019 at 02:35:40PM +0300, Laurent Pinchart wrote:

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:14PM +0200, Jacopo Mondi wrote:

...

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

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

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

...

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

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

Hopefully :)

...

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

drivers/gpu/drm/rcar-du/Kconfig | 7 + drivers/gpu/drm/rcar-du/Makefile | 1 + drivers/gpu/drm/rcar-du/rcar_cmm.c | 197 +++++++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 38 ++++++ 4 files changed, 243 insertions(+) create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.c create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.h

diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig index 1529849e217e..539d232790d1 100644 --- a/drivers/gpu/drm/rcar-du/Kconfig +++ b/drivers/gpu/drm/rcar-du/Kconfig @@ -13,6 +13,13 @@ config DRM_RCAR_DU Choose this option if you have an R-Car chipset. If M is selected the module will be called rcar-du-drm.

+config DRM_RCAR_CMM

• bool "R-Car DU Color Management Module (CMM) Support"
• depends on DRM && OF
• depends on DRM_RCAR_DU
• help

•  Enable support for R-Car Color Management Module (CMM).
  

config DRM_RCAR_DW_HDMI tristate "R-Car DU Gen3 HDMI Encoder Support" depends on DRM && OF diff --git a/drivers/gpu/drm/rcar-du/Makefile b/drivers/gpu/drm/rcar-du/Makefile index 6c2ed9c46467..4d1187ccc3e5 100644 --- a/drivers/gpu/drm/rcar-du/Makefile +++ b/drivers/gpu/drm/rcar-du/Makefile @@ -15,6 +15,7 @@ rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS) += rcar_du_of.o \ rcar-du-drm-$(CONFIG_DRM_RCAR_VSP) += rcar_du_vsp.o rcar-du-drm-$(CONFIG_DRM_RCAR_WRITEBACK) += rcar_du_writeback.o

+obj-$(CONFIG_DRM_RCAR_CMM) += rcar_cmm.o obj-$(CONFIG_DRM_RCAR_DU) += rcar-du-drm.o obj-$(CONFIG_DRM_RCAR_DW_HDMI) += rcar_dw_hdmi.o obj-$(CONFIG_DRM_RCAR_LVDS) += rcar_lvds.o diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c new file mode 100644 index 000000000000..5d9d917b91f4 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -0,0 +1,197 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • rcar_cmm.c -- R-Car Display Unit Color Management Module
• • Copyright (C) 2019 Jacopo Mondi jacopo+renesas@jmondi.org
• */

+#include <linux/clk.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h>

+#include <drm/drm_atomic.h>

+#include "rcar_cmm.h"

+#define CM2_LUT_CTRL 0x00

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

...

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

I would define this as

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

Ack to both

...

+struct rcar_cmm {

• struct clk *clk;
• void __iomem *base;
• bool enabled;
• /* LUT table scratch buffer. */
• struct {

• bool restore;
  

• unsigned int size;
  

• uint32_t table[CMM_GAMMA_LUT_SIZE];
  

• } lut;

+};

+static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg) +{

• return ioread32(rcmm->base + reg);

+}

+static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data) +{

• iowrite32(data, rcmm->base + reg);

+}

+int rcar_cmm_setup(struct platform_device *pdev, struct rcar_cmm_config *config)

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

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

...

+{

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

• return -EINVAL;
  

• /*

• * As cmm_setup is called by atomic commit tail helper, it might be
  

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

• *
  

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

• * programmed at enable time.
  

• */
  

• if (!rcmm->enabled) {

• if (!config->lut.enable)
  

• 	return 0;
  

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

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

• 	rcmm->lut.table[i] = (lut->red & 0xff) << 16 |
  

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

• 			     (lut->blue & 0xff);
  

• }
  

• rcmm->lut.restore = true;
  

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

• return 0;
  

• }
• if (rcar_cmm_read(rcmm, CM2_LUT_CTRL) & CM2_LUT_CTRL_EN &&

•    !config->lut.enable) {
  

• rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
  

• return 0;
  

• }
• /* Enable LUT and program the new gamma table values. */
• rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);

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

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

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

...

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

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

• u32 val = (lut->red & 0xff) << 16 | (lut->green & 0xff) << 8 |
  

• 	  (lut->blue & 0xff);
  

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

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

...

• rcar_cmm_write(rcmm, CM2_LUT_TBLA + i * 4, val);
  

• }
• return 0;

+}

You need to export this and the next two functions.

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

...

+int rcar_cmm_enable(struct platform_device *pdev) +{

• struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
• unsigned int i;
• int ret;
• if (rcmm->enabled)

• return 0;
  

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

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

...

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

• return ret;
  

Could you use pm_runtime_get_sync() instead ?

I'll move to use pm_runtime in next version

...

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

• rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
  

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

• 	rcar_cmm_write(rcmm, CM2_LUT_TBLA + i * 4,
  

• 		       rcmm->lut.table[i]);
  

• rcmm->lut.restore = false;
  

• rcmm->lut.size = 0;
  

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

+}

+void rcar_cmm_disable(struct platform_device *pdev) +{

• struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
• rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
• clk_disable_unprepare(rcmm->clk);
• rcmm->lut.restore = false;
• rcmm->lut.size = 0;
• rcmm->enabled = false;

+}

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

• struct rcar_cmm *rcmm;
• struct resource *res;
• resource_size_t size;
• rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
• if (!rcmm)

• return -ENOMEM;
  

• platform_set_drvdata(pdev, rcmm);
• res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
• size = resource_size(res);
• if (!devm_request_mem_region(&pdev->dev, res->start, size,

• 		     dev_name(&pdev->dev))) {
  

• dev_err(&pdev->dev,
  

• 	"can't request region for resource %pR\n", res);
  

• return -EBUSY;
  

• }
• rcmm->base = devm_ioremap_nocache(&pdev->dev, res->start, size);
• if (IS_ERR(rcmm->base))

• return PTR_ERR(rcmm->base);
  

Anything wrong with devm_ioremap_resource() ?

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

...

• rcmm->clk = devm_clk_get(&pdev->dev, NULL);
• if (IS_ERR(rcmm->clk)) {

• dev_err(&pdev->dev, "Failed to get CMM clock");
  

• return PTR_ERR(rcmm->clk);
  

• }
• rcmm->lut.restore = false;
• rcmm->lut.size = 0;
• rcmm->enabled = false;

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

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

...

• return 0;

+}

+static const struct of_device_id rcar_cmm_of_table[] = {

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

+};

+MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);

+static struct platform_driver rcar_cmm_platform_driver = {

• .probe = rcar_cmm_probe,
• .driver = {

• .name	= "rcar-cmm",
  

• .of_match_table = rcar_cmm_of_table,
  

• },

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

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

...

+};

+module_platform_driver(rcar_cmm_platform_driver);

+MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org");

Missing >.

...

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

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

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

...

+/*

• • rcar_cmm.h -- R-Car Display Unit Color Management Module
• • Copyright (C) 2019 Jacopo Mondi jacopo+renesas@jmondi.org
• */

+#ifndef __RCAR_CMM_H__ +#define __RCAR_CMM_H__

+#include <linux/platform_device.h>

You can forward-declare struct platform_device instead.

...

+#define CMM_GAMMA_LUT_SIZE 256

+struct drm_color_lut;

+/**

• • struct rcar_cmm_config - CMM configuration
• • @lut: 1D-LUT configuration
• • @lut.enable: 1D-LUT enable flag
• • @lut.table: 1D-LUT table entries.
• • @lut.size 1D-LUT number of entries. Max is 256

The last line is missing a colon.

...

• */

+struct rcar_cmm_config {

• struct {

• bool enable;
  

• struct drm_color_lut *table;
  

• unsigned int size;
  

• } lut;

+};

+int rcar_cmm_enable(struct platform_device *);

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

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

...

+void rcar_cmm_disable(struct platform_device *);

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

int foo(int, int, bool);

...

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

Can the second argument be const ?

It probably could right now. Not sure if this will hold when we'll have to read histograms from the CMM, if they ever will go through this structure.

Thanks j

...

+#endif /* __RCAR_CMM_H__ */

-- Regards,

Laurent Pinchart

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:15PM +0200, Jacopo Mondi wrote:

Add CMM to the list of supported features for Gen3 SoCs that provide it:

• R8A7795
• R8A7796
• R8A77965
• R8A7799x

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

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

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

---

drivers/gpu/drm/rcar-du/rcar_du_drv.c | 12 ++++++++---- drivers/gpu/drm/rcar-du/rcar_du_drv.h | 1 + 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 75ab17af13a9..1e69cfa11798 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -247,7 +247,8 @@ static const struct rcar_du_device_info rcar_du_r8a7795_info = { .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK | RCAR_DU_FEATURE_VSP1_SOURCE | RCAR_DU_FEATURE_INTERLACED

•   | RCAR_DU_FEATURE_TVM_SYNC,
  

•   | RCAR_DU_FEATURE_TVM_SYNC
  

•   | RCAR_DU_FEATURE_CMM,
  

  .channels_mask = BIT(3) | BIT(2) | BIT(1) | BIT(0), .routes = { /*

@@ -280,7 +281,8 @@ static const struct rcar_du_device_info rcar_du_r8a7796_info = { .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK | RCAR_DU_FEATURE_VSP1_SOURCE | RCAR_DU_FEATURE_INTERLACED

•   | RCAR_DU_FEATURE_TVM_SYNC,
  

•   | RCAR_DU_FEATURE_TVM_SYNC
  

•   | RCAR_DU_FEATURE_CMM,
  

  .channels_mask = BIT(2) | BIT(1) | BIT(0), .routes = { /*

@@ -309,7 +311,8 @@ static const struct rcar_du_device_info rcar_du_r8a77965_info = { .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK | RCAR_DU_FEATURE_VSP1_SOURCE | RCAR_DU_FEATURE_INTERLACED

•   | RCAR_DU_FEATURE_TVM_SYNC,
  

•   | RCAR_DU_FEATURE_TVM_SYNC
  

•   | RCAR_DU_FEATURE_CMM,
  

  .channels_mask = BIT(3) | BIT(1) | BIT(0), .routes = { /*

@@ -357,7 +360,8 @@ static const struct rcar_du_device_info rcar_du_r8a77970_info = { static const struct rcar_du_device_info rcar_du_r8a7799x_info = { .gen = 3, .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK

•   | RCAR_DU_FEATURE_VSP1_SOURCE,
  

•   | RCAR_DU_FEATURE_VSP1_SOURCE
  

•   | RCAR_DU_FEATURE_CMM,
  

  .channels_mask = BIT(1) | BIT(0), .routes = { /*

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index 1327cd0df90a..a00dccc447aa 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h @@ -28,6 +28,7 @@ struct rcar_du_encoder; #define RCAR_DU_FEATURE_VSP1_SOURCE BIT(1) /* Has inputs from VSP1 */ #define RCAR_DU_FEATURE_INTERLACED BIT(2) /* HW supports interlaced */ #define RCAR_DU_FEATURE_TVM_SYNC BIT(3) /* Has TV switch/sync modes */ +#define RCAR_DU_FEATURE_CMM BIT(4) /* Has CMM */

#define RCAR_DU_QUIRK_ALIGN_128B BIT(0) /* Align pitches to 128 bytes */

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:16PM +0200, Jacopo Mondi wrote:

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

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

The feature is added in the previous patch.

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

drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 7 ++++ drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 + drivers/gpu/drm/rcar-du/rcar_du_drv.h | 3 ++ drivers/gpu/drm/rcar-du/rcar_du_group.h | 2 + drivers/gpu/drm/rcar-du/rcar_du_kms.c | 50 +++++++++++++++++++++++++ 5 files changed, 64 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 2da46e3dc4ae..9f270a54b164 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -1194,6 +1194,13 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex, if (ret < 0) return ret;

• /* CMM might be disabled for this CRTC. */
• if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM) &&

•    rcdu->cmms[swindex]) {
  

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

• rcrtc->cmm = rcdu->cmms[swindex];
  

• rgrp->cmms_mask |= BIT(hwindex % 2);
  

• }

• drm_crtc_helper_add(crtc, &crtc_helper_funcs);

  /* Start with vertical blanking interrupt reporting disabled. */

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h index 3b7fc668996f..5f2940c42225 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h @@ -39,6 +39,7 @@ struct rcar_du_vsp;

• @vblank_wait: wait queue used to signal vertical blanking
• @vblank_count: number of vertical blanking interrupts to wait for
• @group: CRTC group this CRTC belongs to

• • @cmm: CMM associated with this CRTC
  • @vsp: VSP feeding video to this CRTC
  • @vsp_pipe: index of the VSP pipeline feeding video to this CRTC
  • @writeback: the writeback connector

@@ -64,6 +65,7 @@ struct rcar_du_crtc { unsigned int vblank_count;

struct rcar_du_group *group;

• struct platform_device *cmm; struct rcar_du_vsp *vsp; unsigned int vsp_pipe;

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index a00dccc447aa..300ec60ba31b 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h @@ -13,6 +13,7 @@ #include <linux/kernel.h> #include <linux/wait.h>

+#include "rcar_cmm.h" #include "rcar_du_crtc.h" #include "rcar_du_group.h" #include "rcar_du_vsp.h" @@ -70,6 +71,7 @@ struct rcar_du_device_info {

#define RCAR_DU_MAX_CRTCS 4 #define RCAR_DU_MAX_GROUPS DIV_ROUND_UP(RCAR_DU_MAX_CRTCS, 2) +#define RCAR_DU_MAX_CMMS 4 #define RCAR_DU_MAX_VSPS 4

struct rcar_du_device { @@ -86,6 +88,7 @@ struct rcar_du_device { struct rcar_du_encoder *encoders[RCAR_DU_OUTPUT_MAX];

struct rcar_du_group groups[RCAR_DU_MAX_GROUPS];

• struct platform_device *cmms[RCAR_DU_MAX_CMMS]; struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS];

  struct {

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h b/drivers/gpu/drm/rcar-du/rcar_du_group.h index 87950c1f6a52..b0c1466593a3 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_group.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h @@ -22,6 +22,7 @@ struct rcar_du_device;

• @mmio_offset: registers offset in the device memory map
• @index: group index
• @channels_mask: bitmask of populated DU channels in this group

• • @cmms_mask: bitmask of enabled CMMs in this group
  • @num_crtcs: number of CRTCs in this group (1 or 2)
  • @use_count: number of users of the group (rcar_du_group_(get|put))
  • @used_crtcs: number of CRTCs currently in use

@@ -37,6 +38,7 @@ struct rcar_du_group { unsigned int index;

unsigned int channels_mask;

• unsigned int cmms_mask; unsigned int num_crtcs; unsigned int use_count; unsigned int used_crtcs;

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index adbc4f5d8fc5..5a910a04e1d9 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -18,6 +18,7 @@ #include <drm/drm_vblank.h>

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

#include "rcar_du_crtc.h" @@ -614,6 +615,48 @@ static int rcar_du_vsps_init(struct rcar_du_device *rcdu) return ret; }

+static int rcar_du_cmm_init(struct rcar_du_device *rcdu) +{

• const struct device_node *np = rcdu->dev->of_node;
• unsigned int cells;
• unsigned int i;
• cells = of_property_count_u32_elems(np, "cmms");
• if (cells > RCAR_DU_MAX_CMMS || cells > rcdu->num_crtcs) {

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

• return -EINVAL;
  

• }

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

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

• struct platform_device *pdev;
  

• struct device_node *cmm;
  

• cmm = of_parse_phandle(np, "cmms", i);
  

• if (IS_ERR(cmm)) {
  

• 	dev_err(rcdu->dev, "failed to parse 'cmms' property\n");
  

• 	return PTR_ERR(cmm);
  

• }
  

• pdev = of_find_device_by_node(cmm);
  

• if (IS_ERR(pdev)) {
  

• 	dev_err(rcdu->dev, "invalid property 'cmms'[%u]\n", i);
  

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

• 	of_node_put(cmm);
  

• 	return PTR_ERR(pdev);
  

• }
  

• if (!of_device_is_available(cmm)) {
  

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

• 	of_node_put(cmm);
  

• 	continue;
  

• }
  

• of_node_put(cmm);
  

• rcdu->cmms[i] = pdev;
  

• }
• return 0;

+}

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

• /* Initialize the Color Management Modules. */
• if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM)) {

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

• ret = rcar_du_cmm_init(rcdu);
  

• if (ret < 0)
  

• 	return ret;
  

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

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:17PM +0200, Jacopo Mondi wrote:

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

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

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

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

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 9f270a54b164..e6d3df37c827 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -21,6 +21,7 @@ #include <drm/drm_plane_helper.h> #include <drm/drm_vblank.h>

+#include "rcar_cmm.h" #include "rcar_du_crtc.h" #include "rcar_du_drv.h" #include "rcar_du_encoder.h" @@ -523,6 +524,7 @@ static int rcar_du_crtc_get(struct rcar_du_crtc *rcrtc) goto error_group;

rcar_du_crtc_setup(rcrtc);

Unrelated change ?

rcrtc->initialized = true;

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

• if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_CMM) && rcrtc->cmm)

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

• rcar_cmm_disable(rcrtc->cmm);
  

• /*
  • Select switch sync mode. This stops display operation and configures
  • the HSYNC and VSYNC signals as inputs.

@@ -686,6 +691,9 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, }

rcar_du_crtc_start(rcrtc);

• if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_CMM) && rcrtc->cmm)

• rcar_cmm_enable(rcrtc->cmm);
  

}

static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:18PM +0200, Jacopo Mondi wrote:

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

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

drivers/gpu/drm/rcar-du/rcar_du_group.c | 8 ++++++++ drivers/gpu/drm/rcar-du/rcar_du_regs.h | 5 +++++ 2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c index 9eee47969e77..d252c9bb9809 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c @@ -147,6 +147,14 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)

rcar_du_group_setup_pins(rgrp);

• if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM)) {

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

• u32 defr7 = DEFR7_CODE |
  

• 	    (rgrp->cmms_mask & BIT(1) ? DEFR7_CMME1 : 0) |
  

• 	    (rgrp->cmms_mask & BIT(0) ? DEFR7_CMME0 : 0);
  

• rcar_du_group_write(rgrp, DEFR7, defr7);
  

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

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

• }
• if (rcdu->info->gen >= 2) { rcar_du_group_setup_defr8(rgrp); rcar_du_group_setup_didsr(rgrp);

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h index bc87f080b170..fb9964949368 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h @@ -197,6 +197,11 @@ #define DEFR6_MLOS1 (1 << 2) #define DEFR6_DEFAULT (DEFR6_CODE | DEFR6_TCNE1)

+#define DEFR7 0x000ec +#define DEFR7_CODE (0x7779 << 16) +#define DEFR7_CMME1 BIT(6) +#define DEFR7_CMME0 BIT(4)

/* -----------------------------------------------------------------------------

• R8A7790-only Control Registers

*/

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:19PM +0200, Jacopo Mondi wrote:

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

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

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

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

• drm_mode_crtc_set_gamma_size(crtc, CMM_GAMMA_LUT_SIZE);
  

• drm_crtc_enable_color_mgmt(crtc, 0, false, CMM_GAMMA_LUT_SIZE);
  

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

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

}

drm_crtc_helper_add(crtc, &crtc_helper_funcs);

On Fri, Jun 14, 2019 at 10:15:52AM +0200, Jacopo Mondi wrote:

Hi Laurent, thanks for review

On Fri, Jun 07, 2019 at 03:03:04PM +0300, Laurent Pinchart wrote:

...

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:19PM +0200, Jacopo Mondi wrote:

...

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

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

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

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

• drm_mode_crtc_set_gamma_size(crtc, CMM_GAMMA_LUT_SIZE);
  

• drm_crtc_enable_color_mgmt(crtc, 0, false, CMM_GAMMA_LUT_SIZE);
  

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

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

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

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

Thanks j

...

...

}

drm_crtc_helper_add(crtc, &crtc_helper_funcs);

-- Regards,

Laurent Pinchart

On Fri, Jun 14, 2019 at 11:27:45AM +0200, Jacopo Mondi wrote:

Hi Daniel,

On Fri, Jun 14, 2019 at 10:42:51AM +0200, Daniel Vetter wrote:

...

On Fri, Jun 14, 2019 at 10:15:52AM +0200, Jacopo Mondi wrote:

...

Hi Laurent, thanks for review

On Fri, Jun 07, 2019 at 03:03:04PM +0300, Laurent Pinchart wrote:

...

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:19PM +0200, Jacopo Mondi wrote:

...

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

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

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

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

• drm_mode_crtc_set_gamma_size(crtc, CMM_GAMMA_LUT_SIZE);
  

• drm_crtc_enable_color_mgmt(crtc, 0, false, CMM_GAMMA_LUT_SIZE);
  

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

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

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

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

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

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

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

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

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

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

Thanks j

...

-Daniel

...

Thanks j

...

...

}

drm_crtc_helper_add(crtc, &crtc_helper_funcs);

-- Regards,

Laurent Pinchart

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

---

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

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 06, 2019 at 04:22:20PM +0200, Jacopo Mondi wrote:

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

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

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

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

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 5a910a04e1d9..29a2020a46b5 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -21,6 +21,7 @@ #include <linux/of_platform.h> #include <linux/wait.h>

+#include "rcar_cmm.h" #include "rcar_du_crtc.h" #include "rcar_du_drv.h" #include "rcar_du_encoder.h" @@ -367,6 +368,38 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv,

• Atomic Check and Update

*/

+static void rcar_du_atomic_commit_update_cmm(struct drm_crtc *crtc,

• 			     struct drm_crtc_state *old_state)
  

+{

• struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
• struct rcar_cmm_config cmm_config = {};
• if (!rcrtc->cmm || !crtc->state->color_mgmt_changed)

• return;
  

• if (!crtc->state->gamma_lut) {

• cmm_config.lut.enable = false;
  

• rcar_cmm_setup(rcrtc->cmm, &cmm_config);
  

• return;
  

• }
• cmm_config.lut.enable = true;
• cmm_config.lut.table = (struct drm_color_lut *)

• 	       crtc->state->gamma_lut->data;
  

• /* Set LUT table size to 0 if entries should not be updated. */
• if (!old_state->gamma_lut ||

•    (old_state->gamma_lut->base.id !=
  

•    crtc->state->gamma_lut->base.id))
  

• cmm_config.lut.size = crtc->state->gamma_lut->length
  

• 		    / sizeof(cmm_config.lut.table[0]);
  

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

• else

• cmm_config.lut.size = 0;
  

• rcar_cmm_setup(rcrtc->cmm, &cmm_config);

+}

static int rcar_du_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) { @@ -409,6 +442,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state) rcdu->dpad1_source = rcrtc->index; }

• for_each_old_crtc_in_state(old_state, crtc, crtc_state, i)

• rcar_du_atomic_commit_update_cmm(crtc, crtc_state);
  

• /* Apply the atomic update. */ drm_atomic_helper_commit_modeset_disables(dev, old_state); drm_atomic_helper_commit_planes(dev, old_state,

Hi Jacopo,

On Fri, Jun 14, 2019 at 10:19:13AM +0200, Jacopo Mondi wrote:

On Fri, Jun 07, 2019 at 03:06:33PM +0300, Laurent Pinchart wrote:

...

On Thu, Jun 06, 2019 at 04:22:20PM +0200, Jacopo Mondi wrote:

...

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

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

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

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

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 5a910a04e1d9..29a2020a46b5 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -21,6 +21,7 @@ #include <linux/of_platform.h> #include <linux/wait.h>

+#include "rcar_cmm.h" #include "rcar_du_crtc.h" #include "rcar_du_drv.h" #include "rcar_du_encoder.h" @@ -367,6 +368,38 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv,

• Atomic Check and Update

*/

+static void rcar_du_atomic_commit_update_cmm(struct drm_crtc *crtc,

• 			     struct drm_crtc_state *old_state)
  

+{

• struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
• struct rcar_cmm_config cmm_config = {};
• if (!rcrtc->cmm || !crtc->state->color_mgmt_changed)

• return;
  

• if (!crtc->state->gamma_lut) {

• cmm_config.lut.enable = false;
  

• rcar_cmm_setup(rcrtc->cmm, &cmm_config);
  

• return;
  

• }
• cmm_config.lut.enable = true;
• cmm_config.lut.table = (struct drm_color_lut *)

• 	       crtc->state->gamma_lut->data;
  

• /* Set LUT table size to 0 if entries should not be updated. */
• if (!old_state->gamma_lut ||

•    (old_state->gamma_lut->base.id !=
  

•    crtc->state->gamma_lut->base.id))
  

• cmm_config.lut.size = crtc->state->gamma_lut->length
  

• 		    / sizeof(cmm_config.lut.table[0]);
  

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

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

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

...

...

• else

• cmm_config.lut.size = 0;
  

• rcar_cmm_setup(rcrtc->cmm, &cmm_config);

+}

static int rcar_du_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) { @@ -409,6 +442,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state) rcdu->dpad1_source = rcrtc->index; }

• for_each_old_crtc_in_state(old_state, crtc, crtc_state, i)

• rcar_du_atomic_commit_update_cmm(crtc, crtc_state);
  

• /* Apply the atomic update. */ drm_atomic_helper_commit_modeset_disables(dev, old_state); drm_atomic_helper_commit_planes(dev, old_state,