[ Ugh, sorry for the double sending, but I forgot --cc-cover to git-send email and the series has not been delivered to the mailing lists. Sorry about that. ]
Hello, new iteration of CMM support, with quite a few changes compared to v3:
References: A reference to the v1 cover letter, with some background on the CMM is available here: https://lkml.org/lkml/2019/6/6/583 v2: https://lore.kernel.org/linux-renesas-soc/20190706140746.29132-10-jacopo+ren... v3: https://lore.kernel.org/linux-renesas-soc/20190825135154.11488-1-jacopo+rene...
Change log:
*Bindings/DT: - Rebased on renesas-devel-2019-09-03-v5.3-rc7 - Bindings converted to yaml: thanks Laurent for help - s/'cmms'/'renesas,cmms'/ in DU bindings as suggested by Rob - s/cmm-<soctype>/<soctype>-cmm/ as suggested by Geert - squashed CMM addition for Gen3 SoCs in a single path at the end of the series
*CMM/DU: - Only accept fully populated LUT tables, remove the 'size' from the CMM configuration structure as suggested by Laurent - Simplify CMM configuration logic: only rely on color_mgmt_changed flag and unconditionally provide a populated LUT table to the cmm_setup() function - Protect against probing order inversion (DU is operation while CMM still has not been probed) by adding rcar_cmm_init() operation as it is done for VSP as suggested by Laurent - Add CMM function stubs to fix compilation erros when CONFIG_DRM_RCAR_CMM is not selected - Minors in the CMM driver as suggested by Laurent - Remove per-soc strings - Make comments style consistent (not using /** anywhere in the .c file, unify comment style) - s/rcar_cmm_load()/rcar_cmm_write()/ - Squash cmm configuration and suspend/resume support in rcar_du_kms.c
Testing: I have tested by injecting a color inversion LUT table at test program initialization: https://jmondi.org/cgit/kmsxx/commit/?h=gamma_lut&id=3c6af4db165e5b3dc89... And by changing the CMM content to switch between a color inversion table and a linear table every 50 frames: https://jmondi.org/cgit/kmsxx/commit/?h=gamma_lut&id=fe178a43861da7c8e79...
Pretty happy with the result, which seems to be consistent across system suspend/resume.
Testing with real world use cases might be beneficial. Rajesh are you still interested in giving this series a spin?
Thanks j
Jacopo Mondi (9): dt-bindings: display: renesas,cmm: Add R-Car CMM documentation dt-bindings: display, renesas,du: Document cmms property drm: rcar-du: Add support for CMM drm: rcar-du: Claim CMM support for Gen3 SoCs drm: rcar-du: kms: Initialize 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 arm64: dts: renesas: Add CMM units to Gen3 SoCs
.../bindings/display/renesas,cmm.yaml | 64 +++++ .../bindings/display/renesas,du.txt | 5 + arch/arm64/boot/dts/renesas/r8a7795.dtsi | 40 ++- arch/arm64/boot/dts/renesas/r8a7796.dtsi | 28 ++ arch/arm64/boot/dts/renesas/r8a77965.dtsi | 28 ++ arch/arm64/boot/dts/renesas/r8a77990.dtsi | 22 +- arch/arm64/boot/dts/renesas/r8a77995.dtsi | 22 +- drivers/gpu/drm/rcar-du/Kconfig | 7 + drivers/gpu/drm/rcar-du/Makefile | 1 + drivers/gpu/drm/rcar-du/rcar_cmm.c | 251 ++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 61 +++++ 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 | 32 ++- drivers/gpu/drm/rcar-du/rcar_du_drv.h | 3 + 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 | 106 ++++++++ drivers/gpu/drm/rcar-du/rcar_du_regs.h | 5 + 19 files changed, 697 insertions(+), 7 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/renesas,cmm.yaml create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.c create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.h
-- 2.23.0
Add device tree bindings documentation for the Renesas R-Car Display Unit Color Management Module.
CMM is the image enhancement module available on each R-Car DU video channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org --- .../bindings/display/renesas,cmm.yaml | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/renesas,cmm.yaml
diff --git a/Documentation/devicetree/bindings/display/renesas,cmm.yaml b/Documentation/devicetree/bindings/display/renesas,cmm.yaml new file mode 100644 index 000000000000..9e5922689cd7 --- /dev/null +++ b/Documentation/devicetree/bindings/display/renesas,cmm.yaml @@ -0,0 +1,64 @@ +# SPDX-License-Identifier: GPL-2.0 +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/renesas,cmm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Renesas R-Car Color Management Module (CMM) + +maintainers: + - Laurent Pinchart laurent.pinchart@ideasonboard.com + - Kieran Bingham kieran.bingham+renesas@ideasonboard.com + - Jacopo Mondi jacopo+renesas@jmondi.org + +description: |+ + Renesas R-Car color management module connected to R-Car DU video channels. + It provides image enhancement functions such as 1-D look-up tables (LUT), + 3-D look-up tables (CMU), 1D-histogram generation (HGO), and color + space conversion (CSC). + +properties: + compatible: + items: + - enum: + - renesas,r8a7795-cmm + - renesas,r8a7796-cmm + - renesas,r8a77965-cmm + - renesas,r8a77990-cmm + - renesas,r8a77995-cmm + - enum: + - renesas,rcar-gen3-cmm + - renesas,rcar-gen2-cmm + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + resets: + maxItems: 1 + + power-domains: + maxItems: 1 + +required: + - compatible + - reg + - clocks + - resets + - power-domains + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/r8a7796-cpg-mssr.h> + #include <dt-bindings/power/r8a7796-sysc.h> + + cmm0: cmm@fea40000 { + compatible = "renesas,r8a7796-cmm"; + reg = <0 0xfea40000 0 0x1000>; + power-domains = <&sysc R8A7796_PD_ALWAYS_ON>; + clocks = <&cpg CPG_MOD 711>; + resets = <&cpg 711>; + }; -- 2.23.0
Hi Jacopo,
On 06/09/2019 14:54, Jacopo Mondi wrote:
Add device tree bindings documentation for the Renesas R-Car Display Unit Color Management Module.
CMM is the image enhancement module available on each R-Car DU video channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
.../bindings/display/renesas,cmm.yaml | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/renesas,cmm.yaml
diff --git a/Documentation/devicetree/bindings/display/renesas,cmm.yaml b/Documentation/devicetree/bindings/display/renesas,cmm.yaml new file mode 100644 index 000000000000..9e5922689cd7 --- /dev/null +++ b/Documentation/devicetree/bindings/display/renesas,cmm.yaml @@ -0,0 +1,64 @@ +# SPDX-License-Identifier: GPL-2.0 +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/renesas,cmm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#
+title: Renesas R-Car Color Management Module (CMM)
+maintainers:
- Laurent Pinchart laurent.pinchart@ideasonboard.com
- Kieran Bingham kieran.bingham+renesas@ideasonboard.com
- Jacopo Mondi jacopo+renesas@jmondi.org
+description: |+
- Renesas R-Car color management module connected to R-Car DU video channels.
- It provides image enhancement functions such as 1-D look-up tables (LUT),
- 3-D look-up tables (CMU), 1D-histogram generation (HGO), and color
s/CMU/CLU/
- space conversion (CSC).
+properties:
- compatible:
- items:
- enum:- renesas,r8a7795-cmm- renesas,r8a7796-cmm- renesas,r8a77965-cmm- renesas,r8a77990-cmm- renesas,r8a77995-cmm- enum:- renesas,rcar-gen3-cmm- renesas,rcar-gen2-cmm- reg:
- maxItems: 1
- clocks:
- maxItems: 1
- resets:
- maxItems: 1
- power-domains:
- maxItems: 1
+required:
- compatible
- reg
- clocks
- resets
- power-domains
+additionalProperties: false
+examples:
- |
- #include <dt-bindings/clock/r8a7796-cpg-mssr.h>
- #include <dt-bindings/power/r8a7796-sysc.h>
- cmm0: cmm@fea40000 {
compatible = "renesas,r8a7796-cmm";reg = <0 0xfea40000 0 0x1000>;power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;clocks = <&cpg CPG_MOD 711>;resets = <&cpg 711>;- };
-- 2.23.0
On Fri, Sep 06, 2019 at 03:54:28PM +0200, Jacopo Mondi wrote:
Add device tree bindings documentation for the Renesas R-Car Display Unit Color Management Module.
CMM is the image enhancement module available on each R-Car DU video channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
.../bindings/display/renesas,cmm.yaml | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/renesas,cmm.yaml
diff --git a/Documentation/devicetree/bindings/display/renesas,cmm.yaml b/Documentation/devicetree/bindings/display/renesas,cmm.yaml new file mode 100644 index 000000000000..9e5922689cd7 --- /dev/null +++ b/Documentation/devicetree/bindings/display/renesas,cmm.yaml @@ -0,0 +1,64 @@ +# SPDX-License-Identifier: GPL-2.0
For new bindings:
GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/renesas,cmm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#
+title: Renesas R-Car Color Management Module (CMM)
+maintainers:
- Laurent Pinchart laurent.pinchart@ideasonboard.com
- Kieran Bingham kieran.bingham+renesas@ideasonboard.com
- Jacopo Mondi jacopo+renesas@jmondi.org
+description: |+
- Renesas R-Car color management module connected to R-Car DU video channels.
- It provides image enhancement functions such as 1-D look-up tables (LUT),
- 3-D look-up tables (CMU), 1D-histogram generation (HGO), and color
- space conversion (CSC).
+properties:
- compatible:
- items:
- enum:- renesas,r8a7795-cmm- renesas,r8a7796-cmm- renesas,r8a77965-cmm- renesas,r8a77990-cmm- renesas,r8a77995-cmm- enum:- renesas,rcar-gen3-cmm- renesas,rcar-gen2-cmm
This allows 10 valid cases when I imagine there's only really 5. I'm okay leaving it, but might be better to split into 2 under a 'oneOf'.
I imagine there will be a lot of these for Renesas, so just be consistent.
- reg:
- maxItems: 1
- clocks:
- maxItems: 1
- resets:
- maxItems: 1
- power-domains:
- maxItems: 1
+required:
- compatible
- reg
- clocks
- resets
- power-domains
+additionalProperties: false
+examples:
- |
- #include <dt-bindings/clock/r8a7796-cpg-mssr.h>
- #include <dt-bindings/power/r8a7796-sysc.h>
- cmm0: cmm@fea40000 {
compatible = "renesas,r8a7796-cmm";reg = <0 0xfea40000 0 0x1000>;power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;clocks = <&cpg CPG_MOD 711>;resets = <&cpg 711>;- };
-- 2.23.0
Hi Jacopo,
Thank you for the patch.
On Fri, Sep 06, 2019 at 03:54:28PM +0200, Jacopo Mondi wrote:
Add device tree bindings documentation for the Renesas R-Car Display Unit Color Management Module.
CMM is the image enhancement module available on each R-Car DU video channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
With the small issues pointed out by Kieran and Rob fixed,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
.../bindings/display/renesas,cmm.yaml | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/renesas,cmm.yaml
diff --git a/Documentation/devicetree/bindings/display/renesas,cmm.yaml b/Documentation/devicetree/bindings/display/renesas,cmm.yaml new file mode 100644 index 000000000000..9e5922689cd7 --- /dev/null +++ b/Documentation/devicetree/bindings/display/renesas,cmm.yaml @@ -0,0 +1,64 @@ +# SPDX-License-Identifier: GPL-2.0 +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/renesas,cmm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#
+title: Renesas R-Car Color Management Module (CMM)
+maintainers:
- Laurent Pinchart laurent.pinchart@ideasonboard.com
- Kieran Bingham kieran.bingham+renesas@ideasonboard.com
- Jacopo Mondi jacopo+renesas@jmondi.org
+description: |+
- Renesas R-Car color management module connected to R-Car DU video channels.
- It provides image enhancement functions such as 1-D look-up tables (LUT),
- 3-D look-up tables (CMU), 1D-histogram generation (HGO), and color
- space conversion (CSC).
+properties:
- compatible:
- items:
- enum:- renesas,r8a7795-cmm- renesas,r8a7796-cmm- renesas,r8a77965-cmm- renesas,r8a77990-cmm- renesas,r8a77995-cmm- enum:- renesas,rcar-gen3-cmm- renesas,rcar-gen2-cmm- reg:
- maxItems: 1
- clocks:
- maxItems: 1
- resets:
- maxItems: 1
- power-domains:
- maxItems: 1
+required:
- compatible
- reg
- clocks
- resets
- power-domains
+additionalProperties: false
+examples:
- |
- #include <dt-bindings/clock/r8a7796-cpg-mssr.h>
- #include <dt-bindings/power/r8a7796-sysc.h>
- cmm0: cmm@fea40000 {
compatible = "renesas,r8a7796-cmm";reg = <0 0xfea40000 0 0x1000>;power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;clocks = <&cpg CPG_MOD 711>;resets = <&cpg 711>;- };
Document the newly added 'cmms' property which accepts a list of phandle and channel index pairs that point to the CMM units available for each Display Unit output video channel.
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- Documentation/devicetree/bindings/display/renesas,du.txt | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt b/Documentation/devicetree/bindings/display/renesas,du.txt index c97dfacad281..1773b0a2f54f 100644 --- a/Documentation/devicetree/bindings/display/renesas,du.txt +++ b/Documentation/devicetree/bindings/display/renesas,du.txt @@ -45,6 +45,10 @@ Required Properties: instance that serves the DU channel, and the channel index identifies the LIF instance in that VSP.
+ - renesas,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 @@ -91,6 +95,7 @@ Example: R8A7795 (R-Car H3) ES2.0 DU <&cpg CPG_MOD 721>; clock-names = "du.0", "du.1", "du.2", "du.3"; vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd0 1>; + renesas,cmms = <&cmm0 &cmm1 &cmm2 &cmm3>;
ports { #address-cells = <1>; -- 2.23.0
Hi Jacopo,
On 06/09/2019 14:54, Jacopo Mondi wrote:
Document the newly added 'cmms' property which accepts a list of phandle and channel index pairs that point to the CMM units available for each Display Unit output video channel.
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Documentation/devicetree/bindings/display/renesas,du.txt | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt b/Documentation/devicetree/bindings/display/renesas,du.txt index c97dfacad281..1773b0a2f54f 100644 --- a/Documentation/devicetree/bindings/display/renesas,du.txt +++ b/Documentation/devicetree/bindings/display/renesas,du.txt @@ -45,6 +45,10 @@ Required Properties: instance that serves the DU channel, and the channel index identifies the LIF instance in that VSP.
- renesas,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 @@ -91,6 +95,7 @@ Example: R8A7795 (R-Car H3) ES2.0 DU <&cpg CPG_MOD 721>; clock-names = "du.0", "du.1", "du.2", "du.3"; vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd0 1>;
renesas,cmms = <&cmm0 &cmm1 &cmm2 &cmm3>;
Should these be comma separated in the same fashion as the vsps are separated?
I don't really mind either way though ...
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
ports { #address-cells = <1>;-- 2.23.0
On Wed, Sep 11, 2019 at 05:06:33PM +0100, Kieran Bingham wrote:
Hi Jacopo,
On 06/09/2019 14:54, Jacopo Mondi wrote:
Document the newly added 'cmms' property which accepts a list of phandle and channel index pairs that point to the CMM units available for each Display Unit output video channel.
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Documentation/devicetree/bindings/display/renesas,du.txt | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt b/Documentation/devicetree/bindings/display/renesas,du.txt index c97dfacad281..1773b0a2f54f 100644 --- a/Documentation/devicetree/bindings/display/renesas,du.txt +++ b/Documentation/devicetree/bindings/display/renesas,du.txt @@ -45,6 +45,10 @@ Required Properties: instance that serves the DU channel, and the channel index identifies the LIF instance in that VSP.
- renesas,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 @@ -91,6 +95,7 @@ Example: R8A7795 (R-Car H3) ES2.0 DU <&cpg CPG_MOD 721>; clock-names = "du.0", "du.1", "du.2", "du.3"; vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd0 1>;
renesas,cmms = <&cmm0 &cmm1 &cmm2 &cmm3>;Should these be comma separated in the same fashion as the vsps are separated?
Yes. Doesn't matter from a dtb standpoint, but the schema are going to be stricter here. Bracket each unit.
With that,
Reviewed-by: Rob Herring robh@kernel.org
I don't really mind either way though ...
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
ports { #address-cells = <1>;-- 2.23.0
Add a driver for the R-Car Display Unit Color Correction Module.
In most of Gen3 SoCs, each DU output channel is provided with a CMM unit to perform image enhancement and color correction.
Add support for CMM through a driver that supports configuration of the 1-dimensional LUT table. More advanced CMM feature will be implemented on top of this basic one.
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org --- drivers/gpu/drm/rcar-du/Kconfig | 7 + drivers/gpu/drm/rcar-du/Makefile | 1 + drivers/gpu/drm/rcar-du/rcar_cmm.c | 251 +++++++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 61 +++++++ 4 files changed, 320 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..3cacdc4474c7 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -0,0 +1,251 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * rcar_cmm.c -- R-Car Display Unit Color Management Module + * + * Copyright (C) 2019 Jacopo Mondi jacopo+renesas@jmondi.org + */ + +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> + +#include <drm/drm_color_mgmt.h> + +#include "rcar_cmm.h" + +#define CM2_LUT_CTRL 0x0000 +#define CM2_LUT_CTRL_LUT_EN BIT(0) +#define CM2_LUT_TBL_BASE 0x0600 +#define CM2_LUT_TBL(__i) (CM2_LUT_TBL_BASE + (__i) * 4) + +struct rcar_cmm { + void __iomem *base; + bool enabled; + + /* + * @lut: 1D-LUT status + * @lut.enabled: 1D-LUT enabled flag + * @lut.table: Table of 1D-LUT entries scaled to hardware + * precision (8-bits per color component) + */ + struct { + bool enabled; + u32 table[CM2_LUT_SIZE]; + } lut; +}; + +static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg) +{ + return ioread32(rcmm->base + reg); +} + +static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data) +{ + iowrite32(data, rcmm->base + reg); +} + +/* + * rcar_cmm_lut_extract() - Scale down to hardware precision the DRM LUT table + * entries and store them. + * @rcmm: Pointer to the CMM device + * @drm_lut: Pointer to the DRM LUT table + */ +static void rcar_cmm_lut_extract(struct rcar_cmm *rcmm, + const struct drm_color_lut *drm_lut) +{ + unsigned int i; + + for (i = 0; i < CM2_LUT_SIZE; ++i) { + const struct drm_color_lut *lut = &drm_lut[i]; + + rcmm->lut.table[i] = drm_color_lut_extract(lut->red, 8) << 16 + | drm_color_lut_extract(lut->green, 8) << 8 + | drm_color_lut_extract(lut->blue, 8); + } +} + +/* + * rcar_cmm_lut_write() - Write to hardware the LUT table entries from the + * local table. + * @rcmm: Pointer to the CMM device + */ +static void rcar_cmm_lut_write(struct rcar_cmm *rcmm) +{ + unsigned int i; + + for (i = 0; i < CM2_LUT_SIZE; ++i) + rcar_cmm_write(rcmm, CM2_LUT_TBL(i), rcmm->lut.table[i]); +} + +/* + * 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); + + /* + * As rcar_cmm_setup() is called by atomic commit tail helper, it might + * be called when the CMM is disabled. As we can't program the hardware + * in that case, store the configuration internally and apply it when + * the CMM will be enabled by the CRTC through rcar_cmm_enable(). + */ + if (!rcmm->enabled) { + if (!config->lut.enable) + return 0; + + rcar_cmm_lut_extract(rcmm, config->lut.table); + rcmm->lut.enabled = true; + + return 0; + } + + /* Stop LUT operations if requested. */ + if (!config->lut.enable) { + if (rcmm->lut.enabled) { + rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0); + rcmm->lut.enabled = false; + } + + return 0; + } + + /* + * Enable LUT and program the new gamma table values. + * + * FIXME: In order to have stable operations it is required to first + * enable the 1D-LUT and then program its table entries. This seems to + * contradict what the chip manual reports, and will have to be + * reconsidered when implementing support for double buffering. + */ + if (!rcmm->lut.enabled) { + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN); + rcmm->lut.enabled = true; + } + + rcar_cmm_lut_extract(rcmm, config->lut.table); + rcar_cmm_lut_write(rcmm); + + return 0; +} +EXPORT_SYMBOL_GPL(rcar_cmm_setup); + +/* + * rcar_cmm_enable() - Enable the CMM unit. + * @pdev: The platform device associated with the CMM instance + * + * Enable the CMM unit by enabling the parent clock and enabling the CMM + * components, such as 1-D LUT, if requested. + */ +int rcar_cmm_enable(struct platform_device *pdev) +{ + struct rcar_cmm *rcmm = platform_get_drvdata(pdev); + int ret; + + ret = pm_runtime_get_sync(&pdev->dev); + if (ret < 0) + return ret; + + /* Apply the LUT table values saved at rcar_cmm_setup() time. */ + if (rcmm->lut.enabled) { + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN); + rcar_cmm_lut_write(rcmm); + } + + rcmm->enabled = true; + + return 0; +} +EXPORT_SYMBOL_GPL(rcar_cmm_enable); + +/* + * rcar_cmm_disable() - Disable the CMM unit. + * @pdev: The platform device associated with the CMM instance + * + * Disable the CMM unit by stopping the parent clock. + */ +void rcar_cmm_disable(struct platform_device *pdev) +{ + struct rcar_cmm *rcmm = platform_get_drvdata(pdev); + + rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0); + + pm_runtime_put(&pdev->dev); + + rcmm->lut.enabled = false; + rcmm->enabled = false; +} +EXPORT_SYMBOL_GPL(rcar_cmm_disable); + +/* + * rcar_cmm_init() - Make sure the CMM has probed. + * @pdev: The platform device associated with the CMM instance + * + * Return: 0 if the CMM has probed, -EPROBE_DEFER otherwise + */ +int rcar_cmm_init(struct platform_device *pdev) +{ + struct rcar_cmm *rcmm = platform_get_drvdata(pdev); + + if (!rcmm) + return -EPROBE_DEFER; + + return 0; +} +EXPORT_SYMBOL_GPL(rcar_cmm_init); + +static int rcar_cmm_probe(struct platform_device *pdev) +{ + struct rcar_cmm *rcmm; + + rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL); + if (!rcmm) + return -ENOMEM; + platform_set_drvdata(pdev, rcmm); + + rcmm->base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(rcmm->base)) + return PTR_ERR(rcmm->base); + + pm_runtime_enable(&pdev->dev); + + return 0; +} + +static int rcar_cmm_remove(struct platform_device *pdev) +{ + pm_runtime_disable(&pdev->dev); + + return 0; +} + +static const struct of_device_id rcar_cmm_of_table[] = { + { .compatible = "renesas,rcar-gen3-cmm", }, + { .compatible = "renesas,rcar-gen2-cmm", }, + { }, +}; +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table); + +static struct platform_driver rcar_cmm_platform_driver = { + .probe = rcar_cmm_probe, + .remove = rcar_cmm_remove, + .driver = { + .name = "rcar-cmm", + .of_match_table = rcar_cmm_of_table, + }, +}; + +module_platform_driver(rcar_cmm_platform_driver); + +MODULE_AUTHOR("Jacopo Mondi jacopo+renesas@jmondi.org"); +MODULE_DESCRIPTION("Renesas R-Car CMM Driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h new file mode 100644 index 000000000000..15a2c874b6a6 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h @@ -0,0 +1,61 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * rcar_cmm.h -- R-Car Display Unit Color Management Module + * + * Copyright (C) 2019 Jacopo Mondi jacopo+renesas@jmondi.org + */ + +#ifndef __RCAR_CMM_H__ +#define __RCAR_CMM_H__ + +#define CM2_LUT_SIZE 256 + +struct drm_color_lut; +struct platform_device; + +/** + * struct rcar_cmm_config - CMM configuration + * + * @lut: 1D-LUT configuration + * @lut.enable: 1D-LUT enable flag + * @lut.table: 1D-LUT table entries. Might be set to NULL when the CMM has to + * be re-enabled but not re=programmed. + */ +struct rcar_cmm_config { + struct { + bool enable; + struct drm_color_lut *table; + } lut; +}; + +#if IS_ENABLED(CONFIG_DRM_RCAR_CMM) +int rcar_cmm_init(struct platform_device *pdev); + +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); +#else +static inline int rcar_cmm_init(struct platform_device *pdev) +{ + return 0; +} + +static inline int rcar_cmm_enable(struct platform_device *pdev) +{ + return 0; +} + +static inline void rcar_cmm_disable(struct platform_device *pdev) +{ +} + +static int rcar_cmm_setup(struct platform_device *pdev, + const struct rcar_cmm_config *config) +{ + return 0; +} +#endif /* IS_ENABLED(CONFIG_DRM_RCAR_CMM) */ + +#endif /* __RCAR_CMM_H__ */
Hi Jacopo,
<This time replying to the mail with ML's included Doh!>
On 06/09/2019 14:43, 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
s/feature/features/
implemented on top of this basic one.
s/basic/initial/
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
drivers/gpu/drm/rcar-du/Kconfig | 7 + drivers/gpu/drm/rcar-du/Makefile | 1 + drivers/gpu/drm/rcar-du/rcar_cmm.c | 251 +++++++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 61 +++++++ 4 files changed, 320 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..3cacdc4474c7 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -0,0 +1,251 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- rcar_cmm.c -- R-Car Display Unit Color Management Module
- Copyright (C) 2019 Jacopo Mondi jacopo+renesas@jmondi.org
- */
+#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h>
+#include <drm/drm_color_mgmt.h>
+#include "rcar_cmm.h"
+#define CM2_LUT_CTRL 0x0000 +#define CM2_LUT_CTRL_LUT_EN BIT(0)
I'd have a new line here
+#define CM2_LUT_TBL_BASE 0x0600 +#define CM2_LUT_TBL(__i) (CM2_LUT_TBL_BASE + (__i) * 4)
+struct rcar_cmm {
- void __iomem *base;
- bool enabled;
- /*
* @lut: 1D-LUT status* @lut.enabled: 1D-LUT enabled flag* @lut.table: Table of 1D-LUT entries scaled to hardware* precision (8-bits per color component)*/- struct {
bool enabled;u32 table[CM2_LUT_SIZE];- } lut;
+};
+static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg) +{
- return ioread32(rcmm->base + reg);
+}
+static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32
data)
+{
- iowrite32(data, rcmm->base + reg);
+}
+/*
- rcar_cmm_lut_extract() - Scale down to hardware precision the DRM
LUT table
entries and store them.
- @rcmm: Pointer to the CMM device
- @drm_lut: Pointer to the DRM LUT table
- */
+static void rcar_cmm_lut_extract(struct rcar_cmm *rcmm,
const struct drm_color_lut *drm_lut)+{
- unsigned int i;
I think you're missing the following here:
if (!drm_lut) return;
You mention below that drm_lut could be passed in as NULL, which would cause a segfault here otherwise.
- for (i = 0; i < CM2_LUT_SIZE; ++i) {
const struct drm_color_lut *lut = &drm_lut[i];rcmm->lut.table[i] = drm_color_lut_extract(lut->red, 8) << 16| drm_color_lut_extract(lut->green, 8) << 8| drm_color_lut_extract(lut->blue, 8);- }
+}
+/*
- rcar_cmm_lut_write() - Write to hardware the LUT table entries
from the
local table.
- @rcmm: Pointer to the CMM device
- */
+static void rcar_cmm_lut_write(struct rcar_cmm *rcmm) +{
- unsigned int i;
- for (i = 0; i < CM2_LUT_SIZE; ++i)
rcar_cmm_write(rcmm, CM2_LUT_TBL(i), rcmm->lut.table[i]);+}
+/*
- rcar_cmm_setup() - Configure the CMM unit.
- @pdev: The platform device associated with the CMM instance
- @config: The CRTC-provided configuration.
I don't think CRTC-provided should be hyphenated like that. Perhaps just: "The colour management configuration".
As I don't think who provides the configuration is relevant to the actual call.
- Configure the CMM unit with the CRTC-provided configuration.
s/CRTC-provided/given/
- 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);
- /*
* As rcar_cmm_setup() is called by atomic commit tail helper, it might* be called when the CMM is disabled. As we can't program the hardware* in that case, store the configuration internally and apply it when* the CMM will be enabled by the CRTC through rcar_cmm_enable().*/- if (!rcmm->enabled) {
if (!config->lut.enable)return 0;rcar_cmm_lut_extract(rcmm, config->lut.table);rcmm->lut.enabled = true;return 0;- }
- /* Stop LUT operations if requested. */
- if (!config->lut.enable) {
if (rcmm->lut.enabled) {rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);rcmm->lut.enabled = false;}return 0;- }
- /*
* Enable LUT and program the new gamma table values.** FIXME: In order to have stable operations it is required to first* enable the 1D-LUT and then program its table entries. This seems to* contradict what the chip manual reports, and will have to be* reconsidered when implementing support for double buffering.*/- if (!rcmm->lut.enabled) {
rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);rcmm->lut.enabled = true;- }
- rcar_cmm_lut_extract(rcmm, config->lut.table);
- rcar_cmm_lut_write(rcmm);
- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_setup);
+/*
- rcar_cmm_enable() - Enable the CMM unit.
- @pdev: The platform device associated with the CMM instance
- Enable the CMM unit by enabling the parent clock and enabling the CMM
- components, such as 1-D LUT, if requested.
- */
+int rcar_cmm_enable(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- int ret;
- ret = pm_runtime_get_sync(&pdev->dev);
- if (ret < 0)
return ret;- /* Apply the LUT table values saved at rcar_cmm_setup() time. */
- if (rcmm->lut.enabled) {
rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);rcar_cmm_lut_write(rcmm);- }
- rcmm->enabled = true;
- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_enable);
+/*
- rcar_cmm_disable() - Disable the CMM unit.
- @pdev: The platform device associated with the CMM instance
- Disable the CMM unit by stopping the parent clock.
- */
+void rcar_cmm_disable(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
- pm_runtime_put(&pdev->dev);
- rcmm->lut.enabled = false;
- rcmm->enabled = false;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_disable);
+/*
- rcar_cmm_init() - Make sure the CMM has probed.
- @pdev: The platform device associated with the CMM instance
- Return: 0 if the CMM has probed, -EPROBE_DEFER otherwise
- */
+int rcar_cmm_init(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- if (!rcmm)
return -EPROBE_DEFER;- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_init);
+static int rcar_cmm_probe(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm;
- rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
- if (!rcmm)
return -ENOMEM;- platform_set_drvdata(pdev, rcmm);
- rcmm->base = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(rcmm->base))
return PTR_ERR(rcmm->base);- pm_runtime_enable(&pdev->dev);
- return 0;
+}
+static int rcar_cmm_remove(struct platform_device *pdev) +{
- pm_runtime_disable(&pdev->dev);
- return 0;
+}
+static const struct of_device_id rcar_cmm_of_table[] = {
- { .compatible = "renesas,rcar-gen3-cmm", },
- { .compatible = "renesas,rcar-gen2-cmm", },
- { },
+}; +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
+static struct platform_driver rcar_cmm_platform_driver = {
- .probe = rcar_cmm_probe,
- .remove = rcar_cmm_remove,
- .driver = {
.name = "rcar-cmm",.of_match_table = rcar_cmm_of_table,- },
+};
+module_platform_driver(rcar_cmm_platform_driver);
+MODULE_AUTHOR("Jacopo Mondi jacopo+renesas@jmondi.org"); +MODULE_DESCRIPTION("Renesas R-Car CMM Driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h
b/drivers/gpu/drm/rcar-du/rcar_cmm.h
new file mode 100644 index 000000000000..15a2c874b6a6 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h @@ -0,0 +1,61 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- rcar_cmm.h -- R-Car Display Unit Color Management Module
- Copyright (C) 2019 Jacopo Mondi jacopo+renesas@jmondi.org
- */
+#ifndef __RCAR_CMM_H__ +#define __RCAR_CMM_H__
+#define CM2_LUT_SIZE 256
+struct drm_color_lut; +struct platform_device;
+/**
- struct rcar_cmm_config - CMM configuration
- @lut: 1D-LUT configuration
- @lut.enable: 1D-LUT enable flag
- @lut.table: 1D-LUT table entries. Might be set to NULL when the
CMM has to
be re-enabled but not re=programmed.
So lut.table can be NULL ... See comment at rcar_cmm_lut_extract()
- */
+struct rcar_cmm_config {
- struct {
bool enable;struct drm_color_lut *table;- } lut;
+};
+#if IS_ENABLED(CONFIG_DRM_RCAR_CMM) +int rcar_cmm_init(struct platform_device *pdev);
+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);+#else +static inline int rcar_cmm_init(struct platform_device *pdev) +{
- return 0;
+}
+static inline int rcar_cmm_enable(struct platform_device *pdev) +{
- return 0;
+}
+static inline void rcar_cmm_disable(struct platform_device *pdev) +{ +}
+static int rcar_cmm_setup(struct platform_device *pdev,
const struct rcar_cmm_config *config)+{
- return 0;
+} +#endif /* IS_ENABLED(CONFIG_DRM_RCAR_CMM) */
+#endif /* __RCAR_CMM_H__ */
Hi Kiernan, thanks for review
On Wed, Sep 11, 2019 at 04:54:58PM +0100, Kieran Bingham wrote:
Hi Jacopo,
<This time replying to the mail with ML's included Doh!>
On 06/09/2019 14:43, 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
s/feature/features/
implemented on top of this basic one.
s/basic/initial/
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
drivers/gpu/drm/rcar-du/Kconfig | 7 + drivers/gpu/drm/rcar-du/Makefile | 1 + drivers/gpu/drm/rcar-du/rcar_cmm.c | 251 +++++++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 61 +++++++ 4 files changed, 320 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..3cacdc4474c7 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -0,0 +1,251 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- rcar_cmm.c -- R-Car Display Unit Color Management Module
- Copyright (C) 2019 Jacopo Mondi jacopo+renesas@jmondi.org
- */
+#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h>
+#include <drm/drm_color_mgmt.h>
+#include "rcar_cmm.h"
+#define CM2_LUT_CTRL 0x0000 +#define CM2_LUT_CTRL_LUT_EN BIT(0)
I'd have a new line here
+#define CM2_LUT_TBL_BASE 0x0600 +#define CM2_LUT_TBL(__i) (CM2_LUT_TBL_BASE + (__i) * 4)
+struct rcar_cmm {
- void __iomem *base;
- bool enabled;
- /*
* @lut: 1D-LUT status* @lut.enabled: 1D-LUT enabled flag* @lut.table: Table of 1D-LUT entries scaled to hardware* precision (8-bits per color component)*/- struct {
bool enabled;u32 table[CM2_LUT_SIZE];- } lut;
+};
+static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg) +{
- return ioread32(rcmm->base + reg);
+}
+static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32
data)
+{
- iowrite32(data, rcmm->base + reg);
+}
+/*
- rcar_cmm_lut_extract() - Scale down to hardware precision the DRM
LUT table
entries and store them.
- @rcmm: Pointer to the CMM device
- @drm_lut: Pointer to the DRM LUT table
- */
+static void rcar_cmm_lut_extract(struct rcar_cmm *rcmm,
const struct drm_color_lut *drm_lut)+{
- unsigned int i;
I think you're missing the following here:
if (!drm_lut) return;
You mention below that drm_lut could be passed in as NULL, which would cause a segfault here otherwise.
Thanks for spotting, but I should have removed that comment, as I have killed that case in rcar_du_atomic_commit_update_cmm() in patch 8/9, as from my testing it seems it is not possible to provide from userspace a non populated LUT table to just re-enable the CMM.
So we're fine here.
- for (i = 0; i < CM2_LUT_SIZE; ++i) {
const struct drm_color_lut *lut = &drm_lut[i];rcmm->lut.table[i] = drm_color_lut_extract(lut->red, 8) << 16| drm_color_lut_extract(lut->green, 8) << 8| drm_color_lut_extract(lut->blue, 8);- }
+}
+/*
- rcar_cmm_lut_write() - Write to hardware the LUT table entries
from the
local table.
- @rcmm: Pointer to the CMM device
- */
+static void rcar_cmm_lut_write(struct rcar_cmm *rcmm) +{
- unsigned int i;
- for (i = 0; i < CM2_LUT_SIZE; ++i)
rcar_cmm_write(rcmm, CM2_LUT_TBL(i), rcmm->lut.table[i]);+}
+/*
- rcar_cmm_setup() - Configure the CMM unit.
- @pdev: The platform device associated with the CMM instance
- @config: The CRTC-provided configuration.
I don't think CRTC-provided should be hyphenated like that. Perhaps just: "The colour management configuration".
As I don't think who provides the configuration is relevant to the actual call.
I have no idea. I've been bought into this by the comment
"CRTC-provided" is a compound adjective, qualifying the noun "configuration". It thus needs a hyphen.
I'm happy to remove it for a simpler version.
Thanks j
- Configure the CMM unit with the CRTC-provided configuration.
s/CRTC-provided/given/
- 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);
- /*
* As rcar_cmm_setup() is called by atomic commit tail helper, it might* be called when the CMM is disabled. As we can't program the hardware* in that case, store the configuration internally and apply it when* the CMM will be enabled by the CRTC through rcar_cmm_enable().*/- if (!rcmm->enabled) {
if (!config->lut.enable)return 0;rcar_cmm_lut_extract(rcmm, config->lut.table);rcmm->lut.enabled = true;return 0;- }
- /* Stop LUT operations if requested. */
- if (!config->lut.enable) {
if (rcmm->lut.enabled) {rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);rcmm->lut.enabled = false;}return 0;- }
- /*
* Enable LUT and program the new gamma table values.** FIXME: In order to have stable operations it is required to first* enable the 1D-LUT and then program its table entries. This seems to* contradict what the chip manual reports, and will have to be* reconsidered when implementing support for double buffering.*/- if (!rcmm->lut.enabled) {
rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);rcmm->lut.enabled = true;- }
- rcar_cmm_lut_extract(rcmm, config->lut.table);
- rcar_cmm_lut_write(rcmm);
- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_setup);
+/*
- rcar_cmm_enable() - Enable the CMM unit.
- @pdev: The platform device associated with the CMM instance
- Enable the CMM unit by enabling the parent clock and enabling the CMM
- components, such as 1-D LUT, if requested.
- */
+int rcar_cmm_enable(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- int ret;
- ret = pm_runtime_get_sync(&pdev->dev);
- if (ret < 0)
return ret;- /* Apply the LUT table values saved at rcar_cmm_setup() time. */
- if (rcmm->lut.enabled) {
rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);rcar_cmm_lut_write(rcmm);- }
- rcmm->enabled = true;
- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_enable);
+/*
- rcar_cmm_disable() - Disable the CMM unit.
- @pdev: The platform device associated with the CMM instance
- Disable the CMM unit by stopping the parent clock.
- */
+void rcar_cmm_disable(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
- pm_runtime_put(&pdev->dev);
- rcmm->lut.enabled = false;
- rcmm->enabled = false;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_disable);
+/*
- rcar_cmm_init() - Make sure the CMM has probed.
- @pdev: The platform device associated with the CMM instance
- Return: 0 if the CMM has probed, -EPROBE_DEFER otherwise
- */
+int rcar_cmm_init(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- if (!rcmm)
return -EPROBE_DEFER;- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_init);
+static int rcar_cmm_probe(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm;
- rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
- if (!rcmm)
return -ENOMEM;- platform_set_drvdata(pdev, rcmm);
- rcmm->base = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(rcmm->base))
return PTR_ERR(rcmm->base);- pm_runtime_enable(&pdev->dev);
- return 0;
+}
+static int rcar_cmm_remove(struct platform_device *pdev) +{
- pm_runtime_disable(&pdev->dev);
- return 0;
+}
+static const struct of_device_id rcar_cmm_of_table[] = {
- { .compatible = "renesas,rcar-gen3-cmm", },
- { .compatible = "renesas,rcar-gen2-cmm", },
- { },
+}; +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
+static struct platform_driver rcar_cmm_platform_driver = {
- .probe = rcar_cmm_probe,
- .remove = rcar_cmm_remove,
- .driver = {
.name = "rcar-cmm",.of_match_table = rcar_cmm_of_table,- },
+};
+module_platform_driver(rcar_cmm_platform_driver);
+MODULE_AUTHOR("Jacopo Mondi jacopo+renesas@jmondi.org"); +MODULE_DESCRIPTION("Renesas R-Car CMM Driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h
b/drivers/gpu/drm/rcar-du/rcar_cmm.h
new file mode 100644 index 000000000000..15a2c874b6a6 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h @@ -0,0 +1,61 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- rcar_cmm.h -- R-Car Display Unit Color Management Module
- Copyright (C) 2019 Jacopo Mondi jacopo+renesas@jmondi.org
- */
+#ifndef __RCAR_CMM_H__ +#define __RCAR_CMM_H__
+#define CM2_LUT_SIZE 256
+struct drm_color_lut; +struct platform_device;
+/**
- struct rcar_cmm_config - CMM configuration
- @lut: 1D-LUT configuration
- @lut.enable: 1D-LUT enable flag
- @lut.table: 1D-LUT table entries. Might be set to NULL when the
CMM has to
be re-enabled but not re=programmed.So lut.table can be NULL ... See comment at rcar_cmm_lut_extract()
- */
+struct rcar_cmm_config {
- struct {
bool enable;struct drm_color_lut *table;- } lut;
+};
+#if IS_ENABLED(CONFIG_DRM_RCAR_CMM) +int rcar_cmm_init(struct platform_device *pdev);
+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);+#else +static inline int rcar_cmm_init(struct platform_device *pdev) +{
- return 0;
+}
+static inline int rcar_cmm_enable(struct platform_device *pdev) +{
- return 0;
+}
+static inline void rcar_cmm_disable(struct platform_device *pdev) +{ +}
+static int rcar_cmm_setup(struct platform_device *pdev,
const struct rcar_cmm_config *config)+{
- return 0;
+} +#endif /* IS_ENABLED(CONFIG_DRM_RCAR_CMM) */
+#endif /* __RCAR_CMM_H__ */
Hi Jacopo,
On 12/09/2019 08:59, Jacopo Mondi wrote:
Hi Kiernan, thanks for review
On Wed, Sep 11, 2019 at 04:54:58PM +0100, Kieran Bingham wrote:
Hi Jacopo,
<This time replying to the mail with ML's included Doh!>
On 06/09/2019 14:43, 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
s/feature/features/
implemented on top of this basic one.
s/basic/initial/
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
drivers/gpu/drm/rcar-du/Kconfig | 7 + drivers/gpu/drm/rcar-du/Makefile | 1 + drivers/gpu/drm/rcar-du/rcar_cmm.c | 251 +++++++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 61 +++++++ 4 files changed, 320 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..3cacdc4474c7 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -0,0 +1,251 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- rcar_cmm.c -- R-Car Display Unit Color Management Module
- Copyright (C) 2019 Jacopo Mondi jacopo+renesas@jmondi.org
- */
+#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h>
+#include <drm/drm_color_mgmt.h>
+#include "rcar_cmm.h"
+#define CM2_LUT_CTRL 0x0000 +#define CM2_LUT_CTRL_LUT_EN BIT(0)
I'd have a new line here
+#define CM2_LUT_TBL_BASE 0x0600 +#define CM2_LUT_TBL(__i) (CM2_LUT_TBL_BASE + (__i) * 4)
+struct rcar_cmm {
- void __iomem *base;
- bool enabled;
- /*
* @lut: 1D-LUT status* @lut.enabled: 1D-LUT enabled flag* @lut.table: Table of 1D-LUT entries scaled to hardware* precision (8-bits per color component)*/- struct {
bool enabled;u32 table[CM2_LUT_SIZE];- } lut;
+};
+static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg) +{
- return ioread32(rcmm->base + reg);
+}
+static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32
data)
+{
- iowrite32(data, rcmm->base + reg);
+}
+/*
- rcar_cmm_lut_extract() - Scale down to hardware precision the DRM
LUT table
entries and store them.
- @rcmm: Pointer to the CMM device
- @drm_lut: Pointer to the DRM LUT table
- */
+static void rcar_cmm_lut_extract(struct rcar_cmm *rcmm,
const struct drm_color_lut *drm_lut)+{
- unsigned int i;
I think you're missing the following here:
if (!drm_lut) return;
You mention below that drm_lut could be passed in as NULL, which would cause a segfault here otherwise.
Thanks for spotting, but I should have removed that comment, as I have killed that case in rcar_du_atomic_commit_update_cmm() in patch 8/9, as from my testing it seems it is not possible to provide from userspace a non populated LUT table to just re-enable the CMM.
So we're fine here.
Great, in that case then with the comment removed, and the other trivials in this handled however you see fit,
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
- for (i = 0; i < CM2_LUT_SIZE; ++i) {
const struct drm_color_lut *lut = &drm_lut[i];rcmm->lut.table[i] = drm_color_lut_extract(lut->red, 8) << 16| drm_color_lut_extract(lut->green, 8) << 8| drm_color_lut_extract(lut->blue, 8);- }
+}
+/*
- rcar_cmm_lut_write() - Write to hardware the LUT table entries
from the
local table.
- @rcmm: Pointer to the CMM device
- */
+static void rcar_cmm_lut_write(struct rcar_cmm *rcmm) +{
- unsigned int i;
- for (i = 0; i < CM2_LUT_SIZE; ++i)
rcar_cmm_write(rcmm, CM2_LUT_TBL(i), rcmm->lut.table[i]);+}
+/*
- rcar_cmm_setup() - Configure the CMM unit.
- @pdev: The platform device associated with the CMM instance
- @config: The CRTC-provided configuration.
I don't think CRTC-provided should be hyphenated like that. Perhaps just: "The colour management configuration".
As I don't think who provides the configuration is relevant to the actual call.
I have no idea. I've been bought into this by the comment
"CRTC-provided" is a compound adjective, qualifying the noun "configuration". It thus needs a hyphen.
I'm happy to remove it for a simpler version.
CRTC-provided just looks weird to me :-D however you wish to leave it is up to you though :D
+++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
@@ -2320,6 +2320,33 @@ resets = <&cpg 611>; };
+ cmm0: cmm@fea40000 { + compatible = "renesas,r8a77965-cmm", + "renesas,rcar-gen3-cmm"; + reg = <0 0xfea40000 0 0x1000>; + power-domains = <&sysc R8A77965_PD_ALWAYS_ON>; + clocks = <&cpg CPG_MOD 711>; + resets = <&cpg 711>; + }; + + cmm1: cmm@fea50000 { + compatible = "renesas,r8a77965-cmm", + "renesas,rcar-gen3-cmm"; + reg = <0 0xfea50000 0 0x1000>; + power-domains = <&sysc R8A77965_PD_ALWAYS_ON>; + clocks = <&cpg CPG_MOD 710>; + resets = <&cpg 710>; + }; + + cmm3: cmm@fea70000 { + compatible = "renesas,r8a77965-cmm", + "renesas,rcar-gen3-cmm"; + reg = <0 0xfea70000 0 0x1000>; + power-domains = <&sysc R8A77965_PD_ALWAYS_ON>; + clocks = <&cpg CPG_MOD 708>; + resets = <&cpg 708>; + }; + csi20: csi2@fea80000 { compatible = "renesas,r8a77965-csi2"; reg = <0 0xfea80000 0 0x10000>; @@ -2470,6 +2497,7 @@ status = "disabled";
vsps = <&vspd0 0>, <&vspd1 0>, <&vspd0 1>; + renesas,cmms = <&cmm0 &cmm1 &cmm3>;
Thanks j
- Configure the CMM unit with the CRTC-provided configuration.
s/CRTC-provided/given/
- 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);
- /*
* As rcar_cmm_setup() is called by atomic commit tail helper, it might* be called when the CMM is disabled. As we can't program the hardware* in that case, store the configuration internally and apply it when* the CMM will be enabled by the CRTC through rcar_cmm_enable().*/- if (!rcmm->enabled) {
if (!config->lut.enable)return 0;rcar_cmm_lut_extract(rcmm, config->lut.table);rcmm->lut.enabled = true;return 0;- }
- /* Stop LUT operations if requested. */
- if (!config->lut.enable) {
if (rcmm->lut.enabled) {rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);rcmm->lut.enabled = false;}return 0;- }
- /*
* Enable LUT and program the new gamma table values.** FIXME: In order to have stable operations it is required to first* enable the 1D-LUT and then program its table entries. This seems to* contradict what the chip manual reports, and will have to be* reconsidered when implementing support for double buffering.*/- if (!rcmm->lut.enabled) {
rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);rcmm->lut.enabled = true;- }
- rcar_cmm_lut_extract(rcmm, config->lut.table);
- rcar_cmm_lut_write(rcmm);
- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_setup);
+/*
- rcar_cmm_enable() - Enable the CMM unit.
- @pdev: The platform device associated with the CMM instance
- Enable the CMM unit by enabling the parent clock and enabling the CMM
- components, such as 1-D LUT, if requested.
- */
+int rcar_cmm_enable(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- int ret;
- ret = pm_runtime_get_sync(&pdev->dev);
- if (ret < 0)
return ret;- /* Apply the LUT table values saved at rcar_cmm_setup() time. */
- if (rcmm->lut.enabled) {
rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);rcar_cmm_lut_write(rcmm);- }
- rcmm->enabled = true;
- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_enable);
+/*
- rcar_cmm_disable() - Disable the CMM unit.
- @pdev: The platform device associated with the CMM instance
- Disable the CMM unit by stopping the parent clock.
- */
+void rcar_cmm_disable(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
- pm_runtime_put(&pdev->dev);
- rcmm->lut.enabled = false;
- rcmm->enabled = false;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_disable);
+/*
- rcar_cmm_init() - Make sure the CMM has probed.
- @pdev: The platform device associated with the CMM instance
- Return: 0 if the CMM has probed, -EPROBE_DEFER otherwise
- */
+int rcar_cmm_init(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- if (!rcmm)
return -EPROBE_DEFER;- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_init);
+static int rcar_cmm_probe(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm;
- rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
- if (!rcmm)
return -ENOMEM;- platform_set_drvdata(pdev, rcmm);
- rcmm->base = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(rcmm->base))
return PTR_ERR(rcmm->base);- pm_runtime_enable(&pdev->dev);
- return 0;
+}
+static int rcar_cmm_remove(struct platform_device *pdev) +{
- pm_runtime_disable(&pdev->dev);
- return 0;
+}
+static const struct of_device_id rcar_cmm_of_table[] = {
- { .compatible = "renesas,rcar-gen3-cmm", },
- { .compatible = "renesas,rcar-gen2-cmm", },
- { },
+}; +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
+static struct platform_driver rcar_cmm_platform_driver = {
- .probe = rcar_cmm_probe,
- .remove = rcar_cmm_remove,
- .driver = {
.name = "rcar-cmm",.of_match_table = rcar_cmm_of_table,- },
+};
+module_platform_driver(rcar_cmm_platform_driver);
+MODULE_AUTHOR("Jacopo Mondi jacopo+renesas@jmondi.org"); +MODULE_DESCRIPTION("Renesas R-Car CMM Driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h
b/drivers/gpu/drm/rcar-du/rcar_cmm.h
new file mode 100644 index 000000000000..15a2c874b6a6 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h @@ -0,0 +1,61 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- rcar_cmm.h -- R-Car Display Unit Color Management Module
- Copyright (C) 2019 Jacopo Mondi jacopo+renesas@jmondi.org
- */
+#ifndef __RCAR_CMM_H__ +#define __RCAR_CMM_H__
+#define CM2_LUT_SIZE 256
+struct drm_color_lut; +struct platform_device;
+/**
- struct rcar_cmm_config - CMM configuration
- @lut: 1D-LUT configuration
- @lut.enable: 1D-LUT enable flag
- @lut.table: 1D-LUT table entries. Might be set to NULL when the
CMM has to
be re-enabled but not re=programmed.So lut.table can be NULL ... See comment at rcar_cmm_lut_extract()
Ok - so this comment will be removed.
- */
+struct rcar_cmm_config {
- struct {
bool enable;struct drm_color_lut *table;- } lut;
+};
+#if IS_ENABLED(CONFIG_DRM_RCAR_CMM) +int rcar_cmm_init(struct platform_device *pdev);
+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);+#else +static inline int rcar_cmm_init(struct platform_device *pdev) +{
- return 0;
+}
+static inline int rcar_cmm_enable(struct platform_device *pdev) +{
- return 0;
+}
+static inline void rcar_cmm_disable(struct platform_device *pdev) +{ +}
+static int rcar_cmm_setup(struct platform_device *pdev,
const struct rcar_cmm_config *config)+{
- return 0;
+} +#endif /* IS_ENABLED(CONFIG_DRM_RCAR_CMM) */
+#endif /* __RCAR_CMM_H__ */
Hi Jacopo,
Thank you for the patch.
On Fri, Sep 06, 2019 at 03:54:30PM +0200, Jacopo Mondi wrote:
Add a driver for the R-Car Display Unit Color Correction Module.
In most of Gen3 SoCs, each DU output channel is provided with a CMM unit to perform image enhancement and color correction.
Add support for CMM through a driver that supports configuration of the 1-dimensional LUT table. More advanced CMM feature will be implemented on top of this basic one.
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
drivers/gpu/drm/rcar-du/Kconfig | 7 + drivers/gpu/drm/rcar-du/Makefile | 1 + drivers/gpu/drm/rcar-du/rcar_cmm.c | 251 +++++++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 61 +++++++ 4 files changed, 320 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..3cacdc4474c7 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -0,0 +1,251 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- rcar_cmm.c -- R-Car Display Unit Color Management Module
- Copyright (C) 2019 Jacopo Mondi jacopo+renesas@jmondi.org
- */
+#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h>
+#include <drm/drm_color_mgmt.h>
+#include "rcar_cmm.h"
+#define CM2_LUT_CTRL 0x0000 +#define CM2_LUT_CTRL_LUT_EN BIT(0) +#define CM2_LUT_TBL_BASE 0x0600 +#define CM2_LUT_TBL(__i) (CM2_LUT_TBL_BASE + (__i) * 4)
+struct rcar_cmm {
- void __iomem *base;
- bool enabled;
- /*
* @lut: 1D-LUT status* @lut.enabled: 1D-LUT enabled flag* @lut.table: Table of 1D-LUT entries scaled to hardware* precision (8-bits per color component)*/- struct {
bool enabled;u32 table[CM2_LUT_SIZE];- } lut;
+};
+static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg) +{
- return ioread32(rcmm->base + reg);
+}
+static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data) +{
- iowrite32(data, rcmm->base + reg);
+}
+/*
- rcar_cmm_lut_extract() - Scale down to hardware precision the DRM LUT table
entries and store them.
"Scale the DRM LUT table entries to hardware precision and store them."
- @rcmm: Pointer to the CMM device
- @drm_lut: Pointer to the DRM LUT table
- */
+static void rcar_cmm_lut_extract(struct rcar_cmm *rcmm,
const struct drm_color_lut *drm_lut)+{
- unsigned int i;
- for (i = 0; i < CM2_LUT_SIZE; ++i) {
const struct drm_color_lut *lut = &drm_lut[i];rcmm->lut.table[i] = drm_color_lut_extract(lut->red, 8) << 16| drm_color_lut_extract(lut->green, 8) << 8| drm_color_lut_extract(lut->blue, 8);- }
+}
+/*
- rcar_cmm_lut_write() - Write to hardware the LUT table entries from the
local table.
"Write the LUT table entries from the local table to the hardware."
- @rcmm: Pointer to the CMM device
- */
+static void rcar_cmm_lut_write(struct rcar_cmm *rcmm) +{
- unsigned int i;
- for (i = 0; i < CM2_LUT_SIZE; ++i)
rcar_cmm_write(rcmm, CM2_LUT_TBL(i), rcmm->lut.table[i]);+}
+/*
- 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);
- /*
* As rcar_cmm_setup() is called by atomic commit tail helper, it might* be called when the CMM is disabled. As we can't program the hardware* in that case, store the configuration internally and apply it when* the CMM will be enabled by the CRTC through rcar_cmm_enable().*/- if (!rcmm->enabled) {
if (!config->lut.enable)return 0;rcar_cmm_lut_extract(rcmm, config->lut.table);rcmm->lut.enabled = true;return 0;- }
- /* Stop LUT operations if requested. */
- if (!config->lut.enable) {
if (rcmm->lut.enabled) {rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);rcmm->lut.enabled = false;}return 0;- }
- /*
* Enable LUT and program the new gamma table values.** FIXME: In order to have stable operations it is required to first* enable the 1D-LUT and then program its table entries. This seems to* contradict what the chip manual reports, and will have to be* reconsidered when implementing support for double buffering.*/- if (!rcmm->lut.enabled) {
rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);rcmm->lut.enabled = true;- }
- rcar_cmm_lut_extract(rcmm, config->lut.table);
- rcar_cmm_lut_write(rcmm);
- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_setup);
+/*
- rcar_cmm_enable() - Enable the CMM unit.
- @pdev: The platform device associated with the CMM instance
- Enable the CMM unit by enabling the parent clock and enabling the CMM
- components, such as 1-D LUT, if requested.
- */
+int rcar_cmm_enable(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- int ret;
- ret = pm_runtime_get_sync(&pdev->dev);
- if (ret < 0)
return ret;- /* Apply the LUT table values saved at rcar_cmm_setup() time. */
- if (rcmm->lut.enabled) {
rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);rcar_cmm_lut_write(rcmm);- }
- rcmm->enabled = true;
- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_enable);
+/*
- rcar_cmm_disable() - Disable the CMM unit.
- @pdev: The platform device associated with the CMM instance
- Disable the CMM unit by stopping the parent clock.
- */
+void rcar_cmm_disable(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
- pm_runtime_put(&pdev->dev);
- rcmm->lut.enabled = false;
- rcmm->enabled = false;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_disable);
+/*
- rcar_cmm_init() - Make sure the CMM has probed.
I would document this as "Intialize the CMM" to match the function name. We may add more initialization in the future.
- @pdev: The platform device associated with the CMM instance
- Return: 0 if the CMM has probed, -EPROBE_DEFER otherwise
0 on success, -EPROBE_DEFER is the CMM isn't availablet yet
- */
+int rcar_cmm_init(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- if (!rcmm)
return -EPROBE_DEFER;- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_init);
+static int rcar_cmm_probe(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm;
- rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
- if (!rcmm)
return -ENOMEM;- platform_set_drvdata(pdev, rcmm);
- rcmm->base = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(rcmm->base))
return PTR_ERR(rcmm->base);- pm_runtime_enable(&pdev->dev);
- return 0;
+}
+static int rcar_cmm_remove(struct platform_device *pdev) +{
- pm_runtime_disable(&pdev->dev);
- return 0;
+}
+static const struct of_device_id rcar_cmm_of_table[] = {
- { .compatible = "renesas,rcar-gen3-cmm", },
- { .compatible = "renesas,rcar-gen2-cmm", },
- { },
+}; +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
+static struct platform_driver rcar_cmm_platform_driver = {
- .probe = rcar_cmm_probe,
- .remove = rcar_cmm_remove,
- .driver = {
.name = "rcar-cmm",.of_match_table = rcar_cmm_of_table,- },
+};
+module_platform_driver(rcar_cmm_platform_driver);
+MODULE_AUTHOR("Jacopo Mondi jacopo+renesas@jmondi.org"); +MODULE_DESCRIPTION("Renesas R-Car CMM Driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h new file mode 100644 index 000000000000..15a2c874b6a6 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h @@ -0,0 +1,61 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- rcar_cmm.h -- R-Car Display Unit Color Management Module
- Copyright (C) 2019 Jacopo Mondi jacopo+renesas@jmondi.org
- */
+#ifndef __RCAR_CMM_H__ +#define __RCAR_CMM_H__
+#define CM2_LUT_SIZE 256
+struct drm_color_lut; +struct platform_device;
+/**
- struct rcar_cmm_config - CMM configuration
- @lut: 1D-LUT configuration
- @lut.enable: 1D-LUT enable flag
- @lut.table: 1D-LUT table entries. Might be set to NULL when the CMM has to
be re-enabled but not re=programmed.
s/re=programmed/re-programmed/
As discussed offline this can't really happen as far as I can tell. However, it will still be useful when we'll add CLU support, as then a CLU reprogramming without a LUT reprogramming could happen.
I think we should make the documentation a bit clearer:
"1D-LUT table entries. Only valid when lut.enable is true, shall be NULL otherwise. When non-NULL, the LUT table will be programmed with the new values. Otherwise the LUT table will retain its previously programmed values."
This being said, the code in rcar_cmm_setup() will crash if table is NULL. I would either drop the option of table being NULL (and thus update the documentation here) if you don't need this yet in the DU driver, or fix rcar_cmm_setup(). You've posted enough versions of this series in my opinion, so please pick the easiest option, and we'll rework the code when adding CLU support anyway.
With those small issues fixes,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
- */
+struct rcar_cmm_config {
- struct {
bool enable;struct drm_color_lut *table;- } lut;
+};
+#if IS_ENABLED(CONFIG_DRM_RCAR_CMM) +int rcar_cmm_init(struct platform_device *pdev);
+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);+#else +static inline int rcar_cmm_init(struct platform_device *pdev) +{
- return 0;
+}
+static inline int rcar_cmm_enable(struct platform_device *pdev) +{
- return 0;
+}
+static inline void rcar_cmm_disable(struct platform_device *pdev) +{ +}
+static int rcar_cmm_setup(struct platform_device *pdev,
const struct rcar_cmm_config *config)+{
- return 0;
+} +#endif /* IS_ENABLED(CONFIG_DRM_RCAR_CMM) */
+#endif /* __RCAR_CMM_H__ */
Hi Jacopo, Laurent,
On 18/09/2019 23:55, Laurent Pinchart wrote:
Hi Jacopo,
Thank you for the patch.
On Fri, Sep 06, 2019 at 03:54:30PM +0200, Jacopo Mondi wrote:
Add a driver for the R-Car Display Unit Color Correction Module.
In most of Gen3 SoCs, each DU output channel is provided with a CMM unit to perform image enhancement and color correction.
Add support for CMM through a driver that supports configuration of the 1-dimensional LUT table. More advanced CMM feature will be implemented on top of this basic one.
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
drivers/gpu/drm/rcar-du/Kconfig | 7 + drivers/gpu/drm/rcar-du/Makefile | 1 + drivers/gpu/drm/rcar-du/rcar_cmm.c | 251 +++++++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 61 +++++++ 4 files changed, 320 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..3cacdc4474c7 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -0,0 +1,251 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- rcar_cmm.c -- R-Car Display Unit Color Management Module
- Copyright (C) 2019 Jacopo Mondi jacopo+renesas@jmondi.org
- */
+#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h>
+#include <drm/drm_color_mgmt.h>
+#include "rcar_cmm.h"
+#define CM2_LUT_CTRL 0x0000 +#define CM2_LUT_CTRL_LUT_EN BIT(0) +#define CM2_LUT_TBL_BASE 0x0600 +#define CM2_LUT_TBL(__i) (CM2_LUT_TBL_BASE + (__i) * 4)
+struct rcar_cmm {
- void __iomem *base;
- bool enabled;
- /*
* @lut: 1D-LUT status* @lut.enabled: 1D-LUT enabled flag* @lut.table: Table of 1D-LUT entries scaled to hardware* precision (8-bits per color component)*/- struct {
bool enabled;u32 table[CM2_LUT_SIZE];- } lut;
+};
+static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg) +{
- return ioread32(rcmm->base + reg);
+}
+static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data) +{
- iowrite32(data, rcmm->base + reg);
+}
+/*
- rcar_cmm_lut_extract() - Scale down to hardware precision the DRM LUT table
entries and store them."Scale the DRM LUT table entries to hardware precision and store them."
- @rcmm: Pointer to the CMM device
- @drm_lut: Pointer to the DRM LUT table
- */
+static void rcar_cmm_lut_extract(struct rcar_cmm *rcmm,
const struct drm_color_lut *drm_lut)+{
- unsigned int i;
- for (i = 0; i < CM2_LUT_SIZE; ++i) {
const struct drm_color_lut *lut = &drm_lut[i];rcmm->lut.table[i] = drm_color_lut_extract(lut->red, 8) << 16| drm_color_lut_extract(lut->green, 8) << 8| drm_color_lut_extract(lut->blue, 8);- }
+}
+/*
- rcar_cmm_lut_write() - Write to hardware the LUT table entries from the
local table."Write the LUT table entries from the local table to the hardware."
- @rcmm: Pointer to the CMM device
- */
+static void rcar_cmm_lut_write(struct rcar_cmm *rcmm) +{
- unsigned int i;
- for (i = 0; i < CM2_LUT_SIZE; ++i)
rcar_cmm_write(rcmm, CM2_LUT_TBL(i), rcmm->lut.table[i]);+}
+/*
- 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);
- /*
* As rcar_cmm_setup() is called by atomic commit tail helper, it might* be called when the CMM is disabled. As we can't program the hardware* in that case, store the configuration internally and apply it when* the CMM will be enabled by the CRTC through rcar_cmm_enable().*/- if (!rcmm->enabled) {
if (!config->lut.enable)return 0;rcar_cmm_lut_extract(rcmm, config->lut.table);rcmm->lut.enabled = true;return 0;- }
- /* Stop LUT operations if requested. */
- if (!config->lut.enable) {
if (rcmm->lut.enabled) {rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);rcmm->lut.enabled = false;}return 0;- }
- /*
* Enable LUT and program the new gamma table values.** FIXME: In order to have stable operations it is required to first* enable the 1D-LUT and then program its table entries. This seems to* contradict what the chip manual reports, and will have to be* reconsidered when implementing support for double buffering.*/- if (!rcmm->lut.enabled) {
rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);rcmm->lut.enabled = true;- }
- rcar_cmm_lut_extract(rcmm, config->lut.table);
- rcar_cmm_lut_write(rcmm);
- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_setup);
+/*
- rcar_cmm_enable() - Enable the CMM unit.
- @pdev: The platform device associated with the CMM instance
- Enable the CMM unit by enabling the parent clock and enabling the CMM
- components, such as 1-D LUT, if requested.
- */
+int rcar_cmm_enable(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- int ret;
- ret = pm_runtime_get_sync(&pdev->dev);
- if (ret < 0)
return ret;- /* Apply the LUT table values saved at rcar_cmm_setup() time. */
- if (rcmm->lut.enabled) {
rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);rcar_cmm_lut_write(rcmm);- }
- rcmm->enabled = true;
- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_enable);
+/*
- rcar_cmm_disable() - Disable the CMM unit.
- @pdev: The platform device associated with the CMM instance
- Disable the CMM unit by stopping the parent clock.
- */
+void rcar_cmm_disable(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
- pm_runtime_put(&pdev->dev);
- rcmm->lut.enabled = false;
- rcmm->enabled = false;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_disable);
+/*
- rcar_cmm_init() - Make sure the CMM has probed.
I would document this as "Intialize the CMM" to match the function name. We may add more initialization in the future.
- @pdev: The platform device associated with the CMM instance
- Return: 0 if the CMM has probed, -EPROBE_DEFER otherwise
0 on success, -EPROBE_DEFER is the CMM isn't availablet yet
And possibly -ENODEV if the suggestion below in the header is worthy, to return no device if the CMM is disabled at the CONFIG_ level.
- */
+int rcar_cmm_init(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- if (!rcmm)
return -EPROBE_DEFER;- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_init);
+static int rcar_cmm_probe(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm;
- rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
- if (!rcmm)
return -ENOMEM;- platform_set_drvdata(pdev, rcmm);
- rcmm->base = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(rcmm->base))
return PTR_ERR(rcmm->base);- pm_runtime_enable(&pdev->dev);
- return 0;
+}
+static int rcar_cmm_remove(struct platform_device *pdev) +{
- pm_runtime_disable(&pdev->dev);
- return 0;
+}
+static const struct of_device_id rcar_cmm_of_table[] = {
- { .compatible = "renesas,rcar-gen3-cmm", },
- { .compatible = "renesas,rcar-gen2-cmm", },
- { },
+}; +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
+static struct platform_driver rcar_cmm_platform_driver = {
- .probe = rcar_cmm_probe,
- .remove = rcar_cmm_remove,
- .driver = {
.name = "rcar-cmm",.of_match_table = rcar_cmm_of_table,- },
+};
+module_platform_driver(rcar_cmm_platform_driver);
+MODULE_AUTHOR("Jacopo Mondi jacopo+renesas@jmondi.org"); +MODULE_DESCRIPTION("Renesas R-Car CMM Driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h new file mode 100644 index 000000000000..15a2c874b6a6 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h @@ -0,0 +1,61 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- rcar_cmm.h -- R-Car Display Unit Color Management Module
- Copyright (C) 2019 Jacopo Mondi jacopo+renesas@jmondi.org
- */
+#ifndef __RCAR_CMM_H__ +#define __RCAR_CMM_H__
+#define CM2_LUT_SIZE 256
+struct drm_color_lut; +struct platform_device;
+/**
- struct rcar_cmm_config - CMM configuration
- @lut: 1D-LUT configuration
- @lut.enable: 1D-LUT enable flag
- @lut.table: 1D-LUT table entries. Might be set to NULL when the CMM has to
be re-enabled but not re=programmed.s/re=programmed/re-programmed/
As discussed offline this can't really happen as far as I can tell. However, it will still be useful when we'll add CLU support, as then a CLU reprogramming without a LUT reprogramming could happen.
I think we should make the documentation a bit clearer:
"1D-LUT table entries. Only valid when lut.enable is true, shall be NULL otherwise. When non-NULL, the LUT table will be programmed with the new values. Otherwise the LUT table will retain its previously programmed values."
This being said, the code in rcar_cmm_setup() will crash if table is NULL. I would either drop the option of table being NULL (and thus update the documentation here) if you don't need this yet in the DU driver, or fix rcar_cmm_setup(). You've posted enough versions of this series in my opinion, so please pick the easiest option, and we'll rework the code when adding CLU support anyway.
With those small issues fixes,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
- */
+struct rcar_cmm_config {
- struct {
bool enable;struct drm_color_lut *table;- } lut;
+};
+#if IS_ENABLED(CONFIG_DRM_RCAR_CMM) +int rcar_cmm_init(struct platform_device *pdev);
+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);+#else +static inline int rcar_cmm_init(struct platform_device *pdev) +{
- return 0;
I feel like here, this should return -ENODEV if CMM is disabled, and allow rcar_du_cmm_init() to succeed, but leave the rcdu->cmms[i] unpopulated. ...
But this could be updated later too if it's awkward ...
-- Kieran
+}
+static inline int rcar_cmm_enable(struct platform_device *pdev) +{
- return 0;
+}
+static inline void rcar_cmm_disable(struct platform_device *pdev) +{ +}
+static int rcar_cmm_setup(struct platform_device *pdev,
const struct rcar_cmm_config *config)+{
- return 0;
+} +#endif /* IS_ENABLED(CONFIG_DRM_RCAR_CMM) */
+#endif /* __RCAR_CMM_H__ */
Hi Kieran,
On Thu, Sep 19, 2019 at 09:59:39AM +0100, Kieran Bingham wrote:
On 18/09/2019 23:55, Laurent Pinchart wrote:
On Fri, Sep 06, 2019 at 03:54:30PM +0200, Jacopo Mondi wrote:
Add a driver for the R-Car Display Unit Color Correction Module.
In most of Gen3 SoCs, each DU output channel is provided with a CMM unit to perform image enhancement and color correction.
Add support for CMM through a driver that supports configuration of the 1-dimensional LUT table. More advanced CMM feature will be implemented on top of this basic one.
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
drivers/gpu/drm/rcar-du/Kconfig | 7 + drivers/gpu/drm/rcar-du/Makefile | 1 + drivers/gpu/drm/rcar-du/rcar_cmm.c | 251 +++++++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 61 +++++++ 4 files changed, 320 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..3cacdc4474c7 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -0,0 +1,251 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- rcar_cmm.c -- R-Car Display Unit Color Management Module
- Copyright (C) 2019 Jacopo Mondi jacopo+renesas@jmondi.org
- */
+#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h>
+#include <drm/drm_color_mgmt.h>
+#include "rcar_cmm.h"
+#define CM2_LUT_CTRL 0x0000 +#define CM2_LUT_CTRL_LUT_EN BIT(0) +#define CM2_LUT_TBL_BASE 0x0600 +#define CM2_LUT_TBL(__i) (CM2_LUT_TBL_BASE + (__i) * 4)
+struct rcar_cmm {
- void __iomem *base;
- bool enabled;
- /*
* @lut: 1D-LUT status* @lut.enabled: 1D-LUT enabled flag* @lut.table: Table of 1D-LUT entries scaled to hardware* precision (8-bits per color component)*/- struct {
bool enabled;u32 table[CM2_LUT_SIZE];- } lut;
+};
+static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg) +{
- return ioread32(rcmm->base + reg);
+}
+static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data) +{
- iowrite32(data, rcmm->base + reg);
+}
+/*
- rcar_cmm_lut_extract() - Scale down to hardware precision the DRM LUT table
entries and store them."Scale the DRM LUT table entries to hardware precision and store them."
- @rcmm: Pointer to the CMM device
- @drm_lut: Pointer to the DRM LUT table
- */
+static void rcar_cmm_lut_extract(struct rcar_cmm *rcmm,
const struct drm_color_lut *drm_lut)+{
- unsigned int i;
- for (i = 0; i < CM2_LUT_SIZE; ++i) {
const struct drm_color_lut *lut = &drm_lut[i];rcmm->lut.table[i] = drm_color_lut_extract(lut->red, 8) << 16| drm_color_lut_extract(lut->green, 8) << 8| drm_color_lut_extract(lut->blue, 8);- }
+}
+/*
- rcar_cmm_lut_write() - Write to hardware the LUT table entries from the
local table."Write the LUT table entries from the local table to the hardware."
- @rcmm: Pointer to the CMM device
- */
+static void rcar_cmm_lut_write(struct rcar_cmm *rcmm) +{
- unsigned int i;
- for (i = 0; i < CM2_LUT_SIZE; ++i)
rcar_cmm_write(rcmm, CM2_LUT_TBL(i), rcmm->lut.table[i]);+}
+/*
- 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);
- /*
* As rcar_cmm_setup() is called by atomic commit tail helper, it might* be called when the CMM is disabled. As we can't program the hardware* in that case, store the configuration internally and apply it when* the CMM will be enabled by the CRTC through rcar_cmm_enable().*/- if (!rcmm->enabled) {
if (!config->lut.enable)return 0;rcar_cmm_lut_extract(rcmm, config->lut.table);rcmm->lut.enabled = true;return 0;- }
- /* Stop LUT operations if requested. */
- if (!config->lut.enable) {
if (rcmm->lut.enabled) {rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);rcmm->lut.enabled = false;}return 0;- }
- /*
* Enable LUT and program the new gamma table values.** FIXME: In order to have stable operations it is required to first* enable the 1D-LUT and then program its table entries. This seems to* contradict what the chip manual reports, and will have to be* reconsidered when implementing support for double buffering.*/- if (!rcmm->lut.enabled) {
rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);rcmm->lut.enabled = true;- }
- rcar_cmm_lut_extract(rcmm, config->lut.table);
- rcar_cmm_lut_write(rcmm);
- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_setup);
+/*
- rcar_cmm_enable() - Enable the CMM unit.
- @pdev: The platform device associated with the CMM instance
- Enable the CMM unit by enabling the parent clock and enabling the CMM
- components, such as 1-D LUT, if requested.
- */
+int rcar_cmm_enable(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- int ret;
- ret = pm_runtime_get_sync(&pdev->dev);
- if (ret < 0)
return ret;- /* Apply the LUT table values saved at rcar_cmm_setup() time. */
- if (rcmm->lut.enabled) {
rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);rcar_cmm_lut_write(rcmm);- }
- rcmm->enabled = true;
- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_enable);
+/*
- rcar_cmm_disable() - Disable the CMM unit.
- @pdev: The platform device associated with the CMM instance
- Disable the CMM unit by stopping the parent clock.
- */
+void rcar_cmm_disable(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
- pm_runtime_put(&pdev->dev);
- rcmm->lut.enabled = false;
- rcmm->enabled = false;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_disable);
+/*
- rcar_cmm_init() - Make sure the CMM has probed.
I would document this as "Intialize the CMM" to match the function name. We may add more initialization in the future.
- @pdev: The platform device associated with the CMM instance
- Return: 0 if the CMM has probed, -EPROBE_DEFER otherwise
0 on success, -EPROBE_DEFER is the CMM isn't availablet yet
And possibly -ENODEV if the suggestion below in the header is worthy, to return no device if the CMM is disabled at the CONFIG_ level.
- */
+int rcar_cmm_init(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- if (!rcmm)
return -EPROBE_DEFER;- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_init);
+static int rcar_cmm_probe(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm;
- rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
- if (!rcmm)
return -ENOMEM;- platform_set_drvdata(pdev, rcmm);
- rcmm->base = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(rcmm->base))
return PTR_ERR(rcmm->base);- pm_runtime_enable(&pdev->dev);
- return 0;
+}
+static int rcar_cmm_remove(struct platform_device *pdev) +{
- pm_runtime_disable(&pdev->dev);
- return 0;
+}
+static const struct of_device_id rcar_cmm_of_table[] = {
- { .compatible = "renesas,rcar-gen3-cmm", },
- { .compatible = "renesas,rcar-gen2-cmm", },
- { },
+}; +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
+static struct platform_driver rcar_cmm_platform_driver = {
- .probe = rcar_cmm_probe,
- .remove = rcar_cmm_remove,
- .driver = {
.name = "rcar-cmm",.of_match_table = rcar_cmm_of_table,- },
+};
+module_platform_driver(rcar_cmm_platform_driver);
+MODULE_AUTHOR("Jacopo Mondi jacopo+renesas@jmondi.org"); +MODULE_DESCRIPTION("Renesas R-Car CMM Driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h new file mode 100644 index 000000000000..15a2c874b6a6 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h @@ -0,0 +1,61 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- rcar_cmm.h -- R-Car Display Unit Color Management Module
- Copyright (C) 2019 Jacopo Mondi jacopo+renesas@jmondi.org
- */
+#ifndef __RCAR_CMM_H__ +#define __RCAR_CMM_H__
+#define CM2_LUT_SIZE 256
+struct drm_color_lut; +struct platform_device;
+/**
- struct rcar_cmm_config - CMM configuration
- @lut: 1D-LUT configuration
- @lut.enable: 1D-LUT enable flag
- @lut.table: 1D-LUT table entries. Might be set to NULL when the CMM has to
be re-enabled but not re=programmed.s/re=programmed/re-programmed/
As discussed offline this can't really happen as far as I can tell. However, it will still be useful when we'll add CLU support, as then a CLU reprogramming without a LUT reprogramming could happen.
I think we should make the documentation a bit clearer:
"1D-LUT table entries. Only valid when lut.enable is true, shall be NULL otherwise. When non-NULL, the LUT table will be programmed with the new values. Otherwise the LUT table will retain its previously programmed values."
This being said, the code in rcar_cmm_setup() will crash if table is NULL. I would either drop the option of table being NULL (and thus update the documentation here) if you don't need this yet in the DU driver, or fix rcar_cmm_setup(). You've posted enough versions of this series in my opinion, so please pick the easiest option, and we'll rework the code when adding CLU support anyway.
With those small issues fixes,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
- */
+struct rcar_cmm_config {
- struct {
bool enable;struct drm_color_lut *table;- } lut;
+};
+#if IS_ENABLED(CONFIG_DRM_RCAR_CMM) +int rcar_cmm_init(struct platform_device *pdev);
+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);+#else +static inline int rcar_cmm_init(struct platform_device *pdev) +{
- return 0;
I feel like here, this should return -ENODEV if CMM is disabled, and allow rcar_du_cmm_init() to succeed, but leave the rcdu->cmms[i] unpopulated. ...
I think that's a good idea, otherwise the DU driver will believe that CMM initialisation succeeded, and will configure the DU to route its outputs to the CMM. I don't think that would work nicely without a driver :-) Although, in practice, for the processing (non-statistics) path, the DU output clock may be all that is needed, so the CMM might be operational in its default configuration without the MSTP clock. Still, that's probably not a very safe path, so I'd rather see CMM being disabled on the DU side in that case.
But this could be updated later too if it's awkward ...
Hopefully that would be easy to do :-)
+}
+static inline int rcar_cmm_enable(struct platform_device *pdev) +{
- return 0;
+}
+static inline void rcar_cmm_disable(struct platform_device *pdev) +{ +}
+static int rcar_cmm_setup(struct platform_device *pdev,
const struct rcar_cmm_config *config)+{
- return 0;
+} +#endif /* IS_ENABLED(CONFIG_DRM_RCAR_CMM) */
+#endif /* __RCAR_CMM_H__ */
Hi Laurent,
On Thu, Sep 19, 2019 at 01:55:35AM +0300, Laurent Pinchart wrote:
Hi Jacopo,
Thank you for the patch.
On Fri, Sep 06, 2019 at 03:54:30PM +0200, Jacopo Mondi wrote:
Add a driver for the R-Car Display Unit Color Correction Module.
In most of Gen3 SoCs, each DU output channel is provided with a CMM unit to perform image enhancement and color correction.
Add support for CMM through a driver that supports configuration of the 1-dimensional LUT table. More advanced CMM feature will be implemented on top of this basic one.
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
drivers/gpu/drm/rcar-du/Kconfig | 7 + drivers/gpu/drm/rcar-du/Makefile | 1 + drivers/gpu/drm/rcar-du/rcar_cmm.c | 251 +++++++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 61 +++++++ 4 files changed, 320 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..3cacdc4474c7 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -0,0 +1,251 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- rcar_cmm.c -- R-Car Display Unit Color Management Module
- Copyright (C) 2019 Jacopo Mondi jacopo+renesas@jmondi.org
- */
+#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h>
+#include <drm/drm_color_mgmt.h>
+#include "rcar_cmm.h"
+#define CM2_LUT_CTRL 0x0000 +#define CM2_LUT_CTRL_LUT_EN BIT(0) +#define CM2_LUT_TBL_BASE 0x0600 +#define CM2_LUT_TBL(__i) (CM2_LUT_TBL_BASE + (__i) * 4)
+struct rcar_cmm {
- void __iomem *base;
- bool enabled;
- /*
* @lut: 1D-LUT status* @lut.enabled: 1D-LUT enabled flag* @lut.table: Table of 1D-LUT entries scaled to hardware* precision (8-bits per color component)*/- struct {
bool enabled;u32 table[CM2_LUT_SIZE];- } lut;
+};
+static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg) +{
- return ioread32(rcmm->base + reg);
+}
+static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data) +{
- iowrite32(data, rcmm->base + reg);
+}
+/*
- rcar_cmm_lut_extract() - Scale down to hardware precision the DRM LUT table
entries and store them."Scale the DRM LUT table entries to hardware precision and store them."
- @rcmm: Pointer to the CMM device
- @drm_lut: Pointer to the DRM LUT table
- */
+static void rcar_cmm_lut_extract(struct rcar_cmm *rcmm,
const struct drm_color_lut *drm_lut)+{
- unsigned int i;
- for (i = 0; i < CM2_LUT_SIZE; ++i) {
const struct drm_color_lut *lut = &drm_lut[i];rcmm->lut.table[i] = drm_color_lut_extract(lut->red, 8) << 16| drm_color_lut_extract(lut->green, 8) << 8| drm_color_lut_extract(lut->blue, 8);- }
+}
+/*
- rcar_cmm_lut_write() - Write to hardware the LUT table entries from the
local table."Write the LUT table entries from the local table to the hardware."
- @rcmm: Pointer to the CMM device
- */
+static void rcar_cmm_lut_write(struct rcar_cmm *rcmm) +{
- unsigned int i;
- for (i = 0; i < CM2_LUT_SIZE; ++i)
rcar_cmm_write(rcmm, CM2_LUT_TBL(i), rcmm->lut.table[i]);+}
+/*
- 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);
- /*
* As rcar_cmm_setup() is called by atomic commit tail helper, it might* be called when the CMM is disabled. As we can't program the hardware* in that case, store the configuration internally and apply it when* the CMM will be enabled by the CRTC through rcar_cmm_enable().*/- if (!rcmm->enabled) {
if (!config->lut.enable)return 0;rcar_cmm_lut_extract(rcmm, config->lut.table);rcmm->lut.enabled = true;return 0;- }
- /* Stop LUT operations if requested. */
- if (!config->lut.enable) {
if (rcmm->lut.enabled) {rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);rcmm->lut.enabled = false;}return 0;- }
- /*
* Enable LUT and program the new gamma table values.** FIXME: In order to have stable operations it is required to first* enable the 1D-LUT and then program its table entries. This seems to* contradict what the chip manual reports, and will have to be* reconsidered when implementing support for double buffering.*/- if (!rcmm->lut.enabled) {
rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);rcmm->lut.enabled = true;- }
- rcar_cmm_lut_extract(rcmm, config->lut.table);
- rcar_cmm_lut_write(rcmm);
- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_setup);
+/*
- rcar_cmm_enable() - Enable the CMM unit.
- @pdev: The platform device associated with the CMM instance
- Enable the CMM unit by enabling the parent clock and enabling the CMM
- components, such as 1-D LUT, if requested.
- */
+int rcar_cmm_enable(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- int ret;
- ret = pm_runtime_get_sync(&pdev->dev);
- if (ret < 0)
return ret;- /* Apply the LUT table values saved at rcar_cmm_setup() time. */
- if (rcmm->lut.enabled) {
rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);rcar_cmm_lut_write(rcmm);- }
- rcmm->enabled = true;
- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_enable);
+/*
- rcar_cmm_disable() - Disable the CMM unit.
- @pdev: The platform device associated with the CMM instance
- Disable the CMM unit by stopping the parent clock.
- */
+void rcar_cmm_disable(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
- pm_runtime_put(&pdev->dev);
- rcmm->lut.enabled = false;
- rcmm->enabled = false;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_disable);
+/*
- rcar_cmm_init() - Make sure the CMM has probed.
I would document this as "Intialize the CMM" to match the function name. We may add more initialization in the future.
- @pdev: The platform device associated with the CMM instance
- Return: 0 if the CMM has probed, -EPROBE_DEFER otherwise
0 on success, -EPROBE_DEFER is the CMM isn't availablet yet
Just to note I've take Kieran's suggestion in to return -ENODEV when the CMM config option is not set.
- */
+int rcar_cmm_init(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- if (!rcmm)
return -EPROBE_DEFER;- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_init);
+static int rcar_cmm_probe(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm;
- rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
- if (!rcmm)
return -ENOMEM;- platform_set_drvdata(pdev, rcmm);
- rcmm->base = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(rcmm->base))
return PTR_ERR(rcmm->base);- pm_runtime_enable(&pdev->dev);
- return 0;
+}
+static int rcar_cmm_remove(struct platform_device *pdev) +{
- pm_runtime_disable(&pdev->dev);
- return 0;
+}
+static const struct of_device_id rcar_cmm_of_table[] = {
- { .compatible = "renesas,rcar-gen3-cmm", },
- { .compatible = "renesas,rcar-gen2-cmm", },
- { },
+}; +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
+static struct platform_driver rcar_cmm_platform_driver = {
- .probe = rcar_cmm_probe,
- .remove = rcar_cmm_remove,
- .driver = {
.name = "rcar-cmm",.of_match_table = rcar_cmm_of_table,- },
+};
+module_platform_driver(rcar_cmm_platform_driver);
+MODULE_AUTHOR("Jacopo Mondi jacopo+renesas@jmondi.org"); +MODULE_DESCRIPTION("Renesas R-Car CMM Driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h new file mode 100644 index 000000000000..15a2c874b6a6 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h @@ -0,0 +1,61 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- rcar_cmm.h -- R-Car Display Unit Color Management Module
- Copyright (C) 2019 Jacopo Mondi jacopo+renesas@jmondi.org
- */
+#ifndef __RCAR_CMM_H__ +#define __RCAR_CMM_H__
+#define CM2_LUT_SIZE 256
+struct drm_color_lut; +struct platform_device;
+/**
- struct rcar_cmm_config - CMM configuration
- @lut: 1D-LUT configuration
- @lut.enable: 1D-LUT enable flag
- @lut.table: 1D-LUT table entries. Might be set to NULL when the CMM has to
be re-enabled but not re=programmed.s/re=programmed/re-programmed/
As discussed offline this can't really happen as far as I can tell. However, it will still be useful when we'll add CLU support, as then a CLU reprogramming without a LUT reprogramming could happen.
How are we going to program CLU ? Using which DRM property ? I had a look at 'ctm', but it does not seems to apply, as it only provides 9 64bits entries (why 9??) https://elixir.bootlin.com/linux/latest/source/include/uapi/drm/drm_mode.h#L...
The CLU is programmed with pairs of 32bits entries, assembled using 8 bit data/addresses for each color components. Considering the LUT entries have 16 bits per color components, we could re-use those, but seems a bit of an abuse to me. Is there somethig more trivial I am missing ?
Anyway, for now I think you are right, and if the 'crtc->state->gamma_lut' field is set, then the table (provided it's of the right size) cannot be NULL.
Disabling the CMM with: req.add(crtc, { { "GAMMA_LUT", 0 }, });
results in 'crtc->state->gamma_lut' being not set, and this is already handled by setting to false the rcar_cmm_config.lut.enable flag in rcar_du_atomic_commit_update_cmm() so I would rather drop the table == NULL case from documentation and re-think about it once we handle CLU.
I think we should make the documentation a bit clearer:
"1D-LUT table entries. Only valid when lut.enable is true, shall be NULL otherwise. When non-NULL, the LUT table will be programmed with the new values. Otherwise the LUT table will retain its previously programmed values."
This being said, the code in rcar_cmm_setup() will crash if table is NULL. I would either drop the option of table being NULL (and thus update the documentation here) if you don't need this yet in the DU driver, or fix rcar_cmm_setup(). You've posted enough versions of this series in my opinion, so please pick the easiest option, and we'll rework the code when adding CLU support anyway.
Unfortunately I would love to take your tag in and being done with this, but considering Sean's advices on Ezequiel's series, I think I'll now need to move CMM handling to crtc's begin/enable as well, so another review round will likely be required.
With those small issues fixes,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
- */
+struct rcar_cmm_config {
- struct {
bool enable;struct drm_color_lut *table;- } lut;
+};
+#if IS_ENABLED(CONFIG_DRM_RCAR_CMM) +int rcar_cmm_init(struct platform_device *pdev);
+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);+#else +static inline int rcar_cmm_init(struct platform_device *pdev) +{
- return 0;
+}
+static inline int rcar_cmm_enable(struct platform_device *pdev) +{
- return 0;
+}
+static inline void rcar_cmm_disable(struct platform_device *pdev) +{ +}
+static int rcar_cmm_setup(struct platform_device *pdev,
const struct rcar_cmm_config *config)+{
- return 0;
+} +#endif /* IS_ENABLED(CONFIG_DRM_RCAR_CMM) */
+#endif /* __RCAR_CMM_H__ */
-- Regards,
Laurent Pinchart
Hi Jacopo,
On Thu, Oct 10, 2019 at 07:46:28PM +0200, Jacopo Mondi wrote:
On Thu, Sep 19, 2019 at 01:55:35AM +0300, Laurent Pinchart wrote:
On Fri, Sep 06, 2019 at 03:54:30PM +0200, Jacopo Mondi wrote:
Add a driver for the R-Car Display Unit Color Correction Module.
In most of Gen3 SoCs, each DU output channel is provided with a CMM unit to perform image enhancement and color correction.
Add support for CMM through a driver that supports configuration of the 1-dimensional LUT table. More advanced CMM feature will be implemented on top of this basic one.
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
drivers/gpu/drm/rcar-du/Kconfig | 7 + drivers/gpu/drm/rcar-du/Makefile | 1 + drivers/gpu/drm/rcar-du/rcar_cmm.c | 251 +++++++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 61 +++++++ 4 files changed, 320 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..3cacdc4474c7 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -0,0 +1,251 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- rcar_cmm.c -- R-Car Display Unit Color Management Module
- Copyright (C) 2019 Jacopo Mondi jacopo+renesas@jmondi.org
- */
+#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h>
+#include <drm/drm_color_mgmt.h>
+#include "rcar_cmm.h"
+#define CM2_LUT_CTRL 0x0000 +#define CM2_LUT_CTRL_LUT_EN BIT(0) +#define CM2_LUT_TBL_BASE 0x0600 +#define CM2_LUT_TBL(__i) (CM2_LUT_TBL_BASE + (__i) * 4)
+struct rcar_cmm {
- void __iomem *base;
- bool enabled;
- /*
* @lut: 1D-LUT status* @lut.enabled: 1D-LUT enabled flag* @lut.table: Table of 1D-LUT entries scaled to hardware* precision (8-bits per color component)*/- struct {
bool enabled;u32 table[CM2_LUT_SIZE];- } lut;
+};
+static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg) +{
- return ioread32(rcmm->base + reg);
+}
+static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data) +{
- iowrite32(data, rcmm->base + reg);
+}
+/*
- rcar_cmm_lut_extract() - Scale down to hardware precision the DRM LUT table
entries and store them."Scale the DRM LUT table entries to hardware precision and store them."
- @rcmm: Pointer to the CMM device
- @drm_lut: Pointer to the DRM LUT table
- */
+static void rcar_cmm_lut_extract(struct rcar_cmm *rcmm,
const struct drm_color_lut *drm_lut)+{
- unsigned int i;
- for (i = 0; i < CM2_LUT_SIZE; ++i) {
const struct drm_color_lut *lut = &drm_lut[i];rcmm->lut.table[i] = drm_color_lut_extract(lut->red, 8) << 16| drm_color_lut_extract(lut->green, 8) << 8| drm_color_lut_extract(lut->blue, 8);- }
+}
+/*
- rcar_cmm_lut_write() - Write to hardware the LUT table entries from the
local table."Write the LUT table entries from the local table to the hardware."
- @rcmm: Pointer to the CMM device
- */
+static void rcar_cmm_lut_write(struct rcar_cmm *rcmm) +{
- unsigned int i;
- for (i = 0; i < CM2_LUT_SIZE; ++i)
rcar_cmm_write(rcmm, CM2_LUT_TBL(i), rcmm->lut.table[i]);+}
+/*
- 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);
- /*
* As rcar_cmm_setup() is called by atomic commit tail helper, it might* be called when the CMM is disabled. As we can't program the hardware* in that case, store the configuration internally and apply it when* the CMM will be enabled by the CRTC through rcar_cmm_enable().*/- if (!rcmm->enabled) {
if (!config->lut.enable)return 0;rcar_cmm_lut_extract(rcmm, config->lut.table);rcmm->lut.enabled = true;return 0;- }
- /* Stop LUT operations if requested. */
- if (!config->lut.enable) {
if (rcmm->lut.enabled) {rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);rcmm->lut.enabled = false;}return 0;- }
- /*
* Enable LUT and program the new gamma table values.** FIXME: In order to have stable operations it is required to first* enable the 1D-LUT and then program its table entries. This seems to* contradict what the chip manual reports, and will have to be* reconsidered when implementing support for double buffering.*/- if (!rcmm->lut.enabled) {
rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);rcmm->lut.enabled = true;- }
- rcar_cmm_lut_extract(rcmm, config->lut.table);
- rcar_cmm_lut_write(rcmm);
- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_setup);
+/*
- rcar_cmm_enable() - Enable the CMM unit.
- @pdev: The platform device associated with the CMM instance
- Enable the CMM unit by enabling the parent clock and enabling the CMM
- components, such as 1-D LUT, if requested.
- */
+int rcar_cmm_enable(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- int ret;
- ret = pm_runtime_get_sync(&pdev->dev);
- if (ret < 0)
return ret;- /* Apply the LUT table values saved at rcar_cmm_setup() time. */
- if (rcmm->lut.enabled) {
rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);rcar_cmm_lut_write(rcmm);- }
- rcmm->enabled = true;
- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_enable);
+/*
- rcar_cmm_disable() - Disable the CMM unit.
- @pdev: The platform device associated with the CMM instance
- Disable the CMM unit by stopping the parent clock.
- */
+void rcar_cmm_disable(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
- pm_runtime_put(&pdev->dev);
- rcmm->lut.enabled = false;
- rcmm->enabled = false;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_disable);
+/*
- rcar_cmm_init() - Make sure the CMM has probed.
I would document this as "Intialize the CMM" to match the function name. We may add more initialization in the future.
- @pdev: The platform device associated with the CMM instance
- Return: 0 if the CMM has probed, -EPROBE_DEFER otherwise
0 on success, -EPROBE_DEFER is the CMM isn't availablet yet
Just to note I've take Kieran's suggestion in to return -ENODEV when the CMM config option is not set.
- */
+int rcar_cmm_init(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- if (!rcmm)
return -EPROBE_DEFER;- return 0;
+} +EXPORT_SYMBOL_GPL(rcar_cmm_init);
+static int rcar_cmm_probe(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm;
- rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
- if (!rcmm)
return -ENOMEM;- platform_set_drvdata(pdev, rcmm);
- rcmm->base = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(rcmm->base))
return PTR_ERR(rcmm->base);- pm_runtime_enable(&pdev->dev);
- return 0;
+}
+static int rcar_cmm_remove(struct platform_device *pdev) +{
- pm_runtime_disable(&pdev->dev);
- return 0;
+}
+static const struct of_device_id rcar_cmm_of_table[] = {
- { .compatible = "renesas,rcar-gen3-cmm", },
- { .compatible = "renesas,rcar-gen2-cmm", },
- { },
+}; +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
+static struct platform_driver rcar_cmm_platform_driver = {
- .probe = rcar_cmm_probe,
- .remove = rcar_cmm_remove,
- .driver = {
.name = "rcar-cmm",.of_match_table = rcar_cmm_of_table,- },
+};
+module_platform_driver(rcar_cmm_platform_driver);
+MODULE_AUTHOR("Jacopo Mondi jacopo+renesas@jmondi.org"); +MODULE_DESCRIPTION("Renesas R-Car CMM Driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h new file mode 100644 index 000000000000..15a2c874b6a6 --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h @@ -0,0 +1,61 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- rcar_cmm.h -- R-Car Display Unit Color Management Module
- Copyright (C) 2019 Jacopo Mondi jacopo+renesas@jmondi.org
- */
+#ifndef __RCAR_CMM_H__ +#define __RCAR_CMM_H__
+#define CM2_LUT_SIZE 256
+struct drm_color_lut; +struct platform_device;
+/**
- struct rcar_cmm_config - CMM configuration
- @lut: 1D-LUT configuration
- @lut.enable: 1D-LUT enable flag
- @lut.table: 1D-LUT table entries. Might be set to NULL when the CMM has to
be re-enabled but not re=programmed.s/re=programmed/re-programmed/
As discussed offline this can't really happen as far as I can tell. However, it will still be useful when we'll add CLU support, as then a CLU reprogramming without a LUT reprogramming could happen.
How are we going to program CLU ? Using which DRM property ? I had a look at 'ctm', but it does not seems to apply, as it only provides 9 64bits entries (why 9??) https://elixir.bootlin.com/linux/latest/source/include/uapi/drm/drm_mode.h#L...
The CLU is programmed with pairs of 32bits entries, assembled using 8 bit data/addresses for each color components. Considering the LUT entries have 16 bits per color components, we could re-use those, but seems a bit of an abuse to me. Is there somethig more trivial I am missing ?
You're not missing anything, we'll have to create a new property for this. The CLU is a 17x17x17 matrix of 32-bit values, that's what we'll have to pass.
Anyway, for now I think you are right, and if the 'crtc->state->gamma_lut' field is set, then the table (provided it's of the right size) cannot be NULL.
Disabling the CMM with: req.add(crtc, { { "GAMMA_LUT", 0 }, });
results in 'crtc->state->gamma_lut' being not set, and this is already handled by setting to false the rcar_cmm_config.lut.enable flag in rcar_du_atomic_commit_update_cmm() so I would rather drop the table == NULL case from documentation and re-think about it once we handle CLU.
I think we should make the documentation a bit clearer:
"1D-LUT table entries. Only valid when lut.enable is true, shall be NULL otherwise. When non-NULL, the LUT table will be programmed with the new values. Otherwise the LUT table will retain its previously programmed values."
This being said, the code in rcar_cmm_setup() will crash if table is NULL. I would either drop the option of table being NULL (and thus update the documentation here) if you don't need this yet in the DU driver, or fix rcar_cmm_setup(). You've posted enough versions of this series in my opinion, so please pick the easiest option, and we'll rework the code when adding CLU support anyway.
Unfortunately I would love to take your tag in and being done with this, but considering Sean's advices on Ezequiel's series, I think I'll now need to move CMM handling to crtc's begin/enable as well, so another review round will likely be required.
With those small issues fixes,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
- */
+struct rcar_cmm_config {
- struct {
bool enable;struct drm_color_lut *table;- } lut;
+};
+#if IS_ENABLED(CONFIG_DRM_RCAR_CMM) +int rcar_cmm_init(struct platform_device *pdev);
+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);+#else +static inline int rcar_cmm_init(struct platform_device *pdev) +{
- return 0;
+}
+static inline int rcar_cmm_enable(struct platform_device *pdev) +{
- return 0;
+}
+static inline void rcar_cmm_disable(struct platform_device *pdev) +{ +}
+static int rcar_cmm_setup(struct platform_device *pdev,
const struct rcar_cmm_config *config)+{
- return 0;
+} +#endif /* IS_ENABLED(CONFIG_DRM_RCAR_CMM) */
+#endif /* __RCAR_CMM_H__ */
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.
Reviewed-by: Ulrich Hecht uli+renesas@fpond.eu Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org --- drivers/gpu/drm/rcar-du/rcar_du_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 6df37c2a9678..018480a8f35c 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -276,7 +276,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 = { /* @@ -309,7 +310,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 = { /* @@ -338,7 +340,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 = { /* @@ -386,7 +389,8 @@ static const struct rcar_du_device_info rcar_du_r8a77970_info = { static const struct rcar_du_device_info rcar_du_r8a7799x_info = { .gen = 3, .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK - | RCAR_DU_FEATURE_VSP1_SOURCE, + | RCAR_DU_FEATURE_VSP1_SOURCE + | RCAR_DU_FEATURE_CMM, .channels_mask = BIT(1) | BIT(0), .routes = { /* diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index 1327cd0df90a..a00dccc447aa 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h @@ -28,6 +28,7 @@ struct rcar_du_encoder; #define RCAR_DU_FEATURE_VSP1_SOURCE BIT(1) /* Has inputs from VSP1 */ #define RCAR_DU_FEATURE_INTERLACED BIT(2) /* HW supports interlaced */ #define RCAR_DU_FEATURE_TVM_SYNC BIT(3) /* Has TV switch/sync modes */ +#define RCAR_DU_FEATURE_CMM BIT(4) /* Has CMM */
#define RCAR_DU_QUIRK_ALIGN_128B BIT(0) /* Align pitches to 128 bytes */
Hi Jacopo,
On 06/09/2019 14:54, 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.
Reviewed-by: Ulrich Hecht uli+renesas@fpond.eu Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
LGTM.
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
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 6df37c2a9678..018480a8f35c 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -276,7 +276,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,@@ -309,7 +310,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,@@ -338,7 +340,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,@@ -386,7 +389,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,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 */
Implement device tree parsing to collect the available CMM instances described by the 'renesas,cmms' property. Associate CMMs with CRTCs and store a mask of active CMMs in the DU group for later enablement.
Enforce the probe and suspend/resume ordering of DU and CMM by creating a stateless device link between the two.
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org --- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 6 +++ drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 + drivers/gpu/drm/rcar-du/rcar_du_drv.h | 2 + drivers/gpu/drm/rcar-du/rcar_du_group.h | 2 + drivers/gpu/drm/rcar-du/rcar_du_kms.c | 68 +++++++++++++++++++++++++ 5 files changed, 80 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..6316a1c5236c 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" @@ -86,6 +87,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_CRTCS]; struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS];
struct { diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h b/drivers/gpu/drm/rcar-du/rcar_du_group.h index 87950c1f6a52..e9906609c635 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_group.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h @@ -22,6 +22,7 @@ struct rcar_du_device; * @mmio_offset: registers offset in the device memory map * @index: group index * @channels_mask: bitmask of populated DU channels in this group + * @cmms_mask: bitmask of available CMMs in this group * @num_crtcs: number of CRTCs in this group (1 or 2) * @use_count: number of users of the group (rcar_du_group_(get|put)) * @used_crtcs: number of CRTCs currently in use @@ -37,6 +38,7 @@ struct rcar_du_group { unsigned int index;
unsigned int channels_mask; + unsigned int cmms_mask; unsigned int num_crtcs; unsigned int use_count; unsigned int used_crtcs; diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 2dc9caee8767..294630e56992 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -17,7 +17,9 @@ #include <drm/drm_probe_helper.h> #include <drm/drm_vblank.h>
+#include <linux/device.h> #include <linux/of_graph.h> +#include <linux/of_platform.h> #include <linux/wait.h>
#include "rcar_du_crtc.h" @@ -614,6 +616,67 @@ 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, "renesas,cmms"); + if (cells == -EINVAL) + return 0; + + if (cells > rcdu->num_crtcs) { + dev_err(rcdu->dev, + "Invalid number of entries in 'renesas,cmms'\n"); + return -EINVAL; + } + + for (i = 0; i < cells; ++i) { + struct platform_device *pdev; + struct device_link *link; + struct device_node *cmm; + int ret; + + cmm = of_parse_phandle(np, "renesas,cmms", i); + if (IS_ERR(cmm)) { + dev_err(rcdu->dev, + "Failed to parse 'renesas,cmms' property\n"); + return PTR_ERR(cmm); + } + + if (!of_device_is_available(cmm)) { + /* It's fine to have a phandle to a non-enabled CMM. */ + of_node_put(cmm); + continue; + } + + pdev = of_find_device_by_node(cmm); + if (IS_ERR(pdev)) { + dev_err(rcdu->dev, "No device found for CMM%u\n", i); + of_node_put(cmm); + return PTR_ERR(pdev); + } + + of_node_put(cmm); + + ret = rcar_cmm_init(pdev); + if (ret) + return ret; + + link = device_link_add(rcdu->dev, &pdev->dev, DL_FLAG_STATELESS); + if (!link) { + dev_err(rcdu->dev, + "Failed to create device link to CMM%u\n", i); + return -EINVAL; + } + + rcdu->cmms[i] = pdev; + } + + return 0; +} + int rcar_du_modeset_init(struct rcar_du_device *rcdu) { static const unsigned int mmio_offsets[] = { @@ -704,6 +767,11 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu) return ret; }
+ /* Initialize the Color Management Modules. */ + ret = rcar_du_cmm_init(rcdu); + if (ret) + return ret; + /* Create the CRTCs. */ for (swindex = 0, hwindex = 0; swindex < rcdu->num_crtcs; ++hwindex) { struct rcar_du_group *rgrp;
Hi Jacopo,
On 06/09/2019 14:54, Jacopo Mondi wrote:
Implement device tree parsing to collect the available CMM instances described by the 'renesas,cmms' property. Associate CMMs with CRTCs and store a mask of active CMMs in the DU group for later enablement.
Enforce the probe and suspend/resume ordering of DU and CMM by creating a stateless device link between the two.
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 6 +++ drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 + drivers/gpu/drm/rcar-du/rcar_du_drv.h | 2 + drivers/gpu/drm/rcar-du/rcar_du_group.h | 2 + drivers/gpu/drm/rcar-du/rcar_du_kms.c | 68 +++++++++++++++++++++++++ 5 files changed, 80 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);- }
Good - this checks out :-D
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..6316a1c5236c 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" @@ -86,6 +87,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_CRTCS]; struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS];
struct {
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h b/drivers/gpu/drm/rcar-du/rcar_du_group.h index 87950c1f6a52..e9906609c635 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_group.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h @@ -22,6 +22,7 @@ struct rcar_du_device;
- @mmio_offset: registers offset in the device memory map
- @index: group index
- @channels_mask: bitmask of populated DU channels in this group
- @cmms_mask: bitmask of available CMMs in this group
- @num_crtcs: number of CRTCs in this group (1 or 2)
- @use_count: number of users of the group (rcar_du_group_(get|put))
- @used_crtcs: number of CRTCs currently in use
@@ -37,6 +38,7 @@ struct rcar_du_group { unsigned int index;
unsigned int channels_mask;
- unsigned int cmms_mask; unsigned int num_crtcs; unsigned int use_count; unsigned int used_crtcs;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 2dc9caee8767..294630e56992 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -17,7 +17,9 @@ #include <drm/drm_probe_helper.h> #include <drm/drm_vblank.h>
+#include <linux/device.h> #include <linux/of_graph.h> +#include <linux/of_platform.h> #include <linux/wait.h>
#include "rcar_du_crtc.h" @@ -614,6 +616,67 @@ 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, "renesas,cmms");
- if (cells == -EINVAL)
return 0;- if (cells > rcdu->num_crtcs) {
dev_err(rcdu->dev,"Invalid number of entries in 'renesas,cmms'\n");return -EINVAL;- }
- for (i = 0; i < cells; ++i) {
struct platform_device *pdev;struct device_link *link;struct device_node *cmm;int ret;cmm = of_parse_phandle(np, "renesas,cmms", i);if (IS_ERR(cmm)) {dev_err(rcdu->dev,"Failed to parse 'renesas,cmms' property\n");return PTR_ERR(cmm);
Should we have a -ENOENT exception here somewhere?
Otherwise I think platforms without a CMM will fail, and new kernels with old DTBs would also fail ?
(based on my interpretation in __of_parse_phandle_with_args() that not finding a requested phandle returns -ENOENT).
Scratch that - I've just seen above with the cells = of_property_count_u32_elems(np, "renesas,cmms");
which will return 0 if this device doesn't have a CMM.
So - this all looks good to me.
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
}if (!of_device_is_available(cmm)) {/* It's fine to have a phandle to a non-enabled CMM. */of_node_put(cmm);continue;}pdev = of_find_device_by_node(cmm);if (IS_ERR(pdev)) {dev_err(rcdu->dev, "No device found for CMM%u\n", i);of_node_put(cmm);return PTR_ERR(pdev);}of_node_put(cmm);ret = rcar_cmm_init(pdev);if (ret)return ret;link = device_link_add(rcdu->dev, &pdev->dev, DL_FLAG_STATELESS);if (!link) {dev_err(rcdu->dev,"Failed to create device link to CMM%u\n", i);return -EINVAL;}rcdu->cmms[i] = pdev;- }
- return 0;
+}
int rcar_du_modeset_init(struct rcar_du_device *rcdu) { static const unsigned int mmio_offsets[] = { @@ -704,6 +767,11 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu) return ret; }
- /* Initialize the Color Management Modules. */
- ret = rcar_du_cmm_init(rcdu);
- if (ret)
return ret;> +
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.
Reviewed-by: Ulrich Hecht uli+renesas@fpond.eu Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org --- drivers/gpu/drm/rcar-du/rcar_du_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..25d0fc125d7a 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 */
Hi Jacopo,
On 06/09/2019 14:54, Jacopo Mondi 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.
Reviewed-by: Ulrich Hecht uli+renesas@fpond.eu Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
drivers/gpu/drm/rcar-du/rcar_du_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..25d0fc125d7a 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);- }
What's the effect here on platforms with a CMM, but with CONFIG_DRM_RCAR_CMM unset?
Will this incorrectly configure the DU ?
Will it stall the display if the DU tries to interact with another module which is not enabled?
if (rcdu->info->gen >= 2) { rcar_du_group_setup_defr8(rgrp); rcar_du_group_setup_didsr(rgrp); diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h index bc87f080b170..fb9964949368 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h @@ -197,6 +197,11 @@ #define DEFR6_MLOS1 (1 << 2) #define DEFR6_DEFAULT (DEFR6_CODE | DEFR6_TCNE1)
+#define DEFR7 0x000ec +#define DEFR7_CODE (0x7779 << 16) +#define DEFR7_CMME1 BIT(6) +#define DEFR7_CMME0 BIT(4)
/* -----------------------------------------------------------------------------
- R8A7790-only Control Registers
*/
Hi Kieran,
On Wed, Sep 11, 2019 at 07:40:27PM +0100, Kieran Bingham wrote:
Hi Jacopo,
On 06/09/2019 14:54, Jacopo Mondi 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.
Reviewed-by: Ulrich Hecht uli+renesas@fpond.eu Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
drivers/gpu/drm/rcar-du/rcar_du_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..25d0fc125d7a 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);- }
What's the effect here on platforms with a CMM, but with CONFIG_DRM_RCAR_CMM unset?
Will this incorrectly configure the DU ?
Will it stall the display if the DU tries to interact with another module which is not enabled?
I recall I tested that (that's why I had to add stubs for CMM functions, as I had linkage errors otherwise) and thing seems to be fine as the CMM configuration/enblement resolve to an empty function.
Would you prefer to have this guarded by an #if IS_ENABLED() ?
Thanks j
if (rcdu->info->gen >= 2) { rcar_du_group_setup_defr8(rgrp); rcar_du_group_setup_didsr(rgrp); diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h index bc87f080b170..fb9964949368 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h @@ -197,6 +197,11 @@ #define DEFR6_MLOS1 (1 << 2) #define DEFR6_DEFAULT (DEFR6_CODE | DEFR6_TCNE1)
+#define DEFR7 0x000ec +#define DEFR7_CODE (0x7779 << 16) +#define DEFR7_CMME1 BIT(6) +#define DEFR7_CMME0 BIT(4)
/* -----------------------------------------------------------------------------
- R8A7790-only Control Registers
*/
Hi Jacopo,
On 12/09/2019 09:07, Jacopo Mondi wrote:
Hi Kieran,
On Wed, Sep 11, 2019 at 07:40:27PM +0100, Kieran Bingham wrote:
Hi Jacopo,
On 06/09/2019 14:54, Jacopo Mondi 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.
Reviewed-by: Ulrich Hecht uli+renesas@fpond.eu Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
drivers/gpu/drm/rcar-du/rcar_du_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..25d0fc125d7a 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);- }
What's the effect here on platforms with a CMM, but with CONFIG_DRM_RCAR_CMM unset?
Will this incorrectly configure the DU ?
Will it stall the display if the DU tries to interact with another module which is not enabled?
I recall I tested that (that's why I had to add stubs for CMM functions, as I had linkage errors otherwise) and thing seems to be fine as the CMM configuration/enblement resolve to an empty function.
Yes, I see the stubs to allow for linkage, but it's the hardware I'm concerned about. If it passes the tests and doesn't break then that's probably ok ... but I'm really weary that we're enabling a hardware pipeline with a disabled component in the middle.
Would you prefer to have this guarded by an #if IS_ENABLED() ?
I don't think we need a compile time conditional, but I'd say it probably needs to be more about whether the CMM has actually probed or not
Aha, and I see that in rcar_du_cmm_init() we already do a call to rcar_cmm_init(), which if fails will leave rcdu->cmms[i] as NULL. So that's catered for, which results in the rgrp->cmms_mask being correctly representative of whether there is a CMM connected or not.
... so I think that means the ... "if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM))" is somewhat redundant:
This could be:
if (rgrp->cmms_mask) { 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);
Or in fact, if we don't mind writing 0 to DEFR7 when there is no CMM (which is safe by the looks of things as DEFR7 is available on all platforms), then we can even remove the outer conditional, and leave this all up to the ternary operators to write the correct value to the defr7.
Phew ... net result - your current code *is* safe with the CONFIG_DRM_RCAR_CMM option disabled. I'll leave it up to you if you want to simplify the code here and remove the RCAR_DU_FEATURE_CMM.
As this RCAR_DU_FEATURE_CMM flag is only checked here, removing it would however simplify all of the rcar_du_device_info structures.
So - with or without the _FEATURE_CMM" simplification, this patch looks functional and safe so:
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
Thanks j
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
*/
Hello,
On Thu, Sep 12, 2019 at 10:19:30AM +0100, Kieran Bingham wrote:
On 12/09/2019 09:07, Jacopo Mondi wrote:
On Wed, Sep 11, 2019 at 07:40:27PM +0100, Kieran Bingham wrote:
On 06/09/2019 14:54, Jacopo Mondi 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.
Reviewed-by: Ulrich Hecht uli+renesas@fpond.eu Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
drivers/gpu/drm/rcar-du/rcar_du_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..25d0fc125d7a 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);- }
What's the effect here on platforms with a CMM, but with CONFIG_DRM_RCAR_CMM unset?
Will this incorrectly configure the DU ?
Will it stall the display if the DU tries to interact with another module which is not enabled?
I recall I tested that (that's why I had to add stubs for CMM functions, as I had linkage errors otherwise) and thing seems to be fine as the CMM configuration/enblement resolve to an empty function.
Yes, I see the stubs to allow for linkage, but it's the hardware I'm concerned about. If it passes the tests and doesn't break then that's probably ok ... but I'm really weary that we're enabling a hardware pipeline with a disabled component in the middle.
Would you prefer to have this guarded by an #if IS_ENABLED() ?
I don't think we need a compile time conditional, but I'd say it probably needs to be more about whether the CMM has actually probed or not
Aha, and I see that in rcar_du_cmm_init() we already do a call to rcar_cmm_init(), which if fails will leave rcdu->cmms[i] as NULL. So that's catered for, which results in the rgrp->cmms_mask being correctly representative of whether there is a CMM connected or not.
Doesn't this result in probe failure ?
... so I think that means the ... "if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM))" is somewhat redundant:
This could be:
if (rgrp->cmms_mask) { 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);
Or in fact, if we don't mind writing 0 to DEFR7 when there is no CMM (which is safe by the looks of things as DEFR7 is available on all platforms), then we can even remove the outer conditional, and leave this all up to the ternary operators to write the correct value to the defr7.
Phew ... net result - your current code *is* safe with the CONFIG_DRM_RCAR_CMM option disabled. I'll leave it up to you if you want to simplify the code here and remove the RCAR_DU_FEATURE_CMM.
As this RCAR_DU_FEATURE_CMM flag is only checked here, removing it would however simplify all of the rcar_du_device_info structures.
So - with or without the _FEATURE_CMM" simplification, this patch looks functional and safe so:
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
if (rcdu->info->gen >= 2) { rcar_du_group_setup_defr8(rgrp); rcar_du_group_setup_didsr(rgrp); diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h index bc87f080b170..fb9964949368 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h @@ -197,6 +197,11 @@ #define DEFR6_MLOS1 (1 << 2) #define DEFR6_DEFAULT (DEFR6_CODE | DEFR6_TCNE1)
+#define DEFR7 0x000ec +#define DEFR7_CODE (0x7779 << 16) +#define DEFR7_CMME1 BIT(6) +#define DEFR7_CMME0 BIT(4)
/* -----------------------------------------------------------------------------
- R8A7790-only Control Registers
*/
Hi Laurent, / Jacopo
On 19/09/2019 00:23, Laurent Pinchart wrote:
Hello,
On Thu, Sep 12, 2019 at 10:19:30AM +0100, Kieran Bingham wrote:
On 12/09/2019 09:07, Jacopo Mondi wrote:
On Wed, Sep 11, 2019 at 07:40:27PM +0100, Kieran Bingham wrote:
On 06/09/2019 14:54, Jacopo Mondi 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.
Reviewed-by: Ulrich Hecht uli+renesas@fpond.eu Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
drivers/gpu/drm/rcar-du/rcar_du_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..25d0fc125d7a 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);- }
What's the effect here on platforms with a CMM, but with CONFIG_DRM_RCAR_CMM unset?
Will this incorrectly configure the DU ?
Will it stall the display if the DU tries to interact with another module which is not enabled?
I recall I tested that (that's why I had to add stubs for CMM functions, as I had linkage errors otherwise) and thing seems to be fine as the CMM configuration/enblement resolve to an empty function.
Yes, I see the stubs to allow for linkage, but it's the hardware I'm concerned about. If it passes the tests and doesn't break then that's probably ok ... but I'm really weary that we're enabling a hardware pipeline with a disabled component in the middle.
Would you prefer to have this guarded by an #if IS_ENABLED() ?
I don't think we need a compile time conditional, but I'd say it probably needs to be more about whether the CMM has actually probed or not
Aha, and I see that in rcar_du_cmm_init() we already do a call to rcar_cmm_init(), which if fails will leave rcdu->cmms[i] as NULL. So that's catered for, which results in the rgrp->cmms_mask being correctly representative of whether there is a CMM connected or not.
Doesn't this result in probe failure ?
I think I mis-spoke above, I didn't mean "if rcar_cmm_init() fails" I meant "if rcar_du_cmm_init() determines there are no connected CMM's or if they are disabled."
If rcar_cmm_init() returns a failure, then yes we will fail to probe.
But I think it's up to rcar_du_cmm_init() to determine if the CMM exists or not (or is enabled) and if that's not a failure case then it should not prevent the probing of the DU.
In fact, I've now seen that if CONFIG_DRM_RCAR_CMM is not enabled, rcar_cmm_init() returns 0, and I think in fact it should return -ENODEV, with an exception on that return value in rcar_du_cmm_init() so that the DU continues with no CMM attached there.
... so I think that means the ... "if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM))" is somewhat redundant:
This could be:
if (rgrp->cmms_mask) { 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);
Or in fact, if we don't mind writing 0 to DEFR7 when there is no CMM (which is safe by the looks of things as DEFR7 is available on all platforms), then we can even remove the outer conditional, and leave this all up to the ternary operators to write the correct value to the defr7.
Phew ... net result - your current code *is* safe with the CONFIG_DRM_RCAR_CMM option disabled. I'll leave it up to you if you want to simplify the code here and remove the RCAR_DU_FEATURE_CMM.
As this RCAR_DU_FEATURE_CMM flag is only checked here, removing it would however simplify all of the rcar_du_device_info structures.
So - with or without the _FEATURE_CMM" simplification, this patch looks functional and safe so:
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
if (rcdu->info->gen >= 2) { rcar_du_group_setup_defr8(rgrp); rcar_du_group_setup_didsr(rgrp); diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h index bc87f080b170..fb9964949368 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h @@ -197,6 +197,11 @@ #define DEFR6_MLOS1 (1 << 2) #define DEFR6_DEFAULT (DEFR6_CODE | DEFR6_TCNE1)
+#define DEFR7 0x000ec +#define DEFR7_CODE (0x7779 << 16) +#define DEFR7_CMME1 BIT(6) +#define DEFR7_CMME0 BIT(4)
/* -----------------------------------------------------------------------------
- R8A7790-only Control Registers
*/
Hi Kieran,
On Thu, Sep 19, 2019 at 09:08:18AM +0100, Kieran Bingham wrote:
On 19/09/2019 00:23, Laurent Pinchart wrote:
On Thu, Sep 12, 2019 at 10:19:30AM +0100, Kieran Bingham wrote:
On 12/09/2019 09:07, Jacopo Mondi wrote:
On Wed, Sep 11, 2019 at 07:40:27PM +0100, Kieran Bingham wrote:
On 06/09/2019 14:54, Jacopo Mondi 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.
Reviewed-by: Ulrich Hecht uli+renesas@fpond.eu Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
drivers/gpu/drm/rcar-du/rcar_du_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..25d0fc125d7a 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);- }
What's the effect here on platforms with a CMM, but with CONFIG_DRM_RCAR_CMM unset?
Will this incorrectly configure the DU ?
Will it stall the display if the DU tries to interact with another module which is not enabled?
I recall I tested that (that's why I had to add stubs for CMM functions, as I had linkage errors otherwise) and thing seems to be fine as the CMM configuration/enblement resolve to an empty function.
Yes, I see the stubs to allow for linkage, but it's the hardware I'm concerned about. If it passes the tests and doesn't break then that's probably ok ... but I'm really weary that we're enabling a hardware pipeline with a disabled component in the middle.
Would you prefer to have this guarded by an #if IS_ENABLED() ?
I don't think we need a compile time conditional, but I'd say it probably needs to be more about whether the CMM has actually probed or not
Aha, and I see that in rcar_du_cmm_init() we already do a call to rcar_cmm_init(), which if fails will leave rcdu->cmms[i] as NULL. So that's catered for, which results in the rgrp->cmms_mask being correctly representative of whether there is a CMM connected or not.
Doesn't this result in probe failure ?
I think I mis-spoke above, I didn't mean "if rcar_cmm_init() fails" I meant "if rcar_du_cmm_init() determines there are no connected CMM's or if they are disabled."
If rcar_cmm_init() returns a failure, then yes we will fail to probe.
But I think it's up to rcar_du_cmm_init() to determine if the CMM exists or not (or is enabled) and if that's not a failure case then it should not prevent the probing of the DU.
In fact, I've now seen that if CONFIG_DRM_RCAR_CMM is not enabled, rcar_cmm_init() returns 0, and I think in fact it should return -ENODEV, with an exception on that return value in rcar_du_cmm_init() so that the DU continues with no CMM attached there.
I've replied to your other e-mail regarding this, and I agree with you.
... so I think that means the ... "if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM))" is somewhat redundant:
This could be:
if (rgrp->cmms_mask) { 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);
Or in fact, if we don't mind writing 0 to DEFR7 when there is no CMM (which is safe by the looks of things as DEFR7 is available on all platforms), then we can even remove the outer conditional, and leave this all up to the ternary operators to write the correct value to the defr7.
Phew ... net result - your current code *is* safe with the CONFIG_DRM_RCAR_CMM option disabled. I'll leave it up to you if you want to simplify the code here and remove the RCAR_DU_FEATURE_CMM.
As this RCAR_DU_FEATURE_CMM flag is only checked here, removing it would however simplify all of the rcar_du_device_info structures.
So - with or without the _FEATURE_CMM" simplification, this patch looks functional and safe so:
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
if (rcdu->info->gen >= 2) { rcar_du_group_setup_defr8(rgrp); rcar_du_group_setup_didsr(rgrp); diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h index bc87f080b170..fb9964949368 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h @@ -197,6 +197,11 @@ #define DEFR6_MLOS1 (1 << 2) #define DEFR6_DEFAULT (DEFR6_CODE | DEFR6_TCNE1)
+#define DEFR7 0x000ec +#define DEFR7_CODE (0x7779 << 16) +#define DEFR7_CMME1 BIT(6) +#define DEFR7_CMME0 BIT(4)
/* -----------------------------------------------------------------------------
- R8A7790-only Control Registers
*/
Enable the GAMMA_LUT KMS property using the framework helpers to register the property and set the associated gamma table maximum size.
Reviewed-by: Ulrich Hecht uli+renesas@fpond.eu Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org --- drivers/gpu/drm/rcar-du/rcar_du_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..dc59eeec990c 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, CM2_LUT_SIZE); + drm_crtc_enable_color_mgmt(crtc, 0, false, CM2_LUT_SIZE); }
drm_crtc_helper_add(crtc, &crtc_helper_funcs);
Hi Jacopo,
On 06/09/2019 14:54, Jacopo Mondi wrote:
Enable the GAMMA_LUT KMS property using the framework helpers to register the property and set the associated gamma table maximum size.
Reviewed-by: Ulrich Hecht uli+renesas@fpond.eu Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
LGTM.
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
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..dc59eeec990c 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, CM2_LUT_SIZE);drm_crtc_enable_color_mgmt(crtc, 0, false, CM2_LUT_SIZE);}
drm_crtc_helper_add(crtc, &crtc_helper_funcs);
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.
When resuming from system suspend, the DU driver is responsible for reprogramming and enabling the CMM unit if it was in use at the time the system entered the suspend state. Force the color_mgmt_changed flag to true if the DRM gamma lut color transformation property was set in the CRTC state duplicated at suspend time, as the CMM gets reprogrammed only if said flag is active in the rcar_du_atomic_commit_update_cmm() method.
Reviewed-by: Ulrich Hecht uli+renesas@fpond.eu Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org ---
Daniel could you have a look if resume bits are worth being moved to the DRM core? The color_mgmt_changed flag is set to false when the state is duplicated if I read the code correctly, but when this happens in a suspend/resume sequence its value should probably be restored to true if any color management property was set in the crtc state when system entered suspend.
---
drivers/gpu/drm/rcar-du/rcar_du_drv.c | 20 ++++++++++++++ drivers/gpu/drm/rcar-du/rcar_du_kms.c | 38 +++++++++++++++++++++++++++ 2 files changed, 58 insertions(+)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 018480a8f35c..d1003d31cfaf 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -17,6 +17,7 @@ #include <linux/slab.h> #include <linux/wait.h>
+#include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_fb_cma_helper.h> #include <drm/drm_fb_helper.h> @@ -482,6 +483,25 @@ static int rcar_du_pm_suspend(struct device *dev) static int rcar_du_pm_resume(struct device *dev) { struct rcar_du_device *rcdu = dev_get_drvdata(dev); + struct drm_atomic_state *state = rcdu->ddev->mode_config.suspend_state; + unsigned int i; + + for (i = 0; i < rcdu->num_crtcs; ++i) { + struct drm_crtc *crtc = &rcdu->crtcs[i].crtc; + struct drm_crtc_state *crtc_state; + + crtc_state = drm_atomic_get_new_crtc_state(state, crtc); + if (!crtc_state) + continue; + + /* + * Force re-enablement of CMM after system resume if any + * of the DRM color transformation properties was set in + * the state saved at system suspend time. + */ + if (crtc_state->gamma_lut) + crtc_state->color_mgmt_changed = true; + }
return drm_mode_config_helper_resume(rcdu->ddev); } diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 294630e56992..fc30fff0eb8d 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -22,6 +22,7 @@ #include <linux/of_platform.h> #include <linux/wait.h>
+#include "rcar_cmm.h" #include "rcar_du_crtc.h" #include "rcar_du_drv.h" #include "rcar_du_encoder.h" @@ -368,6 +369,40 @@ 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 = {}; + struct device *dev = rcrtc->dev->dev; + struct drm_property_blob *lut_blob; + + 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; + } + + lut_blob = crtc->state->gamma_lut; + if (lut_blob->length != (CM2_LUT_SIZE * sizeof(struct drm_color_lut))) { + /* + * We only accept fully populated LUT tables; + * be loud here, otherwise the CMM gets silently ignored. + */ + dev_err(dev, "invalid gamma lut size of %lu bytes\n", + lut_blob->length); + return; + } + + cmm_config.lut.enable = true; + cmm_config.lut.table = (struct drm_color_lut *)lut_blob->data; + + rcar_cmm_setup(rcrtc->cmm, &cmm_config); +} + static int rcar_du_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) { @@ -410,6 +445,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.23.0
Hi Jacopo,
On 06/09/2019 14:54, 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.
When resuming from system suspend, the DU driver is responsible for reprogramming and enabling the CMM unit if it was in use at the time the system entered the suspend state. Force the color_mgmt_changed flag to true if the DRM gamma lut color transformation property was set in the CRTC state duplicated at suspend time, as the CMM gets reprogrammed only if said flag is active in the rcar_du_atomic_commit_update_cmm() method.
Reviewed-by: Ulrich Hecht uli+renesas@fpond.eu Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
Also throwing in an LGTM here.
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
Daniel could you have a look if resume bits are worth being moved to the DRM core? The color_mgmt_changed flag is set to false when the state is duplicated if I read the code correctly, but when this happens in a suspend/resume sequence its value should probably be restored to true if any color management property was set in the crtc state when system entered suspend.
drivers/gpu/drm/rcar-du/rcar_du_drv.c | 20 ++++++++++++++ drivers/gpu/drm/rcar-du/rcar_du_kms.c | 38 +++++++++++++++++++++++++++ 2 files changed, 58 insertions(+)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 018480a8f35c..d1003d31cfaf 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -17,6 +17,7 @@ #include <linux/slab.h> #include <linux/wait.h>
+#include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_fb_cma_helper.h> #include <drm/drm_fb_helper.h> @@ -482,6 +483,25 @@ static int rcar_du_pm_suspend(struct device *dev) static int rcar_du_pm_resume(struct device *dev) { struct rcar_du_device *rcdu = dev_get_drvdata(dev);
struct drm_atomic_state *state = rcdu->ddev->mode_config.suspend_state;
unsigned int i;
for (i = 0; i < rcdu->num_crtcs; ++i) {
struct drm_crtc *crtc = &rcdu->crtcs[i].crtc;struct drm_crtc_state *crtc_state;crtc_state = drm_atomic_get_new_crtc_state(state, crtc);if (!crtc_state)continue;/** Force re-enablement of CMM after system resume if any* of the DRM color transformation properties was set in* the state saved at system suspend time.*/if (crtc_state->gamma_lut)crtc_state->color_mgmt_changed = true;}
return drm_mode_config_helper_resume(rcdu->ddev);
} diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 294630e56992..fc30fff0eb8d 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -22,6 +22,7 @@ #include <linux/of_platform.h> #include <linux/wait.h>
+#include "rcar_cmm.h" #include "rcar_du_crtc.h" #include "rcar_du_drv.h" #include "rcar_du_encoder.h" @@ -368,6 +369,40 @@ 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 = {};
- struct device *dev = rcrtc->dev->dev;
- struct drm_property_blob *lut_blob;
- 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;- }
- lut_blob = crtc->state->gamma_lut;
- if (lut_blob->length != (CM2_LUT_SIZE * sizeof(struct drm_color_lut))) {
/** We only accept fully populated LUT tables;* be loud here, otherwise the CMM gets silently ignored.*/dev_err(dev, "invalid gamma lut size of %lu bytes\n",lut_blob->length);return;- }
- cmm_config.lut.enable = true;
- cmm_config.lut.table = (struct drm_color_lut *)lut_blob->data;
- rcar_cmm_setup(rcrtc->cmm, &cmm_config);
+}
static int rcar_du_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) { @@ -410,6 +445,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.23.0
Hi Daniel,
On Fri, Sep 06, 2019 at 03:54:35PM +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.
When resuming from system suspend, the DU driver is responsible for reprogramming and enabling the CMM unit if it was in use at the time the system entered the suspend state. Force the color_mgmt_changed flag to true if the DRM gamma lut color transformation property was set in the CRTC state duplicated at suspend time, as the CMM gets reprogrammed only if said flag is active in the rcar_du_atomic_commit_update_cmm() method.
Reviewed-by: Ulrich Hecht uli+renesas@fpond.eu Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
Daniel could you have a look if resume bits are worth being moved to the DRM core? The color_mgmt_changed flag is set to false when the state is duplicated if I read the code correctly, but when this happens in a suspend/resume sequence its value should probably be restored to true if any color management property was set in the crtc state when system entered suspend.
Gentle ping on this.
drivers/gpu/drm/rcar-du/rcar_du_drv.c | 20 ++++++++++++++ drivers/gpu/drm/rcar-du/rcar_du_kms.c | 38 +++++++++++++++++++++++++++ 2 files changed, 58 insertions(+)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 018480a8f35c..d1003d31cfaf 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -17,6 +17,7 @@ #include <linux/slab.h> #include <linux/wait.h>
+#include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_fb_cma_helper.h> #include <drm/drm_fb_helper.h> @@ -482,6 +483,25 @@ static int rcar_du_pm_suspend(struct device *dev) static int rcar_du_pm_resume(struct device *dev) { struct rcar_du_device *rcdu = dev_get_drvdata(dev);
struct drm_atomic_state *state = rcdu->ddev->mode_config.suspend_state;
unsigned int i;
for (i = 0; i < rcdu->num_crtcs; ++i) {
struct drm_crtc *crtc = &rcdu->crtcs[i].crtc;struct drm_crtc_state *crtc_state;crtc_state = drm_atomic_get_new_crtc_state(state, crtc);if (!crtc_state)continue;/** Force re-enablement of CMM after system resume if any* of the DRM color transformation properties was set in* the state saved at system suspend time.*/if (crtc_state->gamma_lut)crtc_state->color_mgmt_changed = true;}
return drm_mode_config_helper_resume(rcdu->ddev);
} diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 294630e56992..fc30fff0eb8d 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -22,6 +22,7 @@ #include <linux/of_platform.h> #include <linux/wait.h>
+#include "rcar_cmm.h" #include "rcar_du_crtc.h" #include "rcar_du_drv.h" #include "rcar_du_encoder.h" @@ -368,6 +369,40 @@ 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 = {};
- struct device *dev = rcrtc->dev->dev;
- struct drm_property_blob *lut_blob;
- 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;- }
- lut_blob = crtc->state->gamma_lut;
- if (lut_blob->length != (CM2_LUT_SIZE * sizeof(struct drm_color_lut))) {
/** We only accept fully populated LUT tables;* be loud here, otherwise the CMM gets silently ignored.*/dev_err(dev, "invalid gamma lut size of %lu bytes\n",lut_blob->length);return;- }
- cmm_config.lut.enable = true;
- cmm_config.lut.table = (struct drm_color_lut *)lut_blob->data;
- rcar_cmm_setup(rcrtc->cmm, &cmm_config);
+}
static int rcar_du_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) { @@ -410,6 +445,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,
+Doug, Heiko:
On Fri, 2019-09-06 at 15:54 +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.
When resuming from system suspend, the DU driver is responsible for reprogramming and enabling the CMM unit if it was in use at the time the system entered the suspend state. Force the color_mgmt_changed flag to true if the DRM gamma lut color transformation property was set in the CRTC state duplicated at suspend time, as the CMM gets reprogrammed only if said flag is active in the rcar_du_atomic_commit_update_cmm() method.
Reviewed-by: Ulrich Hecht uli+renesas@fpond.eu Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
Daniel could you have a look if resume bits are worth being moved to the DRM core? The color_mgmt_changed flag is set to false when the state is duplicated if I read the code correctly, but when this happens in a suspend/resume sequence its value should probably be restored to true if any color management property was set in the crtc state when system entered suspend.
Perhaps we can use the for_each_new_crtc_in_state() helper here, and move it to the core like this:
--- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3234,8 +3234,20 @@ int drm_atomic_helper_resume(struct drm_device *dev, struct drm_atomic_state *state) { struct drm_modeset_acquire_ctx ctx; + struct drm_crtc_state *crtc_state; + struct drm_crtc *crtc; + unsigned int i; int err;
+ for_each_new_crtc_in_state(state, crtc, crtc_state, i) { + /* + * Force re-enablement of CMM after system resume if any + * of the DRM color transformation properties was set in + * the state saved at system suspend time. + */ + if (crtc_state->gamma_lut) + crtc_state->color_mgmt_changed = true; + }
This probably is wrong, and should be instead constrained to some condition of some sort.
FWIW, the Rockchip DRM is going to need this as well.
Any ideas?
Thanks, Ezequiel
Hi Ezequiel,
On Mon, Sep 30, 2019 at 05:53:00PM -0300, Ezequiel Garcia wrote:
+Doug, Heiko:
On Fri, 2019-09-06 at 15:54 +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.
When resuming from system suspend, the DU driver is responsible for reprogramming and enabling the CMM unit if it was in use at the time the system entered the suspend state. Force the color_mgmt_changed flag to true if the DRM gamma lut color transformation property was set in the CRTC state duplicated at suspend time, as the CMM gets reprogrammed only if said flag is active in the rcar_du_atomic_commit_update_cmm() method.
Reviewed-by: Ulrich Hecht uli+renesas@fpond.eu Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
Daniel could you have a look if resume bits are worth being moved to the DRM core? The color_mgmt_changed flag is set to false when the state is duplicated if I read the code correctly, but when this happens in a suspend/resume sequence its value should probably be restored to true if any color management property was set in the crtc state when system entered suspend.
Perhaps we can use the for_each_new_crtc_in_state() helper here, and move it to the core like this:
--- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3234,8 +3234,20 @@ int drm_atomic_helper_resume(struct drm_device *dev, struct drm_atomic_state *state) { struct drm_modeset_acquire_ctx ctx;
struct drm_crtc_state*crtc_state;
struct drm_crtc *crtc;unsigned int i; int err;for_each_new_crtc_in_state(state, crtc, crtc_state, i) {/** Force re-enablement of CMM after system resume if any* of the DRM color transformation propertieswas set in
* the state saved at system suspend time.*/if (crtc_state->gamma_lut)}This probably is wrong, and should be instead constrained to some condition of some sort.
FWIW, the Rockchip DRM is going to need this as well.
Any ideas?
That's more or less what I had in mind, yes. The question is if something like this would make sense. If there's a consensus it would, I think it can be done as part of this series.
Add CMM units to Renesas R-Car Gen3 SoC that support it, and reference them from the Display Unit they are connected to.
Sort the 'vsps' and 'renesas,cmm' entries in the DU unit consistently in all the involved DTS.
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org --- arch/arm64/boot/dts/renesas/r8a7795.dtsi | 40 ++++++++++++++++++++++- arch/arm64/boot/dts/renesas/r8a7796.dtsi | 28 ++++++++++++++++ arch/arm64/boot/dts/renesas/r8a77965.dtsi | 28 ++++++++++++++++ arch/arm64/boot/dts/renesas/r8a77990.dtsi | 22 ++++++++++++- arch/arm64/boot/dts/renesas/r8a77995.dtsi | 22 ++++++++++++- 5 files changed, 137 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi index 6675462f7585..67c242a447bc 100644 --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi @@ -2939,6 +2939,42 @@ iommus = <&ipmmu_vi1 10>; };
+ cmm0: cmm@fea40000 { + compatible = "renesas,r8a7795-cmm", + "renesas,rcar-gen3-cmm"; + reg = <0 0xfea40000 0 0x1000>; + power-domains = <&sysc R8A7795_PD_ALWAYS_ON>; + clocks = <&cpg CPG_MOD 711>; + resets = <&cpg 711>; + }; + + cmm1: cmm@fea50000 { + compatible = "renesas,r8a7795-cmm", + "renesas,rcar-gen3-cmm"; + reg = <0 0xfea50000 0 0x1000>; + power-domains = <&sysc R8A7795_PD_ALWAYS_ON>; + clocks = <&cpg CPG_MOD 710>; + resets = <&cpg 710>; + }; + + cmm2: cmm@fea60000 { + compatible = "renesas,r8a7795-cmm", + "renesas,rcar-gen3-cmm"; + reg = <0 0xfea60000 0 0x1000>; + power-domains = <&sysc R8A7795_PD_ALWAYS_ON>; + clocks = <&cpg CPG_MOD 709>; + resets = <&cpg 709>; + }; + + cmm3: cmm@fea70000 { + compatible = "renesas,r8a7795-cmm", + "renesas,rcar-gen3-cmm"; + reg = <0 0xfea70000 0 0x1000>; + power-domains = <&sysc R8A7795_PD_ALWAYS_ON>; + clocks = <&cpg CPG_MOD 708>; + resets = <&cpg 708>; + }; + csi20: csi2@fea80000 { compatible = "renesas,r8a7795-csi2"; reg = <0 0xfea80000 0 0x10000>; @@ -3142,9 +3178,11 @@ <&cpg CPG_MOD 722>, <&cpg CPG_MOD 721>; clock-names = "du.0", "du.1", "du.2", "du.3"; - vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd0 1>; status = "disabled";
+ vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd0 1>; + renesas,cmms = <&cmm0 &cmm1 &cmm2 &cmm3>; + ports { #address-cells = <1>; #size-cells = <0>; diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi b/arch/arm64/boot/dts/renesas/r8a7796.dtsi index 822c96601d3c..837c3b2da773 100644 --- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi @@ -2641,6 +2641,33 @@ renesas,fcp = <&fcpvi0>; };
+ cmm0: cmm@fea40000 { + compatible = "renesas,r8a7796-cmm", + "renesas,rcar-gen3-cmm"; + reg = <0 0xfea40000 0 0x1000>; + power-domains = <&sysc R8A7796_PD_ALWAYS_ON>; + clocks = <&cpg CPG_MOD 711>; + resets = <&cpg 711>; + }; + + cmm1: cmm@fea50000 { + compatible = "renesas,r8a7796-cmm", + "renesas,rcar-gen3-cmm"; + reg = <0 0xfea50000 0 0x1000>; + power-domains = <&sysc R8A7796_PD_ALWAYS_ON>; + clocks = <&cpg CPG_MOD 710>; + resets = <&cpg 710>; + }; + + cmm2: cmm@fea60000 { + compatible = "renesas,r8a7796-cmm", + "renesas,rcar-gen3-cmm"; + reg = <0 0xfea60000 0 0x1000>; + power-domains = <&sysc R8A7796_PD_ALWAYS_ON>; + clocks = <&cpg CPG_MOD 709>; + resets = <&cpg 709>; + }; + csi20: csi2@fea80000 { compatible = "renesas,r8a7796-csi2"; reg = <0 0xfea80000 0 0x10000>; @@ -2794,6 +2821,7 @@ status = "disabled";
vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>; + renesas,cmms = <&cmm0 &cmm1 &cmm2>;
ports { #address-cells = <1>; diff --git a/arch/arm64/boot/dts/renesas/r8a77965.dtsi b/arch/arm64/boot/dts/renesas/r8a77965.dtsi index 4ae163220f60..c7635e8b261c 100644 --- a/arch/arm64/boot/dts/renesas/r8a77965.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi @@ -2320,6 +2320,33 @@ resets = <&cpg 611>; };
+ cmm0: cmm@fea40000 { + compatible = "renesas,r8a77965-cmm", + "renesas,rcar-gen3-cmm"; + reg = <0 0xfea40000 0 0x1000>; + power-domains = <&sysc R8A77965_PD_ALWAYS_ON>; + clocks = <&cpg CPG_MOD 711>; + resets = <&cpg 711>; + }; + + cmm1: cmm@fea50000 { + compatible = "renesas,r8a77965-cmm", + "renesas,rcar-gen3-cmm"; + reg = <0 0xfea50000 0 0x1000>; + power-domains = <&sysc R8A77965_PD_ALWAYS_ON>; + clocks = <&cpg CPG_MOD 710>; + resets = <&cpg 710>; + }; + + cmm3: cmm@fea70000 { + compatible = "renesas,r8a77965-cmm", + "renesas,rcar-gen3-cmm"; + reg = <0 0xfea70000 0 0x1000>; + power-domains = <&sysc R8A77965_PD_ALWAYS_ON>; + clocks = <&cpg CPG_MOD 708>; + resets = <&cpg 708>; + }; + csi20: csi2@fea80000 { compatible = "renesas,r8a77965-csi2"; reg = <0 0xfea80000 0 0x10000>; @@ -2470,6 +2497,7 @@ status = "disabled";
vsps = <&vspd0 0>, <&vspd1 0>, <&vspd0 1>; + renesas,cmms = <&cmm0 &cmm1 &cmm3>;
ports { #address-cells = <1>; diff --git a/arch/arm64/boot/dts/renesas/r8a77990.dtsi b/arch/arm64/boot/dts/renesas/r8a77990.dtsi index 455954c3d98e..5e3d758a033f 100644 --- a/arch/arm64/boot/dts/renesas/r8a77990.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a77990.dtsi @@ -1727,6 +1727,24 @@ iommus = <&ipmmu_vi0 9>; };
+ cmm0: cmm@fea40000 { + compatible = "renesas,r8a77990-cmm", + "renesas,rcar-gen3-cmm"; + reg = <0 0xfea40000 0 0x1000>; + power-domains = <&sysc R8A77990_PD_ALWAYS_ON>; + clocks = <&cpg CPG_MOD 711>; + resets = <&cpg 711>; + }; + + cmm1: cmm@fea50000 { + compatible = "renesas,r8a77990-cmm", + "renesas,rcar-gen3-cmm"; + reg = <0 0xfea50000 0 0x1000>; + power-domains = <&sysc R8A77990_PD_ALWAYS_ON>; + clocks = <&cpg CPG_MOD 710>; + resets = <&cpg 710>; + }; + csi40: csi2@feaa0000 { compatible = "renesas,r8a77990-csi2"; reg = <0 0xfeaa0000 0 0x10000>; @@ -1768,9 +1786,11 @@ clock-names = "du.0", "du.1"; resets = <&cpg 724>; reset-names = "du.0"; - vsps = <&vspd0 0>, <&vspd1 0>; status = "disabled";
+ vsps = <&vspd0 0>, <&vspd1 0>; + renesas,cmms = <&cmm0 &cmm1>; + ports { #address-cells = <1>; #size-cells = <0>; diff --git a/arch/arm64/boot/dts/renesas/r8a77995.dtsi b/arch/arm64/boot/dts/renesas/r8a77995.dtsi index 183fef86cf7c..6838a81f5caa 100644 --- a/arch/arm64/boot/dts/renesas/r8a77995.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a77995.dtsi @@ -993,6 +993,24 @@ iommus = <&ipmmu_vi0 9>; };
+ cmm0: cmm@fea40000 { + compatible = "renesas,r8a77995-cmm", + "renesas,rcar-gen3-cmm"; + reg = <0 0xfea40000 0 0x1000>; + power-domains = <&sysc R8A77995_PD_ALWAYS_ON>; + clocks = <&cpg CPG_MOD 711>; + resets = <&cpg 711>; + }; + + cmm1: cmm@fea50000 { + compatible = "renesas,r8a77995-cmm", + "renesas,rcar-gen3-cmm"; + reg = <0 0xfea50000 0 0x1000>; + power-domains = <&sysc R8A77995_PD_ALWAYS_ON>; + clocks = <&cpg CPG_MOD 710>; + resets = <&cpg 710>; + }; + du: display@feb00000 { compatible = "renesas,du-r8a77995"; reg = <0 0xfeb00000 0 0x40000>; @@ -1003,9 +1021,11 @@ clock-names = "du.0", "du.1"; resets = <&cpg 724>; reset-names = "du.0"; - vsps = <&vspd0 0>, <&vspd1 0>; status = "disabled";
+ vsps = <&vspd0 0>, <&vspd1 0>; + renesas,cmms = <&cmm0 &cmm1>; + ports { #address-cells = <1>; #size-cells = <0>;
Hi Jacopo,
On 06/09/2019 14:54, Jacopo Mondi wrote:
Add CMM units to Renesas R-Car Gen3 SoC that support it, and reference them from the Display Unit they are connected to.
Sort the 'vsps' and 'renesas,cmm' entries in the DU unit consistently in all the involved DTS.
I think if you chose the ordering in the r8a7795, then you only have to adjust/correct the ordering in the r8a7796 and r8a77965 ...
Especially as you haven't changed the ordering of r8a77970, and r8a77980 which have the status after the vsps entry.
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
arch/arm64/boot/dts/renesas/r8a7795.dtsi | 40 ++++++++++++++++++++++- arch/arm64/boot/dts/renesas/r8a7796.dtsi | 28 ++++++++++++++++ arch/arm64/boot/dts/renesas/r8a77965.dtsi | 28 ++++++++++++++++ arch/arm64/boot/dts/renesas/r8a77990.dtsi | 22 ++++++++++++- arch/arm64/boot/dts/renesas/r8a77995.dtsi | 22 ++++++++++++- 5 files changed, 137 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi index 6675462f7585..67c242a447bc 100644 --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi @@ -2939,6 +2939,42 @@ iommus = <&ipmmu_vi1 10>; };
cmm0: cmm@fea40000 {compatible = "renesas,r8a7795-cmm","renesas,rcar-gen3-cmm";reg = <0 0xfea40000 0 0x1000>;power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;clocks = <&cpg CPG_MOD 711>;resets = <&cpg 711>;};cmm1: cmm@fea50000 {compatible = "renesas,r8a7795-cmm","renesas,rcar-gen3-cmm";reg = <0 0xfea50000 0 0x1000>;power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;clocks = <&cpg CPG_MOD 710>;resets = <&cpg 710>;};cmm2: cmm@fea60000 {compatible = "renesas,r8a7795-cmm","renesas,rcar-gen3-cmm";reg = <0 0xfea60000 0 0x1000>;power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;clocks = <&cpg CPG_MOD 709>;resets = <&cpg 709>;};cmm3: cmm@fea70000 {compatible = "renesas,r8a7795-cmm","renesas,rcar-gen3-cmm";reg = <0 0xfea70000 0 0x1000>;power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;clocks = <&cpg CPG_MOD 708>;resets = <&cpg 708>;};- csi20: csi2@fea80000 { compatible = "renesas,r8a7795-csi2"; reg = <0 0xfea80000 0 0x10000>;
@@ -3142,9 +3178,11 @@ <&cpg CPG_MOD 722>, <&cpg CPG_MOD 721>; clock-names = "du.0", "du.1", "du.2", "du.3";
vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd0 1>; status = "disabled";
I'm not sure the vsps should be below the status = disabled line.
I'd have this as:
clock-names... vsps... renesas,cmms... <blank line> status... <blank line> ports...
vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd0 1>;renesas,cmms = <&cmm0 &cmm1 &cmm2 &cmm3>;
I think these should be separated by comma's to show they are separate references, or references to separate phandles or such.
The only precedence I could find was in pmu_a53:
interrupt-affinity = <&a53_0>, <&a53_1>, <&a53_2>, <&a53_3>;
ports { #address-cells = <1>; #size-cells = <0>;diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi b/arch/arm64/boot/dts/renesas/r8a7796.dtsi index 822c96601d3c..837c3b2da773 100644 --- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi @@ -2641,6 +2641,33 @@ renesas,fcp = <&fcpvi0>; };
cmm0: cmm@fea40000 {compatible = "renesas,r8a7796-cmm","renesas,rcar-gen3-cmm";reg = <0 0xfea40000 0 0x1000>;power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;clocks = <&cpg CPG_MOD 711>;resets = <&cpg 711>;};cmm1: cmm@fea50000 {compatible = "renesas,r8a7796-cmm","renesas,rcar-gen3-cmm";reg = <0 0xfea50000 0 0x1000>;power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;clocks = <&cpg CPG_MOD 710>;resets = <&cpg 710>;};cmm2: cmm@fea60000 {compatible = "renesas,r8a7796-cmm","renesas,rcar-gen3-cmm";reg = <0 0xfea60000 0 0x1000>;power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;clocks = <&cpg CPG_MOD 709>;resets = <&cpg 709>;};- csi20: csi2@fea80000 { compatible = "renesas,r8a7796-csi2"; reg = <0 0xfea80000 0 0x10000>;
@@ -2794,6 +2821,7 @@ status = "disabled";
vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>;
renesas,cmms = <&cmm0 &cmm1 &cmm2>;
Aha, yes, I'd move this vsps rather than the one at r8a7795, which I'd consider to be more 'correct'.
ports { #address-cells = <1>;diff --git a/arch/arm64/boot/dts/renesas/r8a77965.dtsi b/arch/arm64/boot/dts/renesas/r8a77965.dtsi index 4ae163220f60..c7635e8b261c 100644 --- a/arch/arm64/boot/dts/renesas/r8a77965.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi @@ -2320,6 +2320,33 @@ resets = <&cpg 611>; };
cmm0: cmm@fea40000 {compatible = "renesas,r8a77965-cmm","renesas,rcar-gen3-cmm";reg = <0 0xfea40000 0 0x1000>;power-domains = <&sysc R8A77965_PD_ALWAYS_ON>;clocks = <&cpg CPG_MOD 711>;resets = <&cpg 711>;};cmm1: cmm@fea50000 {compatible = "renesas,r8a77965-cmm","renesas,rcar-gen3-cmm";reg = <0 0xfea50000 0 0x1000>;power-domains = <&sysc R8A77965_PD_ALWAYS_ON>;clocks = <&cpg CPG_MOD 710>;resets = <&cpg 710>;};cmm3: cmm@fea70000 {compatible = "renesas,r8a77965-cmm","renesas,rcar-gen3-cmm";reg = <0 0xfea70000 0 0x1000>;power-domains = <&sysc R8A77965_PD_ALWAYS_ON>;clocks = <&cpg CPG_MOD 708>;resets = <&cpg 708>;};- csi20: csi2@fea80000 { compatible = "renesas,r8a77965-csi2"; reg = <0 0xfea80000 0 0x10000>;
@@ -2470,6 +2497,7 @@ status = "disabled";
vsps = <&vspd0 0>, <&vspd1 0>, <&vspd0 1>;
renesas,cmms = <&cmm0 &cmm1 &cmm3>;
Again, I'd consider this the wrong sort order, due to the status' importance. I wouldn't hide it in the middle.
ports { #address-cells = <1>;diff --git a/arch/arm64/boot/dts/renesas/r8a77990.dtsi b/arch/arm64/boot/dts/renesas/r8a77990.dtsi index 455954c3d98e..5e3d758a033f 100644 --- a/arch/arm64/boot/dts/renesas/r8a77990.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a77990.dtsi @@ -1727,6 +1727,24 @@ iommus = <&ipmmu_vi0 9>; };
cmm0: cmm@fea40000 {compatible = "renesas,r8a77990-cmm","renesas,rcar-gen3-cmm";reg = <0 0xfea40000 0 0x1000>;power-domains = <&sysc R8A77990_PD_ALWAYS_ON>;clocks = <&cpg CPG_MOD 711>;resets = <&cpg 711>;};cmm1: cmm@fea50000 {compatible = "renesas,r8a77990-cmm","renesas,rcar-gen3-cmm";reg = <0 0xfea50000 0 0x1000>;power-domains = <&sysc R8A77990_PD_ALWAYS_ON>;clocks = <&cpg CPG_MOD 710>;resets = <&cpg 710>;};- csi40: csi2@feaa0000 { compatible = "renesas,r8a77990-csi2"; reg = <0 0xfeaa0000 0 0x10000>;
@@ -1768,9 +1786,11 @@ clock-names = "du.0", "du.1"; resets = <&cpg 724>; reset-names = "du.0";
vsps = <&vspd0 0>, <&vspd1 0>; status = "disabled";
vsps = <&vspd0 0>, <&vspd1 0>;renesas,cmms = <&cmm0 &cmm1>;
Same ... :D
ports { #address-cells = <1>; #size-cells = <0>;diff --git a/arch/arm64/boot/dts/renesas/r8a77995.dtsi b/arch/arm64/boot/dts/renesas/r8a77995.dtsi index 183fef86cf7c..6838a81f5caa 100644 --- a/arch/arm64/boot/dts/renesas/r8a77995.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a77995.dtsi @@ -993,6 +993,24 @@ iommus = <&ipmmu_vi0 9>; };
cmm0: cmm@fea40000 {compatible = "renesas,r8a77995-cmm","renesas,rcar-gen3-cmm";reg = <0 0xfea40000 0 0x1000>;power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;clocks = <&cpg CPG_MOD 711>;resets = <&cpg 711>;};cmm1: cmm@fea50000 {compatible = "renesas,r8a77995-cmm","renesas,rcar-gen3-cmm";reg = <0 0xfea50000 0 0x1000>;power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;clocks = <&cpg CPG_MOD 710>;resets = <&cpg 710>;};- du: display@feb00000 { compatible = "renesas,du-r8a77995"; reg = <0 0xfeb00000 0 0x40000>;
@@ -1003,9 +1021,11 @@ clock-names = "du.0", "du.1"; resets = <&cpg 724>; reset-names = "du.0";
vsps = <&vspd0 0>, <&vspd1 0>; status = "disabled";
vsps = <&vspd0 0>, <&vspd1 0>;renesas,cmms = <&cmm0 &cmm1>;
Same.
ports { #address-cells = <1>; #size-cells = <0>;
Hi Kieran, Jacopo,
On Wed, Sep 11, 2019 at 8:16 PM Kieran Bingham kieran.bingham+renesas@ideasonboard.com wrote:
On 06/09/2019 14:54, Jacopo Mondi wrote:
Add CMM units to Renesas R-Car Gen3 SoC that support it, and reference them from the Display Unit they are connected to.
Sort the 'vsps' and 'renesas,cmm' entries in the DU unit consistently in all the involved DTS.
I think if you chose the ordering in the r8a7795, then you only have to adjust/correct the ordering in the r8a7796 and r8a77965 ...
Especially as you haven't changed the ordering of r8a77970, and r8a77980 which have the status after the vsps entry.
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
--- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
@@ -3142,9 +3178,11 @@ <&cpg CPG_MOD 722>, <&cpg CPG_MOD 721>; clock-names = "du.0", "du.1", "du.2", "du.3";
vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd0 1>; status = "disabled";I'm not sure the vsps should be below the status = disabled line.
I'd have this as:
clock-names... vsps... renesas,cmms... <blank line> status... <blank line> ports...
Indeed.
And better write "ports { ... }", so it's clear this is a subnode.
vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd0 1>;
And the above will become "renesas,vsps", needing another reordering?
renesas,cmms = <&cmm0 &cmm1 &cmm2 &cmm3>;I think these should be separated by comma's to show they are separate references, or references to separate phandles or such.
Yep, looks better, and makes the grouping clear.
The only precedence I could find was in pmu_a53:
interrupt-affinity = <&a53_0>, <&a53_1>, <&a53_2>, <&a53_3>;
That's because most other phandle stuff has #<foo>-cells as non-zero.
We do have
clocks = ... <&audio_clk_a>, <&audio_clk_b>, <&audio_clk_c>;
Gr{oetje,eeting}s,
Geert
dri-devel@lists.freedesktop.org
-
Ezequiel Garcia -
Geert Uytterhoeven -
Jacopo Mondi -
Jacopo Mondi -
Kieran Bingham -
Laurent Pinchart -
Rob Herring