Hello,
this series add support for the Color Management Module (CMM) found on
the R-Car Display Unit output channels. CMM is present in most of the Gen3
SoCs (V3-H and V3-M excluded) and in Gen2 (which is not supported by the
series).
The CMM allows setting 1-D and 3-D gamma tables and generates histograms on
images before they are directed to the DU's output pads.
The current implementation only supports configuration of the 1-D gamma table,
by using the GAMMA_LUT KMS property. On top of this current version, more
advanced features such as 3-D LUT support and histogram generation can be
later implemented.
The series starts by adding the bindings definitions and by registering the
clock providers for the various SoCs that support the CMM. It then registers
CMMs in the SoC device tree sources and finally adds a driver for the CMM
itself and integrates it in the rcar-du driver infrastructure to register,
enable and update the CMM units.
Tested on M3-W Salvator-x on HDMI connector and on E3 Ebisu on VGA connector
by injecting a 'color inversion' gamma table using a patched version of
the KMS++ kmstest utility[1], and observing the CMM applies the correction
matrix properly.
Series based on Laurent's drm/du/next branch at
git://linuxtv.org/pinchartl/media.git
Thanks
j
[1] https://github.com/tomba/kmsxx/
Jacopo Mondi (20):
dt-bindings: display: renesas,cmm: Add R-Car CMM documentation
dt-bindings: display, renesas,du: Document cmms property
dt-bindings: display, renesas,du: Update 'vsps' in example
clk: renesas: r8a7796: Add CMM clocks
clk: renesas: r8a7795: Add CMM clocks
clk: renesas: r8a77965: Add CMM clocks
clk: renesas: r8a77990: Add CMM clocks
clk: renesas: r8a77995: Add CMM clocks
arm64: dts: renesas: r8a7796: Add CMM units
arm64: dts: renesas: r8a7795: Add CMM units
arm64: dts: renesas: r8a77965: Add CMM units
arm64: dts: renesas: r8a77990: Add CMM units
arm64: dts: renesas: r8a77995: Add CMM units
drm: rcar-du: Add support for CMM
drm: rcar-du: Claim CMM support for Gen3 SoCs
drm: rcar-du: kms: Collect CMM instances
drm: rcar-du: crtc: Enable and disable CMMs
drm: rcar-du: group: Enable DU's CMM extension
drm: rcar-du: crtc: Register GAMMA_LUT properties
drm: rcar-du: kms: Update CMM in atomic commit tail
.../bindings/display/renesas,cmm.txt | 25 +++
.../bindings/display/renesas,du.txt | 7 +-
arch/arm64/boot/dts/renesas/r8a7795.dtsi | 36 +++-
arch/arm64/boot/dts/renesas/r8a7796.dtsi | 25 +++
arch/arm64/boot/dts/renesas/r8a77965.dtsi | 25 +++
arch/arm64/boot/dts/renesas/r8a77990.dtsi | 20 +-
arch/arm64/boot/dts/renesas/r8a77995.dtsi | 20 +-
drivers/clk/renesas/r8a7795-cpg-mssr.c | 4 +
drivers/clk/renesas/r8a7796-cpg-mssr.c | 3 +
drivers/clk/renesas/r8a77965-cpg-mssr.c | 3 +
drivers/clk/renesas/r8a77990-cpg-mssr.c | 2 +
drivers/clk/renesas/r8a77995-cpg-mssr.c | 2 +
drivers/gpu/drm/rcar-du/Kconfig | 7 +
drivers/gpu/drm/rcar-du/Makefile | 1 +
drivers/gpu/drm/rcar-du/rcar_cmm.c | 197 ++++++++++++++++++
drivers/gpu/drm/rcar-du/rcar_cmm.h | 38 ++++
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 18 ++
drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 +
drivers/gpu/drm/rcar-du/rcar_du_drv.c | 12 +-
drivers/gpu/drm/rcar-du/rcar_du_drv.h | 4 +
drivers/gpu/drm/rcar-du/rcar_du_group.c | 8 +
drivers/gpu/drm/rcar-du/rcar_du_group.h | 2 +
drivers/gpu/drm/rcar-du/rcar_du_kms.c | 86 ++++++++
drivers/gpu/drm/rcar-du/rcar_du_regs.h | 5 +
24 files changed, 544 insertions(+), 8 deletions(-)
create mode 100644 Documentation/devicetree/bindings/display/renesas,cmm.txt
create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.c
create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.h
--
2.21.0
Add clock definitions for CMM units on Renesas R-Car Gen3 H3.
Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/clk/renesas/r8a7795-cpg-mssr.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/clk/renesas/r8a7795-cpg-mssr.c b/drivers/clk/renesas/r8a7795-cpg-mssr.c
index 86842c9fd314..e8f1d5ec0455 100644
--- a/drivers/clk/renesas/r8a7795-cpg-mssr.c
+++ b/drivers/clk/renesas/r8a7795-cpg-mssr.c
@@ -200,6 +200,10 @@ static struct mssr_mod_clk r8a7795_mod_clks[] __initdata = {
DEF_MOD("ehci0", 703, R8A7795_CLK_S3D4),
DEF_MOD("hsusb", 704, R8A7795_CLK_S3D4),
DEF_MOD("hsusb3", 705, R8A7795_CLK_S3D4),
+ DEF_MOD("cmm3", 708, R8A7795_CLK_S2D1),
+ DEF_MOD("cmm2", 709, R8A7795_CLK_S2D1),
+ DEF_MOD("cmm1", 710, R8A7795_CLK_S2D1),
+ DEF_MOD("cmm0", 711, R8A7795_CLK_S2D1),
DEF_MOD("csi21", 713, R8A7795_CLK_CSI0), /* ES1.x */
DEF_MOD("csi20", 714, R8A7795_CLK_CSI0),
DEF_MOD("csi41", 715, R8A7795_CLK_CSI0),
--
2.21.0
Add a driver for the R-Car Display Unit Color Correction Module.
Each DU output channel is provided with a CMM unit to perform image
enhancement and color correction.
Add support for CMM through a driver that supports configuration of
the 1-dimensional LUT table. More advanced CMM feature could be
implemented on top of this basic one.
Signed-off-by: Jacopo Mondi <[email protected]>
---
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 <[email protected]>
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#include <drm/drm_atomic.h>
+
+#include "rcar_cmm.h"
+
+#define CM2_LUT_CTRL 0x00
+#define CM2_LUT_CTRL_EN BIT(0)
+#define CM2_LUT_TBLA 0x600
+
+struct rcar_cmm {
+ struct clk *clk;
+ void __iomem *base;
+ bool enabled;
+
+ /* LUT table scratch buffer. */
+ struct {
+ bool restore;
+ unsigned int size;
+ uint32_t table[CMM_GAMMA_LUT_SIZE];
+ } lut;
+};
+
+static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg)
+{
+ return ioread32(rcmm->base + reg);
+}
+
+static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data)
+{
+ iowrite32(data, rcmm->base + reg);
+}
+
+int rcar_cmm_setup(struct platform_device *pdev, struct rcar_cmm_config *config)
+{
+ struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
+ unsigned int i;
+
+ if (config->lut.size > CMM_GAMMA_LUT_SIZE)
+ return -EINVAL;
+
+ /*
+ * As cmm_setup is called by atomic commit tail helper, it might be
+ * called before the enabling the CRTC (which calls cmm_enable()).
+ *
+ * Store the LUT table entries in the scratch buffer to be later
+ * programmed at enable time.
+ */
+ if (!rcmm->enabled) {
+ if (!config->lut.enable)
+ return 0;
+
+ for (i = 0; i < config->lut.size; ++i) {
+ struct drm_color_lut *lut = &config->lut.table[i];
+
+ rcmm->lut.table[i] = (lut->red & 0xff) << 16 |
+ (lut->green & 0xff) << 8 |
+ (lut->blue & 0xff);
+ }
+
+ rcmm->lut.restore = true;
+ rcmm->lut.size = config->lut.size;
+
+ return 0;
+ }
+
+ if (rcar_cmm_read(rcmm, CM2_LUT_CTRL) & CM2_LUT_CTRL_EN &&
+ !config->lut.enable) {
+ rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
+ return 0;
+ }
+
+ /* Enable LUT and program the new gamma table values. */
+ rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
+ for (i = 0; i < config->lut.size; ++i) {
+ struct drm_color_lut *lut = &config->lut.table[i];
+ u32 val = (lut->red & 0xff) << 16 | (lut->green & 0xff) << 8 |
+ (lut->blue & 0xff);
+
+ rcar_cmm_write(rcmm, CM2_LUT_TBLA + i * 4, val);
+ }
+
+ return 0;
+}
+
+int rcar_cmm_enable(struct platform_device *pdev)
+{
+ struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
+ unsigned int i;
+ int ret;
+
+ if (rcmm->enabled)
+ return 0;
+
+ ret = clk_prepare_enable(rcmm->clk);
+ if (ret)
+ return ret;
+
+ /* Apply the LUT table values saved at cmm_setup time. */
+ if (rcmm->lut.restore) {
+ rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
+ for (i = 0; i < rcmm->lut.size; ++i)
+ rcar_cmm_write(rcmm, CM2_LUT_TBLA + i * 4,
+ rcmm->lut.table[i]);
+
+ rcmm->lut.restore = false;
+ rcmm->lut.size = 0;
+ }
+
+ rcmm->enabled = true;
+
+ return 0;
+}
+
+void rcar_cmm_disable(struct platform_device *pdev)
+{
+ struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
+
+ rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
+
+ clk_disable_unprepare(rcmm->clk);
+
+ rcmm->lut.restore = false;
+ rcmm->lut.size = 0;
+ rcmm->enabled = false;
+}
+
+static int rcar_cmm_probe(struct platform_device *pdev)
+{
+ struct rcar_cmm *rcmm;
+ struct resource *res;
+ resource_size_t size;
+
+ rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
+ if (!rcmm)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, rcmm);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ size = resource_size(res);
+ if (!devm_request_mem_region(&pdev->dev, res->start, size,
+ dev_name(&pdev->dev))) {
+ dev_err(&pdev->dev,
+ "can't request region for resource %pR\n", res);
+ return -EBUSY;
+ }
+
+ rcmm->base = devm_ioremap_nocache(&pdev->dev, res->start, size);
+ if (IS_ERR(rcmm->base))
+ return PTR_ERR(rcmm->base);
+
+ rcmm->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(rcmm->clk)) {
+ dev_err(&pdev->dev, "Failed to get CMM clock");
+ return PTR_ERR(rcmm->clk);
+ }
+
+ rcmm->lut.restore = false;
+ rcmm->lut.size = 0;
+ rcmm->enabled = false;
+
+ return 0;
+}
+
+static const struct of_device_id rcar_cmm_of_table[] = {
+ { .compatible = "renesas,cmm-gen3" },
+ { .compatible = "renesas,cmm-gen2" },
+ { },
+};
+
+MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
+
+static struct platform_driver rcar_cmm_platform_driver = {
+ .probe = rcar_cmm_probe,
+ .driver = {
+ .name = "rcar-cmm",
+ .of_match_table = rcar_cmm_of_table,
+ },
+};
+
+module_platform_driver(rcar_cmm_platform_driver);
+
+MODULE_AUTHOR("Jacopo Mondi <[email protected]");
+MODULE_DESCRIPTION("Renesas R-Car CMM Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h
new file mode 100644
index 000000000000..da61a145dc5c
--- /dev/null
+++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * rcar_cmm.h -- R-Car Display Unit Color Management Module
+ *
+ * Copyright (C) 2019 Jacopo Mondi <[email protected]>
+ */
+
+#ifndef __RCAR_CMM_H__
+#define __RCAR_CMM_H__
+
+#include <linux/platform_device.h>
+
+#define CMM_GAMMA_LUT_SIZE 256
+
+struct drm_color_lut;
+
+/**
+ * struct rcar_cmm_config - CMM configuration
+ *
+ * @lut: 1D-LUT configuration
+ * @lut.enable: 1D-LUT enable flag
+ * @lut.table: 1D-LUT table entries.
+ * @lut.size 1D-LUT number of entries. Max is 256
+ */
+struct rcar_cmm_config {
+ struct {
+ bool enable;
+ struct drm_color_lut *table;
+ unsigned int size;
+ } lut;
+};
+
+int rcar_cmm_enable(struct platform_device *);
+void rcar_cmm_disable(struct platform_device *);
+
+int rcar_cmm_setup(struct platform_device *, struct rcar_cmm_config *);
+
+#endif /* __RCAR_CMM_H__ */
--
2.21.0
Add CMM units to Renesas R-Car D3 device tree and reference them from
the Display Unit they are connected to.
While at it, re-sort the du device node properties to match the ordering
found in other SoCs.
Signed-off-by: Jacopo Mondi <[email protected]>
---
arch/arm64/boot/dts/renesas/r8a77995.dtsi | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/renesas/r8a77995.dtsi b/arch/arm64/boot/dts/renesas/r8a77995.dtsi
index 5bf3af246e14..c52d73068304 100644
--- a/arch/arm64/boot/dts/renesas/r8a77995.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a77995.dtsi
@@ -993,6 +993,22 @@
iommus = <&ipmmu_vi0 9>;
};
+ cmm0: cmm@fea40000 {
+ compatible = "renesas,cmm-gen3";
+ reg = <0 0xfea40000 0 0x1000>;
+ clocks = <&cpg CPG_MOD 711>;
+ power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
+ resets = <&cpg 711>;
+ };
+
+ cmm1: cmm@fea50000 {
+ compatible = "renesas,cmm-gen3";
+ reg = <0 0xfea50000 0 0x1000>;
+ clocks = <&cpg CPG_MOD 710>;
+ power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
+ resets = <&cpg 710>;
+ };
+
du: display@feb00000 {
compatible = "renesas,du-r8a77995";
reg = <0 0xfeb00000 0 0x80000>;
@@ -1001,9 +1017,11 @@
clocks = <&cpg CPG_MOD 724>,
<&cpg CPG_MOD 723>;
clock-names = "du.0", "du.1";
- vsps = <&vspd0 0 &vspd1 0>;
status = "disabled";
+ vsps = <&vspd0 0 &vspd1 0>;
+ cmms = <&cmm0 &cmm1>;
+
ports {
#address-cells = <1>;
#size-cells = <0>;
--
2.21.0
Add CMM units to Renesas R-Car E3 device tree and reference them from
the Display Unit they are connected to.
While at it, re-sort the du device node properties to match the ordering
found in other SoCs.
Signed-off-by: Jacopo Mondi <[email protected]>
---
arch/arm64/boot/dts/renesas/r8a77990.dtsi | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/renesas/r8a77990.dtsi b/arch/arm64/boot/dts/renesas/r8a77990.dtsi
index a69faa60ea4d..87453ddbc7e3 100644
--- a/arch/arm64/boot/dts/renesas/r8a77990.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a77990.dtsi
@@ -1656,6 +1656,22 @@
iommus = <&ipmmu_vi0 9>;
};
+ cmm0: cmm@fea40000 {
+ compatible = "renesas,cmm-gen3";
+ reg = <0 0xfea40000 0 0x1000>;
+ clocks = <&cpg CPG_MOD 711>;
+ power-domains = <&sysc R8A77990_PD_ALWAYS_ON>;
+ resets = <&cpg 711>;
+ };
+
+ cmm1: cmm@fea50000 {
+ compatible = "renesas,cmm-gen3";
+ reg = <0 0xfea50000 0 0x1000>;
+ clocks = <&cpg CPG_MOD 710>;
+ power-domains = <&sysc R8A77990_PD_ALWAYS_ON>;
+ resets = <&cpg 710>;
+ };
+
csi40: csi2@feaa0000 {
compatible = "renesas,r8a77990-csi2", "renesas,rcar-gen3-csi2";
reg = <0 0xfeaa0000 0 0x10000>;
@@ -1695,9 +1711,11 @@
clocks = <&cpg CPG_MOD 724>,
<&cpg CPG_MOD 723>;
clock-names = "du.0", "du.1";
- vsps = <&vspd0 0 &vspd1 0>;
status = "disabled";
+ vsps = <&vspd0 0 &vspd1 0>;
+ cmms = <&cmm0 &cmm1>;
+
ports {
#address-cells = <1>;
#size-cells = <0>;
--
2.21.0
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 <[email protected]>
> ---
> 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 <[email protected]>
> DEF_MOD("csi21", 713, R8A7795_CLK_CSI0), /* ES1.x */
> DEF_MOD("csi20", 714, R8A7795_CLK_CSI0),
> DEF_MOD("csi41", 715, R8A7795_CLK_CSI0),
--
Regards,
Laurent Pinchart
Hi Jacopo,
Thank you for the patch.
On Thu, Jun 06, 2019 at 04:22:12PM +0200, Jacopo Mondi wrote:
> Add CMM units to Renesas R-Car E3 device tree and reference them from
> the Display Unit they are connected to.
>
> While at it, re-sort the du device node properties to match the ordering
> found in other SoCs.
>
> Signed-off-by: Jacopo Mondi <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> arch/arm64/boot/dts/renesas/r8a77990.dtsi | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a77990.dtsi b/arch/arm64/boot/dts/renesas/r8a77990.dtsi
> index a69faa60ea4d..87453ddbc7e3 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77990.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a77990.dtsi
> @@ -1656,6 +1656,22 @@
> iommus = <&ipmmu_vi0 9>;
> };
>
> + cmm0: cmm@fea40000 {
> + compatible = "renesas,cmm-gen3";
> + reg = <0 0xfea40000 0 0x1000>;
> + clocks = <&cpg CPG_MOD 711>;
> + power-domains = <&sysc R8A77990_PD_ALWAYS_ON>;
> + resets = <&cpg 711>;
> + };
> +
> + cmm1: cmm@fea50000 {
> + compatible = "renesas,cmm-gen3";
> + reg = <0 0xfea50000 0 0x1000>;
> + clocks = <&cpg CPG_MOD 710>;
> + power-domains = <&sysc R8A77990_PD_ALWAYS_ON>;
> + resets = <&cpg 710>;
> + };
> +
> csi40: csi2@feaa0000 {
> compatible = "renesas,r8a77990-csi2", "renesas,rcar-gen3-csi2";
> reg = <0 0xfeaa0000 0 0x10000>;
> @@ -1695,9 +1711,11 @@
> clocks = <&cpg CPG_MOD 724>,
> <&cpg CPG_MOD 723>;
> clock-names = "du.0", "du.1";
> - vsps = <&vspd0 0 &vspd1 0>;
> status = "disabled";
>
> + vsps = <&vspd0 0 &vspd1 0>;
> + cmms = <&cmm0 &cmm1>;
> +
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
--
Regards,
Laurent Pinchart
Hi Laurent,
On Thu, Jun 06, 2019 at 07:58:57PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Jun 06, 2019 at 04:22:05PM +0200, Jacopo Mondi wrote:
> > Add clock definitions for CMM units on Renesas R-Car Gen3 H3.
> >
> > Signed-off-by: Jacopo Mondi <[email protected]>
> > ---
> > drivers/clk/renesas/r8a7795-cpg-mssr.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/clk/renesas/r8a7795-cpg-mssr.c b/drivers/clk/renesas/r8a7795-cpg-mssr.c
> > index 86842c9fd314..e8f1d5ec0455 100644
> > --- a/drivers/clk/renesas/r8a7795-cpg-mssr.c
> > +++ b/drivers/clk/renesas/r8a7795-cpg-mssr.c
> > @@ -200,6 +200,10 @@ static struct mssr_mod_clk r8a7795_mod_clks[] __initdata = {
> > DEF_MOD("ehci0", 703, R8A7795_CLK_S3D4),
> > DEF_MOD("hsusb", 704, R8A7795_CLK_S3D4),
> > DEF_MOD("hsusb3", 705, R8A7795_CLK_S3D4),
> > + DEF_MOD("cmm3", 708, R8A7795_CLK_S2D1),
> > + DEF_MOD("cmm2", 709, R8A7795_CLK_S2D1),
> > + DEF_MOD("cmm1", 710, R8A7795_CLK_S2D1),
> > + DEF_MOD("cmm0", 711, R8A7795_CLK_S2D1),
>
> Could you try to get confirmation that S2D1 is the right parent (for all
> SoCs) ? Apart from that,
It's not.. for r8a7799x it's S1D1, the same parent as the DU clock.
The patches in the BSP use the same clocks I have used here, so I
assume at least that part is correct.
>
> Reviewed-by: Laurent Pinchart <[email protected]>
Thanks
j
>
> > DEF_MOD("csi21", 713, R8A7795_CLK_CSI0), /* ES1.x */
> > DEF_MOD("csi20", 714, R8A7795_CLK_CSI0),
> > DEF_MOD("csi41", 715, R8A7795_CLK_CSI0),
>
> --
> Regards,
>
> Laurent Pinchart
Hi Jacopo,
Thank you for the patch.
On Thu, Jun 06, 2019 at 04:22:13PM +0200, Jacopo Mondi wrote:
> Add CMM units to Renesas R-Car D3 device tree and reference them from
> the Display Unit they are connected to.
>
> While at it, re-sort the du device node properties to match the ordering
> found in other SoCs.
>
> Signed-off-by: Jacopo Mondi <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> arch/arm64/boot/dts/renesas/r8a77995.dtsi | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a77995.dtsi b/arch/arm64/boot/dts/renesas/r8a77995.dtsi
> index 5bf3af246e14..c52d73068304 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77995.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a77995.dtsi
> @@ -993,6 +993,22 @@
> iommus = <&ipmmu_vi0 9>;
> };
>
> + cmm0: cmm@fea40000 {
> + compatible = "renesas,cmm-gen3";
> + reg = <0 0xfea40000 0 0x1000>;
> + clocks = <&cpg CPG_MOD 711>;
> + power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
> + resets = <&cpg 711>;
> + };
> +
> + cmm1: cmm@fea50000 {
> + compatible = "renesas,cmm-gen3";
> + reg = <0 0xfea50000 0 0x1000>;
> + clocks = <&cpg CPG_MOD 710>;
> + power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
> + resets = <&cpg 710>;
> + };
> +
> du: display@feb00000 {
> compatible = "renesas,du-r8a77995";
> reg = <0 0xfeb00000 0 0x80000>;
> @@ -1001,9 +1017,11 @@
> clocks = <&cpg CPG_MOD 724>,
> <&cpg CPG_MOD 723>;
> clock-names = "du.0", "du.1";
> - vsps = <&vspd0 0 &vspd1 0>;
> status = "disabled";
>
> + vsps = <&vspd0 0 &vspd1 0>;
> + cmms = <&cmm0 &cmm1>;
> +
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
--
Regards,
Laurent Pinchart
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 <[email protected]>
> ---
> 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 <[email protected]>
> + */
> +
> +#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 <[email protected]");
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 <[email protected]>
> + */
> +
> +#ifndef __RCAR_CMM_H__
> +#define __RCAR_CMM_H__
> +
> +#include <linux/platform_device.h>
You can forward-declare struct platform_device instead.
> +
> +#define CMM_GAMMA_LUT_SIZE 256
> +
> +struct drm_color_lut;
> +
> +/**
> + * struct rcar_cmm_config - CMM configuration
> + *
> + * @lut: 1D-LUT configuration
> + * @lut.enable: 1D-LUT enable flag
> + * @lut.table: 1D-LUT table entries.
> + * @lut.size 1D-LUT number of entries. Max is 256
The last line is missing a colon.
> + */
> +struct rcar_cmm_config {
> + struct {
> + bool enable;
> + struct drm_color_lut *table;
> + unsigned int size;
> + } lut;
> +};
> +
> +int rcar_cmm_enable(struct platform_device *);
As the OF API looks up a struct device from a device_node, should we use
struct device here ?
> +void rcar_cmm_disable(struct platform_device *);
I find headers more readable when function arguments are named. In this
case the types probably provide enough information, but good luck trying
to read a function such as
int foo(int, int, bool);
> +
> +int rcar_cmm_setup(struct platform_device *, struct rcar_cmm_config *);
Can the second argument be const ?
> +
> +#endif /* __RCAR_CMM_H__ */
--
Regards,
Laurent Pinchart
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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > + */
> > +
> > +#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 <[email protected]");
>
> 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 <[email protected]>
> > + */
> > +
> > +#ifndef __RCAR_CMM_H__
> > +#define __RCAR_CMM_H__
> > +
> > +#include <linux/platform_device.h>
>
> You can forward-declare struct platform_device instead.
>
> > +
> > +#define CMM_GAMMA_LUT_SIZE 256
> > +
> > +struct drm_color_lut;
> > +
> > +/**
> > + * struct rcar_cmm_config - CMM configuration
> > + *
> > + * @lut: 1D-LUT configuration
> > + * @lut.enable: 1D-LUT enable flag
> > + * @lut.table: 1D-LUT table entries.
> > + * @lut.size 1D-LUT number of entries. Max is 256
>
> The last line is missing a colon.
>
> > + */
> > +struct rcar_cmm_config {
> > + struct {
> > + bool enable;
> > + struct drm_color_lut *table;
> > + unsigned int size;
> > + } lut;
> > +};
> > +
> > +int rcar_cmm_enable(struct platform_device *);
>
> As the OF API looks up a struct device from a device_node, should we use
> struct device here ?
My bad, the API actually returns a struct platform_device. I would still
prefer using struct device, but it then becomes more of a personal
preference.
> > +void rcar_cmm_disable(struct platform_device *);
>
> I find headers more readable when function arguments are named. In this
> case the types probably provide enough information, but good luck trying
> to read a function such as
>
> int foo(int, int, bool);
>
> > +
> > +int rcar_cmm_setup(struct platform_device *, struct rcar_cmm_config *);
>
> Can the second argument be const ?
>
> > +
> > +#endif /* __RCAR_CMM_H__ */
--
Regards,
Laurent Pinchart
Hi Jacopo,
Now that I've read the whole series, here are a few comments.
On Fri, Jun 07, 2019 at 02:35:40PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Jun 06, 2019 at 04:22:14PM +0200, Jacopo Mondi wrote:
> > Add a driver for the R-Car Display Unit Color Correction Module.
> >
> > Each DU output channel is provided with a CMM unit to perform image
> > enhancement and color correction.
>
> I would say "On most Gen3 SoCs, each DU ..." as V3* SoCs have no CMM.
>
> >
> > Add support for CMM through a driver that supports configuration of
> > the 1-dimensional LUT table. More advanced CMM feature could be
> > implemented on top of this basic one.
>
> s/could be/will be/ ? :-)
>
> >
> > Signed-off-by: Jacopo Mondi <[email protected]>
> > ---
> > 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 <[email protected]>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <drm/drm_atomic.h>
> > +
> > +#include "rcar_cmm.h"
> > +
> > +#define CM2_LUT_CTRL 0x00
>
> I would write all register addresses with 3 (or 4) digits.
>
> > +#define CM2_LUT_CTRL_EN BIT(0)
> > +#define CM2_LUT_TBLA 0x600
>
> I would define this as
>
> #define CM2_LUT_TBLA(n) (0x600 + (n) * 4)
>
> > +
> > +struct rcar_cmm {
> > + struct clk *clk;
> > + void __iomem *base;
> > + bool enabled;
> > +
> > + /* LUT table scratch buffer. */
> > + struct {
> > + bool restore;
> > + unsigned int size;
> > + uint32_t table[CMM_GAMMA_LUT_SIZE];
> > + } lut;
> > +};
> > +
> > +static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg)
> > +{
> > + return ioread32(rcmm->base + reg);
> > +}
> > +
> > +static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data)
> > +{
> > + iowrite32(data, rcmm->base + reg);
> > +}
> > +
> > +int rcar_cmm_setup(struct platform_device *pdev, struct rcar_cmm_config *config)
>
> Please document the functions exposed to the DU driver. It's hard to
> understand the setup vs. enable/disable split by just reading this
> driver.
>
> > +{
> > + struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> > + unsigned int i;
> > +
> > + if (config->lut.size > CMM_GAMMA_LUT_SIZE)
> > + return -EINVAL;
> > +
> > + /*
> > + * As cmm_setup is called by atomic commit tail helper, it might be
> > + * called before the enabling the CRTC (which calls cmm_enable()).
> > + *
> > + * Store the LUT table entries in the scratch buffer to be later
> > + * programmed at enable time.
> > + */
I think we'll be able to simplify this once Kieran's DU group handling
patches land. Let's see which series gets merged first :-)
> > + if (!rcmm->enabled) {
> > + if (!config->lut.enable)
> > + return 0;
> > +
> > + for (i = 0; i < config->lut.size; ++i) {
> > + struct drm_color_lut *lut = &config->lut.table[i];
> > +
> > + rcmm->lut.table[i] = (lut->red & 0xff) << 16 |
> > + (lut->green & 0xff) << 8 |
> > + (lut->blue & 0xff);
> > + }
> > +
> > + rcmm->lut.restore = true;
> > + rcmm->lut.size = config->lut.size;
> > +
> > + return 0;
> > + }
> > +
> > + if (rcar_cmm_read(rcmm, CM2_LUT_CTRL) & CM2_LUT_CTRL_EN &&
> > + !config->lut.enable) {
> > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> > + return 0;
> > + }
> > +
> > + /* Enable LUT and program the new gamma table values. */
> > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
>
> Shouldn't you write the LUT contents before enabling it (same below) ?
>
> > + for (i = 0; i < config->lut.size; ++i) {
> > + struct drm_color_lut *lut = &config->lut.table[i];
> > + u32 val = (lut->red & 0xff) << 16 | (lut->green & 0xff) << 8 |
> > + (lut->blue & 0xff);
>
> Do you need to recompute the value, can't you use rcmm->lut.table ?
>
> > +
> > + rcar_cmm_write(rcmm, CM2_LUT_TBLA + i * 4, val);
> > + }
> > +
> > + return 0;
> > +}
>
> You need to export this and the next two functions.
>
> > +
> > +int rcar_cmm_enable(struct platform_device *pdev)
> > +{
> > + struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
What if this function gets called before the CMM is probed ? I think you
need an equivalent to vsp1_du_init() to handle this.
> > + unsigned int i;
> > + int ret;
> > +
> > + if (rcmm->enabled)
> > + return 0;
>
> Can this happen without a bug in the caller ? If it can, and assuming
> the caller balances the enable and disable calls, you will have
> unbalanced clk_prepare_enable() and clk_disable_unprepare() calls.
>
> > +
> > + ret = clk_prepare_enable(rcmm->clk);
> > + if (ret)
> > + return ret;
>
> Could you use pm_runtime_get_sync() instead ?
>
> > +
> > + /* Apply the LUT table values saved at cmm_setup time. */
> > + if (rcmm->lut.restore) {
> > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> > + for (i = 0; i < rcmm->lut.size; ++i)
> > + rcar_cmm_write(rcmm, CM2_LUT_TBLA + i * 4,
> > + rcmm->lut.table[i]);
> > +
> > + rcmm->lut.restore = false;
> > + rcmm->lut.size = 0;
> > + }
I would move this code to a separate function called by both
rcar_cmm_setup() and rcar_cmm_enable().
> > +
> > + rcmm->enabled = true;
> > +
> > + return 0;
> > +}
> > +
> > +void rcar_cmm_disable(struct platform_device *pdev)
> > +{
> > + struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> > +
> > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> > +
> > + clk_disable_unprepare(rcmm->clk);
> > +
> > + rcmm->lut.restore = false;
> > + rcmm->lut.size = 0;
> > + rcmm->enabled = false;
> > +}
> > +
> > +static int rcar_cmm_probe(struct platform_device *pdev)
> > +{
> > + struct rcar_cmm *rcmm;
> > + struct resource *res;
> > + resource_size_t size;
> > +
> > + rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
> > + if (!rcmm)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, rcmm);
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + size = resource_size(res);
> > + if (!devm_request_mem_region(&pdev->dev, res->start, size,
> > + dev_name(&pdev->dev))) {
> > + dev_err(&pdev->dev,
> > + "can't request region for resource %pR\n", res);
> > + return -EBUSY;
> > + }
> > +
> > + rcmm->base = devm_ioremap_nocache(&pdev->dev, res->start, size);
> > + if (IS_ERR(rcmm->base))
> > + return PTR_ERR(rcmm->base);
>
> Anything wrong with devm_ioremap_resource() ?
>
> > +
> > + rcmm->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(rcmm->clk)) {
> > + dev_err(&pdev->dev, "Failed to get CMM clock");
> > + return PTR_ERR(rcmm->clk);
> > + }
> > +
> > + rcmm->lut.restore = false;
> > + rcmm->lut.size = 0;
> > + rcmm->enabled = false;
>
> As you allocate memory with kzalloc() you could skip this.
>
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id rcar_cmm_of_table[] = {
> > + { .compatible = "renesas,cmm-gen3" },
> > + { .compatible = "renesas,cmm-gen2" },
> > + { },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
> > +
> > +static struct platform_driver rcar_cmm_platform_driver = {
> > + .probe = rcar_cmm_probe,
> > + .driver = {
> > + .name = "rcar-cmm",
> > + .of_match_table = rcar_cmm_of_table,
> > + },
>
> No need for suspend/resume support ? The DU driver should disable/enable
> the CMM in its suspend/resume paths, so this should be fine, but won't
> the LUT contents be lost and need to be restored ?
>
> > +};
> > +
> > +module_platform_driver(rcar_cmm_platform_driver);
> > +
> > +MODULE_AUTHOR("Jacopo Mondi <[email protected]");
>
> 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 <[email protected]>
> > + */
> > +
> > +#ifndef __RCAR_CMM_H__
> > +#define __RCAR_CMM_H__
> > +
> > +#include <linux/platform_device.h>
>
> You can forward-declare struct platform_device instead.
>
> > +
> > +#define CMM_GAMMA_LUT_SIZE 256
> > +
> > +struct drm_color_lut;
> > +
> > +/**
> > + * struct rcar_cmm_config - CMM configuration
> > + *
> > + * @lut: 1D-LUT configuration
> > + * @lut.enable: 1D-LUT enable flag
> > + * @lut.table: 1D-LUT table entries.
> > + * @lut.size 1D-LUT number of entries. Max is 256
>
> The last line is missing a colon.
>
> > + */
> > +struct rcar_cmm_config {
> > + struct {
> > + bool enable;
> > + struct drm_color_lut *table;
> > + unsigned int size;
> > + } lut;
> > +};
> > +
> > +int rcar_cmm_enable(struct platform_device *);
>
> As the OF API looks up a struct device from a device_node, should we use
> struct device here ?
>
> > +void rcar_cmm_disable(struct platform_device *);
>
> I find headers more readable when function arguments are named. In this
> case the types probably provide enough information, but good luck trying
> to read a function such as
>
> int foo(int, int, bool);
>
> > +
> > +int rcar_cmm_setup(struct platform_device *, struct rcar_cmm_config *);
>
> Can the second argument be const ?
>
> > +
> > +#endif /* __RCAR_CMM_H__ */
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Laurent Pinchart
On Thu, Jun 6, 2019 at 4:22 PM Jacopo Mondi <[email protected]> wrote:
> Add clock definitions for CMM units on Renesas R-Car Gen3 H3.
>
> Signed-off-by: Jacopo Mondi <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
i.e. will queue in clk-renesas-for-v5.3.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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,
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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > + */
> > +
> > +#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 <[email protected]");
>
> 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 <[email protected]>
> > + */
> > +
> > +#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