2019-07-06 14:08:11

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v2 00/19] drm: rcar-du: Add Color Management Module (CMM)

Hello,
second iteration of CMM support for Renesas R-Car devices, where I have
fixed comments from Laurent, Geert and Daniel.

A reference to the v1 cover letter, with some background on the CMM is
available here:
https://lkml.org/lkml/2019/6/6/583

Notable changes:
- Rebased on v5.2-rc7
- clock patches rebased, but already collected by Geert for v5.3
- Changed cmm compatible string as suggested by Geert in bindings and
DTS files
- CMM driver updated to include comments from Laurent, thanks!
- Integration in R-Car DU is very similar, I have squashed a few patches
- Add legagy gamma interface support with .gamma_set callback as suggested
by Daniel.

Thanks
j

Jacopo Mondi (19):
dt-bindings: display: renesas,cmm: Add R-Car CMM documentation
dt-bindings: display, renesas,du: Document cmms property
arm64: renesas: Update 'vsps' property
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: 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 | 5 +
arch/arm64/boot/dts/renesas/r8a774c0.dtsi | 2 +-
arch/arm64/boot/dts/renesas/r8a7795.dtsi | 36 ++-
arch/arm64/boot/dts/renesas/r8a7796.dtsi | 25 ++
arch/arm64/boot/dts/renesas/r8a77965.dtsi | 27 +-
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 | 291 ++++++++++++++++++
drivers/gpu/drm/rcar-du/rcar_cmm.h | 38 +++
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 17 +
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 +
25 files changed, 638 insertions(+), 9 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


2019-07-06 14:08:11

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v2 04/19] clk: renesas: r8a7796: Add CMM clocks

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

Reviewed-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/clk/renesas/r8a7796-cpg-mssr.c | 3 +++
1 file changed, 3 insertions(+)

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

2019-07-06 14:08:11

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v2 02/19] dt-bindings: display, renesas,du: Document cmms property

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

Signed-off-by: Jacopo Mondi <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
Documentation/devicetree/bindings/display/renesas,du.txt | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt b/Documentation/devicetree/bindings/display/renesas,du.txt
index aedb22b4d161..0f42af5b91cf 100644
--- a/Documentation/devicetree/bindings/display/renesas,du.txt
+++ b/Documentation/devicetree/bindings/display/renesas,du.txt
@@ -44,6 +44,10 @@ Required Properties:
instance that serves the DU channel, and the channel index identifies the
LIF instance in that VSP.

+ - cmms: A list of phandles to the CMM instances present in the SoC, one
+ for each available DU channel. The property shall not be specified for
+ SoCs that do not provide any CMM (such as V3M and V3H).
+
Required nodes:

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

ports {
#address-cells = <1>;
--
2.21.0

2019-07-06 14:08:11

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v2 08/19] clk: renesas: r8a77995: Add CMM clocks

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

Reviewed-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
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 68707277b17b..962bb337f2e7 100644
--- a/drivers/clk/renesas/r8a77995-cpg-mssr.c
+++ b/drivers/clk/renesas/r8a77995-cpg-mssr.c
@@ -146,6 +146,8 @@ static const struct mssr_mod_clk r8a77995_mod_clks[] __initconst = {
DEF_MOD("vspbs", 627, R8A77995_CLK_S0D1),
DEF_MOD("ehci0", 703, R8A77995_CLK_S3D2),
DEF_MOD("hsusb", 704, R8A77995_CLK_S3D2),
+ DEF_MOD("cmm1", 710, R8A77995_CLK_S1D1),
+ DEF_MOD("cmm0", 711, R8A77995_CLK_S1D1),
DEF_MOD("du1", 723, R8A77995_CLK_S1D1),
DEF_MOD("du0", 724, R8A77995_CLK_S1D1),
DEF_MOD("lvds", 727, R8A77995_CLK_S2D1),
--
2.21.0

2019-07-06 14:08:28

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v2 15/19] drm: rcar-du: Claim CMM support for Gen3 SoCs

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 <[email protected]>
---
drivers/gpu/drm/rcar-du/rcar_du_drv.c | 12 ++++++++----
drivers/gpu/drm/rcar-du/rcar_du_drv.h | 1 +
2 files changed, 9 insertions(+), 4 deletions(-)

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

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

--
2.21.0

2019-07-06 14:08:34

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v2 16/19] drm: rcar-du: kms: Collect CMM instances

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

Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 6 +++
drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 +
drivers/gpu/drm/rcar-du/rcar_du_drv.h | 3 ++
drivers/gpu/drm/rcar-du/rcar_du_group.h | 2 +
drivers/gpu/drm/rcar-du/rcar_du_kms.c | 50 +++++++++++++++++++++++++
5 files changed, 63 insertions(+)

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

+ /* CMM might be disabled for this CRTC. */
+ if (rcdu->cmms[swindex]) {
+ rcrtc->cmm = rcdu->cmms[swindex];
+ rgrp->cmms_mask |= BIT(hwindex % 2);
+ }
+
drm_crtc_helper_add(crtc, &crtc_helper_funcs);

/* Start with vertical blanking interrupt reporting disabled. */
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
index 3b7fc668996f..5f2940c42225 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -39,6 +39,7 @@ struct rcar_du_vsp;
* @vblank_wait: wait queue used to signal vertical blanking
* @vblank_count: number of vertical blanking interrupts to wait for
* @group: CRTC group this CRTC belongs to
+ * @cmm: CMM associated with this CRTC
* @vsp: VSP feeding video to this CRTC
* @vsp_pipe: index of the VSP pipeline feeding video to this CRTC
* @writeback: the writeback connector
@@ -64,6 +65,7 @@ struct rcar_du_crtc {
unsigned int vblank_count;

struct rcar_du_group *group;
+ struct platform_device *cmm;
struct rcar_du_vsp *vsp;
unsigned int vsp_pipe;

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

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

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

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

struct rcar_du_group groups[RCAR_DU_MAX_GROUPS];
+ struct platform_device *cmms[RCAR_DU_MAX_CMMS];
struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS];

struct {
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h b/drivers/gpu/drm/rcar-du/rcar_du_group.h
index 87950c1f6a52..b0c1466593a3 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h
@@ -22,6 +22,7 @@ struct rcar_du_device;
* @mmio_offset: registers offset in the device memory map
* @index: group index
* @channels_mask: bitmask of populated DU channels in this group
+ * @cmms_mask: bitmask of enabled CMMs in this group
* @num_crtcs: number of CRTCs in this group (1 or 2)
* @use_count: number of users of the group (rcar_du_group_(get|put))
* @used_crtcs: number of CRTCs currently in use
@@ -37,6 +38,7 @@ struct rcar_du_group {
unsigned int index;

unsigned int channels_mask;
+ unsigned int cmms_mask;
unsigned int num_crtcs;
unsigned int use_count;
unsigned int used_crtcs;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index f8f7fff34dff..b79cda2f5531 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -18,6 +18,7 @@
#include <drm/drm_vblank.h>

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

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

+static int rcar_du_cmm_init(struct rcar_du_device *rcdu)
+{
+ const struct device_node *np = rcdu->dev->of_node;
+ unsigned int i;
+ int cells;
+
+ cells = of_property_count_u32_elems(np, "cmms");
+ if (cells == -EINVAL)
+ return 0;
+
+ if (cells > RCAR_DU_MAX_CMMS || cells > rcdu->num_crtcs) {
+ dev_err(rcdu->dev, "Invalid 'cmms' property format\n");
+ return -EINVAL;
+ }
+
+ for (i = 0; i < cells; ++i) {
+ struct platform_device *pdev;
+ struct device_node *cmm;
+
+ cmm = of_parse_phandle(np, "cmms", i);
+ if (IS_ERR(cmm)) {
+ dev_err(rcdu->dev, "Failed to parse 'cmms' property\n");
+ return PTR_ERR(cmm);
+ }
+
+ pdev = of_find_device_by_node(cmm);
+ if (IS_ERR(pdev)) {
+ dev_err(rcdu->dev, "No device found for cmms[%u]\n", i);
+ of_node_put(cmm);
+ return PTR_ERR(pdev);
+ }
+
+ if (!of_device_is_available(cmm)) {
+ /* It's fine to have a phandle to a non-enabled CMM. */
+ of_node_put(cmm);
+ continue;
+ }
+
+ of_node_put(cmm);
+ rcdu->cmms[i] = pdev;
+ }
+
+ return 0;
+}
+
int rcar_du_modeset_init(struct rcar_du_device *rcdu)
{
static const unsigned int mmio_offsets[] = {
@@ -624,6 +670,10 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
return ret;
}

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

2019-07-06 14:08:50

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v2 18/19] drm: rcar-du: crtc: Register GAMMA_LUT properties

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 <[email protected]>
---
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 4 ++++
1 file changed, 4 insertions(+)

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

/* -----------------------------------------------------------------------------
@@ -1205,6 +1206,9 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
if (rcdu->cmms[swindex]) {
rcrtc->cmm = rcdu->cmms[swindex];
rgrp->cmms_mask |= BIT(hwindex % 2);
+
+ drm_mode_crtc_set_gamma_size(crtc, CMM_GAMMA_LUT_SIZE);
+ drm_crtc_enable_color_mgmt(crtc, 0, false, CMM_GAMMA_LUT_SIZE);
}

drm_crtc_helper_add(crtc, &crtc_helper_funcs);
--
2.21.0

2019-07-06 14:09:12

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v2 17/19] drm: rcar-du: crtc: Enable and disable CMMs

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

Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 7 +++++++
drivers/gpu/drm/rcar-du/rcar_du_group.c | 8 ++++++++
drivers/gpu/drm/rcar-du/rcar_du_regs.h | 5 +++++
3 files changed, 20 insertions(+)

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

+#include "rcar_cmm.h"
#include "rcar_du_crtc.h"
#include "rcar_du_drv.h"
#include "rcar_du_encoder.h"
@@ -619,6 +620,9 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
rcar_du_vsp_disable(rcrtc);

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

rcar_du_crtc_start(rcrtc);
+
+ if (rcrtc->cmm)
+ rcar_cmm_enable(rcrtc->cmm);
}

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

rcar_du_group_setup_pins(rgrp);

+ if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM)) {
+ u32 defr7 = DEFR7_CODE |
+ (rgrp->cmms_mask & BIT(1) ? DEFR7_CMME1 : 0) |
+ (rgrp->cmms_mask & BIT(0) ? DEFR7_CMME0 : 0);
+
+ rcar_du_group_write(rgrp, DEFR7, defr7);
+ }
+
if (rcdu->info->gen >= 2) {
rcar_du_group_setup_defr8(rgrp);
rcar_du_group_setup_didsr(rgrp);
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
index bc87f080b170..fb9964949368 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
@@ -197,6 +197,11 @@
#define DEFR6_MLOS1 (1 << 2)
#define DEFR6_DEFAULT (DEFR6_CODE | DEFR6_TCNE1)

+#define DEFR7 0x000ec
+#define DEFR7_CODE (0x7779 << 16)
+#define DEFR7_CMME1 BIT(6)
+#define DEFR7_CMME0 BIT(4)
+
/* -----------------------------------------------------------------------------
* R8A7790-only Control Registers
*/
--
2.21.0

2019-07-06 14:09:12

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v2 13/19] arm64: dts: renesas: r8a77995: Add CMM units

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.

Reviewed-by: Laurent Pinchart <[email protected]>
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 49a11b4f55bd..1669740bfa28 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,rcar-gen3-cmm";
+ 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,rcar-gen3-cmm";
+ 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

2019-07-06 14:09:12

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v2 14/19] drm: rcar-du: Add support for CMM

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

In most of Gen3 SoCs, each DU output channel is provided with a CMM unit
to perform image enhancement and color correction.

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

Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/gpu/drm/rcar-du/Kconfig | 7 +
drivers/gpu/drm/rcar-du/Makefile | 1 +
drivers/gpu/drm/rcar-du/rcar_cmm.c | 292 +++++++++++++++++++++++++++++
drivers/gpu/drm/rcar-du/rcar_cmm.h | 38 ++++
4 files changed, 338 insertions(+)
create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.c
create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.h

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

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

+obj-$(CONFIG_DRM_RCAR_CMM) += rcar_cmm.o
obj-$(CONFIG_DRM_RCAR_DU) += rcar-du-drm.o
obj-$(CONFIG_DRM_RCAR_DW_HDMI) += rcar_dw_hdmi.o
obj-$(CONFIG_DRM_RCAR_LVDS) += rcar_lvds.o
diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c
new file mode 100644
index 000000000000..76ed3fce2b33
--- /dev/null
+++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c
@@ -0,0 +1,292 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * rcar_cmm.c -- R-Car Display Unit Color Management Module
+ *
+ * Copyright (C) 2019 Jacopo Mondi <[email protected]>
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+
+#include <drm/drm_atomic.h>
+
+#include "rcar_cmm.h"
+
+#define CM2_LUT_CTRL 0x0000
+#define CM2_LUT_CTRL_EN BIT(0)
+#define CM2_LUT_TBLA_BASE 0x0600
+#define CM2_LUT_TBLA(__i) (CM2_LUT_TBLA_BASE + (__i) * 4)
+
+struct rcar_cmm {
+ struct clk *clk;
+ void __iomem *base;
+ bool enabled;
+
+ /*
+ * restore: LUT restore flag
+ * running: LUT operating flag
+ * size: Number of programmed entries in the LUT table
+ * table: Scratch buffer where to store the LUT table entries to be
+ * later restored.
+ */
+ struct {
+ bool restore;
+ bool running;
+ unsigned int size;
+ struct drm_color_lut table[CMM_GAMMA_LUT_SIZE];
+ } lut;
+};
+
+static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg)
+{
+ return ioread32(rcmm->base + reg);
+}
+
+static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data)
+{
+ iowrite32(data, rcmm->base + reg);
+}
+
+static void rcar_cmm_lut_write(struct rcar_cmm *rcmm, unsigned int size,
+ struct drm_color_lut *lut)
+{
+ unsigned int i;
+
+ for (i = 0; i < size; ++i) {
+ struct drm_color_lut *entry = &lut[i];
+
+ u32 val = (entry->red & 0xff) << 16 |
+ (entry->green & 0xff) << 8 |
+ (entry->blue & 0xff);
+ rcar_cmm_write(rcmm, CM2_LUT_TBLA(i), val);
+ }
+}
+
+/**
+ * rcar_cmm_setup() - configure the CMM unit
+ *
+ * @pdev: The platform device associated with the CMM instance
+ * @config: The CRTC provided configuration.
+ *
+ * Configure the CMM unit with the CRTC provided configuration.
+ * Currently enabling, disabling and programming of the 1-D LUT unit is
+ * supported.
+ */
+int rcar_cmm_setup(struct platform_device *pdev,
+ const struct rcar_cmm_config *config)
+{
+ struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
+ unsigned int i;
+
+ if (config->lut.size > CMM_GAMMA_LUT_SIZE)
+ return -EINVAL;
+
+ /*
+ * As cmm_setup is called by atomic commit tail helper, it might be
+ * called before enabling the CRTC (which calls cmm_enable()).
+ */
+ if (!rcmm->enabled) {
+ if (!config->lut.enable)
+ return 0;
+
+ /*
+ * Store the LUT table entries in the scratch buffer to be later
+ * programmed at enable time.
+ */
+ for (i = 0; i < config->lut.size; ++i)
+ rcmm->lut.table[i] = config->lut.table[i];
+
+ rcmm->lut.size = config->lut.size;
+ rcmm->lut.restore = true;
+
+ return 0;
+ }
+
+ /* Stop LUT operations, if requested. */
+ if (rcmm->lut.running && !config->lut.enable) {
+ rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
+ rcmm->lut.running = 0;
+ rcmm->lut.size = 0;
+
+ return 0;
+ }
+
+ /* Enable LUT and program the new gamma table values. */
+ if (!rcmm->lut.running) {
+ rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
+ rcmm->lut.running = true;
+ }
+
+ rcar_cmm_lut_write(rcmm, config->lut.size, config->lut.table);
+ rcmm->lut.size = config->lut.size;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(rcar_cmm_setup);
+
+/**
+ * rcar_cmm_enable - enable the CMM unit
+ *
+ * @pdev: The platform device associated with the CMM instance
+ *
+ * Enable the CMM unit by enabling the parent clock and enabling the CMM
+ * components, such as 1-D LUT, if requested.
+ */
+int rcar_cmm_enable(struct platform_device *pdev)
+{
+ struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
+ int ret;
+
+ if (!rcmm)
+ return -EPROBE_DEFER;
+
+ ret = clk_prepare_enable(rcmm->clk);
+ if (ret)
+ return ret;
+
+ /* Apply the LUT table values saved at cmm_setup time. */
+ if (rcmm->lut.restore) {
+ rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
+ rcar_cmm_lut_write(rcmm, rcmm->lut.size, rcmm->lut.table);
+
+ rcmm->lut.restore = false;
+ rcmm->lut.running = true;
+ }
+
+ rcmm->enabled = true;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(rcar_cmm_enable);
+
+/**
+ * rcar_cmm_disable() - disable the CMM unit
+ *
+ * Disable the CMM unit by stopping the parent clock.
+ *
+ * @pdev: The platform device associated with the CMM instance
+ */
+void rcar_cmm_disable(struct platform_device *pdev)
+{
+ struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
+
+ rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
+
+ clk_disable_unprepare(rcmm->clk);
+
+ rcmm->lut.restore = false;
+ rcmm->lut.running = false;
+ rcmm->lut.size = 0;
+
+ rcmm->enabled = false;
+}
+EXPORT_SYMBOL_GPL(rcar_cmm_disable);
+
+#ifdef CONFIG_PM_SLEEP
+static int rcar_cmm_pm_suspend(struct device *dev)
+{
+ struct rcar_cmm *rcmm = dev_get_drvdata(dev);
+ unsigned int i;
+
+ if (!(rcmm->lut.running || rcmm->lut.restore))
+ return 0;
+
+ /* Save the LUT table entries in the scratch buffer table. */
+ for (i = 0; i < rcmm->lut.size; ++i) {
+ int entry = rcar_cmm_read(rcmm, CM2_LUT_TBLA(i));
+ struct drm_color_lut *lut = &rcmm->lut.table[i];
+
+ lut->blue = entry & 0xff;
+ lut->green = (entry >> 8) & 0xff;
+ lut->red = (entry >> 16) & 0xff;
+ }
+
+ rcmm->lut.restore = true;
+ rcmm->lut.running = false;
+
+ rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
+
+ return 0;
+}
+
+static int rcar_cmm_pm_resume(struct device *dev)
+{
+ struct rcar_cmm *rcmm = dev_get_drvdata(dev);
+
+ if (!rcmm->lut.restore)
+ return 0;
+
+ /* Program the LUT entries saved at suspend time. */
+ rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
+ rcar_cmm_lut_write(rcmm, rcmm->lut.size, rcmm->lut.table);
+ rcmm->lut.running = true;
+ rcmm->lut.restore = false;
+
+ return 0;
+}
+#endif
+
+static const struct dev_pm_ops rcar_cmm_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(rcar_cmm_pm_suspend, rcar_cmm_pm_resume)
+};
+
+static int rcar_cmm_probe(struct platform_device *pdev)
+{
+ struct rcar_cmm *rcmm;
+ struct resource *res;
+ resource_size_t size;
+
+ rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
+ if (!rcmm)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, rcmm);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ size = resource_size(res);
+ if (!devm_request_mem_region(&pdev->dev, res->start, size,
+ dev_name(&pdev->dev))) {
+ dev_err(&pdev->dev,
+ "can't request region for resource %pR\n", res);
+ return -EBUSY;
+ }
+
+ rcmm->base = devm_ioremap_nocache(&pdev->dev, res->start, size);
+ if (IS_ERR(rcmm->base))
+ return PTR_ERR(rcmm->base);
+
+ rcmm->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(rcmm->clk)) {
+ dev_err(&pdev->dev, "Failed to get CMM clock");
+ return PTR_ERR(rcmm->clk);
+ }
+
+ return 0;
+}
+
+static const struct of_device_id rcar_cmm_of_table[] = {
+ { .compatible = "renesas,rcar-gen3-cmm", },
+ { .compatible = "renesas,rcar-gen2-cmm", },
+ { },
+};
+
+MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
+
+static struct platform_driver rcar_cmm_platform_driver = {
+ .probe = rcar_cmm_probe,
+ .driver = {
+ .name = "rcar-cmm",
+ .pm = &rcar_cmm_pm_ops,
+ .of_match_table = rcar_cmm_of_table,
+ },
+};
+
+module_platform_driver(rcar_cmm_platform_driver);
+
+MODULE_AUTHOR("Jacopo Mondi <[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..8744e72f32cd
--- /dev/null
+++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * rcar_cmm.h -- R-Car Display Unit Color Management Module
+ *
+ * Copyright (C) 2019 Jacopo Mondi <[email protected]>
+ */
+
+#ifndef __RCAR_CMM_H__
+#define __RCAR_CMM_H__
+
+#define CMM_GAMMA_LUT_SIZE 256
+
+struct platform_device;
+struct drm_color_lut;
+
+/**
+ * struct rcar_cmm_config - CMM configuration
+ *
+ * @lut: 1D-LUT configuration
+ * @lut.enable: 1D-LUT enable flag
+ * @lut.table: 1D-LUT table entries.
+ * @lut.size 1D-LUT number of entries. Max is 256.
+ */
+struct rcar_cmm_config {
+ struct {
+ bool enable;
+ struct drm_color_lut *table;
+ unsigned int size;
+ } lut;
+};
+
+int rcar_cmm_enable(struct platform_device *pdev);
+void rcar_cmm_disable(struct platform_device *pdev);
+
+int rcar_cmm_setup(struct platform_device *pdev,
+ const struct rcar_cmm_config *config);
+
+#endif /* __RCAR_CMM_H__ */
--
2.21.0

2019-07-06 14:09:13

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v2 12/19] arm64: dts: renesas: r8a77990: Add CMM units

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.

Reviewed-by: Laurent Pinchart <[email protected]>
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 79db5441b7e7..6743d1bd2470 100644
--- a/arch/arm64/boot/dts/renesas/r8a77990.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a77990.dtsi
@@ -1725,6 +1725,22 @@
iommus = <&ipmmu_vi0 9>;
};

+ cmm0: cmm@fea40000 {
+ compatible = "renesas,rcar-gen3-cmm";
+ 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,rcar-gen3-cmm";
+ reg = <0 0xfea50000 0 0x1000>;
+ clocks = <&cpg CPG_MOD 710>;
+ power-domains = <&sysc R8A77990_PD_ALWAYS_ON>;
+ resets = <&cpg 710>;
+ };
+
csi40: csi2@feaa0000 {
compatible = "renesas,r8a77990-csi2";
reg = <0 0xfeaa0000 0 0x10000>;
@@ -1764,9 +1780,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

2019-07-06 14:09:44

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v2 05/19] clk: renesas: r8a7795: Add CMM clocks

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

Reviewed-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
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 9e9a6f2c31e8..d55b1d9c8f72 100644
--- a/drivers/clk/renesas/r8a7795-cpg-mssr.c
+++ b/drivers/clk/renesas/r8a7795-cpg-mssr.c
@@ -201,6 +201,10 @@ static struct mssr_mod_clk r8a7795_mod_clks[] __initdata = {
DEF_MOD("ehci0", 703, R8A7795_CLK_S3D2),
DEF_MOD("hsusb", 704, R8A7795_CLK_S3D2),
DEF_MOD("hsusb3", 705, R8A7795_CLK_S3D2),
+ 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

2019-07-06 14:10:10

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v2 01/19] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation

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

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

Signed-off-by: Jacopo Mondi <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
.../bindings/display/renesas,cmm.txt | 25 +++++++++++++++++++
1 file changed, 25 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/renesas,cmm.txt

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

2019-07-06 14:11:00

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v2 19/19] drm: rcar-du: kms: Update CMM in atomic commit tail

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 <[email protected]>
---
drivers/gpu/drm/rcar-du/rcar_du_kms.c | 35 +++++++++++++++++++++++++++
1 file changed, 35 insertions(+)

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

+#include "rcar_cmm.h"
#include "rcar_du_crtc.h"
#include "rcar_du_drv.h"
#include "rcar_du_encoder.h"
@@ -287,6 +288,37 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv,
* Atomic Check and Update
*/

+static void rcar_du_atomic_commit_update_cmm(struct drm_crtc *crtc,
+ struct drm_crtc_state *old_state)
+{
+ struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
+ struct rcar_cmm_config cmm_config = {};
+
+ if (!rcrtc->cmm || !crtc->state->color_mgmt_changed)
+ return;
+
+ if (!crtc->state->gamma_lut) {
+ cmm_config.lut.enable = false;
+ rcar_cmm_setup(rcrtc->cmm, &cmm_config);
+
+ return;
+ }
+
+ cmm_config.lut.enable = true;
+ cmm_config.lut.table = (struct drm_color_lut *)
+ crtc->state->gamma_lut->data;
+
+ /* Set LUT table size to 0 if entries should not be updated. */
+ if (!old_state->gamma_lut ||
+ old_state->gamma_lut->base.id != crtc->state->gamma_lut->base.id)
+ cmm_config.lut.size = crtc->state->gamma_lut->length
+ / sizeof(cmm_config.lut.table[0]);
+ else
+ cmm_config.lut.size = 0;
+
+ rcar_cmm_setup(rcrtc->cmm, &cmm_config);
+}
+
static int rcar_du_atomic_check(struct drm_device *dev,
struct drm_atomic_state *state)
{
@@ -329,6 +361,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
rcdu->dpad1_source = rcrtc->index;
}

+ for_each_old_crtc_in_state(old_state, crtc, crtc_state, i)
+ rcar_du_atomic_commit_update_cmm(crtc, crtc_state);
+
/* Apply the atomic update. */
drm_atomic_helper_commit_modeset_disables(dev, old_state);
drm_atomic_helper_commit_planes(dev, old_state,
--
2.21.0

2019-07-08 11:03:31

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v2 01/19] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation

Hi Jacopo,

On Sat, Jul 6, 2019 at 4:07 PM Jacopo Mondi <[email protected]> wrote:
> Add device tree bindings documentation for the Renesas R-Car Display
> Unit Color Management Module.
>
> CMM is the image enhancement module available on each R-Car DU video
> channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).
>
> Signed-off-by: Jacopo Mondi <[email protected]>
> Reviewed-by: Laurent Pinchart <[email protected]>

Thanks for your patch!

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

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

Gr{oetje,eeting}s,

Geert

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

2019-07-16 13:23:23

by Ulrich Hecht

[permalink] [raw]
Subject: Re: [PATCH v2 16/19] drm: rcar-du: kms: Collect CMM instances


> On July 6, 2019 at 4:07 PM Jacopo Mondi <[email protected]> wrote:
>
>
> Implement device tree parsing to collect the available CMM instances
> described by the 'cmms' property. Associate CMMs with CRTCs and store a
> mask of active CMMs in the DU group for later enablement.
>
> Signed-off-by: Jacopo Mondi <[email protected]>
> ---
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 6 +++
> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 +
> drivers/gpu/drm/rcar-du/rcar_du_drv.h | 3 ++
> drivers/gpu/drm/rcar-du/rcar_du_group.h | 2 +
> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 50 +++++++++++++++++++++++++
> 5 files changed, 63 insertions(+)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 2da46e3dc4ae..23f1d6cc1719 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -1194,6 +1194,12 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
> if (ret < 0)
> return ret;
>
> + /* CMM might be disabled for this CRTC. */
> + if (rcdu->cmms[swindex]) {
> + rcrtc->cmm = rcdu->cmms[swindex];
> + rgrp->cmms_mask |= BIT(hwindex % 2);
> + }
> +
> drm_crtc_helper_add(crtc, &crtc_helper_funcs);
>
> /* Start with vertical blanking interrupt reporting disabled. */
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> index 3b7fc668996f..5f2940c42225 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -39,6 +39,7 @@ struct rcar_du_vsp;
> * @vblank_wait: wait queue used to signal vertical blanking
> * @vblank_count: number of vertical blanking interrupts to wait for
> * @group: CRTC group this CRTC belongs to
> + * @cmm: CMM associated with this CRTC
> * @vsp: VSP feeding video to this CRTC
> * @vsp_pipe: index of the VSP pipeline feeding video to this CRTC
> * @writeback: the writeback connector
> @@ -64,6 +65,7 @@ struct rcar_du_crtc {
> unsigned int vblank_count;
>
> struct rcar_du_group *group;
> + struct platform_device *cmm;
> struct rcar_du_vsp *vsp;
> unsigned int vsp_pipe;
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> index a00dccc447aa..300ec60ba31b 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> @@ -13,6 +13,7 @@
> #include <linux/kernel.h>
> #include <linux/wait.h>
>
> +#include "rcar_cmm.h"
> #include "rcar_du_crtc.h"
> #include "rcar_du_group.h"
> #include "rcar_du_vsp.h"
> @@ -70,6 +71,7 @@ struct rcar_du_device_info {
>
> #define RCAR_DU_MAX_CRTCS 4
> #define RCAR_DU_MAX_GROUPS DIV_ROUND_UP(RCAR_DU_MAX_CRTCS, 2)
> +#define RCAR_DU_MAX_CMMS 4
> #define RCAR_DU_MAX_VSPS 4
>
> struct rcar_du_device {
> @@ -86,6 +88,7 @@ struct rcar_du_device {
> struct rcar_du_encoder *encoders[RCAR_DU_OUTPUT_MAX];
>
> struct rcar_du_group groups[RCAR_DU_MAX_GROUPS];
> + struct platform_device *cmms[RCAR_DU_MAX_CMMS];
> struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS];
>
> struct {
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> index 87950c1f6a52..b0c1466593a3 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> @@ -22,6 +22,7 @@ struct rcar_du_device;
> * @mmio_offset: registers offset in the device memory map
> * @index: group index
> * @channels_mask: bitmask of populated DU channels in this group
> + * @cmms_mask: bitmask of enabled CMMs in this group
> * @num_crtcs: number of CRTCs in this group (1 or 2)
> * @use_count: number of users of the group (rcar_du_group_(get|put))
> * @used_crtcs: number of CRTCs currently in use
> @@ -37,6 +38,7 @@ struct rcar_du_group {
> unsigned int index;
>
> unsigned int channels_mask;
> + unsigned int cmms_mask;
> unsigned int num_crtcs;
> unsigned int use_count;
> unsigned int used_crtcs;
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index f8f7fff34dff..b79cda2f5531 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -18,6 +18,7 @@
> #include <drm/drm_vblank.h>
>
> #include <linux/of_graph.h>
> +#include <linux/of_platform.h>
> #include <linux/wait.h>
>
> #include "rcar_du_crtc.h"
> @@ -534,6 +535,51 @@ static int rcar_du_vsps_init(struct rcar_du_device *rcdu)
> return ret;
> }
>
> +static int rcar_du_cmm_init(struct rcar_du_device *rcdu)
> +{
> + const struct device_node *np = rcdu->dev->of_node;
> + unsigned int i;
> + int cells;
> +
> + cells = of_property_count_u32_elems(np, "cmms");
> + if (cells == -EINVAL)
> + return 0;
> +
> + if (cells > RCAR_DU_MAX_CMMS || cells > rcdu->num_crtcs) {
> + dev_err(rcdu->dev, "Invalid 'cmms' property format\n");
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < cells; ++i) {
> + struct platform_device *pdev;
> + struct device_node *cmm;
> +
> + cmm = of_parse_phandle(np, "cmms", i);
> + if (IS_ERR(cmm)) {
> + dev_err(rcdu->dev, "Failed to parse 'cmms' property\n");
> + return PTR_ERR(cmm);
> + }
> +
> + pdev = of_find_device_by_node(cmm);
> + if (IS_ERR(pdev)) {
> + dev_err(rcdu->dev, "No device found for cmms[%u]\n", i);
> + of_node_put(cmm);
> + return PTR_ERR(pdev);
> + }
> +
> + if (!of_device_is_available(cmm)) {
> + /* It's fine to have a phandle to a non-enabled CMM. */
> + of_node_put(cmm);
> + continue;
> + }
> +
> + of_node_put(cmm);
> + rcdu->cmms[i] = pdev;
> + }
> +
> + return 0;
> +}
> +
> int rcar_du_modeset_init(struct rcar_du_device *rcdu)
> {
> static const unsigned int mmio_offsets[] = {
> @@ -624,6 +670,10 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
> return ret;
> }
>
> + /* Initialize the Color Management Modules. */
> + if (rcar_du_cmm_init(rcdu))
> + return ret;
> +
> /* Create the CRTCs. */
> for (swindex = 0, hwindex = 0; swindex < rcdu->num_crtcs; ++hwindex) {
> struct rcar_du_group *rgrp;
> --
> 2.21.0
>

Reviewed-by: Ulrich Hecht <[email protected]>

CU
Uli

2019-07-16 13:26:42

by Ulrich Hecht

[permalink] [raw]
Subject: Re: [PATCH v2 15/19] drm: rcar-du: Claim CMM support for Gen3 SoCs


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

Reviewed-by: Ulrich Hecht <[email protected]>

CU
Uli

2019-07-16 13:26:53

by Ulrich Hecht

[permalink] [raw]
Subject: Re: [PATCH v2 14/19] drm: rcar-du: Add support for CMM

Thanks for your patch!

> On July 6, 2019 at 4:07 PM Jacopo Mondi <[email protected]> wrote:
>
>
> Add a driver for the R-Car Display Unit Color Correction Module.
>
> In most of Gen3 SoCs, each DU output channel is provided with a CMM unit
> to perform image enhancement and color correction.
>
> Add support for CMM through a driver that supports configuration of
> the 1-dimensional LUT table. More advanced CMM feature will be
> implemented on top of this basic one.
>
> Signed-off-by: Jacopo Mondi <[email protected]>
> ---
> drivers/gpu/drm/rcar-du/Kconfig | 7 +
> drivers/gpu/drm/rcar-du/Makefile | 1 +
> drivers/gpu/drm/rcar-du/rcar_cmm.c | 292 +++++++++++++++++++++++++++++
> drivers/gpu/drm/rcar-du/rcar_cmm.h | 38 ++++
> 4 files changed, 338 insertions(+)
> create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.c
> create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.h
>
> diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
> index 1529849e217e..539d232790d1 100644
> --- a/drivers/gpu/drm/rcar-du/Kconfig
> +++ b/drivers/gpu/drm/rcar-du/Kconfig
> @@ -13,6 +13,13 @@ config DRM_RCAR_DU
> Choose this option if you have an R-Car chipset.
> If M is selected the module will be called rcar-du-drm.
>
> +config DRM_RCAR_CMM
> + bool "R-Car DU Color Management Module (CMM) Support"
> + depends on DRM && OF
> + depends on DRM_RCAR_DU
> + help
> + Enable support for R-Car Color Management Module (CMM).
> +
> config DRM_RCAR_DW_HDMI
> tristate "R-Car DU Gen3 HDMI Encoder Support"
> depends on DRM && OF
> diff --git a/drivers/gpu/drm/rcar-du/Makefile b/drivers/gpu/drm/rcar-du/Makefile
> index 6c2ed9c46467..4d1187ccc3e5 100644
> --- a/drivers/gpu/drm/rcar-du/Makefile
> +++ b/drivers/gpu/drm/rcar-du/Makefile
> @@ -15,6 +15,7 @@ rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS) += rcar_du_of.o \
> rcar-du-drm-$(CONFIG_DRM_RCAR_VSP) += rcar_du_vsp.o
> rcar-du-drm-$(CONFIG_DRM_RCAR_WRITEBACK) += rcar_du_writeback.o
>
> +obj-$(CONFIG_DRM_RCAR_CMM) += rcar_cmm.o
> obj-$(CONFIG_DRM_RCAR_DU) += rcar-du-drm.o
> obj-$(CONFIG_DRM_RCAR_DW_HDMI) += rcar_dw_hdmi.o
> obj-$(CONFIG_DRM_RCAR_LVDS) += rcar_lvds.o
> diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> new file mode 100644
> index 000000000000..76ed3fce2b33
> --- /dev/null
> +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> @@ -0,0 +1,292 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * rcar_cmm.c -- R-Car Display Unit Color Management Module
> + *
> + * Copyright (C) 2019 Jacopo Mondi <[email protected]>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +
> +#include <drm/drm_atomic.h>
> +
> +#include "rcar_cmm.h"
> +
> +#define CM2_LUT_CTRL 0x0000
> +#define CM2_LUT_CTRL_EN BIT(0)
> +#define CM2_LUT_TBLA_BASE 0x0600
> +#define CM2_LUT_TBLA(__i) (CM2_LUT_TBLA_BASE + (__i) * 4)
> +
> +struct rcar_cmm {
> + struct clk *clk;
> + void __iomem *base;
> + bool enabled;
> +
> + /*
> + * restore: LUT restore flag
> + * running: LUT operating flag
> + * size: Number of programmed entries in the LUT table
> + * table: Scratch buffer where to store the LUT table entries to be
> + * later restored.
> + */
> + struct {
> + bool restore;
> + bool running;
> + unsigned int size;
> + struct drm_color_lut table[CMM_GAMMA_LUT_SIZE];
> + } lut;
> +};
> +
> +static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg)
> +{
> + return ioread32(rcmm->base + reg);
> +}
> +
> +static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data)
> +{
> + iowrite32(data, rcmm->base + reg);
> +}
> +
> +static void rcar_cmm_lut_write(struct rcar_cmm *rcmm, unsigned int size,
> + struct drm_color_lut *lut)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < size; ++i) {
> + struct drm_color_lut *entry = &lut[i];
> +
> + u32 val = (entry->red & 0xff) << 16 |
> + (entry->green & 0xff) << 8 |
> + (entry->blue & 0xff);

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

> + rcar_cmm_write(rcmm, CM2_LUT_TBLA(i), val);
> + }
> +}
> +
> +/**
> + * rcar_cmm_setup() - configure the CMM unit
> + *
> + * @pdev: The platform device associated with the CMM instance
> + * @config: The CRTC provided configuration.
> + *
> + * Configure the CMM unit with the CRTC provided configuration.
> + * Currently enabling, disabling and programming of the 1-D LUT unit is
> + * supported.
> + */
> +int rcar_cmm_setup(struct platform_device *pdev,
> + const struct rcar_cmm_config *config)
> +{
> + struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> + unsigned int i;
> +
> + if (config->lut.size > CMM_GAMMA_LUT_SIZE)
> + return -EINVAL;
> +
> + /*
> + * As cmm_setup is called by atomic commit tail helper, it might be
> + * called before enabling the CRTC (which calls cmm_enable()).
> + */
> + if (!rcmm->enabled) {
> + if (!config->lut.enable)
> + return 0;
> +
> + /*
> + * Store the LUT table entries in the scratch buffer to be later
> + * programmed at enable time.
> + */
> + for (i = 0; i < config->lut.size; ++i)
> + rcmm->lut.table[i] = config->lut.table[i];
> +
> + rcmm->lut.size = config->lut.size;
> + rcmm->lut.restore = true;
> +
> + return 0;
> + }
> +
> + /* Stop LUT operations, if requested. */
> + if (rcmm->lut.running && !config->lut.enable) {
> + rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> + rcmm->lut.running = 0;
> + rcmm->lut.size = 0;
> +
> + return 0;
> + }
> +
> + /* Enable LUT and program the new gamma table values. */
> + if (!rcmm->lut.running) {
> + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> + rcmm->lut.running = true;
> + }
> +
> + rcar_cmm_lut_write(rcmm, config->lut.size, config->lut.table);
> + rcmm->lut.size = config->lut.size;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(rcar_cmm_setup);
> +
> +/**
> + * rcar_cmm_enable - enable the CMM unit
> + *
> + * @pdev: The platform device associated with the CMM instance
> + *
> + * Enable the CMM unit by enabling the parent clock and enabling the CMM
> + * components, such as 1-D LUT, if requested.
> + */
> +int rcar_cmm_enable(struct platform_device *pdev)
> +{
> + struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> + int ret;
> +
> + if (!rcmm)
> + return -EPROBE_DEFER;
> +
> + ret = clk_prepare_enable(rcmm->clk);
> + if (ret)
> + return ret;
> +
> + /* Apply the LUT table values saved at cmm_setup time. */
> + if (rcmm->lut.restore) {
> + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> + rcar_cmm_lut_write(rcmm, rcmm->lut.size, rcmm->lut.table);
> +
> + rcmm->lut.restore = false;
> + rcmm->lut.running = true;
> + }
> +
> + rcmm->enabled = true;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(rcar_cmm_enable);
> +
> +/**
> + * rcar_cmm_disable() - disable the CMM unit
> + *
> + * Disable the CMM unit by stopping the parent clock.
> + *
> + * @pdev: The platform device associated with the CMM instance
> + */
> +void rcar_cmm_disable(struct platform_device *pdev)
> +{
> + struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> +
> + rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> +
> + clk_disable_unprepare(rcmm->clk);
> +
> + rcmm->lut.restore = false;
> + rcmm->lut.running = false;
> + rcmm->lut.size = 0;
> +
> + rcmm->enabled = false;
> +}
> +EXPORT_SYMBOL_GPL(rcar_cmm_disable);
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int rcar_cmm_pm_suspend(struct device *dev)
> +{
> + struct rcar_cmm *rcmm = dev_get_drvdata(dev);
> + unsigned int i;
> +
> + if (!(rcmm->lut.running || rcmm->lut.restore))
> + return 0;
> +
> + /* Save the LUT table entries in the scratch buffer table. */
> + for (i = 0; i < rcmm->lut.size; ++i) {
> + int entry = rcar_cmm_read(rcmm, CM2_LUT_TBLA(i));
> + struct drm_color_lut *lut = &rcmm->lut.table[i];
> +
> + lut->blue = entry & 0xff;
> + lut->green = (entry >> 8) & 0xff;
> + lut->red = (entry >> 16) & 0xff;

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

> + }
> +
> + rcmm->lut.restore = true;
> + rcmm->lut.running = false;
> +
> + rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> +
> + return 0;
> +}
> +
> +static int rcar_cmm_pm_resume(struct device *dev)
> +{
> + struct rcar_cmm *rcmm = dev_get_drvdata(dev);
> +
> + if (!rcmm->lut.restore)
> + return 0;
> +
> + /* Program the LUT entries saved at suspend time. */
> + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> + rcar_cmm_lut_write(rcmm, rcmm->lut.size, rcmm->lut.table);
> + rcmm->lut.running = true;
> + rcmm->lut.restore = false;
> +
> + return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops rcar_cmm_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(rcar_cmm_pm_suspend, rcar_cmm_pm_resume)
> +};
> +
> +static int rcar_cmm_probe(struct platform_device *pdev)
> +{
> + struct rcar_cmm *rcmm;
> + struct resource *res;
> + resource_size_t size;
> +
> + rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
> + if (!rcmm)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, rcmm);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + size = resource_size(res);
> + if (!devm_request_mem_region(&pdev->dev, res->start, size,
> + dev_name(&pdev->dev))) {
> + dev_err(&pdev->dev,
> + "can't request region for resource %pR\n", res);
> + return -EBUSY;
> + }
> +
> + rcmm->base = devm_ioremap_nocache(&pdev->dev, res->start, size);
> + if (IS_ERR(rcmm->base))
> + return PTR_ERR(rcmm->base);
> +
> + rcmm->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(rcmm->clk)) {
> + dev_err(&pdev->dev, "Failed to get CMM clock");
> + return PTR_ERR(rcmm->clk);
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id rcar_cmm_of_table[] = {
> + { .compatible = "renesas,rcar-gen3-cmm", },
> + { .compatible = "renesas,rcar-gen2-cmm", },
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
> +
> +static struct platform_driver rcar_cmm_platform_driver = {
> + .probe = rcar_cmm_probe,
> + .driver = {
> + .name = "rcar-cmm",
> + .pm = &rcar_cmm_pm_ops,
> + .of_match_table = rcar_cmm_of_table,
> + },
> +};
> +
> +module_platform_driver(rcar_cmm_platform_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <[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..8744e72f32cd
> --- /dev/null
> +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * rcar_cmm.h -- R-Car Display Unit Color Management Module
> + *
> + * Copyright (C) 2019 Jacopo Mondi <[email protected]>
> + */
> +
> +#ifndef __RCAR_CMM_H__
> +#define __RCAR_CMM_H__
> +
> +#define CMM_GAMMA_LUT_SIZE 256
> +
> +struct platform_device;
> +struct drm_color_lut;
> +
> +/**
> + * struct rcar_cmm_config - CMM configuration
> + *
> + * @lut: 1D-LUT configuration
> + * @lut.enable: 1D-LUT enable flag
> + * @lut.table: 1D-LUT table entries.
> + * @lut.size 1D-LUT number of entries. Max is 256.
> + */
> +struct rcar_cmm_config {
> + struct {
> + bool enable;
> + struct drm_color_lut *table;
> + unsigned int size;
> + } lut;
> +};
> +
> +int rcar_cmm_enable(struct platform_device *pdev);
> +void rcar_cmm_disable(struct platform_device *pdev);
> +
> +int rcar_cmm_setup(struct platform_device *pdev,
> + const struct rcar_cmm_config *config);
> +
> +#endif /* __RCAR_CMM_H__ */
> --
> 2.21.0
>

CU
Uli

2019-07-16 13:33:01

by Ulrich Hecht

[permalink] [raw]
Subject: Re: [PATCH v2 17/19] drm: rcar-du: crtc: Enable and disable CMMs


> On July 6, 2019 at 4:07 PM Jacopo Mondi <[email protected]> wrote:
>
>
> Enable/disable the CMM associated with a CRTC at crtc start and stop
> time and enable the CMM unit through the Display Extensional Functions
> register at group setup time.
>
> Signed-off-by: Jacopo Mondi <[email protected]>
> ---
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 7 +++++++
> drivers/gpu/drm/rcar-du/rcar_du_group.c | 8 ++++++++
> drivers/gpu/drm/rcar-du/rcar_du_regs.h | 5 +++++
> 3 files changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 23f1d6cc1719..3dac605c3a67 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -21,6 +21,7 @@
> #include <drm/drm_plane_helper.h>
> #include <drm/drm_vblank.h>
>
> +#include "rcar_cmm.h"
> #include "rcar_du_crtc.h"
> #include "rcar_du_drv.h"
> #include "rcar_du_encoder.h"
> @@ -619,6 +620,9 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
> if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> rcar_du_vsp_disable(rcrtc);
>
> + if (rcrtc->cmm)
> + rcar_cmm_disable(rcrtc->cmm);
> +
> /*
> * Select switch sync mode. This stops display operation and configures
> * the HSYNC and VSYNC signals as inputs.
> @@ -686,6 +690,9 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
> }
>
> rcar_du_crtc_start(rcrtc);
> +
> + if (rcrtc->cmm)
> + rcar_cmm_enable(rcrtc->cmm);
> }
>
> static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> index 9eee47969e77..d252c9bb9809 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> @@ -147,6 +147,14 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
>
> rcar_du_group_setup_pins(rgrp);
>
> + if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM)) {
> + u32 defr7 = DEFR7_CODE |
> + (rgrp->cmms_mask & BIT(1) ? DEFR7_CMME1 : 0) |
> + (rgrp->cmms_mask & BIT(0) ? DEFR7_CMME0 : 0);
> +
> + rcar_du_group_write(rgrp, DEFR7, defr7);
> + }
> +
> if (rcdu->info->gen >= 2) {
> rcar_du_group_setup_defr8(rgrp);
> rcar_du_group_setup_didsr(rgrp);
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> index bc87f080b170..fb9964949368 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> @@ -197,6 +197,11 @@
> #define DEFR6_MLOS1 (1 << 2)
> #define DEFR6_DEFAULT (DEFR6_CODE | DEFR6_TCNE1)
>
> +#define DEFR7 0x000ec
> +#define DEFR7_CODE (0x7779 << 16)
> +#define DEFR7_CMME1 BIT(6)
> +#define DEFR7_CMME0 BIT(4)
> +

Reviewed-by: Ulrich Hecht <[email protected]>

CU
Uli

2019-07-16 13:34:44

by Ulrich Hecht

[permalink] [raw]
Subject: Re: [PATCH v2 18/19] drm: rcar-du: crtc: Register GAMMA_LUT properties


> On July 6, 2019 at 4:07 PM Jacopo Mondi <[email protected]> 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 <[email protected]>
> ---
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 3dac605c3a67..222ccc20d6d8 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -1082,6 +1082,7 @@ static const struct drm_crtc_funcs crtc_funcs_gen3 = {
> .set_crc_source = rcar_du_crtc_set_crc_source,
> .verify_crc_source = rcar_du_crtc_verify_crc_source,
> .get_crc_sources = rcar_du_crtc_get_crc_sources,
> + .gamma_set = drm_atomic_helper_legacy_gamma_set,
> };
>
> /* -----------------------------------------------------------------------------
> @@ -1205,6 +1206,9 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
> if (rcdu->cmms[swindex]) {
> rcrtc->cmm = rcdu->cmms[swindex];
> rgrp->cmms_mask |= BIT(hwindex % 2);
> +
> + drm_mode_crtc_set_gamma_size(crtc, CMM_GAMMA_LUT_SIZE);
> + drm_crtc_enable_color_mgmt(crtc, 0, false, CMM_GAMMA_LUT_SIZE);
> }
>
> drm_crtc_helper_add(crtc, &crtc_helper_funcs);
> --
> 2.21.0
>

Reviewed-by: Ulrich Hecht <[email protected]>

CU
Uli

2019-07-16 13:35:13

by Ulrich Hecht

[permalink] [raw]
Subject: Re: [PATCH v2 19/19] drm: rcar-du: kms: Update CMM in atomic commit tail


> On July 6, 2019 at 4:07 PM Jacopo Mondi <[email protected]> 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 <[email protected]>
> ---
> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 35 +++++++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index b79cda2f5531..f9aece78ca5f 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -21,6 +21,7 @@
> #include <linux/of_platform.h>
> #include <linux/wait.h>
>
> +#include "rcar_cmm.h"
> #include "rcar_du_crtc.h"
> #include "rcar_du_drv.h"
> #include "rcar_du_encoder.h"
> @@ -287,6 +288,37 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv,
> * Atomic Check and Update
> */
>
> +static void rcar_du_atomic_commit_update_cmm(struct drm_crtc *crtc,
> + struct drm_crtc_state *old_state)
> +{
> + struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> + struct rcar_cmm_config cmm_config = {};
> +
> + if (!rcrtc->cmm || !crtc->state->color_mgmt_changed)
> + return;
> +
> + if (!crtc->state->gamma_lut) {
> + cmm_config.lut.enable = false;
> + rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> +
> + return;
> + }
> +
> + cmm_config.lut.enable = true;
> + cmm_config.lut.table = (struct drm_color_lut *)
> + crtc->state->gamma_lut->data;
> +
> + /* Set LUT table size to 0 if entries should not be updated. */
> + if (!old_state->gamma_lut ||
> + old_state->gamma_lut->base.id != crtc->state->gamma_lut->base.id)
> + cmm_config.lut.size = crtc->state->gamma_lut->length
> + / sizeof(cmm_config.lut.table[0]);
> + else
> + cmm_config.lut.size = 0;
> +
> + rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> +}
> +
> static int rcar_du_atomic_check(struct drm_device *dev,
> struct drm_atomic_state *state)
> {
> @@ -329,6 +361,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
> rcdu->dpad1_source = rcrtc->index;
> }
>
> + for_each_old_crtc_in_state(old_state, crtc, crtc_state, i)
> + rcar_du_atomic_commit_update_cmm(crtc, crtc_state);
> +
> /* Apply the atomic update. */
> drm_atomic_helper_commit_modeset_disables(dev, old_state);
> drm_atomic_helper_commit_planes(dev, old_state,
> --
> 2.21.0
>

Reviewed-by: Ulrich Hecht <[email protected]>

CU
Uli

2019-08-19 13:47:15

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v2 01/19] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation

Hi Jacopo,

On Mon, Jul 8, 2019 at 9:58 AM Geert Uytterhoeven <[email protected]> wrote:
> On Sat, Jul 6, 2019 at 4:07 PM Jacopo Mondi <[email protected]> wrote:
> > Add device tree bindings documentation for the Renesas R-Car Display
> > Unit Color Management Module.
> >
> > CMM is the image enhancement module available on each R-Car DU video
> > channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).
> >
> > Signed-off-by: Jacopo Mondi <[email protected]>
> > Reviewed-by: Laurent Pinchart <[email protected]>
>
> Thanks for your patch!
>
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> > @@ -0,0 +1,25 @@
> > +* Renesas R-Car Color Management Module (CMM)
> > +
> > +Renesas R-Car image enhancement module connected to R-Car DU video channels.
> > +
> > +Required properties:
> > + - compatible: shall be one of:
> > + - "renesas,rcar-gen3-cmm"
> > + - "renesas,rcar-gen2-cmm"
>
> Why do you think you do not need SoC-specific compatible values?
> What if you discover a different across the R-Car Gen3 line tomorrow?
> Does the IP block have a version register?

Do you have an answer to these questions?
Thanks!

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

2019-08-20 07:48:19

by jacopo mondi

[permalink] [raw]
Subject: Re: [PATCH v2 01/19] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation

Hi Geert,
sorry for the delayed response..

On Mon, Aug 19, 2019 at 03:45:54PM +0200, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Mon, Jul 8, 2019 at 9:58 AM Geert Uytterhoeven <[email protected]> wrote:
> > On Sat, Jul 6, 2019 at 4:07 PM Jacopo Mondi <[email protected]> wrote:
> > > Add device tree bindings documentation for the Renesas R-Car Display
> > > Unit Color Management Module.
> > >
> > > CMM is the image enhancement module available on each R-Car DU video
> > > channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).
> > >
> > > Signed-off-by: Jacopo Mondi <[email protected]>
> > > Reviewed-by: Laurent Pinchart <[email protected]>
> >
> > Thanks for your patch!
> >
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> > > @@ -0,0 +1,25 @@
> > > +* Renesas R-Car Color Management Module (CMM)
> > > +
> > > +Renesas R-Car image enhancement module connected to R-Car DU video channels.
> > > +
> > > +Required properties:
> > > + - compatible: shall be one of:
> > > + - "renesas,rcar-gen3-cmm"
> > > + - "renesas,rcar-gen2-cmm"
> >
> > Why do you think you do not need SoC-specific compatible values?
> > What if you discover a different across the R-Car Gen3 line tomorrow?
> > Does the IP block have a version register?
>
> Do you have an answer to these questions?

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

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

Thanks
j


> Thanks!
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [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


Attachments:
(No filename) (2.12 kB)
signature.asc (849.00 B)
Download all attachments

2019-08-20 07:55:01

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v2 01/19] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation

Hi Jacopo,

On Tue, Aug 20, 2019 at 9:47 AM Jacopo Mondi <[email protected]> wrote:
> On Mon, Aug 19, 2019 at 03:45:54PM +0200, Geert Uytterhoeven wrote:
> > On Mon, Jul 8, 2019 at 9:58 AM Geert Uytterhoeven <[email protected]> wrote:
> > > On Sat, Jul 6, 2019 at 4:07 PM Jacopo Mondi <[email protected]> wrote:
> > > > Add device tree bindings documentation for the Renesas R-Car Display
> > > > Unit Color Management Module.
> > > >
> > > > CMM is the image enhancement module available on each R-Car DU video
> > > > channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).
> > > >
> > > > Signed-off-by: Jacopo Mondi <[email protected]>
> > > > Reviewed-by: Laurent Pinchart <[email protected]>
> > >
> > > Thanks for your patch!
> > >
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> > > > @@ -0,0 +1,25 @@
> > > > +* Renesas R-Car Color Management Module (CMM)
> > > > +
> > > > +Renesas R-Car image enhancement module connected to R-Car DU video channels.
> > > > +
> > > > +Required properties:
> > > > + - compatible: shall be one of:
> > > > + - "renesas,rcar-gen3-cmm"
> > > > + - "renesas,rcar-gen2-cmm"
> > >
> > > Why do you think you do not need SoC-specific compatible values?
> > > What if you discover a different across the R-Car Gen3 line tomorrow?
> > > Does the IP block have a version register?
> >
> > Do you have an answer to these questions?
>
> It does not seem to me that CMM has any version register, nor there
> are differences between the different Gen3 SoCs..
>
> However, even if we now define a single compatible property for
> gen3/gen2 and we later find out one of the SoC needs a soc-specific
> property we can safely add it and keep the generic gen3/gen2 one as
> fallback.. Does it work for you?

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

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

2019-08-20 08:05:12

by jacopo mondi

[permalink] [raw]
Subject: Re: [PATCH v2 01/19] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation

Hi Geert,

On Tue, Aug 20, 2019 at 09:53:44AM +0200, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Tue, Aug 20, 2019 at 9:47 AM Jacopo Mondi <[email protected]> wrote:
> > On Mon, Aug 19, 2019 at 03:45:54PM +0200, Geert Uytterhoeven wrote:
> > > On Mon, Jul 8, 2019 at 9:58 AM Geert Uytterhoeven <[email protected]> wrote:
> > > > On Sat, Jul 6, 2019 at 4:07 PM Jacopo Mondi <[email protected]> wrote:
> > > > > Add device tree bindings documentation for the Renesas R-Car Display
> > > > > Unit Color Management Module.
> > > > >
> > > > > CMM is the image enhancement module available on each R-Car DU video
> > > > > channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <[email protected]>
> > > > > Reviewed-by: Laurent Pinchart <[email protected]>
> > > >
> > > > Thanks for your patch!
> > > >
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> > > > > @@ -0,0 +1,25 @@
> > > > > +* Renesas R-Car Color Management Module (CMM)
> > > > > +
> > > > > +Renesas R-Car image enhancement module connected to R-Car DU video channels.
> > > > > +
> > > > > +Required properties:
> > > > > + - compatible: shall be one of:
> > > > > + - "renesas,rcar-gen3-cmm"
> > > > > + - "renesas,rcar-gen2-cmm"
> > > >
> > > > Why do you think you do not need SoC-specific compatible values?
> > > > What if you discover a different across the R-Car Gen3 line tomorrow?
> > > > Does the IP block have a version register?
> > >
> > > Do you have an answer to these questions?
> >
> > It does not seem to me that CMM has any version register, nor there
> > are differences between the different Gen3 SoCs..
> >
> > However, even if we now define a single compatible property for
> > gen3/gen2 and we later find out one of the SoC needs a soc-specific
> > property we can safely add it and keep the generic gen3/gen2 one as
> > fallback.. Does it work for you?
>
> Unfortunately that won't work, as the existing DTBs won't have the
> soc-specific compatible value.

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


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

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


>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [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


Attachments:
(No filename) (2.91 kB)
signature.asc (849.00 B)
Download all attachments

2019-08-20 17:36:05

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v2 14/19] drm: rcar-du: Add support for CMM

Hi Jacopo,

Thank you for the patch.

On Sat, Jul 06, 2019 at 04:07:41PM +0200, Jacopo Mondi wrote:
> Add a driver for the R-Car Display Unit Color Correction Module.
>
> In most of Gen3 SoCs, each DU output channel is provided with a CMM unit
> to perform image enhancement and color correction.
>
> Add support for CMM through a driver that supports configuration of
> the 1-dimensional LUT table. More advanced CMM feature will be
> implemented on top of this basic one.
>
> Signed-off-by: Jacopo Mondi <[email protected]>
> ---
> drivers/gpu/drm/rcar-du/Kconfig | 7 +
> drivers/gpu/drm/rcar-du/Makefile | 1 +
> drivers/gpu/drm/rcar-du/rcar_cmm.c | 292 +++++++++++++++++++++++++++++
> drivers/gpu/drm/rcar-du/rcar_cmm.h | 38 ++++
> 4 files changed, 338 insertions(+)
> create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.c
> create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.h
>
> diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
> index 1529849e217e..539d232790d1 100644
> --- a/drivers/gpu/drm/rcar-du/Kconfig
> +++ b/drivers/gpu/drm/rcar-du/Kconfig
> @@ -13,6 +13,13 @@ config DRM_RCAR_DU
> Choose this option if you have an R-Car chipset.
> If M is selected the module will be called rcar-du-drm.
>
> +config DRM_RCAR_CMM
> + bool "R-Car DU Color Management Module (CMM) Support"
> + depends on DRM && OF
> + depends on DRM_RCAR_DU
> + help
> + Enable support for R-Car Color Management Module (CMM).
> +
> config DRM_RCAR_DW_HDMI
> tristate "R-Car DU Gen3 HDMI Encoder Support"
> depends on DRM && OF
> diff --git a/drivers/gpu/drm/rcar-du/Makefile b/drivers/gpu/drm/rcar-du/Makefile
> index 6c2ed9c46467..4d1187ccc3e5 100644
> --- a/drivers/gpu/drm/rcar-du/Makefile
> +++ b/drivers/gpu/drm/rcar-du/Makefile
> @@ -15,6 +15,7 @@ rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS) += rcar_du_of.o \
> rcar-du-drm-$(CONFIG_DRM_RCAR_VSP) += rcar_du_vsp.o
> rcar-du-drm-$(CONFIG_DRM_RCAR_WRITEBACK) += rcar_du_writeback.o
>
> +obj-$(CONFIG_DRM_RCAR_CMM) += rcar_cmm.o
> obj-$(CONFIG_DRM_RCAR_DU) += rcar-du-drm.o
> obj-$(CONFIG_DRM_RCAR_DW_HDMI) += rcar_dw_hdmi.o
> obj-$(CONFIG_DRM_RCAR_LVDS) += rcar_lvds.o
> diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> new file mode 100644
> index 000000000000..76ed3fce2b33
> --- /dev/null
> +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> @@ -0,0 +1,292 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * rcar_cmm.c -- R-Car Display Unit Color Management Module
> + *
> + * Copyright (C) 2019 Jacopo Mondi <[email protected]>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +
> +#include <drm/drm_atomic.h>

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

> +#include "rcar_cmm.h"
> +
> +#define CM2_LUT_CTRL 0x0000
> +#define CM2_LUT_CTRL_EN BIT(0)

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

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

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

> +
> +struct rcar_cmm {
> + struct clk *clk;
> + void __iomem *base;
> + bool enabled;
> +
> + /*
> + * restore: LUT restore flag

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

> + * running: LUT operating flag
> + * size: Number of programmed entries in the LUT table
> + * table: Scratch buffer where to store the LUT table entries to be
> + * later restored.

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

* @lut.restore: ...
...

> + */
> + struct {
> + bool restore;
> + bool running;
> + unsigned int size;
> + struct drm_color_lut table[CMM_GAMMA_LUT_SIZE];
> + } lut;

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

> +};
> +
> +static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg)
> +{
> + return ioread32(rcmm->base + reg);
> +}
> +
> +static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data)
> +{
> + iowrite32(data, rcmm->base + reg);
> +}
> +
> +static void rcar_cmm_lut_write(struct rcar_cmm *rcmm, unsigned int size,

s/unsigned int/size_t/ ?

> + struct drm_color_lut *lut)

You can make this pointer const.

> +{
> + unsigned int i;
> +
> + for (i = 0; i < size; ++i) {
> + struct drm_color_lut *entry = &lut[i];
> +
> + u32 val = (entry->red & 0xff) << 16 |
> + (entry->green & 0xff) << 8 |
> + (entry->blue & 0xff);
> + rcar_cmm_write(rcmm, CM2_LUT_TBLA(i), val);
> + }
> +}
> +
> +/**
> + * rcar_cmm_setup() - configure the CMM unit
> + *
> + * @pdev: The platform device associated with the CMM instance
> + * @config: The CRTC provided configuration.
> + *
> + * Configure the CMM unit with the CRTC provided configuration.

s/CRTC provided/CRTC-provided/

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

> + * Currently enabling, disabling and programming of the 1-D LUT unit is
> + * supported.
> + */
> +int rcar_cmm_setup(struct platform_device *pdev,
> + const struct rcar_cmm_config *config)
> +{
> + struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> + unsigned int i;
> +
> + if (config->lut.size > CMM_GAMMA_LUT_SIZE)
> + return -EINVAL;
> +
> + /*
> + * As cmm_setup is called by atomic commit tail helper, it might be

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

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

I would phrase this as "... it might be called when the CMM is disabled.
We can't program the hardware in that case, store the configuration
internally and apply it when the CMM will be enabled by the CRTC through
by rcar_cmm_enable()." and remove the next comment.

> + */
> + if (!rcmm->enabled) {
> + if (!config->lut.enable)
> + return 0;
> +
> + /*
> + * Store the LUT table entries in the scratch buffer to be later
> + * programmed at enable time.
> + */
> + for (i = 0; i < config->lut.size; ++i)
> + rcmm->lut.table[i] = config->lut.table[i];

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

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

> +
> + rcmm->lut.size = config->lut.size;
> + rcmm->lut.restore = true;
> +
> + return 0;
> + }
> +
> + /* Stop LUT operations, if requested. */
> + if (rcmm->lut.running && !config->lut.enable) {
> + rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> + rcmm->lut.running = 0;
> + rcmm->lut.size = 0;
> +
> + return 0;
> + }
> +
> + /* Enable LUT and program the new gamma table values. */
> + if (!rcmm->lut.running) {

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

> + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> + rcmm->lut.running = true;
> + }
> +
> + rcar_cmm_lut_write(rcmm, config->lut.size, config->lut.table);

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

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

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

> + rcmm->lut.size = config->lut.size;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(rcar_cmm_setup);
> +
> +/**
> + * rcar_cmm_enable - enable the CMM unit
> + *
> + * @pdev: The platform device associated with the CMM instance
> + *
> + * Enable the CMM unit by enabling the parent clock and enabling the CMM
> + * components, such as 1-D LUT, if requested.
> + */
> +int rcar_cmm_enable(struct platform_device *pdev)
> +{
> + struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> + int ret;
> +
> + if (!rcmm)
> + return -EPROBE_DEFER;
> +
> + ret = clk_prepare_enable(rcmm->clk);
> + if (ret)
> + return ret;
> +

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

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

rcar_cmm_setup() here too.

> + if (rcmm->lut.restore) {
> + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> + rcar_cmm_lut_write(rcmm, rcmm->lut.size, rcmm->lut.table);
> +
> + rcmm->lut.restore = false;
> + rcmm->lut.running = true;
> + }
> +
> + rcmm->enabled = true;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(rcar_cmm_enable);
> +
> +/**
> + * rcar_cmm_disable() - disable the CMM unit
> + *
> + * Disable the CMM unit by stopping the parent clock.
> + *
> + * @pdev: The platform device associated with the CMM instance

Parameters usually go before the description test.

> + */
> +void rcar_cmm_disable(struct platform_device *pdev)
> +{
> + struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> +
> + rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> +
> + clk_disable_unprepare(rcmm->clk);
> +
> + rcmm->lut.restore = false;
> + rcmm->lut.running = false;
> + rcmm->lut.size = 0;
> +
> + rcmm->enabled = false;
> +}
> +EXPORT_SYMBOL_GPL(rcar_cmm_disable);
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int rcar_cmm_pm_suspend(struct device *dev)
> +{
> + struct rcar_cmm *rcmm = dev_get_drvdata(dev);
> + unsigned int i;
> +
> + if (!(rcmm->lut.running || rcmm->lut.restore))

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

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

if (!rcmm->enabled)
return 0;

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

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

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

> + return 0;
> +
> + /* Save the LUT table entries in the scratch buffer table. */

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

> + for (i = 0; i < rcmm->lut.size; ++i) {
> + int entry = rcar_cmm_read(rcmm, CM2_LUT_TBLA(i));
> + struct drm_color_lut *lut = &rcmm->lut.table[i];
> +
> + lut->blue = entry & 0xff;
> + lut->green = (entry >> 8) & 0xff;
> + lut->red = (entry >> 16) & 0xff;
> + }
> +
> + rcmm->lut.restore = true;
> + rcmm->lut.running = false;
> +
> + rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> +
> + return 0;
> +}
> +
> +static int rcar_cmm_pm_resume(struct device *dev)
> +{
> + struct rcar_cmm *rcmm = dev_get_drvdata(dev);
> +
> + if (!rcmm->lut.restore)
> + return 0;
> +
> + /* Program the LUT entries saved at suspend time. */
> + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> + rcar_cmm_lut_write(rcmm, rcmm->lut.size, rcmm->lut.table);
> + rcmm->lut.running = true;
> + rcmm->lut.restore = false;

To be kept in sync with the modifications proposed above,


if (!rcmm->enabled)
return 0;

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

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

> +
> + return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops rcar_cmm_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(rcar_cmm_pm_suspend, rcar_cmm_pm_resume)
> +};
> +
> +static int rcar_cmm_probe(struct platform_device *pdev)
> +{
> + struct rcar_cmm *rcmm;
> + struct resource *res;
> + resource_size_t size;
> +
> + rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
> + if (!rcmm)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, rcmm);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + size = resource_size(res);
> + if (!devm_request_mem_region(&pdev->dev, res->start, size,
> + dev_name(&pdev->dev))) {
> + dev_err(&pdev->dev,
> + "can't request region for resource %pR\n", res);
> + return -EBUSY;
> + }
> +
> + rcmm->base = devm_ioremap_nocache(&pdev->dev, res->start, size);
> + if (IS_ERR(rcmm->base))
> + return PTR_ERR(rcmm->base);

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

> +
> + rcmm->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(rcmm->clk)) {
> + dev_err(&pdev->dev, "Failed to get CMM clock");
> + return PTR_ERR(rcmm->clk);
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id rcar_cmm_of_table[] = {
> + { .compatible = "renesas,rcar-gen3-cmm", },
> + { .compatible = "renesas,rcar-gen2-cmm", },
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
> +
> +static struct platform_driver rcar_cmm_platform_driver = {
> + .probe = rcar_cmm_probe,
> + .driver = {
> + .name = "rcar-cmm",
> + .pm = &rcar_cmm_pm_ops,
> + .of_match_table = rcar_cmm_of_table,
> + },
> +};
> +
> +module_platform_driver(rcar_cmm_platform_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <[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..8744e72f32cd
> --- /dev/null
> +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * rcar_cmm.h -- R-Car Display Unit Color Management Module
> + *
> + * Copyright (C) 2019 Jacopo Mondi <[email protected]>
> + */
> +
> +#ifndef __RCAR_CMM_H__
> +#define __RCAR_CMM_H__
> +
> +#define CMM_GAMMA_LUT_SIZE 256
> +
> +struct platform_device;
> +struct drm_color_lut;

Could you please sort the forward declarations alphabetically ?

> +
> +/**
> + * struct rcar_cmm_config - CMM configuration
> + *
> + * @lut: 1D-LUT configuration
> + * @lut.enable: 1D-LUT enable flag
> + * @lut.table: 1D-LUT table entries.

s/\.$//

> + * @lut.size 1D-LUT number of entries. Max is 256.

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

> + */
> +struct rcar_cmm_config {
> + struct {
> + bool enable;
> + struct drm_color_lut *table;
> + unsigned int size;
> + } lut;
> +};
> +
> +int rcar_cmm_enable(struct platform_device *pdev);
> +void rcar_cmm_disable(struct platform_device *pdev);
> +
> +int rcar_cmm_setup(struct platform_device *pdev,
> + const struct rcar_cmm_config *config);
> +
> +#endif /* __RCAR_CMM_H__ */

--
Regards,

Laurent Pinchart

2019-08-20 17:38:56

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v2 14/19] drm: rcar-du: Add support for CMM

Hello,

On Tue, Jul 16, 2019 at 03:17:04PM +0200, Ulrich Hecht wrote:
> > On July 6, 2019 at 4:07 PM Jacopo Mondi <[email protected]> wrote:
> >
> > Add a driver for the R-Car Display Unit Color Correction Module.
> >
> > In most of Gen3 SoCs, each DU output channel is provided with a CMM unit
> > to perform image enhancement and color correction.
> >
> > Add support for CMM through a driver that supports configuration of
> > the 1-dimensional LUT table. More advanced CMM feature will be
> > implemented on top of this basic one.
> >
> > Signed-off-by: Jacopo Mondi <[email protected]>
> > ---
> > drivers/gpu/drm/rcar-du/Kconfig | 7 +
> > drivers/gpu/drm/rcar-du/Makefile | 1 +
> > drivers/gpu/drm/rcar-du/rcar_cmm.c | 292 +++++++++++++++++++++++++++++
> > drivers/gpu/drm/rcar-du/rcar_cmm.h | 38 ++++
> > 4 files changed, 338 insertions(+)
> > create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.c
> > create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.h
> >
> > diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
> > index 1529849e217e..539d232790d1 100644
> > --- a/drivers/gpu/drm/rcar-du/Kconfig
> > +++ b/drivers/gpu/drm/rcar-du/Kconfig
> > @@ -13,6 +13,13 @@ config DRM_RCAR_DU
> > Choose this option if you have an R-Car chipset.
> > If M is selected the module will be called rcar-du-drm.
> >
> > +config DRM_RCAR_CMM
> > + bool "R-Car DU Color Management Module (CMM) Support"
> > + depends on DRM && OF
> > + depends on DRM_RCAR_DU
> > + help
> > + Enable support for R-Car Color Management Module (CMM).
> > +
> > config DRM_RCAR_DW_HDMI
> > tristate "R-Car DU Gen3 HDMI Encoder Support"
> > depends on DRM && OF
> > diff --git a/drivers/gpu/drm/rcar-du/Makefile b/drivers/gpu/drm/rcar-du/Makefile
> > index 6c2ed9c46467..4d1187ccc3e5 100644
> > --- a/drivers/gpu/drm/rcar-du/Makefile
> > +++ b/drivers/gpu/drm/rcar-du/Makefile
> > @@ -15,6 +15,7 @@ rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS) += rcar_du_of.o \
> > rcar-du-drm-$(CONFIG_DRM_RCAR_VSP) += rcar_du_vsp.o
> > rcar-du-drm-$(CONFIG_DRM_RCAR_WRITEBACK) += rcar_du_writeback.o
> >
> > +obj-$(CONFIG_DRM_RCAR_CMM) += rcar_cmm.o
> > obj-$(CONFIG_DRM_RCAR_DU) += rcar-du-drm.o
> > obj-$(CONFIG_DRM_RCAR_DW_HDMI) += rcar_dw_hdmi.o
> > obj-$(CONFIG_DRM_RCAR_LVDS) += rcar_lvds.o
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> > new file mode 100644
> > index 000000000000..76ed3fce2b33
> > --- /dev/null
> > +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> > @@ -0,0 +1,292 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * rcar_cmm.c -- R-Car Display Unit Color Management Module
> > + *
> > + * Copyright (C) 2019 Jacopo Mondi <[email protected]>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm.h>
> > +
> > +#include <drm/drm_atomic.h>
> > +
> > +#include "rcar_cmm.h"
> > +
> > +#define CM2_LUT_CTRL 0x0000
> > +#define CM2_LUT_CTRL_EN BIT(0)
> > +#define CM2_LUT_TBLA_BASE 0x0600
> > +#define CM2_LUT_TBLA(__i) (CM2_LUT_TBLA_BASE + (__i) * 4)
> > +
> > +struct rcar_cmm {
> > + struct clk *clk;
> > + void __iomem *base;
> > + bool enabled;
> > +
> > + /*
> > + * restore: LUT restore flag
> > + * running: LUT operating flag
> > + * size: Number of programmed entries in the LUT table
> > + * table: Scratch buffer where to store the LUT table entries to be
> > + * later restored.
> > + */
> > + struct {
> > + bool restore;
> > + bool running;
> > + unsigned int size;
> > + struct drm_color_lut table[CMM_GAMMA_LUT_SIZE];
> > + } lut;
> > +};
> > +
> > +static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg)
> > +{
> > + return ioread32(rcmm->base + reg);
> > +}
> > +
> > +static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data)
> > +{
> > + iowrite32(data, rcmm->base + reg);
> > +}
> > +
> > +static void rcar_cmm_lut_write(struct rcar_cmm *rcmm, unsigned int size,
> > + struct drm_color_lut *lut)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < size; ++i) {
> > + struct drm_color_lut *entry = &lut[i];
> > +
> > + u32 val = (entry->red & 0xff) << 16 |
> > + (entry->green & 0xff) << 8 |
> > + (entry->blue & 0xff);
>
> I don't think it's correct to cut off the high bits here. There is a
> function "drm_color_lut_extract()" for converting drm_color_lut values
> to hardware precision.
>
> > + rcar_cmm_write(rcmm, CM2_LUT_TBLA(i), val);
> > + }
> > +}
> > +
> > +/**
> > + * rcar_cmm_setup() - configure the CMM unit
> > + *
> > + * @pdev: The platform device associated with the CMM instance
> > + * @config: The CRTC provided configuration.
> > + *
> > + * Configure the CMM unit with the CRTC provided configuration.
> > + * Currently enabling, disabling and programming of the 1-D LUT unit is
> > + * supported.
> > + */
> > +int rcar_cmm_setup(struct platform_device *pdev,
> > + const struct rcar_cmm_config *config)
> > +{
> > + struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> > + unsigned int i;
> > +
> > + if (config->lut.size > CMM_GAMMA_LUT_SIZE)
> > + return -EINVAL;
> > +
> > + /*
> > + * As cmm_setup is called by atomic commit tail helper, it might be
> > + * called before enabling the CRTC (which calls cmm_enable()).
> > + */
> > + if (!rcmm->enabled) {
> > + if (!config->lut.enable)
> > + return 0;
> > +
> > + /*
> > + * Store the LUT table entries in the scratch buffer to be later
> > + * programmed at enable time.
> > + */
> > + for (i = 0; i < config->lut.size; ++i)
> > + rcmm->lut.table[i] = config->lut.table[i];
> > +
> > + rcmm->lut.size = config->lut.size;
> > + rcmm->lut.restore = true;
> > +
> > + return 0;
> > + }
> > +
> > + /* Stop LUT operations, if requested. */
> > + if (rcmm->lut.running && !config->lut.enable) {
> > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> > + rcmm->lut.running = 0;
> > + rcmm->lut.size = 0;
> > +
> > + return 0;
> > + }
> > +
> > + /* Enable LUT and program the new gamma table values. */
> > + if (!rcmm->lut.running) {
> > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> > + rcmm->lut.running = true;
> > + }
> > +
> > + rcar_cmm_lut_write(rcmm, config->lut.size, config->lut.table);
> > + rcmm->lut.size = config->lut.size;
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(rcar_cmm_setup);
> > +
> > +/**
> > + * rcar_cmm_enable - enable the CMM unit
> > + *
> > + * @pdev: The platform device associated with the CMM instance
> > + *
> > + * Enable the CMM unit by enabling the parent clock and enabling the CMM
> > + * components, such as 1-D LUT, if requested.
> > + */
> > +int rcar_cmm_enable(struct platform_device *pdev)
> > +{
> > + struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> > + int ret;
> > +
> > + if (!rcmm)
> > + return -EPROBE_DEFER;
> > +
> > + ret = clk_prepare_enable(rcmm->clk);
> > + if (ret)
> > + return ret;
> > +
> > + /* Apply the LUT table values saved at cmm_setup time. */
> > + if (rcmm->lut.restore) {
> > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> > + rcar_cmm_lut_write(rcmm, rcmm->lut.size, rcmm->lut.table);
> > +
> > + rcmm->lut.restore = false;
> > + rcmm->lut.running = true;
> > + }
> > +
> > + rcmm->enabled = true;
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(rcar_cmm_enable);
> > +
> > +/**
> > + * rcar_cmm_disable() - disable the CMM unit
> > + *
> > + * Disable the CMM unit by stopping the parent clock.
> > + *
> > + * @pdev: The platform device associated with the CMM instance
> > + */
> > +void rcar_cmm_disable(struct platform_device *pdev)
> > +{
> > + struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> > +
> > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> > +
> > + clk_disable_unprepare(rcmm->clk);
> > +
> > + rcmm->lut.restore = false;
> > + rcmm->lut.running = false;
> > + rcmm->lut.size = 0;
> > +
> > + rcmm->enabled = false;
> > +}
> > +EXPORT_SYMBOL_GPL(rcar_cmm_disable);
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int rcar_cmm_pm_suspend(struct device *dev)
> > +{
> > + struct rcar_cmm *rcmm = dev_get_drvdata(dev);
> > + unsigned int i;
> > +
> > + if (!(rcmm->lut.running || rcmm->lut.restore))
> > + return 0;
> > +
> > + /* Save the LUT table entries in the scratch buffer table. */
> > + for (i = 0; i < rcmm->lut.size; ++i) {
> > + int entry = rcar_cmm_read(rcmm, CM2_LUT_TBLA(i));
> > + struct drm_color_lut *lut = &rcmm->lut.table[i];
> > +
> > + lut->blue = entry & 0xff;
> > + lut->green = (entry >> 8) & 0xff;
> > + lut->red = (entry >> 16) & 0xff;
>
> Need to convert the values back to drm_color_lut scale here. I could
> not find a counterpart to drm_color_lut_extract(), though...

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

> > + }
> > +
> > + rcmm->lut.restore = true;
> > + rcmm->lut.running = false;
> > +
> > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> > +
> > + return 0;
> > +}
> > +
> > +static int rcar_cmm_pm_resume(struct device *dev)
> > +{
> > + struct rcar_cmm *rcmm = dev_get_drvdata(dev);
> > +
> > + if (!rcmm->lut.restore)
> > + return 0;
> > +
> > + /* Program the LUT entries saved at suspend time. */
> > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> > + rcar_cmm_lut_write(rcmm, rcmm->lut.size, rcmm->lut.table);
> > + rcmm->lut.running = true;
> > + rcmm->lut.restore = false;
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > +static const struct dev_pm_ops rcar_cmm_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(rcar_cmm_pm_suspend, rcar_cmm_pm_resume)
> > +};
> > +
> > +static int rcar_cmm_probe(struct platform_device *pdev)
> > +{
> > + struct rcar_cmm *rcmm;
> > + struct resource *res;
> > + resource_size_t size;
> > +
> > + rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
> > + if (!rcmm)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, rcmm);
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + size = resource_size(res);
> > + if (!devm_request_mem_region(&pdev->dev, res->start, size,
> > + dev_name(&pdev->dev))) {
> > + dev_err(&pdev->dev,
> > + "can't request region for resource %pR\n", res);
> > + return -EBUSY;
> > + }
> > +
> > + rcmm->base = devm_ioremap_nocache(&pdev->dev, res->start, size);
> > + if (IS_ERR(rcmm->base))
> > + return PTR_ERR(rcmm->base);
> > +
> > + rcmm->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(rcmm->clk)) {
> > + dev_err(&pdev->dev, "Failed to get CMM clock");
> > + return PTR_ERR(rcmm->clk);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id rcar_cmm_of_table[] = {
> > + { .compatible = "renesas,rcar-gen3-cmm", },
> > + { .compatible = "renesas,rcar-gen2-cmm", },
> > + { },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
> > +
> > +static struct platform_driver rcar_cmm_platform_driver = {
> > + .probe = rcar_cmm_probe,
> > + .driver = {
> > + .name = "rcar-cmm",
> > + .pm = &rcar_cmm_pm_ops,
> > + .of_match_table = rcar_cmm_of_table,
> > + },
> > +};
> > +
> > +module_platform_driver(rcar_cmm_platform_driver);
> > +
> > +MODULE_AUTHOR("Jacopo Mondi <[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..8744e72f32cd
> > --- /dev/null
> > +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h
> > @@ -0,0 +1,38 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * rcar_cmm.h -- R-Car Display Unit Color Management Module
> > + *
> > + * Copyright (C) 2019 Jacopo Mondi <[email protected]>
> > + */
> > +
> > +#ifndef __RCAR_CMM_H__
> > +#define __RCAR_CMM_H__
> > +
> > +#define CMM_GAMMA_LUT_SIZE 256
> > +
> > +struct platform_device;
> > +struct drm_color_lut;
> > +
> > +/**
> > + * struct rcar_cmm_config - CMM configuration
> > + *
> > + * @lut: 1D-LUT configuration
> > + * @lut.enable: 1D-LUT enable flag
> > + * @lut.table: 1D-LUT table entries.
> > + * @lut.size 1D-LUT number of entries. Max is 256.
> > + */
> > +struct rcar_cmm_config {
> > + struct {
> > + bool enable;
> > + struct drm_color_lut *table;
> > + unsigned int size;
> > + } lut;
> > +};
> > +
> > +int rcar_cmm_enable(struct platform_device *pdev);
> > +void rcar_cmm_disable(struct platform_device *pdev);
> > +
> > +int rcar_cmm_setup(struct platform_device *pdev,
> > + const struct rcar_cmm_config *config);
> > +
> > +#endif /* __RCAR_CMM_H__ */

--
Regards,

Laurent Pinchart

2019-08-20 17:42:48

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v2 01/19] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation

Hi Geert,

On Tue, Aug 20, 2019 at 09:53:44AM +0200, Geert Uytterhoeven wrote:
> On Tue, Aug 20, 2019 at 9:47 AM Jacopo Mondi <[email protected]> wrote:
> > On Mon, Aug 19, 2019 at 03:45:54PM +0200, Geert Uytterhoeven wrote:
> >> On Mon, Jul 8, 2019 at 9:58 AM Geert Uytterhoeven <[email protected]> wrote:
> >>> On Sat, Jul 6, 2019 at 4:07 PM Jacopo Mondi <[email protected]> wrote:
> >>>> Add device tree bindings documentation for the Renesas R-Car Display
> >>>> Unit Color Management Module.
> >>>>
> >>>> CMM is the image enhancement module available on each R-Car DU video
> >>>> channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).
> >>>>
> >>>> Signed-off-by: Jacopo Mondi <[email protected]>
> >>>> Reviewed-by: Laurent Pinchart <[email protected]>
> >>>
> >>> Thanks for your patch!
> >>>
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> >>>> @@ -0,0 +1,25 @@
> >>>> +* Renesas R-Car Color Management Module (CMM)
> >>>> +
> >>>> +Renesas R-Car image enhancement module connected to R-Car DU video channels.
> >>>> +
> >>>> +Required properties:
> >>>> + - compatible: shall be one of:
> >>>> + - "renesas,rcar-gen3-cmm"
> >>>> + - "renesas,rcar-gen2-cmm"
> >>>
> >>> Why do you think you do not need SoC-specific compatible values?
> >>> What if you discover a different across the R-Car Gen3 line tomorrow?
> >>> Does the IP block have a version register?
> >>
> >> Do you have an answer to these questions?
> >
> > It does not seem to me that CMM has any version register, nor there
> > are differences between the different Gen3 SoCs..
> >
> > However, even if we now define a single compatible property for
> > gen3/gen2 and we later find out one of the SoC needs a soc-specific
> > property we can safely add it and keep the generic gen3/gen2 one as
> > fallback.. Does it work for you?
>
> Unfortunately that won't work, as the existing DTBs won't have the
> soc-specific compatible value.
> You could still resort to soc_device_match(), but it is better to avoid that.

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

--
Regards,

Laurent Pinchart

2019-08-20 17:48:52

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v2 15/19] drm: rcar-du: Claim CMM support for Gen3 SoCs

Hi Jacopo,

Thank you for the patch.

On Sat, Jul 06, 2019 at 04:07:42PM +0200, Jacopo Mondi wrote:
> Add CMM to the list of supported features for Gen3 SoCs that provide it:
> - R8A7795
> - R8A7796
> - R8A77965
> - R8A7799x
>
> Leave R8A77970 out as V3M and V3H are the only Gen3 SoCs that do not
> support CMM.
>
> Signed-off-by: Jacopo Mondi <[email protected]>

Reviewed-by: Laurent Pinchart <[email protected]>

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

--
Regards,

Laurent Pinchart

2019-08-20 17:57:28

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v2 16/19] drm: rcar-du: kms: Collect CMM instances

Hi Jacopo,

Thank you for the patch.

On Sat, Jul 06, 2019 at 04:07:43PM +0200, Jacopo Mondi wrote:
> Implement device tree parsing to collect the available CMM instances
> described by the 'cmms' property. Associate CMMs with CRTCs and store a
> mask of active CMMs in the DU group for later enablement.
>
> Signed-off-by: Jacopo Mondi <[email protected]>
> ---
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 6 +++
> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 +
> drivers/gpu/drm/rcar-du/rcar_du_drv.h | 3 ++
> drivers/gpu/drm/rcar-du/rcar_du_group.h | 2 +
> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 50 +++++++++++++++++++++++++
> 5 files changed, 63 insertions(+)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 2da46e3dc4ae..23f1d6cc1719 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -1194,6 +1194,12 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
> if (ret < 0)
> return ret;
>
> + /* CMM might be disabled for this CRTC. */
> + if (rcdu->cmms[swindex]) {
> + rcrtc->cmm = rcdu->cmms[swindex];
> + rgrp->cmms_mask |= BIT(hwindex % 2);
> + }
> +
> drm_crtc_helper_add(crtc, &crtc_helper_funcs);
>
> /* Start with vertical blanking interrupt reporting disabled. */
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> index 3b7fc668996f..5f2940c42225 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -39,6 +39,7 @@ struct rcar_du_vsp;
> * @vblank_wait: wait queue used to signal vertical blanking
> * @vblank_count: number of vertical blanking interrupts to wait for
> * @group: CRTC group this CRTC belongs to
> + * @cmm: CMM associated with this CRTC
> * @vsp: VSP feeding video to this CRTC
> * @vsp_pipe: index of the VSP pipeline feeding video to this CRTC
> * @writeback: the writeback connector
> @@ -64,6 +65,7 @@ struct rcar_du_crtc {
> unsigned int vblank_count;
>
> struct rcar_du_group *group;
> + struct platform_device *cmm;
> struct rcar_du_vsp *vsp;
> unsigned int vsp_pipe;
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> index a00dccc447aa..300ec60ba31b 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> @@ -13,6 +13,7 @@
> #include <linux/kernel.h>
> #include <linux/wait.h>
>
> +#include "rcar_cmm.h"
> #include "rcar_du_crtc.h"
> #include "rcar_du_group.h"
> #include "rcar_du_vsp.h"
> @@ -70,6 +71,7 @@ struct rcar_du_device_info {
>
> #define RCAR_DU_MAX_CRTCS 4
> #define RCAR_DU_MAX_GROUPS DIV_ROUND_UP(RCAR_DU_MAX_CRTCS, 2)
> +#define RCAR_DU_MAX_CMMS 4
> #define RCAR_DU_MAX_VSPS 4
>
> struct rcar_du_device {
> @@ -86,6 +88,7 @@ struct rcar_du_device {
> struct rcar_du_encoder *encoders[RCAR_DU_OUTPUT_MAX];
>
> struct rcar_du_group groups[RCAR_DU_MAX_GROUPS];
> + struct platform_device *cmms[RCAR_DU_MAX_CMMS];
> struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS];
>
> struct {
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> index 87950c1f6a52..b0c1466593a3 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> @@ -22,6 +22,7 @@ struct rcar_du_device;
> * @mmio_offset: registers offset in the device memory map
> * @index: group index
> * @channels_mask: bitmask of populated DU channels in this group
> + * @cmms_mask: bitmask of enabled CMMs in this group

enabled or available ?

> * @num_crtcs: number of CRTCs in this group (1 or 2)
> * @use_count: number of users of the group (rcar_du_group_(get|put))
> * @used_crtcs: number of CRTCs currently in use
> @@ -37,6 +38,7 @@ struct rcar_du_group {
> unsigned int index;
>
> unsigned int channels_mask;
> + unsigned int cmms_mask;
> unsigned int num_crtcs;
> unsigned int use_count;
> unsigned int used_crtcs;
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index f8f7fff34dff..b79cda2f5531 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -18,6 +18,7 @@
> #include <drm/drm_vblank.h>
>
> #include <linux/of_graph.h>
> +#include <linux/of_platform.h>
> #include <linux/wait.h>
>
> #include "rcar_du_crtc.h"
> @@ -534,6 +535,51 @@ static int rcar_du_vsps_init(struct rcar_du_device *rcdu)
> return ret;
> }
>
> +static int rcar_du_cmm_init(struct rcar_du_device *rcdu)
> +{
> + const struct device_node *np = rcdu->dev->of_node;
> + unsigned int i;
> + int cells;
> +
> + cells = of_property_count_u32_elems(np, "cmms");
> + if (cells == -EINVAL)
> + return 0;
> +
> + if (cells > RCAR_DU_MAX_CMMS || cells > rcdu->num_crtcs) {

Should this be

if (cells != rcdu->num_crtcs)

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

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

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

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

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

> + of_node_put(cmm);
> + return PTR_ERR(pdev);
> + }
> +
> + if (!of_device_is_available(cmm)) {

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

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

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

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

--
Regards,

Laurent Pinchart

2019-08-20 17:59:08

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v2 17/19] drm: rcar-du: crtc: Enable and disable CMMs

Hi Jacopo,

Thank you for the patch.

On Sat, Jul 06, 2019 at 04:07:44PM +0200, Jacopo Mondi wrote:
> Enable/disable the CMM associated with a CRTC at crtc start and stop

s/crtc/CRTC/

> time and enable the CMM unit through the Display Extensional Functions
> register at group setup time.
>
> Signed-off-by: Jacopo Mondi <[email protected]>
> ---
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 7 +++++++
> drivers/gpu/drm/rcar-du/rcar_du_group.c | 8 ++++++++
> drivers/gpu/drm/rcar-du/rcar_du_regs.h | 5 +++++
> 3 files changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 23f1d6cc1719..3dac605c3a67 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -21,6 +21,7 @@
> #include <drm/drm_plane_helper.h>
> #include <drm/drm_vblank.h>
>
> +#include "rcar_cmm.h"
> #include "rcar_du_crtc.h"
> #include "rcar_du_drv.h"
> #include "rcar_du_encoder.h"
> @@ -619,6 +620,9 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
> if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> rcar_du_vsp_disable(rcrtc);
>
> + if (rcrtc->cmm)
> + rcar_cmm_disable(rcrtc->cmm);
> +
> /*
> * Select switch sync mode. This stops display operation and configures
> * the HSYNC and VSYNC signals as inputs.
> @@ -686,6 +690,9 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
> }
>
> rcar_du_crtc_start(rcrtc);
> +
> + if (rcrtc->cmm)
> + rcar_cmm_enable(rcrtc->cmm);
> }
>
> static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> index 9eee47969e77..d252c9bb9809 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> @@ -147,6 +147,14 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
>
> rcar_du_group_setup_pins(rgrp);
>
> + if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM)) {
> + u32 defr7 = DEFR7_CODE |
> + (rgrp->cmms_mask & BIT(1) ? DEFR7_CMME1 : 0) |
> + (rgrp->cmms_mask & BIT(0) ? DEFR7_CMME0 : 0);

Nitpicking, the DU driver usually puts the | at the beginning of the
next line for such assignments.

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

Apart from that,

Reviewed-by: Laurent Pinchart <[email protected]>

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

--
Regards,

Laurent Pinchart

2019-08-20 18:04:01

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v2 18/19] drm: rcar-du: crtc: Register GAMMA_LUT properties

Hi Jacopo,

Thank you for the patch.

On Sat, Jul 06, 2019 at 04:07:45PM +0200, Jacopo Mondi wrote:
> Enable the GAMMA_LUT KMS property using the framework helpers to
> register the proeprty and the associated gamma table size maximum size.

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

>
> Signed-off-by: Jacopo Mondi <[email protected]>

Reviewed-by: Laurent Pinchart <[email protected]>

> ---
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 3dac605c3a67..222ccc20d6d8 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -1082,6 +1082,7 @@ static const struct drm_crtc_funcs crtc_funcs_gen3 = {
> .set_crc_source = rcar_du_crtc_set_crc_source,
> .verify_crc_source = rcar_du_crtc_verify_crc_source,
> .get_crc_sources = rcar_du_crtc_get_crc_sources,
> + .gamma_set = drm_atomic_helper_legacy_gamma_set,
> };
>
> /* -----------------------------------------------------------------------------
> @@ -1205,6 +1206,9 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
> if (rcdu->cmms[swindex]) {
> rcrtc->cmm = rcdu->cmms[swindex];
> rgrp->cmms_mask |= BIT(hwindex % 2);
> +
> + drm_mode_crtc_set_gamma_size(crtc, CMM_GAMMA_LUT_SIZE);
> + drm_crtc_enable_color_mgmt(crtc, 0, false, CMM_GAMMA_LUT_SIZE);
> }
>
> drm_crtc_helper_add(crtc, &crtc_helper_funcs);

--
Regards,

Laurent Pinchart

2019-08-20 18:10:00

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v2 01/19] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation

Hi Laurent,

On Tue, Aug 20, 2019 at 7:41 PM Laurent Pinchart
<[email protected]> wrote:
> On Tue, Aug 20, 2019 at 09:53:44AM +0200, Geert Uytterhoeven wrote:
> > On Tue, Aug 20, 2019 at 9:47 AM Jacopo Mondi <[email protected]> wrote:
> > > On Mon, Aug 19, 2019 at 03:45:54PM +0200, Geert Uytterhoeven wrote:
> > >> On Mon, Jul 8, 2019 at 9:58 AM Geert Uytterhoeven <[email protected]> wrote:
> > >>> On Sat, Jul 6, 2019 at 4:07 PM Jacopo Mondi <[email protected]> wrote:
> > >>>> Add device tree bindings documentation for the Renesas R-Car Display
> > >>>> Unit Color Management Module.
> > >>>>
> > >>>> CMM is the image enhancement module available on each R-Car DU video
> > >>>> channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).
> > >>>>
> > >>>> Signed-off-by: Jacopo Mondi <[email protected]>
> > >>>> Reviewed-by: Laurent Pinchart <[email protected]>
> > >>>
> > >>> Thanks for your patch!
> > >>>
> > >>>> --- /dev/null
> > >>>> +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> > >>>> @@ -0,0 +1,25 @@
> > >>>> +* Renesas R-Car Color Management Module (CMM)
> > >>>> +
> > >>>> +Renesas R-Car image enhancement module connected to R-Car DU video channels.
> > >>>> +
> > >>>> +Required properties:
> > >>>> + - compatible: shall be one of:
> > >>>> + - "renesas,rcar-gen3-cmm"
> > >>>> + - "renesas,rcar-gen2-cmm"
> > >>>
> > >>> Why do you think you do not need SoC-specific compatible values?
> > >>> What if you discover a different across the R-Car Gen3 line tomorrow?
> > >>> Does the IP block have a version register?
> > >>
> > >> Do you have an answer to these questions?
> > >
> > > It does not seem to me that CMM has any version register, nor there
> > > are differences between the different Gen3 SoCs..
> > >
> > > However, even if we now define a single compatible property for
> > > gen3/gen2 and we later find out one of the SoC needs a soc-specific
> > > property we can safely add it and keep the generic gen3/gen2 one as
> > > fallback.. Does it work for you?
> >
> > Unfortunately that won't work, as the existing DTBs won't have the
> > soc-specific compatible value.
> > You could still resort to soc_device_match(), but it is better to avoid that.
>
> We've had the same discussion over and over for quite a long time :-) I
> wonder, now that we have implemented SoC-specific compatible values for
> many IP cores, how many of them have actually benefited from it ? I'm
> not considering IP cores where we knew from the start that each SoC was
> different (such as pinctrl or clocks for instance), but IP cores where
> we though all SoCs would be handled in the same way. I also wouldn't
> count ES-specific differences, as those are handled by
> soc_device_match() anyway.

Thank you for making me dive into this ;-)

For R-Car Gen3 only:

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

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

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

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

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

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

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

I may have missed some...

Convinced?

Gr{oetje,eeting}s,

Geert

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

2019-08-20 18:43:41

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v2 19/19] drm: rcar-du: kms: Update CMM in atomic commit tail

Hi Jacopo,

Thank you for the patch.

On Sat, Jul 06, 2019 at 04:07:46PM +0200, Jacopo Mondi wrote:
> Update CMM settings at in the atomic commit tail helper method.
>
> The CMM is updated with new gamma values provided to the driver
> in the GAMMA_LUT blob property.
>
> Signed-off-by: Jacopo Mondi <[email protected]>
> ---
> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 35 +++++++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index b79cda2f5531..f9aece78ca5f 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -21,6 +21,7 @@
> #include <linux/of_platform.h>
> #include <linux/wait.h>
>
> +#include "rcar_cmm.h"
> #include "rcar_du_crtc.h"
> #include "rcar_du_drv.h"
> #include "rcar_du_encoder.h"
> @@ -287,6 +288,37 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv,
> * Atomic Check and Update
> */
>
> +static void rcar_du_atomic_commit_update_cmm(struct drm_crtc *crtc,
> + struct drm_crtc_state *old_state)
> +{
> + struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> + struct rcar_cmm_config cmm_config = {};
> +
> + if (!rcrtc->cmm || !crtc->state->color_mgmt_changed)
> + return;
> +
> + if (!crtc->state->gamma_lut) {
> + cmm_config.lut.enable = false;
> + rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> +
> + return;
> + }
> +
> + cmm_config.lut.enable = true;
> + cmm_config.lut.table = (struct drm_color_lut *)
> + crtc->state->gamma_lut->data;
> +
> + /* Set LUT table size to 0 if entries should not be updated. */
> + if (!old_state->gamma_lut ||
> + old_state->gamma_lut->base.id != crtc->state->gamma_lut->base.id)
> + cmm_config.lut.size = crtc->state->gamma_lut->length
> + / sizeof(cmm_config.lut.table[0]);
> + else
> + cmm_config.lut.size = 0;
> +
> + rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> +}
> +
> static int rcar_du_atomic_check(struct drm_device *dev,
> struct drm_atomic_state *state)
> {
> @@ -329,6 +361,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
> rcdu->dpad1_source = rcrtc->index;
> }
>
> + for_each_old_crtc_in_state(old_state, crtc, crtc_state, i)
> + rcar_du_atomic_commit_update_cmm(crtc, crtc_state);
> +

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

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

--
Regards,

Laurent Pinchart

2019-08-22 14:52:17

by jacopo mondi

[permalink] [raw]
Subject: Re: [PATCH v2 14/19] drm: rcar-du: Add support for CMM

Hi Laurent,
thanks for review

On Tue, Aug 20, 2019 at 08:34:44PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Sat, Jul 06, 2019 at 04:07:41PM +0200, Jacopo Mondi wrote:
> > Add a driver for the R-Car Display Unit Color Correction Module.
> >
> > In most of Gen3 SoCs, each DU output channel is provided with a CMM unit
> > to perform image enhancement and color correction.
> >
> > Add support for CMM through a driver that supports configuration of
> > the 1-dimensional LUT table. More advanced CMM feature will be
> > implemented on top of this basic one.
> >
> > Signed-off-by: Jacopo Mondi <[email protected]>
> > ---
> > drivers/gpu/drm/rcar-du/Kconfig | 7 +
> > drivers/gpu/drm/rcar-du/Makefile | 1 +
> > drivers/gpu/drm/rcar-du/rcar_cmm.c | 292 +++++++++++++++++++++++++++++
> > drivers/gpu/drm/rcar-du/rcar_cmm.h | 38 ++++
> > 4 files changed, 338 insertions(+)
> > create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.c
> > create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.h
> >
> > diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
> > index 1529849e217e..539d232790d1 100644
> > --- a/drivers/gpu/drm/rcar-du/Kconfig
> > +++ b/drivers/gpu/drm/rcar-du/Kconfig
> > @@ -13,6 +13,13 @@ config DRM_RCAR_DU
> > Choose this option if you have an R-Car chipset.
> > If M is selected the module will be called rcar-du-drm.
> >
> > +config DRM_RCAR_CMM
> > + bool "R-Car DU Color Management Module (CMM) Support"
> > + depends on DRM && OF
> > + depends on DRM_RCAR_DU
> > + help
> > + Enable support for R-Car Color Management Module (CMM).
> > +
> > config DRM_RCAR_DW_HDMI
> > tristate "R-Car DU Gen3 HDMI Encoder Support"
> > depends on DRM && OF
> > diff --git a/drivers/gpu/drm/rcar-du/Makefile b/drivers/gpu/drm/rcar-du/Makefile
> > index 6c2ed9c46467..4d1187ccc3e5 100644
> > --- a/drivers/gpu/drm/rcar-du/Makefile
> > +++ b/drivers/gpu/drm/rcar-du/Makefile
> > @@ -15,6 +15,7 @@ rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS) += rcar_du_of.o \
> > rcar-du-drm-$(CONFIG_DRM_RCAR_VSP) += rcar_du_vsp.o
> > rcar-du-drm-$(CONFIG_DRM_RCAR_WRITEBACK) += rcar_du_writeback.o
> >
> > +obj-$(CONFIG_DRM_RCAR_CMM) += rcar_cmm.o
> > obj-$(CONFIG_DRM_RCAR_DU) += rcar-du-drm.o
> > obj-$(CONFIG_DRM_RCAR_DW_HDMI) += rcar_dw_hdmi.o
> > obj-$(CONFIG_DRM_RCAR_LVDS) += rcar_lvds.o
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> > new file mode 100644
> > index 000000000000..76ed3fce2b33
> > --- /dev/null
> > +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> > @@ -0,0 +1,292 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * rcar_cmm.c -- R-Car Display Unit Color Management Module
> > + *
> > + * Copyright (C) 2019 Jacopo Mondi <[email protected]>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm.h>
> > +
> > +#include <drm/drm_atomic.h>
>
> The only thing you need from DRM is the drm_color_lut structure, so I
> would include drm/drm_mode.h instead.
>
> > +#include "rcar_cmm.h"
> > +
> > +#define CM2_LUT_CTRL 0x0000
> > +#define CM2_LUT_CTRL_EN BIT(0)
>
> The datasheet names the bit LUT_EN, I would thus rename the macro to
> CM2_LUT_CTRL_LUT_EN.
>
> > +#define CM2_LUT_TBLA_BASE 0x0600
> > +#define CM2_LUT_TBLA(__i) (CM2_LUT_TBLA_BASE + (__i) * 4)
>
> Similarly, the datasheet names the register CM2_LUT_TBL (and the LUT
> table B is named CM2_LUT_TBL2), would it make sense to stick to those
> names ?
>

Ack on all of these

> > +
> > +struct rcar_cmm {
> > + struct clk *clk;
> > + void __iomem *base;
> > + bool enabled;
> > +
> > + /*
> > + * restore: LUT restore flag
>
> I'm none the wiser after reading this comment :-)
>
> > + * running: LUT operating flag
> > + * size: Number of programmed entries in the LUT table
> > + * table: Scratch buffer where to store the LUT table entries to be
> > + * later restored.
>
> If you want to document individual fields I propose using kerneldoc
> syntax.
>
> * @lut.restore: ...
> ...
>

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

> > + */
> > + struct {
> > + bool restore;
> > + bool running;
> > + unsigned int size;
> > + struct drm_color_lut table[CMM_GAMMA_LUT_SIZE];
> > + } lut;
>
> I think the lut.running field name is a bit confusing, as you modify it
> based on the lut.enable field in the cmm config structure. I would name
> it lut.enabled. That could then possibly be confused with cmm.enabled
> (although in my opinion that's fine), if you're concerned by that I
> would rename that field to running. It would be more logical to consider
> the CMM as a whole as running or stopped, with the LUT (and later the
> CLU) enabled or disabled.
>

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

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

> > +};
> > +
> > +static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg)
> > +{
> > + return ioread32(rcmm->base + reg);
> > +}
> > +
> > +static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data)
> > +{
> > + iowrite32(data, rcmm->base + reg);
> > +}
> > +
> > +static void rcar_cmm_lut_write(struct rcar_cmm *rcmm, unsigned int size,
>
> s/unsigned int/size_t/ ?
>
> > + struct drm_color_lut *lut)
>
> You can make this pointer const.
>

Ack

> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < size; ++i) {
> > + struct drm_color_lut *entry = &lut[i];
> > +
> > + u32 val = (entry->red & 0xff) << 16 |
> > + (entry->green & 0xff) << 8 |
> > + (entry->blue & 0xff);
> > + rcar_cmm_write(rcmm, CM2_LUT_TBLA(i), val);
> > + }
> > +}
> > +
> > +/**
> > + * rcar_cmm_setup() - configure the CMM unit
> > + *
> > + * @pdev: The platform device associated with the CMM instance
> > + * @config: The CRTC provided configuration.
> > + *
> > + * Configure the CMM unit with the CRTC provided configuration.
>
> s/CRTC provided/CRTC-provided/
>
> "CRTC-provided" is a compound adjective, qualifying the noun
> "configuration". It thus needs a hyphen. If you had written "The process
> uses the CRTC provided to this function", then no hyphen would be
> needed, as "provided" then qualifies the noun "CRTC", without the
> combination being used as an adjective.
>
> > + * Currently enabling, disabling and programming of the 1-D LUT unit is
> > + * supported.
> > + */
> > +int rcar_cmm_setup(struct platform_device *pdev,
> > + const struct rcar_cmm_config *config)
> > +{
> > + struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> > + unsigned int i;
> > +
> > + if (config->lut.size > CMM_GAMMA_LUT_SIZE)
> > + return -EINVAL;
> > +
> > + /*
> > + * As cmm_setup is called by atomic commit tail helper, it might be
>
> s/cmm_setup/rcar_cmm_setup()/ (or just "this function").
>
> > + * called before enabling the CRTC (which calls cmm_enable()).
>
> I would phrase this as "... it might be called when the CMM is disabled.
> We can't program the hardware in that case, store the configuration
> internally and apply it when the CMM will be enabled by the CRTC through
> by rcar_cmm_enable()." and remove the next comment.
>

Ack

> > + */
> > + if (!rcmm->enabled) {
> > + if (!config->lut.enable)
> > + return 0;
> > +
> > + /*
> > + * Store the LUT table entries in the scratch buffer to be later
> > + * programmed at enable time.
> > + */
> > + for (i = 0; i < config->lut.size; ++i)
> > + rcmm->lut.table[i] = config->lut.table[i];
>
> Can you do a memcpy() over the whole table ?
>
> memcpy(rcmm->lut.table, config->lut.table,
> sizeof(*rcmm->lut.table) * config.lut.size);
>

Yeah, probably better

> > +
> > + rcmm->lut.size = config->lut.size;
> > + rcmm->lut.restore = true;
> > +
> > + return 0;
> > + }
> > +
> > + /* Stop LUT operations, if requested. */
> > + if (rcmm->lut.running && !config->lut.enable) {
> > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> > + rcmm->lut.running = 0;
> > + rcmm->lut.size = 0;
> > +
> > + return 0;
> > + }
> > +
> > + /* Enable LUT and program the new gamma table values. */
> > + if (!rcmm->lut.running) {
>
> Should this be !rcmm->lut.running && config->lut.enable ? Or do you rely
> on the caller to not try to disable the LUT when it's not currently
> enabled ? If you rely on the caller than I think you should rely on it
> for the enabling case above too, and write is if (!config->lut.enabled).
> Otherwise I think you're mishandling the !running && !enable, it will
> end up enabling the LUT.
>

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

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

> > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> > + rcmm->lut.running = true;
> > + }
> > +
> > + rcar_cmm_lut_write(rcmm, config->lut.size, config->lut.table);
>
> I'm still very puzzled by the fact that you have to write the LUT
> contents after enabling the LUT. The datasheet states
>

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


> "Note that if the module that references that space is operating, read
> and write accesses to the relevant space are prohibited. In case of
> double buffer mode, referenced side of LUT is distinguished by
> CM2_CTL1.BFS."
>
> It looks to me like you may have to implement double-buffering, but even
> without that,

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

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

>
> > + rcmm->lut.size = config->lut.size;
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(rcar_cmm_setup);
> > +
> > +/**
> > + * rcar_cmm_enable - enable the CMM unit
> > + *
> > + * @pdev: The platform device associated with the CMM instance
> > + *
> > + * Enable the CMM unit by enabling the parent clock and enabling the CMM
> > + * components, such as 1-D LUT, if requested.
> > + */
> > +int rcar_cmm_enable(struct platform_device *pdev)
> > +{
> > + struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> > + int ret;
> > +
> > + if (!rcmm)
> > + return -EPROBE_DEFER;
> > +
> > + ret = clk_prepare_enable(rcmm->clk);
> > + if (ret)
> > + return ret;
> > +
>
> Didn't you say this version would use runtime PM ? :-)
>

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

> > + /* Apply the LUT table values saved at cmm_setup time. */
>
> rcar_cmm_setup() here too.
>
> > + if (rcmm->lut.restore) {
> > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> > + rcar_cmm_lut_write(rcmm, rcmm->lut.size, rcmm->lut.table);
> > +
> > + rcmm->lut.restore = false;
> > + rcmm->lut.running = true;
> > + }
> > +
> > + rcmm->enabled = true;
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(rcar_cmm_enable);
> > +
> > +/**
> > + * rcar_cmm_disable() - disable the CMM unit
> > + *
> > + * Disable the CMM unit by stopping the parent clock.
> > + *
> > + * @pdev: The platform device associated with the CMM instance
>
> Parameters usually go before the description test.
>

Indeed, sorry about this.

> > + */
> > +void rcar_cmm_disable(struct platform_device *pdev)
> > +{
> > + struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> > +
> > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> > +
> > + clk_disable_unprepare(rcmm->clk);
> > +
> > + rcmm->lut.restore = false;
> > + rcmm->lut.running = false;
> > + rcmm->lut.size = 0;
> > +
> > + rcmm->enabled = false;
> > +}
> > +EXPORT_SYMBOL_GPL(rcar_cmm_disable);
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int rcar_cmm_pm_suspend(struct device *dev)
> > +{
> > + struct rcar_cmm *rcmm = dev_get_drvdata(dev);
> > + unsigned int i;
> > +
> > + if (!(rcmm->lut.running || rcmm->lut.restore))
>
> Do you need the second part of this condition ? If a cached copy of the
> LUT data has been stored but not applied yet because the CMM is
> disabled, why would you need to overwrite that cached copy with values
> from the hardware ?

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

>
> I think you should have a first check on rcmm->enabled :
>
> if (!rcmm->enabled)
> return 0;
>
> as in that case you can't access the hardware. Then, you can save the
> LUT content only if it's running :
>
> if (rcmm->lut.running) {
> /* Save the content */
> ...
> rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> }
>
> I wouldn't clear rcmm->lut.running here, as from a software point of
> view I think we still want to record that it's enabled. There's also no
> need to touch the restore flag, see below.
>
> > + return 0;
> > +
> > + /* Save the LUT table entries in the scratch buffer table. */
>
> Should we call this a cache instead of a scratch buffer ?
>
> > + for (i = 0; i < rcmm->lut.size; ++i) {
> > + int entry = rcar_cmm_read(rcmm, CM2_LUT_TBLA(i));
> > + struct drm_color_lut *lut = &rcmm->lut.table[i];
> > +
> > + lut->blue = entry & 0xff;
> > + lut->green = (entry >> 8) & 0xff;
> > + lut->red = (entry >> 16) & 0xff;
> > + }
> > +
> > + rcmm->lut.restore = true;
> > + rcmm->lut.running = false;
> > +
> > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> > +
> > + return 0;
> > +}
> > +
> > +static int rcar_cmm_pm_resume(struct device *dev)
> > +{
> > + struct rcar_cmm *rcmm = dev_get_drvdata(dev);
> > +
> > + if (!rcmm->lut.restore)
> > + return 0;
> > +
> > + /* Program the LUT entries saved at suspend time. */
> > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> > + rcar_cmm_lut_write(rcmm, rcmm->lut.size, rcmm->lut.table);
> > + rcmm->lut.running = true;
> > + rcmm->lut.restore = false;
>
> To be kept in sync with the modifications proposed above,
>
>
> if (!rcmm->enabled)
> return 0;
>
> if (rcmm->lut.running) {
> /* Program the LUT entries saved at suspend time. */
> rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> rcar_cmm_lut_write(rcmm, rcmm->lut.size, rcmm->lut.table);
> }
>
> I think you can remove the restore field completely, as it's the only
> used in rcar_cmm_enable(), and there you can check rcmm->lut.running
> instead if you set rcmm->lut.running to true in rcar_cmm_setup() when
> the CMM is disabled and the config requests the LUT to be enabled. The
> overall logic should become simpler, with less corner cases.
>

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

> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > +static const struct dev_pm_ops rcar_cmm_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(rcar_cmm_pm_suspend, rcar_cmm_pm_resume)
> > +};
> > +
> > +static int rcar_cmm_probe(struct platform_device *pdev)
> > +{
> > + struct rcar_cmm *rcmm;
> > + struct resource *res;
> > + resource_size_t size;
> > +
> > + rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
> > + if (!rcmm)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, rcmm);
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + size = resource_size(res);
> > + if (!devm_request_mem_region(&pdev->dev, res->start, size,
> > + dev_name(&pdev->dev))) {
> > + dev_err(&pdev->dev,
> > + "can't request region for resource %pR\n", res);
> > + return -EBUSY;
> > + }
> > +
> > + rcmm->base = devm_ioremap_nocache(&pdev->dev, res->start, size);
> > + if (IS_ERR(rcmm->base))
> > + return PTR_ERR(rcmm->base);
>
> I still think you can use devm_ioremap_resource(). I agree it doesn't
> explicitly request an uncached mapping, but I think the magic happens
> behind the scene with the IO accessors to ensure it will work fine.
>

Probably, but does using the _nocache version hurt somehow ?


> > +
> > + rcmm->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(rcmm->clk)) {
> > + dev_err(&pdev->dev, "Failed to get CMM clock");
> > + return PTR_ERR(rcmm->clk);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id rcar_cmm_of_table[] = {
> > + { .compatible = "renesas,rcar-gen3-cmm", },
> > + { .compatible = "renesas,rcar-gen2-cmm", },
> > + { },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
> > +
> > +static struct platform_driver rcar_cmm_platform_driver = {
> > + .probe = rcar_cmm_probe,
> > + .driver = {
> > + .name = "rcar-cmm",
> > + .pm = &rcar_cmm_pm_ops,
> > + .of_match_table = rcar_cmm_of_table,
> > + },
> > +};
> > +
> > +module_platform_driver(rcar_cmm_platform_driver);
> > +
> > +MODULE_AUTHOR("Jacopo Mondi <[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..8744e72f32cd
> > --- /dev/null
> > +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h
> > @@ -0,0 +1,38 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * rcar_cmm.h -- R-Car Display Unit Color Management Module
> > + *
> > + * Copyright (C) 2019 Jacopo Mondi <[email protected]>
> > + */
> > +
> > +#ifndef __RCAR_CMM_H__
> > +#define __RCAR_CMM_H__
> > +
> > +#define CMM_GAMMA_LUT_SIZE 256
> > +
> > +struct platform_device;
> > +struct drm_color_lut;
>
> Could you please sort the forward declarations alphabetically ?
>
> > +
> > +/**
> > + * struct rcar_cmm_config - CMM configuration
> > + *
> > + * @lut: 1D-LUT configuration
> > + * @lut.enable: 1D-LUT enable flag
> > + * @lut.table: 1D-LUT table entries.
>
> s/\.$//
>
> > + * @lut.size 1D-LUT number of entries. Max is 256.
>
> "Number of 1D-LUT entries (max 256)"
>

And missing : after @lut.size

Thanks for review
j

> > + */
> > +struct rcar_cmm_config {
> > + struct {
> > + bool enable;
> > + struct drm_color_lut *table;
> > + unsigned int size;
> > + } lut;
> > +};
> > +
> > +int rcar_cmm_enable(struct platform_device *pdev);
> > +void rcar_cmm_disable(struct platform_device *pdev);
> > +
> > +int rcar_cmm_setup(struct platform_device *pdev,
> > + const struct rcar_cmm_config *config);
> > +
> > +#endif /* __RCAR_CMM_H__ */
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


Attachments:
(No filename) (18.99 kB)
signature.asc (849.00 B)
Download all attachments

2019-08-22 17:23:21

by jacopo mondi

[permalink] [raw]
Subject: Re: [PATCH v2 14/19] drm: rcar-du: Add support for CMM

Hi Ulrich,

On Tue, Aug 20, 2019 at 08:37:44PM +0300, Laurent Pinchart wrote:
> Hello,
>
> On Tue, Jul 16, 2019 at 03:17:04PM +0200, Ulrich Hecht wrote:
> > > On July 6, 2019 at 4:07 PM Jacopo Mondi <[email protected]> wrote:
> > >
> > > Add a driver for the R-Car Display Unit Color Correction Module.
> > >
> > > In most of Gen3 SoCs, each DU output channel is provided with a CMM unit
> > > to perform image enhancement and color correction.
> > >
> > > Add support for CMM through a driver that supports configuration of
> > > the 1-dimensional LUT table. More advanced CMM feature will be
> > > implemented on top of this basic one.
> > >
> > > Signed-off-by: Jacopo Mondi <[email protected]>
> > > ---
> > > drivers/gpu/drm/rcar-du/Kconfig | 7 +
> > > drivers/gpu/drm/rcar-du/Makefile | 1 +
> > > drivers/gpu/drm/rcar-du/rcar_cmm.c | 292 +++++++++++++++++++++++++++++
> > > drivers/gpu/drm/rcar-du/rcar_cmm.h | 38 ++++
> > > 4 files changed, 338 insertions(+)
> > > create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.c
> > > create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.h
> > >
> > > diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
> > > index 1529849e217e..539d232790d1 100644
> > > --- a/drivers/gpu/drm/rcar-du/Kconfig
> > > +++ b/drivers/gpu/drm/rcar-du/Kconfig
> > > @@ -13,6 +13,13 @@ config DRM_RCAR_DU
> > > Choose this option if you have an R-Car chipset.
> > > If M is selected the module will be called rcar-du-drm.
> > >
> > > +config DRM_RCAR_CMM
> > > + bool "R-Car DU Color Management Module (CMM) Support"
> > > + depends on DRM && OF
> > > + depends on DRM_RCAR_DU
> > > + help
> > > + Enable support for R-Car Color Management Module (CMM).
> > > +
> > > config DRM_RCAR_DW_HDMI
> > > tristate "R-Car DU Gen3 HDMI Encoder Support"
> > > depends on DRM && OF
> > > diff --git a/drivers/gpu/drm/rcar-du/Makefile b/drivers/gpu/drm/rcar-du/Makefile
> > > index 6c2ed9c46467..4d1187ccc3e5 100644
> > > --- a/drivers/gpu/drm/rcar-du/Makefile
> > > +++ b/drivers/gpu/drm/rcar-du/Makefile
> > > @@ -15,6 +15,7 @@ rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS) += rcar_du_of.o \
> > > rcar-du-drm-$(CONFIG_DRM_RCAR_VSP) += rcar_du_vsp.o
> > > rcar-du-drm-$(CONFIG_DRM_RCAR_WRITEBACK) += rcar_du_writeback.o
> > >
> > > +obj-$(CONFIG_DRM_RCAR_CMM) += rcar_cmm.o
> > > obj-$(CONFIG_DRM_RCAR_DU) += rcar-du-drm.o
> > > obj-$(CONFIG_DRM_RCAR_DW_HDMI) += rcar_dw_hdmi.o
> > > obj-$(CONFIG_DRM_RCAR_LVDS) += rcar_lvds.o
> > > diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> > > new file mode 100644
> > > index 000000000000..76ed3fce2b33
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> > > @@ -0,0 +1,292 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * rcar_cmm.c -- R-Car Display Unit Color Management Module
> > > + *
> > > + * Copyright (C) 2019 Jacopo Mondi <[email protected]>
> > > + */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/io.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/pm.h>
> > > +
> > > +#include <drm/drm_atomic.h>
> > > +
> > > +#include "rcar_cmm.h"
> > > +
> > > +#define CM2_LUT_CTRL 0x0000
> > > +#define CM2_LUT_CTRL_EN BIT(0)
> > > +#define CM2_LUT_TBLA_BASE 0x0600
> > > +#define CM2_LUT_TBLA(__i) (CM2_LUT_TBLA_BASE + (__i) * 4)
> > > +
> > > +struct rcar_cmm {
> > > + struct clk *clk;
> > > + void __iomem *base;
> > > + bool enabled;
> > > +
> > > + /*
> > > + * restore: LUT restore flag
> > > + * running: LUT operating flag
> > > + * size: Number of programmed entries in the LUT table
> > > + * table: Scratch buffer where to store the LUT table entries to be
> > > + * later restored.
> > > + */
> > > + struct {
> > > + bool restore;
> > > + bool running;
> > > + unsigned int size;
> > > + struct drm_color_lut table[CMM_GAMMA_LUT_SIZE];
> > > + } lut;
> > > +};
> > > +
> > > +static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg)
> > > +{
> > > + return ioread32(rcmm->base + reg);
> > > +}
> > > +
> > > +static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data)
> > > +{
> > > + iowrite32(data, rcmm->base + reg);
> > > +}
> > > +
> > > +static void rcar_cmm_lut_write(struct rcar_cmm *rcmm, unsigned int size,
> > > + struct drm_color_lut *lut)
> > > +{
> > > + unsigned int i;
> > > +
> > > + for (i = 0; i < size; ++i) {
> > > + struct drm_color_lut *entry = &lut[i];
> > > +
> > > + u32 val = (entry->red & 0xff) << 16 |
> > > + (entry->green & 0xff) << 8 |
> > > + (entry->blue & 0xff);
> >
> > I don't think it's correct to cut off the high bits here. There is a
> > function "drm_color_lut_extract()" for converting drm_color_lut values
> > to hardware precision.
> >

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

> > > + rcar_cmm_write(rcmm, CM2_LUT_TBLA(i), val);
> > > + }
> > > +}
> > > +
> > > +/**
> > > + * rcar_cmm_setup() - configure the CMM unit
> > > + *
> > > + * @pdev: The platform device associated with the CMM instance
> > > + * @config: The CRTC provided configuration.
> > > + *
> > > + * Configure the CMM unit with the CRTC provided configuration.
> > > + * Currently enabling, disabling and programming of the 1-D LUT unit is
> > > + * supported.
> > > + */
> > > +int rcar_cmm_setup(struct platform_device *pdev,
> > > + const struct rcar_cmm_config *config)
> > > +{
> > > + struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> > > + unsigned int i;
> > > +
> > > + if (config->lut.size > CMM_GAMMA_LUT_SIZE)
> > > + return -EINVAL;
> > > +
> > > + /*
> > > + * As cmm_setup is called by atomic commit tail helper, it might be
> > > + * called before enabling the CRTC (which calls cmm_enable()).
> > > + */
> > > + if (!rcmm->enabled) {
> > > + if (!config->lut.enable)
> > > + return 0;
> > > +
> > > + /*
> > > + * Store the LUT table entries in the scratch buffer to be later
> > > + * programmed at enable time.
> > > + */
> > > + for (i = 0; i < config->lut.size; ++i)
> > > + rcmm->lut.table[i] = config->lut.table[i];
> > > +
> > > + rcmm->lut.size = config->lut.size;
> > > + rcmm->lut.restore = true;
> > > +
> > > + return 0;
> > > + }
> > > +
> > > + /* Stop LUT operations, if requested. */
> > > + if (rcmm->lut.running && !config->lut.enable) {
> > > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> > > + rcmm->lut.running = 0;
> > > + rcmm->lut.size = 0;
> > > +
> > > + return 0;
> > > + }
> > > +
> > > + /* Enable LUT and program the new gamma table values. */
> > > + if (!rcmm->lut.running) {
> > > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> > > + rcmm->lut.running = true;
> > > + }
> > > +
> > > + rcar_cmm_lut_write(rcmm, config->lut.size, config->lut.table);
> > > + rcmm->lut.size = config->lut.size;
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(rcar_cmm_setup);
> > > +
> > > +/**
> > > + * rcar_cmm_enable - enable the CMM unit
> > > + *
> > > + * @pdev: The platform device associated with the CMM instance
> > > + *
> > > + * Enable the CMM unit by enabling the parent clock and enabling the CMM
> > > + * components, such as 1-D LUT, if requested.
> > > + */
> > > +int rcar_cmm_enable(struct platform_device *pdev)
> > > +{
> > > + struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> > > + int ret;
> > > +
> > > + if (!rcmm)
> > > + return -EPROBE_DEFER;
> > > +
> > > + ret = clk_prepare_enable(rcmm->clk);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + /* Apply the LUT table values saved at cmm_setup time. */
> > > + if (rcmm->lut.restore) {
> > > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> > > + rcar_cmm_lut_write(rcmm, rcmm->lut.size, rcmm->lut.table);
> > > +
> > > + rcmm->lut.restore = false;
> > > + rcmm->lut.running = true;
> > > + }
> > > +
> > > + rcmm->enabled = true;
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(rcar_cmm_enable);
> > > +
> > > +/**
> > > + * rcar_cmm_disable() - disable the CMM unit
> > > + *
> > > + * Disable the CMM unit by stopping the parent clock.
> > > + *
> > > + * @pdev: The platform device associated with the CMM instance
> > > + */
> > > +void rcar_cmm_disable(struct platform_device *pdev)
> > > +{
> > > + struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> > > +
> > > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> > > +
> > > + clk_disable_unprepare(rcmm->clk);
> > > +
> > > + rcmm->lut.restore = false;
> > > + rcmm->lut.running = false;
> > > + rcmm->lut.size = 0;
> > > +
> > > + rcmm->enabled = false;
> > > +}
> > > +EXPORT_SYMBOL_GPL(rcar_cmm_disable);
> > > +
> > > +#ifdef CONFIG_PM_SLEEP
> > > +static int rcar_cmm_pm_suspend(struct device *dev)
> > > +{
> > > + struct rcar_cmm *rcmm = dev_get_drvdata(dev);
> > > + unsigned int i;
> > > +
> > > + if (!(rcmm->lut.running || rcmm->lut.restore))
> > > + return 0;
> > > +
> > > + /* Save the LUT table entries in the scratch buffer table. */
> > > + for (i = 0; i < rcmm->lut.size; ++i) {
> > > + int entry = rcar_cmm_read(rcmm, CM2_LUT_TBLA(i));
> > > + struct drm_color_lut *lut = &rcmm->lut.table[i];
> > > +
> > > + lut->blue = entry & 0xff;
> > > + lut->green = (entry >> 8) & 0xff;
> > > + lut->red = (entry >> 16) & 0xff;
> >
> > Need to convert the values back to drm_color_lut scale here. I could
> > not find a counterpart to drm_color_lut_extract(), though...
>
> How about storing the LUT in hardware format internally ? I wonder if we
> should always update the cached copy, in which case rcar_cmm_lut_write()
> could just write the LUT values from the cache.
>

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

Thanks both
j

> > > + }
> > > +
> > > + rcmm->lut.restore = true;
> > > + rcmm->lut.running = false;
> > > +
> > > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int rcar_cmm_pm_resume(struct device *dev)
> > > +{
> > > + struct rcar_cmm *rcmm = dev_get_drvdata(dev);
> > > +
> > > + if (!rcmm->lut.restore)
> > > + return 0;
> > > +
> > > + /* Program the LUT entries saved at suspend time. */
> > > + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> > > + rcar_cmm_lut_write(rcmm, rcmm->lut.size, rcmm->lut.table);
> > > + rcmm->lut.running = true;
> > > + rcmm->lut.restore = false;
> > > +
> > > + return 0;
> > > +}
> > > +#endif
> > > +
> > > +static const struct dev_pm_ops rcar_cmm_pm_ops = {
> > > + SET_SYSTEM_SLEEP_PM_OPS(rcar_cmm_pm_suspend, rcar_cmm_pm_resume)
> > > +};
> > > +
> > > +static int rcar_cmm_probe(struct platform_device *pdev)
> > > +{
> > > + struct rcar_cmm *rcmm;
> > > + struct resource *res;
> > > + resource_size_t size;
> > > +
> > > + rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
> > > + if (!rcmm)
> > > + return -ENOMEM;
> > > +
> > > + platform_set_drvdata(pdev, rcmm);
> > > +
> > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > + size = resource_size(res);
> > > + if (!devm_request_mem_region(&pdev->dev, res->start, size,
> > > + dev_name(&pdev->dev))) {
> > > + dev_err(&pdev->dev,
> > > + "can't request region for resource %pR\n", res);
> > > + return -EBUSY;
> > > + }
> > > +
> > > + rcmm->base = devm_ioremap_nocache(&pdev->dev, res->start, size);
> > > + if (IS_ERR(rcmm->base))
> > > + return PTR_ERR(rcmm->base);
> > > +
> > > + rcmm->clk = devm_clk_get(&pdev->dev, NULL);
> > > + if (IS_ERR(rcmm->clk)) {
> > > + dev_err(&pdev->dev, "Failed to get CMM clock");
> > > + return PTR_ERR(rcmm->clk);
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct of_device_id rcar_cmm_of_table[] = {
> > > + { .compatible = "renesas,rcar-gen3-cmm", },
> > > + { .compatible = "renesas,rcar-gen2-cmm", },
> > > + { },
> > > +};
> > > +
> > > +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
> > > +
> > > +static struct platform_driver rcar_cmm_platform_driver = {
> > > + .probe = rcar_cmm_probe,
> > > + .driver = {
> > > + .name = "rcar-cmm",
> > > + .pm = &rcar_cmm_pm_ops,
> > > + .of_match_table = rcar_cmm_of_table,
> > > + },
> > > +};
> > > +
> > > +module_platform_driver(rcar_cmm_platform_driver);
> > > +
> > > +MODULE_AUTHOR("Jacopo Mondi <[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..8744e72f32cd
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h
> > > @@ -0,0 +1,38 @@
> > > +/* SPDX-License-Identifier: GPL-2.0+ */
> > > +/*
> > > + * rcar_cmm.h -- R-Car Display Unit Color Management Module
> > > + *
> > > + * Copyright (C) 2019 Jacopo Mondi <[email protected]>
> > > + */
> > > +
> > > +#ifndef __RCAR_CMM_H__
> > > +#define __RCAR_CMM_H__
> > > +
> > > +#define CMM_GAMMA_LUT_SIZE 256
> > > +
> > > +struct platform_device;
> > > +struct drm_color_lut;
> > > +
> > > +/**
> > > + * struct rcar_cmm_config - CMM configuration
> > > + *
> > > + * @lut: 1D-LUT configuration
> > > + * @lut.enable: 1D-LUT enable flag
> > > + * @lut.table: 1D-LUT table entries.
> > > + * @lut.size 1D-LUT number of entries. Max is 256.
> > > + */
> > > +struct rcar_cmm_config {
> > > + struct {
> > > + bool enable;
> > > + struct drm_color_lut *table;
> > > + unsigned int size;
> > > + } lut;
> > > +};
> > > +
> > > +int rcar_cmm_enable(struct platform_device *pdev);
> > > +void rcar_cmm_disable(struct platform_device *pdev);
> > > +
> > > +int rcar_cmm_setup(struct platform_device *pdev,
> > > + const struct rcar_cmm_config *config);
> > > +
> > > +#endif /* __RCAR_CMM_H__ */
>
> --
> Regards,
>
> Laurent Pinchart


Attachments:
(No filename) (14.00 kB)
signature.asc (849.00 B)
Download all attachments

2019-08-22 19:46:26

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v2 14/19] drm: rcar-du: Add support for CMM

Hi Jacopo,

On Thu, Aug 22, 2019 at 03:01:46PM +0200, Jacopo Mondi wrote:
> On Tue, Aug 20, 2019 at 08:34:44PM +0300, Laurent Pinchart wrote:
> > On Sat, Jul 06, 2019 at 04:07:41PM +0200, Jacopo Mondi wrote:
> >> Add a driver for the R-Car Display Unit Color Correction Module.
> >>
> >> In most of Gen3 SoCs, each DU output channel is provided with a CMM unit
> >> to perform image enhancement and color correction.
> >>
> >> Add support for CMM through a driver that supports configuration of
> >> the 1-dimensional LUT table. More advanced CMM feature will be
> >> implemented on top of this basic one.
> >>
> >> Signed-off-by: Jacopo Mondi <[email protected]>
> >> ---
> >> drivers/gpu/drm/rcar-du/Kconfig | 7 +
> >> drivers/gpu/drm/rcar-du/Makefile | 1 +
> >> drivers/gpu/drm/rcar-du/rcar_cmm.c | 292 +++++++++++++++++++++++++++++
> >> drivers/gpu/drm/rcar-du/rcar_cmm.h | 38 ++++
> >> 4 files changed, 338 insertions(+)
> >> create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.c
> >> create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.h
> >>
> >> diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
> >> index 1529849e217e..539d232790d1 100644
> >> --- a/drivers/gpu/drm/rcar-du/Kconfig
> >> +++ b/drivers/gpu/drm/rcar-du/Kconfig
> >> @@ -13,6 +13,13 @@ config DRM_RCAR_DU
> >> Choose this option if you have an R-Car chipset.
> >> If M is selected the module will be called rcar-du-drm.
> >>
> >> +config DRM_RCAR_CMM
> >> + bool "R-Car DU Color Management Module (CMM) Support"
> >> + depends on DRM && OF
> >> + depends on DRM_RCAR_DU
> >> + help
> >> + Enable support for R-Car Color Management Module (CMM).
> >> +
> >> config DRM_RCAR_DW_HDMI
> >> tristate "R-Car DU Gen3 HDMI Encoder Support"
> >> depends on DRM && OF
> >> diff --git a/drivers/gpu/drm/rcar-du/Makefile b/drivers/gpu/drm/rcar-du/Makefile
> >> index 6c2ed9c46467..4d1187ccc3e5 100644
> >> --- a/drivers/gpu/drm/rcar-du/Makefile
> >> +++ b/drivers/gpu/drm/rcar-du/Makefile
> >> @@ -15,6 +15,7 @@ rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS) += rcar_du_of.o \
> >> rcar-du-drm-$(CONFIG_DRM_RCAR_VSP) += rcar_du_vsp.o
> >> rcar-du-drm-$(CONFIG_DRM_RCAR_WRITEBACK) += rcar_du_writeback.o
> >>
> >> +obj-$(CONFIG_DRM_RCAR_CMM) += rcar_cmm.o
> >> obj-$(CONFIG_DRM_RCAR_DU) += rcar-du-drm.o
> >> obj-$(CONFIG_DRM_RCAR_DW_HDMI) += rcar_dw_hdmi.o
> >> obj-$(CONFIG_DRM_RCAR_LVDS) += rcar_lvds.o
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> >> new file mode 100644
> >> index 000000000000..76ed3fce2b33
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c
> >> @@ -0,0 +1,292 @@
> >> +// SPDX-License-Identifier: GPL-2.0+
> >> +/*
> >> + * rcar_cmm.c -- R-Car Display Unit Color Management Module
> >> + *
> >> + * Copyright (C) 2019 Jacopo Mondi <[email protected]>
> >> + */
> >> +
> >> +#include <linux/clk.h>
> >> +#include <linux/io.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/pm.h>
> >> +
> >> +#include <drm/drm_atomic.h>
> >
> > The only thing you need from DRM is the drm_color_lut structure, so I
> > would include drm/drm_mode.h instead.
> >
> >> +#include "rcar_cmm.h"
> >> +
> >> +#define CM2_LUT_CTRL 0x0000
> >> +#define CM2_LUT_CTRL_EN BIT(0)
> >
> > The datasheet names the bit LUT_EN, I would thus rename the macro to
> > CM2_LUT_CTRL_LUT_EN.
> >
> >> +#define CM2_LUT_TBLA_BASE 0x0600
> >> +#define CM2_LUT_TBLA(__i) (CM2_LUT_TBLA_BASE + (__i) * 4)
> >
> > Similarly, the datasheet names the register CM2_LUT_TBL (and the LUT
> > table B is named CM2_LUT_TBL2), would it make sense to stick to those
> > names ?
>
> Ack on all of these
>
> >> +
> >> +struct rcar_cmm {
> >> + struct clk *clk;
> >> + void __iomem *base;
> >> + bool enabled;
> >> +
> >> + /*
> >> + * restore: LUT restore flag
> >
> > I'm none the wiser after reading this comment :-)
> >
> >> + * running: LUT operating flag
> >> + * size: Number of programmed entries in the LUT table
> >> + * table: Scratch buffer where to store the LUT table entries to be
> >> + * later restored.
> >
> > If you want to document individual fields I propose using kerneldoc
> > syntax.
> >
> > * @lut.restore: ...
> > ...
>
> Yeah, might be a bit of over-documentation here. I don't mind it to be
> honest, but I'm fine if someone wants this to be dropped.

I think it's important to document all fields.

> >> + */
> >> + struct {
> >> + bool restore;
> >> + bool running;
> >> + unsigned int size;
> >> + struct drm_color_lut table[CMM_GAMMA_LUT_SIZE];
> >> + } lut;
> >
> > I think the lut.running field name is a bit confusing, as you modify it
> > based on the lut.enable field in the cmm config structure. I would name
> > it lut.enabled. That could then possibly be confused with cmm.enabled
> > (although in my opinion that's fine), if you're concerned by that I
> > would rename that field to running. It would be more logical to consider
> > the CMM as a whole as running or stopped, with the LUT (and later the
> > CLU) enabled or disabled.
>
> I'm a bit bothered that we would have
> rcmm.enabled
> rcmm.lut.enabled
> rcmm_config.lut.enable
>
> I'll see how it looks. enabled is probably the right name for all of
> these fields, but it might get confusing...

As long as we keep cmm->enabled and cmm->lut.enabled in the code (and
don't create alias local variables for cmm of cmm->lut with confusing
names such as dev for instance) I think it will be OK.

> >> +};
> >> +
> >> +static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg)
> >> +{
> >> + return ioread32(rcmm->base + reg);
> >> +}
> >> +
> >> +static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data)
> >> +{
> >> + iowrite32(data, rcmm->base + reg);
> >> +}
> >> +
> >> +static void rcar_cmm_lut_write(struct rcar_cmm *rcmm, unsigned int size,
> >
> > s/unsigned int/size_t/ ?
> >
> >> + struct drm_color_lut *lut)
> >
> > You can make this pointer const.
>
> Ack
>
> >> +{
> >> + unsigned int i;
> >> +
> >> + for (i = 0; i < size; ++i) {
> >> + struct drm_color_lut *entry = &lut[i];
> >> +
> >> + u32 val = (entry->red & 0xff) << 16 |
> >> + (entry->green & 0xff) << 8 |
> >> + (entry->blue & 0xff);
> >> + rcar_cmm_write(rcmm, CM2_LUT_TBLA(i), val);
> >> + }
> >> +}
> >> +
> >> +/**
> >> + * rcar_cmm_setup() - configure the CMM unit
> >> + *
> >> + * @pdev: The platform device associated with the CMM instance
> >> + * @config: The CRTC provided configuration.
> >> + *
> >> + * Configure the CMM unit with the CRTC provided configuration.
> >
> > s/CRTC provided/CRTC-provided/
> >
> > "CRTC-provided" is a compound adjective, qualifying the noun
> > "configuration". It thus needs a hyphen. If you had written "The process
> > uses the CRTC provided to this function", then no hyphen would be
> > needed, as "provided" then qualifies the noun "CRTC", without the
> > combination being used as an adjective.
> >
> >> + * Currently enabling, disabling and programming of the 1-D LUT unit is
> >> + * supported.
> >> + */
> >> +int rcar_cmm_setup(struct platform_device *pdev,
> >> + const struct rcar_cmm_config *config)
> >> +{
> >> + struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> >> + unsigned int i;
> >> +
> >> + if (config->lut.size > CMM_GAMMA_LUT_SIZE)
> >> + return -EINVAL;
> >> +
> >> + /*
> >> + * As cmm_setup is called by atomic commit tail helper, it might be
> >
> > s/cmm_setup/rcar_cmm_setup()/ (or just "this function").
> >
> >> + * called before enabling the CRTC (which calls cmm_enable()).
> >
> > I would phrase this as "... it might be called when the CMM is disabled.
> > We can't program the hardware in that case, store the configuration
> > internally and apply it when the CMM will be enabled by the CRTC through
> > by rcar_cmm_enable()." and remove the next comment.
>
> Ack
>
> >> + */
> >> + if (!rcmm->enabled) {
> >> + if (!config->lut.enable)
> >> + return 0;
> >> +
> >> + /*
> >> + * Store the LUT table entries in the scratch buffer to be later
> >> + * programmed at enable time.
> >> + */
> >> + for (i = 0; i < config->lut.size; ++i)
> >> + rcmm->lut.table[i] = config->lut.table[i];
> >
> > Can you do a memcpy() over the whole table ?
> >
> > memcpy(rcmm->lut.table, config->lut.table,
> > sizeof(*rcmm->lut.table) * config.lut.size);
>
> Yeah, probably better
>
> >> +
> >> + rcmm->lut.size = config->lut.size;
> >> + rcmm->lut.restore = true;
> >> +
> >> + return 0;
> >> + }
> >> +
> >> + /* Stop LUT operations, if requested. */
> >> + if (rcmm->lut.running && !config->lut.enable) {
> >> + rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> >> + rcmm->lut.running = 0;
> >> + rcmm->lut.size = 0;
> >> +
> >> + return 0;
> >> + }
> >> +
> >> + /* Enable LUT and program the new gamma table values. */
> >> + if (!rcmm->lut.running) {
> >
> > Should this be !rcmm->lut.running && config->lut.enable ? Or do you rely
> > on the caller to not try to disable the LUT when it's not currently
> > enabled ? If you rely on the caller than I think you should rely on it
> > for the enabling case above too, and write is if (!config->lut.enabled).
> > Otherwise I think you're mishandling the !running && !enable, it will
> > end up enabling the LUT.
>
> I think it's fine, as if (!rcmm->enable) then (!rcmm->lut.running) so
> the (!running && !enable) you have pointed out gets caught by the very
> first condition check here above (!rcmm->enabled).
>
> I'll try to restructure this to be more readable and as you suggested
> get rid of the restore field.
>
> >> + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> >> + rcmm->lut.running = true;
> >> + }
> >> +
> >> + rcar_cmm_lut_write(rcmm, config->lut.size, config->lut.table);
> >
> > I'm still very puzzled by the fact that you have to write the LUT
> > contents after enabling the LUT. The datasheet states
>
> I know -\_(;.;)_/- (is this the crying cthulhu emoticon ?)
>
> > "Note that if the module that references that space is operating, read
> > and write accesses to the relevant space are prohibited. In case of
> > double buffer mode, referenced side of LUT is distinguished by
> > CM2_CTL1.BFS."
> >
> > It looks to me like you may have to implement double-buffering, but even
> > without that,
>
> I think you have left out the end of the sentence, but I agree that
> what the manual reports suggests the table should be programmed when
> it is not operating, but it also mentions double buffering, so in case
> we're using single buffer mode maybe this does not apply?

I think it does, I'm sorry :-)

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

Then please add a FIXME comment explaining that this is weird and needs
to be figured out.

> >> + rcmm->lut.size = config->lut.size;
> >> +
> >> + return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(rcar_cmm_setup);
> >> +
> >> +/**
> >> + * rcar_cmm_enable - enable the CMM unit
> >> + *
> >> + * @pdev: The platform device associated with the CMM instance
> >> + *
> >> + * Enable the CMM unit by enabling the parent clock and enabling the CMM
> >> + * components, such as 1-D LUT, if requested.
> >> + */
> >> +int rcar_cmm_enable(struct platform_device *pdev)
> >> +{
> >> + struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> >> + int ret;
> >> +
> >> + if (!rcmm)
> >> + return -EPROBE_DEFER;
> >> +
> >> + ret = clk_prepare_enable(rcmm->clk);
> >> + if (ret)
> >> + return ret;
> >> +
> >
> > Didn't you say this version would use runtime PM ? :-)
>
> Ups. I do for suspend/resume not for runtime_suspend/resume. It will
> be trivial to add.
>
> >> + /* Apply the LUT table values saved at cmm_setup time. */
> >
> > rcar_cmm_setup() here too.
> >
> >> + if (rcmm->lut.restore) {
> >> + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> >> + rcar_cmm_lut_write(rcmm, rcmm->lut.size, rcmm->lut.table);
> >> +
> >> + rcmm->lut.restore = false;
> >> + rcmm->lut.running = true;
> >> + }
> >> +
> >> + rcmm->enabled = true;
> >> +
> >> + return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(rcar_cmm_enable);
> >> +
> >> +/**
> >> + * rcar_cmm_disable() - disable the CMM unit
> >> + *
> >> + * Disable the CMM unit by stopping the parent clock.
> >> + *
> >> + * @pdev: The platform device associated with the CMM instance
> >
> > Parameters usually go before the description test.
>
> Indeed, sorry about this.
>
> >> + */
> >> +void rcar_cmm_disable(struct platform_device *pdev)
> >> +{
> >> + struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> >> +
> >> + rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> >> +
> >> + clk_disable_unprepare(rcmm->clk);
> >> +
> >> + rcmm->lut.restore = false;
> >> + rcmm->lut.running = false;
> >> + rcmm->lut.size = 0;
> >> +
> >> + rcmm->enabled = false;
> >> +}
> >> +EXPORT_SYMBOL_GPL(rcar_cmm_disable);
> >> +
> >> +#ifdef CONFIG_PM_SLEEP
> >> +static int rcar_cmm_pm_suspend(struct device *dev)
> >> +{
> >> + struct rcar_cmm *rcmm = dev_get_drvdata(dev);
> >> + unsigned int i;
> >> +
> >> + if (!(rcmm->lut.running || rcmm->lut.restore))
> >
> > Do you need the second part of this condition ? If a cached copy of the
> > LUT data has been stored but not applied yet because the CMM is
> > disabled, why would you need to overwrite that cached copy with values
> > from the hardware ?
>
> You are right, the second part of the condition is not needed (if not
> wrong).
>
> > I think you should have a first check on rcmm->enabled :
> >
> > if (!rcmm->enabled)
> > return 0;
> >
> > as in that case you can't access the hardware. Then, you can save the
> > LUT content only if it's running :
> >
> > if (rcmm->lut.running) {
> > /* Save the content */
> > ...
> > rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> > }
> >
> > I wouldn't clear rcmm->lut.running here, as from a software point of
> > view I think we still want to record that it's enabled. There's also no
> > need to touch the restore flag, see below.
> >
> >> + return 0;
> >> +
> >> + /* Save the LUT table entries in the scratch buffer table. */
> >
> > Should we call this a cache instead of a scratch buffer ?
> >
> >> + for (i = 0; i < rcmm->lut.size; ++i) {
> >> + int entry = rcar_cmm_read(rcmm, CM2_LUT_TBLA(i));
> >> + struct drm_color_lut *lut = &rcmm->lut.table[i];
> >> +
> >> + lut->blue = entry & 0xff;
> >> + lut->green = (entry >> 8) & 0xff;
> >> + lut->red = (entry >> 16) & 0xff;
> >> + }
> >> +
> >> + rcmm->lut.restore = true;
> >> + rcmm->lut.running = false;
> >> +
> >> + rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int rcar_cmm_pm_resume(struct device *dev)
> >> +{
> >> + struct rcar_cmm *rcmm = dev_get_drvdata(dev);
> >> +
> >> + if (!rcmm->lut.restore)
> >> + return 0;
> >> +
> >> + /* Program the LUT entries saved at suspend time. */
> >> + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> >> + rcar_cmm_lut_write(rcmm, rcmm->lut.size, rcmm->lut.table);
> >> + rcmm->lut.running = true;
> >> + rcmm->lut.restore = false;
> >
> > To be kept in sync with the modifications proposed above,
> >
> >
> > if (!rcmm->enabled)
> > return 0;
> >
> > if (rcmm->lut.running) {
> > /* Program the LUT entries saved at suspend time. */
> > rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_EN);
> > rcar_cmm_lut_write(rcmm, rcmm->lut.size, rcmm->lut.table);
> > }
> >
> > I think you can remove the restore field completely, as it's the only
> > used in rcar_cmm_enable(), and there you can check rcmm->lut.running
> > instead if you set rcmm->lut.running to true in rcar_cmm_setup() when
> > the CMM is disabled and the config requests the LUT to be enabled. The
> > overall logic should become simpler, with less corner cases.
>
> Thanks, very good suggestion, I probably don't need any restore flag
> at all. I'll see how it looks like, thanks.
>
> >> +
> >> + return 0;
> >> +}
> >> +#endif
> >> +
> >> +static const struct dev_pm_ops rcar_cmm_pm_ops = {
> >> + SET_SYSTEM_SLEEP_PM_OPS(rcar_cmm_pm_suspend, rcar_cmm_pm_resume)
> >> +};
> >> +
> >> +static int rcar_cmm_probe(struct platform_device *pdev)
> >> +{
> >> + struct rcar_cmm *rcmm;
> >> + struct resource *res;
> >> + resource_size_t size;
> >> +
> >> + rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
> >> + if (!rcmm)
> >> + return -ENOMEM;
> >> +
> >> + platform_set_drvdata(pdev, rcmm);
> >> +
> >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> + size = resource_size(res);
> >> + if (!devm_request_mem_region(&pdev->dev, res->start, size,
> >> + dev_name(&pdev->dev))) {
> >> + dev_err(&pdev->dev,
> >> + "can't request region for resource %pR\n", res);
> >> + return -EBUSY;
> >> + }
> >> +
> >> + rcmm->base = devm_ioremap_nocache(&pdev->dev, res->start, size);
> >> + if (IS_ERR(rcmm->base))
> >> + return PTR_ERR(rcmm->base);
> >
> > I still think you can use devm_ioremap_resource(). I agree it doesn't
> > explicitly request an uncached mapping, but I think the magic happens
> > behind the scene with the IO accessors to ensure it will work fine.
>
> Probably, but does using the _nocache version hurt somehow ?

Probably not, but it's more code :-) kmalloc + memset doesn't hurt
either, but kzalloc is still preferred.

> >> +
> >> + rcmm->clk = devm_clk_get(&pdev->dev, NULL);
> >> + if (IS_ERR(rcmm->clk)) {
> >> + dev_err(&pdev->dev, "Failed to get CMM clock");
> >> + return PTR_ERR(rcmm->clk);
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static const struct of_device_id rcar_cmm_of_table[] = {
> >> + { .compatible = "renesas,rcar-gen3-cmm", },
> >> + { .compatible = "renesas,rcar-gen2-cmm", },
> >> + { },
> >> +};
> >> +
> >> +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
> >> +
> >> +static struct platform_driver rcar_cmm_platform_driver = {
> >> + .probe = rcar_cmm_probe,
> >> + .driver = {
> >> + .name = "rcar-cmm",
> >> + .pm = &rcar_cmm_pm_ops,
> >> + .of_match_table = rcar_cmm_of_table,
> >> + },
> >> +};
> >> +
> >> +module_platform_driver(rcar_cmm_platform_driver);
> >> +
> >> +MODULE_AUTHOR("Jacopo Mondi <[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..8744e72f32cd
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h
> >> @@ -0,0 +1,38 @@
> >> +/* SPDX-License-Identifier: GPL-2.0+ */
> >> +/*
> >> + * rcar_cmm.h -- R-Car Display Unit Color Management Module
> >> + *
> >> + * Copyright (C) 2019 Jacopo Mondi <[email protected]>
> >> + */
> >> +
> >> +#ifndef __RCAR_CMM_H__
> >> +#define __RCAR_CMM_H__
> >> +
> >> +#define CMM_GAMMA_LUT_SIZE 256
> >> +
> >> +struct platform_device;
> >> +struct drm_color_lut;
> >
> > Could you please sort the forward declarations alphabetically ?
> >
> >> +
> >> +/**
> >> + * struct rcar_cmm_config - CMM configuration
> >> + *
> >> + * @lut: 1D-LUT configuration
> >> + * @lut.enable: 1D-LUT enable flag
> >> + * @lut.table: 1D-LUT table entries.
> >
> > s/\.$//
> >
> >> + * @lut.size 1D-LUT number of entries. Max is 256.
> >
> > "Number of 1D-LUT entries (max 256)"
> >
>
> And missing : after @lut.size
>
> >> + */
> >> +struct rcar_cmm_config {
> >> + struct {
> >> + bool enable;
> >> + struct drm_color_lut *table;
> >> + unsigned int size;
> >> + } lut;
> >> +};
> >> +
> >> +int rcar_cmm_enable(struct platform_device *pdev);
> >> +void rcar_cmm_disable(struct platform_device *pdev);
> >> +
> >> +int rcar_cmm_setup(struct platform_device *pdev,
> >> + const struct rcar_cmm_config *config);
> >> +
> >> +#endif /* __RCAR_CMM_H__ */

--
Regards,

Laurent Pinchart

2019-08-23 07:28:10

by jacopo mondi

[permalink] [raw]
Subject: Re: [PATCH v2 16/19] drm: rcar-du: kms: Collect CMM instances

Hi Laurent,

On Tue, Aug 20, 2019 at 08:54:57PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Sat, Jul 06, 2019 at 04:07:43PM +0200, Jacopo Mondi wrote:
> > Implement device tree parsing to collect the available CMM instances
> > described by the 'cmms' property. Associate CMMs with CRTCs and store a
> > mask of active CMMs in the DU group for later enablement.
> >
> > Signed-off-by: Jacopo Mondi <[email protected]>
> > ---
> > drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 6 +++
> > drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 +
> > drivers/gpu/drm/rcar-du/rcar_du_drv.h | 3 ++
> > drivers/gpu/drm/rcar-du/rcar_du_group.h | 2 +
> > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 50 +++++++++++++++++++++++++
> > 5 files changed, 63 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > index 2da46e3dc4ae..23f1d6cc1719 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > @@ -1194,6 +1194,12 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
> > if (ret < 0)
> > return ret;
> >
> > + /* CMM might be disabled for this CRTC. */
> > + if (rcdu->cmms[swindex]) {
> > + rcrtc->cmm = rcdu->cmms[swindex];
> > + rgrp->cmms_mask |= BIT(hwindex % 2);
> > + }
> > +
> > drm_crtc_helper_add(crtc, &crtc_helper_funcs);
> >
> > /* Start with vertical blanking interrupt reporting disabled. */
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > index 3b7fc668996f..5f2940c42225 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > @@ -39,6 +39,7 @@ struct rcar_du_vsp;
> > * @vblank_wait: wait queue used to signal vertical blanking
> > * @vblank_count: number of vertical blanking interrupts to wait for
> > * @group: CRTC group this CRTC belongs to
> > + * @cmm: CMM associated with this CRTC
> > * @vsp: VSP feeding video to this CRTC
> > * @vsp_pipe: index of the VSP pipeline feeding video to this CRTC
> > * @writeback: the writeback connector
> > @@ -64,6 +65,7 @@ struct rcar_du_crtc {
> > unsigned int vblank_count;
> >
> > struct rcar_du_group *group;
> > + struct platform_device *cmm;
> > struct rcar_du_vsp *vsp;
> > unsigned int vsp_pipe;
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > index a00dccc447aa..300ec60ba31b 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > @@ -13,6 +13,7 @@
> > #include <linux/kernel.h>
> > #include <linux/wait.h>
> >
> > +#include "rcar_cmm.h"
> > #include "rcar_du_crtc.h"
> > #include "rcar_du_group.h"
> > #include "rcar_du_vsp.h"
> > @@ -70,6 +71,7 @@ struct rcar_du_device_info {
> >
> > #define RCAR_DU_MAX_CRTCS 4
> > #define RCAR_DU_MAX_GROUPS DIV_ROUND_UP(RCAR_DU_MAX_CRTCS, 2)
> > +#define RCAR_DU_MAX_CMMS 4
> > #define RCAR_DU_MAX_VSPS 4
> >
> > struct rcar_du_device {
> > @@ -86,6 +88,7 @@ struct rcar_du_device {
> > struct rcar_du_encoder *encoders[RCAR_DU_OUTPUT_MAX];
> >
> > struct rcar_du_group groups[RCAR_DU_MAX_GROUPS];
> > + struct platform_device *cmms[RCAR_DU_MAX_CMMS];
> > struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS];
> >
> > struct {
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> > index 87950c1f6a52..b0c1466593a3 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_group.h
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> > @@ -22,6 +22,7 @@ struct rcar_du_device;
> > * @mmio_offset: registers offset in the device memory map
> > * @index: group index
> > * @channels_mask: bitmask of populated DU channels in this group
> > + * @cmms_mask: bitmask of enabled CMMs in this group
>
> enabled or available ?
>

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

> > * @num_crtcs: number of CRTCs in this group (1 or 2)
> > * @use_count: number of users of the group (rcar_du_group_(get|put))
> > * @used_crtcs: number of CRTCs currently in use
> > @@ -37,6 +38,7 @@ struct rcar_du_group {
> > unsigned int index;
> >
> > unsigned int channels_mask;
> > + unsigned int cmms_mask;
> > unsigned int num_crtcs;
> > unsigned int use_count;
> > unsigned int used_crtcs;
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > index f8f7fff34dff..b79cda2f5531 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > @@ -18,6 +18,7 @@
> > #include <drm/drm_vblank.h>
> >
> > #include <linux/of_graph.h>
> > +#include <linux/of_platform.h>
> > #include <linux/wait.h>
> >
> > #include "rcar_du_crtc.h"
> > @@ -534,6 +535,51 @@ static int rcar_du_vsps_init(struct rcar_du_device *rcdu)
> > return ret;
> > }
> >
> > +static int rcar_du_cmm_init(struct rcar_du_device *rcdu)
> > +{
> > + const struct device_node *np = rcdu->dev->of_node;
> > + unsigned int i;
> > + int cells;
> > +
> > + cells = of_property_count_u32_elems(np, "cmms");
> > + if (cells == -EINVAL)
> > + return 0;
> > +
> > + if (cells > RCAR_DU_MAX_CMMS || cells > rcdu->num_crtcs) {
>
> Should this be
>
> if (cells != rcdu->num_crtcs)
>
> or do we want to support cases where not all DU channels have a CMM in
> DT ?
>

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

> > + dev_err(rcdu->dev, "Invalid 'cmms' property format\n");
>
> How about "Invalid number of entries in 'cmms' property" ?
>

Ok

> > + return -EINVAL;
> > + }
> > +
> > + for (i = 0; i < cells; ++i) {
> > + struct platform_device *pdev;
> > + struct device_node *cmm;
> > +
> > + cmm = of_parse_phandle(np, "cmms", i);
> > + if (IS_ERR(cmm)) {
> > + dev_err(rcdu->dev, "Failed to parse 'cmms' property\n");
> > + return PTR_ERR(cmm);
> > + }
> > +
> > + pdev = of_find_device_by_node(cmm);
> > + if (IS_ERR(pdev)) {
> > + dev_err(rcdu->dev, "No device found for cmms[%u]\n", i);
>
> s/cmms[%u]/CMM%u/ ?
>
> > + of_node_put(cmm);
> > + return PTR_ERR(pdev);
> > + }
> > +
> > + if (!of_device_is_available(cmm)) {
>
> Should this come before the pdev check, as there will be no pdev in that
> case ?
>

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

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

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

Thanks
j

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


Attachments:
(No filename) (7.53 kB)
signature.asc (849.00 B)
Download all attachments

2019-08-23 07:35:21

by jacopo mondi

[permalink] [raw]
Subject: Re: [PATCH v2 19/19] drm: rcar-du: kms: Update CMM in atomic commit tail

Hi Laurent,

On Tue, Aug 20, 2019 at 09:42:15PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Sat, Jul 06, 2019 at 04:07:46PM +0200, Jacopo Mondi wrote:
> > Update CMM settings at in the atomic commit tail helper method.
> >
> > The CMM is updated with new gamma values provided to the driver
> > in the GAMMA_LUT blob property.
> >
> > Signed-off-by: Jacopo Mondi <[email protected]>
> > ---
> > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 35 +++++++++++++++++++++++++++
> > 1 file changed, 35 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > index b79cda2f5531..f9aece78ca5f 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > @@ -21,6 +21,7 @@
> > #include <linux/of_platform.h>
> > #include <linux/wait.h>
> >
> > +#include "rcar_cmm.h"
> > #include "rcar_du_crtc.h"
> > #include "rcar_du_drv.h"
> > #include "rcar_du_encoder.h"
> > @@ -287,6 +288,37 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv,
> > * Atomic Check and Update
> > */
> >
> > +static void rcar_du_atomic_commit_update_cmm(struct drm_crtc *crtc,
> > + struct drm_crtc_state *old_state)
> > +{
> > + struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> > + struct rcar_cmm_config cmm_config = {};
> > +
> > + if (!rcrtc->cmm || !crtc->state->color_mgmt_changed)
> > + return;
> > +
> > + if (!crtc->state->gamma_lut) {
> > + cmm_config.lut.enable = false;
> > + rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> > +
> > + return;
> > + }
> > +
> > + cmm_config.lut.enable = true;
> > + cmm_config.lut.table = (struct drm_color_lut *)
> > + crtc->state->gamma_lut->data;
> > +
> > + /* Set LUT table size to 0 if entries should not be updated. */
> > + if (!old_state->gamma_lut ||
> > + old_state->gamma_lut->base.id != crtc->state->gamma_lut->base.id)
> > + cmm_config.lut.size = crtc->state->gamma_lut->length
> > + / sizeof(cmm_config.lut.table[0]);
> > + else
> > + cmm_config.lut.size = 0;
> > +
> > + rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> > +}
> > +
> > static int rcar_du_atomic_check(struct drm_device *dev,
> > struct drm_atomic_state *state)
> > {
> > @@ -329,6 +361,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
> > rcdu->dpad1_source = rcrtc->index;
> > }
> >
> > + for_each_old_crtc_in_state(old_state, crtc, crtc_state, i)
> > + rcar_du_atomic_commit_update_cmm(crtc, crtc_state);
> > +
>
> I think this looks good overall, but I wonder if we couldn't simplify
> the CMM driver suspend/resume and LUT caching due to config while not
> enabled by handling it on the DU side. I have a rework on the commit
> tail handler in progress, I'll think how this could be done. For now I
> think you can leave it as is.
>

Does this mean I have your R-b tag ? :)

Thanks
j

> > /* Apply the atomic update. */
> > drm_atomic_helper_commit_modeset_disables(dev, old_state);
> > drm_atomic_helper_commit_planes(dev, old_state,
>
> --
> Regards,
>
> Laurent Pinchart


Attachments:
(No filename) (3.14 kB)
signature.asc (849.00 B)
Download all attachments

2019-08-23 07:59:11

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v2 19/19] drm: rcar-du: kms: Update CMM in atomic commit tail

Hi Jacopo,

On Thu, Aug 22, 2019 at 09:19:25PM +0200, Jacopo Mondi wrote:
> On Tue, Aug 20, 2019 at 09:42:15PM +0300, Laurent Pinchart wrote:
> > On Sat, Jul 06, 2019 at 04:07:46PM +0200, Jacopo Mondi wrote:
> > > Update CMM settings at in the atomic commit tail helper method.
> > >
> > > The CMM is updated with new gamma values provided to the driver
> > > in the GAMMA_LUT blob property.
> > >
> > > Signed-off-by: Jacopo Mondi <[email protected]>
> > > ---
> > > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 35 +++++++++++++++++++++++++++
> > > 1 file changed, 35 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > > index b79cda2f5531..f9aece78ca5f 100644
> > > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > > @@ -21,6 +21,7 @@
> > > #include <linux/of_platform.h>
> > > #include <linux/wait.h>
> > >
> > > +#include "rcar_cmm.h"
> > > #include "rcar_du_crtc.h"
> > > #include "rcar_du_drv.h"
> > > #include "rcar_du_encoder.h"
> > > @@ -287,6 +288,37 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv,
> > > * Atomic Check and Update
> > > */
> > >
> > > +static void rcar_du_atomic_commit_update_cmm(struct drm_crtc *crtc,
> > > + struct drm_crtc_state *old_state)
> > > +{
> > > + struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> > > + struct rcar_cmm_config cmm_config = {};
> > > +
> > > + if (!rcrtc->cmm || !crtc->state->color_mgmt_changed)
> > > + return;
> > > +
> > > + if (!crtc->state->gamma_lut) {
> > > + cmm_config.lut.enable = false;
> > > + rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> > > +
> > > + return;
> > > + }
> > > +
> > > + cmm_config.lut.enable = true;
> > > + cmm_config.lut.table = (struct drm_color_lut *)
> > > + crtc->state->gamma_lut->data;
> > > +
> > > + /* Set LUT table size to 0 if entries should not be updated. */
> > > + if (!old_state->gamma_lut ||
> > > + old_state->gamma_lut->base.id != crtc->state->gamma_lut->base.id)
> > > + cmm_config.lut.size = crtc->state->gamma_lut->length
> > > + / sizeof(cmm_config.lut.table[0]);
> > > + else
> > > + cmm_config.lut.size = 0;
> > > +
> > > + rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> > > +}
> > > +
> > > static int rcar_du_atomic_check(struct drm_device *dev,
> > > struct drm_atomic_state *state)
> > > {
> > > @@ -329,6 +361,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
> > > rcdu->dpad1_source = rcrtc->index;
> > > }
> > >
> > > + for_each_old_crtc_in_state(old_state, crtc, crtc_state, i)
> > > + rcar_du_atomic_commit_update_cmm(crtc, crtc_state);
> > > +
> >
> > I think this looks good overall, but I wonder if we couldn't simplify
> > the CMM driver suspend/resume and LUT caching due to config while not
> > enabled by handling it on the DU side. I have a rework on the commit
> > tail handler in progress, I'll think how this could be done. For now I
> > think you can leave it as is.
>
> Does this mean I have your R-b tag ? :)

I'd like to review this in the context of v3 :-)

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

--
Regards,

Laurent Pinchart