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... v4: https://lore.kernel.org/linux-renesas-soc/20190906135436.10622-1-jacopo+rene...
Again, quite a consistent changelog, mostly due to the developments happened on Ezequiel's VOP unit following Sean's advices.
I here implemented the same, and moved the CMM handling to the crtc being and enable callbacks. As a result the overall implementation results quite a lot simplified, mostly on the CMM driver side.
I have dropped tags and acks on the CMM driver and CMM enablement patches in DU crtc driver because of the number of changes.
A more detailed change log:
- Rebased on renesas-devel-2019-10-07-v5.4-rc4
* Bindings/DT - Included Rob's comments on the yaml file license and the use of 'OneOf' in the compatible property description - Use the bracketed style suggested by Kieran for the 'renesas,cmm' property introduced in patch 2 - Re-order the properties in the SoC DTS files as suggested by Kieran
* CMM/DU - As anticipated, moved CMM management to the crtc from the atomic commit tail helper where it was implemented in v4 This allow to correctly support resume/suspend and proper ordering of the CMM enable and setup operations (enable -before- setup) - As a consequence the CMM driver is greatly simplified by removing the need to cache the LUT table entries provided to cmm_setup() and later re-apply them at enable time. - Better support handling of disabled CMM config option by returning -ENODEV at cmm_init() time as suggested by Kieran.
* Testing I have tested by injecting a color inversion LUT table and enabling/disabling it every 50 displayed frames: https://jmondi.org/cgit/kmsxx/log/?h=gamma_lut
CMM functionalities are retained between suspend/resume cycles (tested with suspend-to-idle) without requiring a re-programming of the LUT tables.
Testing with real world use cases might be beneficial. Rajesh are you still interested in giving this series a spin?
Laurent, Kieran, could we fast-track review of this and hopefully try to have it merged for v5.5 ?
Thanks Ezequiel for having suggested me this solution.
Thanks j
Jacopo Mondi (8): 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: kms: Initialize CMM instances drm: rcar-du: crtc: Control CMM operations drm: rcar-du: crtc: Register GAMMA_LUT properties arm64: dts: renesas: Add CMM units to Gen3 SoCs drm: rcar-du: kms: Expand comment in vsps parsing routine
.../bindings/display/renesas,cmm.yaml | 67 ++++++ .../bindings/display/renesas,du.txt | 5 + arch/arm64/boot/dts/renesas/r8a7795.dtsi | 39 ++++ arch/arm64/boot/dts/renesas/r8a7796.dtsi | 31 ++- arch/arm64/boot/dts/renesas/r8a77965.dtsi | 31 ++- arch/arm64/boot/dts/renesas/r8a77990.dtsi | 21 ++ arch/arm64/boot/dts/renesas/r8a77995.dtsi | 21 ++ drivers/gpu/drm/rcar-du/Kconfig | 7 + drivers/gpu/drm/rcar-du/Makefile | 1 + drivers/gpu/drm/rcar-du/rcar_cmm.c | 198 ++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 60 ++++++ drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 89 ++++++++ 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.c | 5 + drivers/gpu/drm/rcar-du/rcar_du_group.h | 2 + drivers/gpu/drm/rcar-du/rcar_du_kms.c | 82 +++++++- drivers/gpu/drm/rcar-du/rcar_du_regs.h | 5 + 18 files changed, 665 insertions(+), 3 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).
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org --- .../bindings/display/renesas,cmm.yaml | 67 +++++++++++++++++++ 1 file changed, 67 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..a57037b9e9ba --- /dev/null +++ b/Documentation/devicetree/bindings/display/renesas,cmm.yaml @@ -0,0 +1,67 @@ +# SPDX-License-Identifier: GPL-2.0-only +%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 (CLU), 1D-histogram generation (HGO), and color + space conversion (CSC). + +properties: + compatible: + oneOf: + - items: + - enum: + - renesas,r8a7795-cmm + - renesas,r8a7796-cmm + - renesas,r8a77965-cmm + - renesas,r8a77990-cmm + - renesas,r8a77995-cmm + - const: renesas,rcar-gen3-cmm + - items: + - const: 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", + "renesas,rcar-gen3-cmm"; + reg = <0 0xfea40000 0 0x1000>; + power-domains = <&sysc R8A7796_PD_ALWAYS_ON>; + clocks = <&cpg CPG_MOD 711>; + resets = <&cpg 711>; + }; -- 2.23.0
Hi Jacopo,
On 15/10/2019 11:46, 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).
LGTM. Good to see how we get started with the new yaml bindings. I guess we've got plenty of conversion work to do there ..
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
.../bindings/display/renesas,cmm.yaml | 67 +++++++++++++++++++ 1 file changed, 67 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..a57037b9e9ba --- /dev/null +++ b/Documentation/devicetree/bindings/display/renesas,cmm.yaml @@ -0,0 +1,67 @@ +# SPDX-License-Identifier: GPL-2.0-only +%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 (CLU), 1D-histogram generation (HGO), and color
- space conversion (CSC).
+properties:
- compatible:
- oneOf:
- items:- enum:- renesas,r8a7795-cmm- renesas,r8a7796-cmm- renesas,r8a77965-cmm- renesas,r8a77990-cmm- renesas,r8a77995-cmm- const: renesas,rcar-gen3-cmm- items:- const: 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","renesas,rcar-gen3-cmm";reg = <0 0xfea40000 0 0x1000>;power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;clocks = <&cpg CPG_MOD 711>;resets = <&cpg 711>;- };
-- 2.23.0
Hi Jacopo,
On Tue, Oct 15, 2019 at 12:44 PM Jacopo Mondi jacopo+renesas@jmondi.org wrote:
Add device tree bindings documentation for the Renesas R-Car Display Unit Color Management Module.
CMM is the image enhancement module available on each R-Car DU video channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).
V2H is excluded, too.
Gr{oetje,eeting}s,
Geert
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.
Reviewed-by: Rob Herring robh@kernel.org Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org --- 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..3d9809b486b6 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
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 features will be implemented on top of this initial 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 | 198 +++++++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 60 +++++++++ 4 files changed, 266 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..9675a8587dee --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -0,0 +1,198 @@ +// 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; + + /* + * @lut: 1D-LUT status + * @lut.enabled: 1D-LUT enabled flag + */ + struct { + bool enabled; + } 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_write() - Scale the DRM LUT table entries to hardware precision + * and write to the CMM registers. + * @rcmm: Pointer to the CMM device + * @drm_lut: Pointer to the DRM LUT table + */ +static void rcar_cmm_lut_write(struct rcar_cmm *rcmm, + const struct drm_color_lut *drm_lut) +{ + unsigned int i; + + for (i = 0; i < CM2_LUT_SIZE; ++i) { + u32 entry = drm_color_lut_extract(drm_lut[i].red, 8) << 16 + | drm_color_lut_extract(drm_lut[i].green, 8) << 8 + | drm_color_lut_extract(drm_lut[i].blue, 8); + + rcar_cmm_write(rcmm, CM2_LUT_TBL(i), entry); + } +} + +/* + * rcar_cmm_setup() - Configure the CMM unit. + * @pdev: The platform device associated with the CMM instance + * @config: The CMM unit configuration + * + * Configure the CMM unit with the given 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); + + /* Disable LUT 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. */ + if (!rcmm->lut.enabled) { + rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN); + rcmm->lut.enabled = true; + } + + rcar_cmm_lut_write(rcmm, config->lut.table); + + 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. + */ +int rcar_cmm_enable(struct platform_device *pdev) +{ + int ret; + + ret = pm_runtime_get_sync(&pdev->dev); + if (ret < 0) + return ret; + + 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); + rcmm->lut.enabled = false; + + pm_runtime_put(&pdev->dev); +} +EXPORT_SYMBOL_GPL(rcar_cmm_disable); + +/* + * rcar_cmm_init() - Initialize the CMM + * @pdev: The platform device associated with the CMM instance + * + * Return: 0 on success, -EPROBE_DEFER if the CMM is not available yet, + * -ENODEV if the DRM_RCAR_CMM config option is disabled + */ +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..358ec03cf48a --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h @@ -0,0 +1,60 @@ +/* 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 + */ +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 -ENODEV; +} + +static inline int rcar_cmm_enable(struct platform_device *pdev) +{ + return 0; +} + +static inline void rcar_cmm_disable(struct platform_device *pdev) +{ +} + +static inline 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 15/10/2019 11:46, 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 features will be implemented on top of this initial one.
This is looking good to me.
I have patches which start to add CLU on top of this, which refactor things slightly - but I don't think that refactoring needs to be in this series, and this version is independent and looks good ... thus:
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
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 | 198 +++++++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 60 +++++++++ 4 files changed, 266 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
DRM_RCAR_DU already depends on both DRM && OF, so I wonder if those are needed to be specified explicitly.
Doesn't hurt of course, but I see DRM_RCAR_DW_HDMI does the same, and so does DRM_RCAR_LVDS, so I don't think you need to remove it.
- 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..9675a8587dee --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -0,0 +1,198 @@ +// 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;
- /*
* @lut: 1D-LUT status* @lut.enabled: 1D-LUT enabled flag*/- struct {
bool enabled;- } lut;
This used to be a more complex structure in an earlier version storing a cached version of the table. Now that the cached entry is removed, does this need to be such a complex structure rather than just say, a bool lut_enabled?
(We will soon add an equivalent clu_enabled too, but I don't know what other per-table options we'll need.)
In fact, we'll potentially have other options specific to the HGO, and CSC at some point in the future - so grouping by entity is indeed a good thing IMO.
+};
+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_write() - Scale the DRM LUT table entries to hardware precision
and write to the CMM registers.
- @rcmm: Pointer to the CMM device
- @drm_lut: Pointer to the DRM LUT table
- */
+static void rcar_cmm_lut_write(struct rcar_cmm *rcmm,
const struct drm_color_lut *drm_lut)+{
- unsigned int i;
- for (i = 0; i < CM2_LUT_SIZE; ++i) {
u32 entry = drm_color_lut_extract(drm_lut[i].red, 8) << 16| drm_color_lut_extract(drm_lut[i].green, 8) << 8| drm_color_lut_extract(drm_lut[i].blue, 8);rcar_cmm_write(rcmm, CM2_LUT_TBL(i), entry);- }
+}
+/*
- rcar_cmm_setup() - Configure the CMM unit.
- @pdev: The platform device associated with the CMM instance
- @config: The CMM unit configuration
- Configure the CMM unit with the given 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);
- /* Disable LUT 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. */
- if (!rcmm->lut.enabled) {
rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);rcmm->lut.enabled = true;- }
- rcar_cmm_lut_write(rcmm, config->lut.table);
- 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.
- */
+int rcar_cmm_enable(struct platform_device *pdev) +{
- int ret;
- ret = pm_runtime_get_sync(&pdev->dev);
- if (ret < 0)
return ret;- 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);
- rcmm->lut.enabled = false;
- pm_runtime_put(&pdev->dev);
+} +EXPORT_SYMBOL_GPL(rcar_cmm_disable);
+/*
- rcar_cmm_init() - Initialize the CMM
- @pdev: The platform device associated with the CMM instance
- Return: 0 on success, -EPROBE_DEFER if the CMM is not available yet,
-ENODEV if the DRM_RCAR_CMM config option is disabled- */
+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..358ec03cf48a --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h @@ -0,0 +1,60 @@ +/* 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
- */
+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 -ENODEV;
Excellent :-D
+}
+static inline int rcar_cmm_enable(struct platform_device *pdev) +{
- return 0;
+}
+static inline void rcar_cmm_disable(struct platform_device *pdev) +{ +}
+static inline 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,
One minor additional catch here:
On 15/10/2019 12:53, Kieran Bingham wrote:
Hi Jacopo,
<snipped>
+/*
- rcar_cmm_lut_write() - Scale the DRM LUT table entries to hardware precision
and write to the CMM registers.
- @rcmm: Pointer to the CMM device
- @drm_lut: Pointer to the DRM LUT table
- */
+static void rcar_cmm_lut_write(struct rcar_cmm *rcmm,
const struct drm_color_lut *drm_lut)+{
- unsigned int i;
- for (i = 0; i < CM2_LUT_SIZE; ++i) {
u32 entry = drm_color_lut_extract(drm_lut[i].red, 8) << 16
There's an extra space between = and 'drm_color...' here.
<snipped>
-- Kieran
Hi Kieran, thanks for review
On Tue, Oct 15, 2019 at 12:53:55PM +0100, Kieran Bingham wrote:
Hi Jacopo,
On 15/10/2019 11:46, 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 features will be implemented on top of this initial one.
This is looking good to me.
I have patches which start to add CLU on top of this, which refactor things slightly - but I don't think that refactoring needs to be in this series, and this version is independent and looks good ... thus:
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
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 | 198 +++++++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 60 +++++++++ 4 files changed, 266 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
DRM_RCAR_DU already depends on both DRM && OF, so I wonder if those are needed to be specified explicitly.
Doesn't hurt of course, but I see DRM_RCAR_DW_HDMI does the same, and so does DRM_RCAR_LVDS, so I don't think you need to remove it.
I did the same as it is done for HDMI and LVDS here. The extra dependencies could be dropped yes, I chose to be consistent.
- 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..9675a8587dee --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -0,0 +1,198 @@ +// 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;
- /*
* @lut: 1D-LUT status* @lut.enabled: 1D-LUT enabled flag*/- struct {
bool enabled;- } lut;
This used to be a more complex structure in an earlier version storing a cached version of the table. Now that the cached entry is removed, does this need to be such a complex structure rather than just say, a bool lut_enabled?
(We will soon add an equivalent clu_enabled too, but I don't know what other per-table options we'll need.)
In fact, we'll potentially have other options specific to the HGO, and CSC at some point in the future - so grouping by entity is indeed a good thing IMO.
You are right, I pondered a bit it this was worth it, but I assume the other CMM functions would have required some more complex fields so I chose to keep it separate. I have no problem to make this a lut_enabled, but I fear as soon as we support say, double buffering for the lut, having a dedicated struct would be nice.
Is it ok if I keep this the way it is?
+};
+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_write() - Scale the DRM LUT table entries to hardware precision
and write to the CMM registers.
- @rcmm: Pointer to the CMM device
- @drm_lut: Pointer to the DRM LUT table
- */
+static void rcar_cmm_lut_write(struct rcar_cmm *rcmm,
const struct drm_color_lut *drm_lut)+{
- unsigned int i;
- for (i = 0; i < CM2_LUT_SIZE; ++i) {
u32 entry = drm_color_lut_extract(drm_lut[i].red, 8) << 16| drm_color_lut_extract(drm_lut[i].green, 8) << 8| drm_color_lut_extract(drm_lut[i].blue, 8);rcar_cmm_write(rcmm, CM2_LUT_TBL(i), entry);- }
+}
+/*
- rcar_cmm_setup() - Configure the CMM unit.
- @pdev: The platform device associated with the CMM instance
- @config: The CMM unit configuration
- Configure the CMM unit with the given 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);
- /* Disable LUT 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. */
- if (!rcmm->lut.enabled) {
rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);rcmm->lut.enabled = true;- }
- rcar_cmm_lut_write(rcmm, config->lut.table);
- 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.
- */
+int rcar_cmm_enable(struct platform_device *pdev) +{
- int ret;
- ret = pm_runtime_get_sync(&pdev->dev);
- if (ret < 0)
return ret;- 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);
- rcmm->lut.enabled = false;
- pm_runtime_put(&pdev->dev);
+} +EXPORT_SYMBOL_GPL(rcar_cmm_disable);
+/*
- rcar_cmm_init() - Initialize the CMM
- @pdev: The platform device associated with the CMM instance
- Return: 0 on success, -EPROBE_DEFER if the CMM is not available yet,
-ENODEV if the DRM_RCAR_CMM config option is disabled- */
+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..358ec03cf48a --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h @@ -0,0 +1,60 @@ +/* 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
- */
+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 -ENODEV;
Excellent :-D
+}
+static inline int rcar_cmm_enable(struct platform_device *pdev) +{
- return 0;
+}
+static inline void rcar_cmm_disable(struct platform_device *pdev) +{ +}
+static inline 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__ */
On 15/10/2019 14:33, Jacopo Mondi wrote:
Hi Kieran, thanks for review
<snip>
+config DRM_RCAR_CMM
- bool "R-Car DU Color Management Module (CMM) Support"
- depends on DRM && OF
- depends on DRM_RCAR_DU
DRM_RCAR_DU already depends on both DRM && OF, so I wonder if those are needed to be specified explicitly.
Doesn't hurt of course, but I see DRM_RCAR_DW_HDMI does the same, and so does DRM_RCAR_LVDS, so I don't think you need to remove it.
I did the same as it is done for HDMI and LVDS here. The extra dependencies could be dropped yes, I chose to be consistent.
Consistent is fine with me.
<snip>
+struct rcar_cmm {
- void __iomem *base;
- /*
* @lut: 1D-LUT status* @lut.enabled: 1D-LUT enabled flag*/- struct {
bool enabled;- } lut;
This used to be a more complex structure in an earlier version storing a cached version of the table. Now that the cached entry is removed, does this need to be such a complex structure rather than just say, a bool lut_enabled?
(We will soon add an equivalent clu_enabled too, but I don't know what other per-table options we'll need.)
In fact, we'll potentially have other options specific to the HGO, and CSC at some point in the future - so grouping by entity is indeed a good thing IMO.
You are right, I pondered a bit it this was worth it, but I assume the other CMM functions would have required some more complex fields so I chose to keep it separate. I have no problem to make this a lut_enabled, but I fear as soon as we support say, double buffering for the lut, having a dedicated struct would be nice.
Is it ok if I keep this the way it is?
Certainly fine for me. (That's what I tried to imply with "so grouping by entity is indeed a good thing IMO.")
<snip> -- Kieran
Hi Jacopo,
Thank you for the patch.
On Tue, Oct 15, 2019 at 12:46:16PM +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 features will be implemented on top of this initial 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 | 198 +++++++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 60 +++++++++ 4 files changed, 266 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..9675a8587dee --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c @@ -0,0 +1,198 @@ +// 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;
- /*
* @lut: 1D-LUT status
s/status/state/ ?
* @lut.enabled: 1D-LUT enabled flag*/- struct {
bool enabled;- } 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_write() - Scale the DRM LUT table entries to hardware precision
and write to the CMM registers.
Some of you brief descriptions end with a period, some don't. I don't have a preference, but could you please pick one option and use it consistently ?
- @rcmm: Pointer to the CMM device
- @drm_lut: Pointer to the DRM LUT table
- */
+static void rcar_cmm_lut_write(struct rcar_cmm *rcmm,
const struct drm_color_lut *drm_lut)+{
- unsigned int i;
- for (i = 0; i < CM2_LUT_SIZE; ++i) {
u32 entry = drm_color_lut_extract(drm_lut[i].red, 8) << 16
As reported by Kieran, s/= /= /
| drm_color_lut_extract(drm_lut[i].green, 8) << 8| drm_color_lut_extract(drm_lut[i].blue, 8);rcar_cmm_write(rcmm, CM2_LUT_TBL(i), entry);- }
+}
+/*
- rcar_cmm_setup() - Configure the CMM unit.
- @pdev: The platform device associated with the CMM instance
- @config: The CMM unit configuration
- Configure the CMM unit with the given 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);
- /* Disable LUT 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. */
- if (!rcmm->lut.enabled) {
rcar_cmm_write(rcmm, CM2_LUT_CTRL, CM2_LUT_CTRL_LUT_EN);rcmm->lut.enabled = true;- }
- rcar_cmm_lut_write(rcmm, config->lut.table);
- 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.
It would be better to explain how the function should be used rather than what it does internally.
* Enable the CMM unit. When the output of the corresponding DU channel is * routed to the CMM unit, the unit shall be enabled before the DU channel is * started, and remain enabled until the channel is stopped. The CMM unit shall * the be disabled with rcar_cmm_disable(). * * Calls to rcar_cmm_enable() and rcar_cmm_disable() are not reference-counted. * It is an error to attempt to enable an already enabled CMM unit, or to * attempt to disable a disabled unit.
The relationship with rcar_cmm_setup() should also be documented. Looking at the code, I believe rcar_cmm_enable() has to be called first, otherwise the CMM MSTP will be disabled, and registers can't be accessed. I would document this as part of the rcar_cmm_setup() function.
- */
+int rcar_cmm_enable(struct platform_device *pdev) +{
- int ret;
- ret = pm_runtime_get_sync(&pdev->dev);
- if (ret < 0)
return ret;- 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.
Same here, but you can simply reference rcar_cmm_enable().
* Disable the CMM unit. See rcar_cmm_enable() for usage information. * * Disabling the CMM unit disable all the internal processing blocks. The CMM * state shall thus be restored with rcar_cmm_setup() when re-enabling the CMM * unit after the next rcar_cmm_enable() call.
- */
+void rcar_cmm_disable(struct platform_device *pdev) +{
- struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
- rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
- rcmm->lut.enabled = false;
- pm_runtime_put(&pdev->dev);
+} +EXPORT_SYMBOL_GPL(rcar_cmm_disable);
+/*
- rcar_cmm_init() - Initialize the CMM
- @pdev: The platform device associated with the CMM instance
- Return: 0 on success, -EPROBE_DEFER if the CMM is not available yet,
-ENODEV if the DRM_RCAR_CMM config option is disabled- */
+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..358ec03cf48a --- /dev/null +++ b/drivers/gpu/drm/rcar-du/rcar_cmm.h @@ -0,0 +1,60 @@ +/* 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
- */
+struct rcar_cmm_config {
- struct {
bool enable;struct drm_color_lut *table;
As enable == true implies table != NULL, and enable == false implies table == NULL, could we drop the enable field ?
With those small issues addressed,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
- } 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 -ENODEV;
+}
+static inline int rcar_cmm_enable(struct platform_device *pdev) +{
- return 0;
+}
+static inline void rcar_cmm_disable(struct platform_device *pdev) +{ +}
+static inline 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__ */
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: Kieran Bingham kieran.bingham+renesas@ideasonboard.com 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 | 76 +++++++++++++++++++++++++ 5 files changed, 88 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 1327cd0df90a..61504c54e2ec 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" @@ -85,6 +86,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..7c9fb5860e54 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,75 @@ 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); + + /* + * -ENODEV is used to report that the CMM config option is + * disabled: return 0 and let the DU continue probing. + */ + ret = rcar_cmm_init(pdev); + if (ret) + return ret == -ENODEV ? 0 : ret; + + /* + * Enforce suspend/resume ordering by making the CMM a provider + * of the DU: CMM is suspended after and resumed before the DU. + */ + 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 +775,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;
Implement CMM handling in the crtc begin and enable atomic callbacks, and enable CMM unit through the Display Extensional Functions register at group setup time.
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org --- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 79 +++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_du_group.c | 5 ++ drivers/gpu/drm/rcar-du/rcar_du_regs.h | 5 ++ 3 files changed, 89 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..4bc50a3f4a00 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" @@ -474,6 +475,70 @@ static void rcar_du_crtc_wait_page_flip(struct rcar_du_crtc *rcrtc) rcar_du_crtc_finish_page_flip(rcrtc); }
+/* ----------------------------------------------------------------------------- + * Color Management Module (CMM) + */ +static int rcar_du_cmm_enable(struct drm_crtc *crtc) +{ + struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc); + + if (!rcrtc->cmm) + return 0; + + return rcar_cmm_enable(rcrtc->cmm); +} + +static void rcar_du_cmm_disable(struct drm_crtc *crtc) +{ + struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc); + + if (!rcrtc->cmm) + return; + + rcar_cmm_disable(rcrtc->cmm); +} + +static int rcar_du_cmm_check(struct drm_crtc *crtc, + struct drm_crtc_state *state) +{ + struct drm_property_blob *drm_lut = state->gamma_lut; + struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc); + struct device *dev = rcrtc->dev->dev; + + if (!rcrtc->cmm || !drm_lut) + return 0; + + /* We only accept fully populated LUT tables. */ + if (CM2_LUT_SIZE * sizeof(struct drm_color_lut) != + drm_lut->length) { + dev_err(dev, "invalid gamma lut size: %lu bytes\n", + drm_lut->length); + return -EINVAL; + } + + return 0; +} + +static void rcar_du_cmm_setup(struct drm_crtc *crtc) +{ + struct drm_property_blob *drm_lut = crtc->state->gamma_lut; + struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc); + struct rcar_cmm_config cmm_config = {}; + + if (!rcrtc->cmm) + return; + + if (drm_lut) { + cmm_config.lut.enable = true; + cmm_config.lut.table = (struct drm_color_lut *)drm_lut->data; + + } else { + cmm_config.lut.enable = false; + } + + rcar_cmm_setup(rcrtc->cmm, &cmm_config); +} + /* ----------------------------------------------------------------------------- * Start/Stop and Suspend/Resume */ @@ -619,6 +684,8 @@ 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);
+ rcar_du_cmm_disable(crtc); + /* * Select switch sync mode. This stops display operation and configures * the HSYNC and VSYNC signals as inputs. @@ -631,6 +698,7 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc) DSYSR_TVM_SWITCH);
rcar_du_group_start_stop(rcrtc->group, false); + }
/* ----------------------------------------------------------------------------- @@ -642,6 +710,11 @@ static int rcar_du_crtc_atomic_check(struct drm_crtc *crtc, { struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(state); struct drm_encoder *encoder; + int ret; + + ret = rcar_du_cmm_check(crtc, state); + if (ret) + return ret;
/* Store the routes from the CRTC output to the DU outputs. */ rstate->outputs = 0; @@ -667,6 +740,7 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(crtc->state); struct rcar_du_device *rcdu = rcrtc->dev;
+ rcar_du_cmm_enable(crtc); rcar_du_crtc_get(rcrtc);
/* @@ -686,6 +760,7 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, }
rcar_du_crtc_start(rcrtc); + rcar_du_cmm_setup(crtc); }
static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc, @@ -739,6 +814,10 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc, */ rcar_du_crtc_get(rcrtc);
+ /* If the active state changed, we let .atomic_enable handle CMM. */ + if (crtc->state->color_mgmt_changed && !crtc->state->active_changed) + rcar_du_cmm_setup(crtc); + if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) rcar_du_vsp_atomic_begin(rcrtc); } diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c index 9eee47969e77..583de800a66d 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c @@ -135,6 +135,7 @@ static void rcar_du_group_setup_didsr(struct rcar_du_group *rgrp) static void rcar_du_group_setup(struct rcar_du_group *rgrp) { struct rcar_du_device *rcdu = rgrp->dev; + u32 defr7 = DEFR7_CODE;
/* Enable extended features */ rcar_du_group_write(rgrp, DEFR, DEFR_CODE | DEFR_DEFE); @@ -147,6 +148,10 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
rcar_du_group_setup_pins(rgrp);
+ defr7 |= (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 15/10/2019 11:46, Jacopo Mondi wrote:
Implement CMM handling in the crtc begin and enable atomic callbacks, and enable CMM unit through the Display Extensional Functions
Extensional ?
Perhaps this should just be Display Extension Functions? Wow - that's actually what they call it in the data-sheet.
register at group setup time.
Only a trivial extra blank line below that I can find... so
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 | 79 +++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_du_group.c | 5 ++ drivers/gpu/drm/rcar-du/rcar_du_regs.h | 5 ++ 3 files changed, 89 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..4bc50a3f4a00 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" @@ -474,6 +475,70 @@ static void rcar_du_crtc_wait_page_flip(struct rcar_du_crtc *rcrtc) rcar_du_crtc_finish_page_flip(rcrtc); }
+/* -----------------------------------------------------------------------------
- Color Management Module (CMM)
- */
+static int rcar_du_cmm_enable(struct drm_crtc *crtc) +{
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- if (!rcrtc->cmm)
return 0;- return rcar_cmm_enable(rcrtc->cmm);
+}
+static void rcar_du_cmm_disable(struct drm_crtc *crtc) +{
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- if (!rcrtc->cmm)
return;- rcar_cmm_disable(rcrtc->cmm);
+}
+static int rcar_du_cmm_check(struct drm_crtc *crtc,
struct drm_crtc_state *state)+{
- struct drm_property_blob *drm_lut = state->gamma_lut;
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- struct device *dev = rcrtc->dev->dev;
- if (!rcrtc->cmm || !drm_lut)
return 0;- /* We only accept fully populated LUT tables. */
- if (CM2_LUT_SIZE * sizeof(struct drm_color_lut) !=
drm_lut->length) {dev_err(dev, "invalid gamma lut size: %lu bytes\n",drm_lut->length);return -EINVAL;- }
- return 0;
+}
+static void rcar_du_cmm_setup(struct drm_crtc *crtc) +{
- struct drm_property_blob *drm_lut = crtc->state->gamma_lut;
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- struct rcar_cmm_config cmm_config = {};
- if (!rcrtc->cmm)
return;- if (drm_lut) {
cmm_config.lut.enable = true;cmm_config.lut.table = (struct drm_color_lut *)drm_lut->data;- } else {
cmm_config.lut.enable = false;- }
- rcar_cmm_setup(rcrtc->cmm, &cmm_config);
+}
/* -----------------------------------------------------------------------------
- Start/Stop and Suspend/Resume
*/ @@ -619,6 +684,8 @@ 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);
- rcar_du_cmm_disable(crtc);
- /*
- Select switch sync mode. This stops display operation and configures
- the HSYNC and VSYNC signals as inputs.
@@ -631,6 +698,7 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc) DSYSR_TVM_SWITCH);
rcar_du_group_start_stop(rcrtc->group, false);
Extra blank line...
}
/* ----------------------------------------------------------------------------- @@ -642,6 +710,11 @@ static int rcar_du_crtc_atomic_check(struct drm_crtc *crtc, { struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(state); struct drm_encoder *encoder;
int ret;
ret = rcar_du_cmm_check(crtc, state);
if (ret)
return ret;/* Store the routes from the CRTC output to the DU outputs. */ rstate->outputs = 0;
@@ -667,6 +740,7 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(crtc->state); struct rcar_du_device *rcdu = rcrtc->dev;
rcar_du_cmm_enable(crtc); rcar_du_crtc_get(rcrtc);
/*
@@ -686,6 +760,7 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, }
rcar_du_crtc_start(rcrtc);
- rcar_du_cmm_setup(crtc);
}
static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc, @@ -739,6 +814,10 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc, */ rcar_du_crtc_get(rcrtc);
- /* If the active state changed, we let .atomic_enable handle CMM. */
- if (crtc->state->color_mgmt_changed && !crtc->state->active_changed)
rcar_du_cmm_setup(crtc);
Aha, this is quite neat for handling the timings.
- if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) rcar_du_vsp_atomic_begin(rcrtc);
} diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c index 9eee47969e77..583de800a66d 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c @@ -135,6 +135,7 @@ static void rcar_du_group_setup_didsr(struct rcar_du_group *rgrp) static void rcar_du_group_setup(struct rcar_du_group *rgrp) { struct rcar_du_device *rcdu = rgrp->dev;
u32 defr7 = DEFR7_CODE;
/* Enable extended features */ rcar_du_group_write(rgrp, DEFR, DEFR_CODE | DEFR_DEFE);
@@ -147,6 +148,10 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
rcar_du_group_setup_pins(rgrp);
- defr7 |= (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 Kieran,
On Tue, Oct 15, 2019 at 02:15:35PM +0100, Kieran Bingham wrote:
Hi Jacopo,
On 15/10/2019 11:46, Jacopo Mondi wrote:
Implement CMM handling in the crtc begin and enable atomic callbacks, and enable CMM unit through the Display Extensional Functions
Extensional ?
Perhaps this should just be Display Extension Functions? Wow - that's actually what they call it in the data-sheet.
register at group setup time.
Only a trivial extra blank line below that I can find... so
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 | 79 +++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_du_group.c | 5 ++ drivers/gpu/drm/rcar-du/rcar_du_regs.h | 5 ++ 3 files changed, 89 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..4bc50a3f4a00 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" @@ -474,6 +475,70 @@ static void rcar_du_crtc_wait_page_flip(struct rcar_du_crtc *rcrtc) rcar_du_crtc_finish_page_flip(rcrtc); }
+/* -----------------------------------------------------------------------------
- Color Management Module (CMM)
- */
+static int rcar_du_cmm_enable(struct drm_crtc *crtc) +{
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- if (!rcrtc->cmm)
return 0;- return rcar_cmm_enable(rcrtc->cmm);
+}
+static void rcar_du_cmm_disable(struct drm_crtc *crtc) +{
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- if (!rcrtc->cmm)
return;- rcar_cmm_disable(rcrtc->cmm);
+}
+static int rcar_du_cmm_check(struct drm_crtc *crtc,
struct drm_crtc_state *state)+{
- struct drm_property_blob *drm_lut = state->gamma_lut;
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- struct device *dev = rcrtc->dev->dev;
- if (!rcrtc->cmm || !drm_lut)
return 0;- /* We only accept fully populated LUT tables. */
- if (CM2_LUT_SIZE * sizeof(struct drm_color_lut) !=
drm_lut->length) {dev_err(dev, "invalid gamma lut size: %lu bytes\n",drm_lut->length);return -EINVAL;- }
- return 0;
+}
+static void rcar_du_cmm_setup(struct drm_crtc *crtc) +{
- struct drm_property_blob *drm_lut = crtc->state->gamma_lut;
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- struct rcar_cmm_config cmm_config = {};
- if (!rcrtc->cmm)
return;- if (drm_lut) {
cmm_config.lut.enable = true;cmm_config.lut.table = (struct drm_color_lut *)drm_lut->data;- } else {
cmm_config.lut.enable = false;- }
- rcar_cmm_setup(rcrtc->cmm, &cmm_config);
+}
/* -----------------------------------------------------------------------------
- Start/Stop and Suspend/Resume
*/ @@ -619,6 +684,8 @@ 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);
- rcar_du_cmm_disable(crtc);
- /*
- Select switch sync mode. This stops display operation and configures
- the HSYNC and VSYNC signals as inputs.
@@ -631,6 +698,7 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc) DSYSR_TVM_SWITCH);
rcar_du_group_start_stop(rcrtc->group, false);
Extra blank line...
Thanks for spotting this. I'm quite sure I run checkpatch (I just re-did) and not warnings for the extra white space in the previous patch, or this extra blank line o_0
}
/* ----------------------------------------------------------------------------- @@ -642,6 +710,11 @@ static int rcar_du_crtc_atomic_check(struct drm_crtc *crtc, { struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(state); struct drm_encoder *encoder;
int ret;
ret = rcar_du_cmm_check(crtc, state);
if (ret)
return ret;/* Store the routes from the CRTC output to the DU outputs. */ rstate->outputs = 0;
@@ -667,6 +740,7 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(crtc->state); struct rcar_du_device *rcdu = rcrtc->dev;
rcar_du_cmm_enable(crtc); rcar_du_crtc_get(rcrtc);
/*
@@ -686,6 +760,7 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, }
rcar_du_crtc_start(rcrtc);
- rcar_du_cmm_setup(crtc);
}
static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc, @@ -739,6 +814,10 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc, */ rcar_du_crtc_get(rcrtc);
- /* If the active state changed, we let .atomic_enable handle CMM. */
- if (crtc->state->color_mgmt_changed && !crtc->state->active_changed)
rcar_du_cmm_setup(crtc);Aha, this is quite neat for handling the timings.
Yes, much more streamlined than what we had. Thanks Sean and Ezequiel :)
- if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) rcar_du_vsp_atomic_begin(rcrtc);
} diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c index 9eee47969e77..583de800a66d 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c @@ -135,6 +135,7 @@ static void rcar_du_group_setup_didsr(struct rcar_du_group *rgrp) static void rcar_du_group_setup(struct rcar_du_group *rgrp) { struct rcar_du_device *rcdu = rgrp->dev;
u32 defr7 = DEFR7_CODE;
/* Enable extended features */ rcar_du_group_write(rgrp, DEFR, DEFR_CODE | DEFR_DEFE);
@@ -147,6 +148,10 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
rcar_du_group_setup_pins(rgrp);
- defr7 |= (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,
Thank you for the patch.
On Tue, Oct 15, 2019 at 03:37:52PM +0200, Jacopo Mondi wrote:
On Tue, Oct 15, 2019 at 02:15:35PM +0100, Kieran Bingham wrote:
On 15/10/2019 11:46, Jacopo Mondi wrote:
Implement CMM handling in the crtc begin and enable atomic callbacks, and enable CMM unit through the Display Extensional Functions
Extensional ?
Perhaps this should just be Display Extension Functions? Wow - that's actually what they call it in the data-sheet.
register at group setup time.
Only a trivial extra blank line below that I can find... so
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 | 79 +++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_du_group.c | 5 ++ drivers/gpu/drm/rcar-du/rcar_du_regs.h | 5 ++ 3 files changed, 89 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..4bc50a3f4a00 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" @@ -474,6 +475,70 @@ static void rcar_du_crtc_wait_page_flip(struct rcar_du_crtc *rcrtc) rcar_du_crtc_finish_page_flip(rcrtc); }
+/* -----------------------------------------------------------------------------
- Color Management Module (CMM)
- */
Missing blank line.
+static int rcar_du_cmm_enable(struct drm_crtc *crtc) +{
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- if (!rcrtc->cmm)
return 0;- return rcar_cmm_enable(rcrtc->cmm);
+}
+static void rcar_du_cmm_disable(struct drm_crtc *crtc) +{
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- if (!rcrtc->cmm)
return;- rcar_cmm_disable(rcrtc->cmm);
+}
I think I would have inlined those two functions in their only call site as
if (rcrtc->cmm) rcar_cmm_enable(rcrtc->cmm);
but that's up to you.
+static int rcar_du_cmm_check(struct drm_crtc *crtc,
struct drm_crtc_state *state)+{
- struct drm_property_blob *drm_lut = state->gamma_lut;
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- struct device *dev = rcrtc->dev->dev;
- if (!rcrtc->cmm || !drm_lut)
return 0;- /* We only accept fully populated LUT tables. */
- if (CM2_LUT_SIZE * sizeof(struct drm_color_lut) !=
drm_lut->length) {
How about
if (drm_color_lut_size(drm_lut) != CM2_LUT_SIZE)
?
dev_err(dev, "invalid gamma lut size: %lu bytes\n",drm_lut->length);return -EINVAL;- }
- return 0;
+}
+static void rcar_du_cmm_setup(struct drm_crtc *crtc) +{
- struct drm_property_blob *drm_lut = crtc->state->gamma_lut;
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- struct rcar_cmm_config cmm_config = {};
- if (!rcrtc->cmm)
return;- if (drm_lut) {
cmm_config.lut.enable = true;cmm_config.lut.table = (struct drm_color_lut *)drm_lut->data;- } else {
cmm_config.lut.enable = false;- }
This could be changed to
if (drm_lut) cmm_config.lut.table = (struct drm_color_lut *)drm_lut->data;
if we dropped the enable field.
- rcar_cmm_setup(rcrtc->cmm, &cmm_config);
+}
/* -----------------------------------------------------------------------------
- Start/Stop and Suspend/Resume
*/ @@ -619,6 +684,8 @@ 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);
- rcar_du_cmm_disable(crtc);
- /*
- Select switch sync mode. This stops display operation and configures
- the HSYNC and VSYNC signals as inputs.
@@ -631,6 +698,7 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc) DSYSR_TVM_SWITCH);
rcar_du_group_start_stop(rcrtc->group, false);
Extra blank line...
Thanks for spotting this. I'm quite sure I run checkpatch (I just re-did) and not warnings for the extra white space in the previous patch, or this extra blank line o_0
}
/* ----------------------------------------------------------------------------- @@ -642,6 +710,11 @@ static int rcar_du_crtc_atomic_check(struct drm_crtc *crtc, { struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(state); struct drm_encoder *encoder;
int ret;
ret = rcar_du_cmm_check(crtc, state);
if (ret)
return ret;/* Store the routes from the CRTC output to the DU outputs. */ rstate->outputs = 0;
@@ -667,6 +740,7 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(crtc->state); struct rcar_du_device *rcdu = rcrtc->dev;
rcar_du_cmm_enable(crtc); rcar_du_crtc_get(rcrtc);
/*
@@ -686,6 +760,7 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, }
rcar_du_crtc_start(rcrtc);
- rcar_du_cmm_setup(crtc);
This is the only part that really bothers me, we setup the LUT after starting the CRTC, so the first frame will be output with a disabled LUT, or possibly even with the LUT enabled in the middle of the frame. Do I recall correctly that moving setup before start causes issues ? Could you explain what happens ?
}
static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc, @@ -739,6 +814,10 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc, */ rcar_du_crtc_get(rcrtc);
- /* If the active state changed, we let .atomic_enable handle CMM. */
- if (crtc->state->color_mgmt_changed && !crtc->state->active_changed)
rcar_du_cmm_setup(crtc);Aha, this is quite neat for handling the timings.
Yes, much more streamlined than what we had. Thanks Sean and Ezequiel :)
Yes, it guarantees that the CRTC is enabled, so we should be safe with the assumption from patch 3/8 that rcar_du_cmm_setup() is always called with the CMM enabled.
Interestingly though, this doesn't implement atomicity. That will be a very interesting challenge. We should use double buffering of the LUT in the CMM to avoid it being modified in the middle of the frame, but how to update it in sync with the commit, and thus the VSP, remains to be researched.
Could you maybe add a TODO comment in patch 3/8 to mention that we should use double buffering ?
- if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) rcar_du_vsp_atomic_begin(rcrtc);
} diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c index 9eee47969e77..583de800a66d 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c @@ -135,6 +135,7 @@ static void rcar_du_group_setup_didsr(struct rcar_du_group *rgrp) static void rcar_du_group_setup(struct rcar_du_group *rgrp) { struct rcar_du_device *rcdu = rgrp->dev;
u32 defr7 = DEFR7_CODE;
/* Enable extended features */ rcar_du_group_write(rgrp, DEFR, DEFR_CODE | DEFR_DEFE);
@@ -147,6 +148,10 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
rcar_du_group_setup_pins(rgrp);
Could you please add a comment here to mention that we shouldn't route through CMM if no color management feature is used ?
/* * TODO: Handling routing the DU output to CMM dynamically, as we should * bypass CMM completely when no color management feature is used. */
- defr7 |= (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 Laurent,
On Tue, Oct 15, 2019 at 08:54:22PM +0300, Laurent Pinchart wrote:
Hi Jacopo,
Thank you for the patch.
On Tue, Oct 15, 2019 at 03:37:52PM +0200, Jacopo Mondi wrote:
On Tue, Oct 15, 2019 at 02:15:35PM +0100, Kieran Bingham wrote:
On 15/10/2019 11:46, Jacopo Mondi wrote:
Implement CMM handling in the crtc begin and enable atomic callbacks, and enable CMM unit through the Display Extensional Functions
Extensional ?
Perhaps this should just be Display Extension Functions? Wow - that's actually what they call it in the data-sheet.
register at group setup time.
Only a trivial extra blank line below that I can find... so
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 | 79 +++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_du_group.c | 5 ++ drivers/gpu/drm/rcar-du/rcar_du_regs.h | 5 ++ 3 files changed, 89 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..4bc50a3f4a00 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" @@ -474,6 +475,70 @@ static void rcar_du_crtc_wait_page_flip(struct rcar_du_crtc *rcrtc) rcar_du_crtc_finish_page_flip(rcrtc); }
+/* -----------------------------------------------------------------------------
- Color Management Module (CMM)
- */
Missing blank line.
+static int rcar_du_cmm_enable(struct drm_crtc *crtc) +{
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- if (!rcrtc->cmm)
return 0;- return rcar_cmm_enable(rcrtc->cmm);
+}
+static void rcar_du_cmm_disable(struct drm_crtc *crtc) +{
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- if (!rcrtc->cmm)
return;- rcar_cmm_disable(rcrtc->cmm);
+}
I think I would have inlined those two functions in their only call site as
if (rcrtc->cmm) rcar_cmm_enable(rcrtc->cmm);
but that's up to you.
Mmm, I preferred to avoid if() in the caller, but as this is a small increment to an existing big driver, I'll go for the most consistent way
+static int rcar_du_cmm_check(struct drm_crtc *crtc,
struct drm_crtc_state *state)+{
- struct drm_property_blob *drm_lut = state->gamma_lut;
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- struct device *dev = rcrtc->dev->dev;
- if (!rcrtc->cmm || !drm_lut)
return 0;- /* We only accept fully populated LUT tables. */
- if (CM2_LUT_SIZE * sizeof(struct drm_color_lut) !=
drm_lut->length) {How about
if (drm_color_lut_size(drm_lut) != CM2_LUT_SIZE)
?
Better :)
dev_err(dev, "invalid gamma lut size: %lu bytes\n",drm_lut->length);return -EINVAL;- }
- return 0;
+}
+static void rcar_du_cmm_setup(struct drm_crtc *crtc) +{
- struct drm_property_blob *drm_lut = crtc->state->gamma_lut;
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- struct rcar_cmm_config cmm_config = {};
- if (!rcrtc->cmm)
return;- if (drm_lut) {
cmm_config.lut.enable = true;cmm_config.lut.table = (struct drm_color_lut *)drm_lut->data;- } else {
cmm_config.lut.enable = false;- }
This could be changed to
if (drm_lut) cmm_config.lut.table = (struct drm_color_lut *)drm_lut->data;
if we dropped the enable field.
Dropping enable seems good
- rcar_cmm_setup(rcrtc->cmm, &cmm_config);
+}
/* -----------------------------------------------------------------------------
- Start/Stop and Suspend/Resume
*/ @@ -619,6 +684,8 @@ 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);
- rcar_du_cmm_disable(crtc);
- /*
- Select switch sync mode. This stops display operation and configures
- the HSYNC and VSYNC signals as inputs.
@@ -631,6 +698,7 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc) DSYSR_TVM_SWITCH);
rcar_du_group_start_stop(rcrtc->group, false);
Extra blank line...
Thanks for spotting this. I'm quite sure I run checkpatch (I just re-did) and not warnings for the extra white space in the previous patch, or this extra blank line o_0
}
/* ----------------------------------------------------------------------------- @@ -642,6 +710,11 @@ static int rcar_du_crtc_atomic_check(struct drm_crtc *crtc, { struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(state); struct drm_encoder *encoder;
int ret;
ret = rcar_du_cmm_check(crtc, state);
if (ret)
return ret;/* Store the routes from the CRTC output to the DU outputs. */ rstate->outputs = 0;
@@ -667,6 +740,7 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(crtc->state); struct rcar_du_device *rcdu = rcrtc->dev;
rcar_du_cmm_enable(crtc); rcar_du_crtc_get(rcrtc);
/*
@@ -686,6 +760,7 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, }
rcar_du_crtc_start(rcrtc);
- rcar_du_cmm_setup(crtc);
This is the only part that really bothers me, we setup the LUT after starting the CRTC, so the first frame will be output with a disabled LUT, or possibly even with the LUT enabled in the middle of the frame.
I tested by programming a gamma_table to the very first frame, and the table gets set. I cannot tell at the naked eye if it actually takes effect on the first one though :)
Do I recall correctly that moving setup before start causes issues ? Could you explain what happens ?
Simply, the machine hangs :0
The chip manual prescribes to ("35A.3.2 CMM Setting")
4) LUT Activation: Set LUT_EN bit of CMM_LUT_CTRL register to 1 5) DU Activation 6) LUT updates: (In case of single buffer mode) Any accesses to update LUT table after next frame start, output value from CMM is not guaranteed.
What we do is actually 5) 4) 6) as 4 and 6 happens in cmm_setup()
It seems anyway from point 6 description that there are not many ways to programm a LUT table -before- DU activation, but it might depends if DU activation implies only clock and power activation, or the actual routing and compositor activation as it happens in rcar_du_crtc_start().
I could actually try to program the CMM just before the VSP compositor is activated... Is it worth it ?
}
static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc, @@ -739,6 +814,10 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc, */ rcar_du_crtc_get(rcrtc);
- /* If the active state changed, we let .atomic_enable handle CMM. */
- if (crtc->state->color_mgmt_changed && !crtc->state->active_changed)
rcar_du_cmm_setup(crtc);Aha, this is quite neat for handling the timings.
Yes, much more streamlined than what we had. Thanks Sean and Ezequiel :)
Yes, it guarantees that the CRTC is enabled, so we should be safe with the assumption from patch 3/8 that rcar_du_cmm_setup() is always called with the CMM enabled.
Interestingly though, this doesn't implement atomicity. That will be a very interesting challenge. We should use double buffering of the LUT in the CMM to avoid it being modified in the middle of the frame, but how to update it in sync with the commit, and thus the VSP, remains to be researched.
Could you maybe add a TODO comment in patch 3/8 to mention that we should use double buffering ?
Yes, that should probably be supported
- if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) rcar_du_vsp_atomic_begin(rcrtc);
} diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c index 9eee47969e77..583de800a66d 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c @@ -135,6 +135,7 @@ static void rcar_du_group_setup_didsr(struct rcar_du_group *rgrp) static void rcar_du_group_setup(struct rcar_du_group *rgrp) { struct rcar_du_device *rcdu = rgrp->dev;
u32 defr7 = DEFR7_CODE;
/* Enable extended features */ rcar_du_group_write(rgrp, DEFR, DEFR_CODE | DEFR_DEFE);
@@ -147,6 +148,10 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
rcar_du_group_setup_pins(rgrp);
Could you please add a comment here to mention that we shouldn't route through CMM if no color management feature is used ?
/* * TODO: Handling routing the DU output to CMM dynamically, as we should * bypass CMM completely when no color management feature is used. */
I'm not sure I get what you mean. If CMM is not enabled we do not enable the corresponding DEFR7_CMMEx
Or do you mean we should disable CMM if not gamma_table (or CLU table, or other CMM functions) is provided ? I assume if any of those functions is not enabled the pixels are not routed through the CMM (in example if LUT_EN is not set, not lookup is performed). Am I wrong ?
Thanks j
- defr7 |= (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
*/
-- Regards,
Laurent Pinchart
Hi Jacopo,
On Tue, Oct 15, 2019 at 09:17:54PM +0200, Jacopo Mondi wrote:
On Tue, Oct 15, 2019 at 08:54:22PM +0300, Laurent Pinchart wrote:
On Tue, Oct 15, 2019 at 03:37:52PM +0200, Jacopo Mondi wrote:
On Tue, Oct 15, 2019 at 02:15:35PM +0100, Kieran Bingham wrote:
On 15/10/2019 11:46, Jacopo Mondi wrote:
Implement CMM handling in the crtc begin and enable atomic callbacks, and enable CMM unit through the Display Extensional Functions
Extensional ?
Perhaps this should just be Display Extension Functions? Wow - that's actually what they call it in the data-sheet.
register at group setup time.
Only a trivial extra blank line below that I can find... so
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 | 79 +++++++++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_du_group.c | 5 ++ drivers/gpu/drm/rcar-du/rcar_du_regs.h | 5 ++ 3 files changed, 89 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..4bc50a3f4a00 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" @@ -474,6 +475,70 @@ static void rcar_du_crtc_wait_page_flip(struct rcar_du_crtc *rcrtc) rcar_du_crtc_finish_page_flip(rcrtc); }
+/* -----------------------------------------------------------------------------
- Color Management Module (CMM)
- */
Missing blank line.
+static int rcar_du_cmm_enable(struct drm_crtc *crtc) +{
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- if (!rcrtc->cmm)
return 0;- return rcar_cmm_enable(rcrtc->cmm);
+}
+static void rcar_du_cmm_disable(struct drm_crtc *crtc) +{
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- if (!rcrtc->cmm)
return;- rcar_cmm_disable(rcrtc->cmm);
+}
I think I would have inlined those two functions in their only call site as
if (rcrtc->cmm) rcar_cmm_enable(rcrtc->cmm);
but that's up to you.
Mmm, I preferred to avoid if() in the caller, but as this is a small increment to an existing big driver, I'll go for the most consistent way
+static int rcar_du_cmm_check(struct drm_crtc *crtc,
struct drm_crtc_state *state)+{
- struct drm_property_blob *drm_lut = state->gamma_lut;
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- struct device *dev = rcrtc->dev->dev;
- if (!rcrtc->cmm || !drm_lut)
return 0;- /* We only accept fully populated LUT tables. */
- if (CM2_LUT_SIZE * sizeof(struct drm_color_lut) !=
drm_lut->length) {How about
if (drm_color_lut_size(drm_lut) != CM2_LUT_SIZE)
?
Better :)
dev_err(dev, "invalid gamma lut size: %lu bytes\n",drm_lut->length);return -EINVAL;- }
- return 0;
+}
+static void rcar_du_cmm_setup(struct drm_crtc *crtc) +{
- struct drm_property_blob *drm_lut = crtc->state->gamma_lut;
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- struct rcar_cmm_config cmm_config = {};
- if (!rcrtc->cmm)
return;- if (drm_lut) {
cmm_config.lut.enable = true;cmm_config.lut.table = (struct drm_color_lut *)drm_lut->data;- } else {
cmm_config.lut.enable = false;- }
This could be changed to
if (drm_lut) cmm_config.lut.table = (struct drm_color_lut *)drm_lut->data;
if we dropped the enable field.
Dropping enable seems good
- rcar_cmm_setup(rcrtc->cmm, &cmm_config);
+}
/* -----------------------------------------------------------------------------
- Start/Stop and Suspend/Resume
*/ @@ -619,6 +684,8 @@ 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);
- rcar_du_cmm_disable(crtc);
- /*
- Select switch sync mode. This stops display operation and configures
- the HSYNC and VSYNC signals as inputs.
@@ -631,6 +698,7 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc) DSYSR_TVM_SWITCH);
rcar_du_group_start_stop(rcrtc->group, false);
Extra blank line...
Thanks for spotting this. I'm quite sure I run checkpatch (I just re-did) and not warnings for the extra white space in the previous patch, or this extra blank line o_0
}
/* ----------------------------------------------------------------------------- @@ -642,6 +710,11 @@ static int rcar_du_crtc_atomic_check(struct drm_crtc *crtc, { struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(state); struct drm_encoder *encoder;
int ret;
ret = rcar_du_cmm_check(crtc, state);
if (ret)
return ret;/* Store the routes from the CRTC output to the DU outputs. */ rstate->outputs = 0;
@@ -667,6 +740,7 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(crtc->state); struct rcar_du_device *rcdu = rcrtc->dev;
rcar_du_cmm_enable(crtc); rcar_du_crtc_get(rcrtc);
/*
@@ -686,6 +760,7 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, }
rcar_du_crtc_start(rcrtc);
- rcar_du_cmm_setup(crtc);
This is the only part that really bothers me, we setup the LUT after starting the CRTC, so the first frame will be output with a disabled LUT, or possibly even with the LUT enabled in the middle of the frame.
I tested by programming a gamma_table to the very first frame, and the table gets set. I cannot tell at the naked eye if it actually takes effect on the first one though :)
Do I recall correctly that moving setup before start causes issues ? Could you explain what happens ?
Simply, the machine hangs :0
The chip manual prescribes to ("35A.3.2 CMM Setting")
- LUT Activation: Set LUT_EN bit of CMM_LUT_CTRL register to 1
- DU Activation
- LUT updates: (In case of single buffer mode) Any accesses to update LUT table after next frame start, output value from CMM is not guaranteed.
What we do is actually 5) 4) 6) as 4 and 6 happens in cmm_setup()
It seems anyway from point 6 description that there are not many ways to programm a LUT table -before- DU activation, but it might depends if DU activation implies only clock and power activation, or the actual routing and compositor activation as it happens in rcar_du_crtc_start().
I could actually try to program the CMM just before the VSP compositor is activated... Is it worth it ?
Whatever could give us a better understanding of the problem could be useful I think. Maybe you could send this question to Renesas, to figure out what the recommended way to program the LUT for the first frame is ? Maybe we'll be told it's not possible for some reason, but at least we will then know for sure.
I'm fine doing so on top of this series, but then I'd like a TODO item here to explain the problem (and of course I'd like you to ask Renesas about it :-)).
}
static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc, @@ -739,6 +814,10 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc, */ rcar_du_crtc_get(rcrtc);
- /* If the active state changed, we let .atomic_enable handle CMM. */
- if (crtc->state->color_mgmt_changed && !crtc->state->active_changed)
rcar_du_cmm_setup(crtc);Aha, this is quite neat for handling the timings.
Yes, much more streamlined than what we had. Thanks Sean and Ezequiel :)
Yes, it guarantees that the CRTC is enabled, so we should be safe with the assumption from patch 3/8 that rcar_du_cmm_setup() is always called with the CMM enabled.
Interestingly though, this doesn't implement atomicity. That will be a very interesting challenge. We should use double buffering of the LUT in the CMM to avoid it being modified in the middle of the frame, but how to update it in sync with the commit, and thus the VSP, remains to be researched.
Could you maybe add a TODO comment in patch 3/8 to mention that we should use double buffering ?
Yes, that should probably be supported
- if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) rcar_du_vsp_atomic_begin(rcrtc);
} diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c index 9eee47969e77..583de800a66d 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c @@ -135,6 +135,7 @@ static void rcar_du_group_setup_didsr(struct rcar_du_group *rgrp) static void rcar_du_group_setup(struct rcar_du_group *rgrp) { struct rcar_du_device *rcdu = rgrp->dev;
u32 defr7 = DEFR7_CODE;
/* Enable extended features */ rcar_du_group_write(rgrp, DEFR, DEFR_CODE | DEFR_DEFE);
@@ -147,6 +148,10 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
rcar_du_group_setup_pins(rgrp);
Could you please add a comment here to mention that we shouldn't route through CMM if no color management feature is used ?
/* * TODO: Handling routing the DU output to CMM dynamically, as we should * bypass CMM completely when no color management feature is used. */
I'm not sure I get what you mean. If CMM is not enabled we do not enable the corresponding DEFR7_CMMEx
Or do you mean we should disable CMM if not gamma_table (or CLU table, or other CMM functions) is provided ? I assume if any of those functions is not enabled the pixels are not routed through the CMM (in example if LUT_EN is not set, not lookup is performed). Am I wrong ?
If the LUT is disabled then the CMM will by pass the LUT, but pixels will still go through the CMM. We could, when all color correction is disabled, disable CMM completely, which would require controlling DEFR7 dynamically. This should save a bit of power.
- defr7 |= (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
*/
Enable the GAMMA_LUT KMS property using the framework helpers to register the property and set the associated gamma table maximum size.
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com 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 4bc50a3f4a00..603f9f716721 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -1154,6 +1154,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, };
/* ----------------------------------------------------------------------------- @@ -1277,6 +1278,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);
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', 'renesas,cmm' and 'status' properties 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 | 39 +++++++++++++++++++++++ arch/arm64/boot/dts/renesas/r8a7796.dtsi | 31 +++++++++++++++++- arch/arm64/boot/dts/renesas/r8a77965.dtsi | 31 +++++++++++++++++- arch/arm64/boot/dts/renesas/r8a77990.dtsi | 21 ++++++++++++ arch/arm64/boot/dts/renesas/r8a77995.dtsi | 21 ++++++++++++ 5 files changed, 141 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi index 6675462f7585..e16757af8c27 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,7 +3178,10 @@ <&cpg CPG_MOD 722>, <&cpg CPG_MOD 721>; clock-names = "du.0", "du.1", "du.2", "du.3"; + + renesas,cmms = <&cmm0>, <&cmm1>, <&cmm2>, <&cmm3>; vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd0 1>; + status = "disabled";
ports { diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi b/arch/arm64/boot/dts/renesas/r8a7796.dtsi index 822c96601d3c..597c47f3f994 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>; @@ -2791,10 +2818,12 @@ <&cpg CPG_MOD 723>, <&cpg CPG_MOD 722>; clock-names = "du.0", "du.1", "du.2"; - status = "disabled";
+ renesas,cmms = <&cmm0>, <&cmm1>, <&cmm2>; vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>;
+ status = "disabled"; + ports { #address-cells = <1>; #size-cells = <0>; diff --git a/arch/arm64/boot/dts/renesas/r8a77965.dtsi b/arch/arm64/boot/dts/renesas/r8a77965.dtsi index 4ae163220f60..c3da8d26ccba 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>; @@ -2467,10 +2494,12 @@ <&cpg CPG_MOD 723>, <&cpg CPG_MOD 721>; clock-names = "du.0", "du.1", "du.3"; - status = "disabled";
+ renesas,cmms = <&cmm0>, <&cmm1>, <&cmm3>; vsps = <&vspd0 0>, <&vspd1 0>, <&vspd0 1>;
+ status = "disabled"; + ports { #address-cells = <1>; #size-cells = <0>; diff --git a/arch/arm64/boot/dts/renesas/r8a77990.dtsi b/arch/arm64/boot/dts/renesas/r8a77990.dtsi index 455954c3d98e..bab9b7f96c72 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,7 +1786,10 @@ clock-names = "du.0", "du.1"; resets = <&cpg 724>; reset-names = "du.0"; + + renesas,cmms = <&cmm0>, <&cmm1>; vsps = <&vspd0 0>, <&vspd1 0>; + status = "disabled";
ports { diff --git a/arch/arm64/boot/dts/renesas/r8a77995.dtsi b/arch/arm64/boot/dts/renesas/r8a77995.dtsi index 183fef86cf7c..871c70cc2d2e 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,7 +1021,10 @@ clock-names = "du.0", "du.1"; resets = <&cpg 724>; reset-names = "du.0"; + + renesas,cmms = <&cmm0>, <&cmm1>; vsps = <&vspd0 0>, <&vspd1 0>; + status = "disabled";
ports { -- 2.23.0
Hi Jacopo,
On 15/10/2019 11:46, 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', 'renesas,cmm' and 'status' properties in the DU unit consistently in all the involved DTS.
Going through this, I think I'm happy, except for a 'future' gotcha detailed below.
The H3-N is possibly going to cause some issues (not supporting/connecting/using the CMM2) ... but as we don't really have that yet ... I'm going to say "la la la " ... and put this here:
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
arch/arm64/boot/dts/renesas/r8a7795.dtsi | 39 +++++++++++++++++++++++ arch/arm64/boot/dts/renesas/r8a7796.dtsi | 31 +++++++++++++++++- arch/arm64/boot/dts/renesas/r8a77965.dtsi | 31 +++++++++++++++++- arch/arm64/boot/dts/renesas/r8a77990.dtsi | 21 ++++++++++++ arch/arm64/boot/dts/renesas/r8a77995.dtsi | 21 ++++++++++++ 5 files changed, 141 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi index 6675462f7585..e16757af8c27 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>;};
Yeouch. CMM2 is not available on the H3-N - but as far as I can tell the H3-N is an R8A7795 ...
Geert, How will we differentiate this, or perhaps it just won't matter.
The key part here will be handling it in the DU perhaps anyway.
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,7 +3178,10 @@ <&cpg CPG_MOD 722>, <&cpg CPG_MOD 721>; clock-names = "du.0", "du.1", "du.2", "du.3";
renesas,cmms = <&cmm0>, <&cmm1>, <&cmm2>, <&cmm3>; vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd0 1>;status = "disabled"; ports {diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi b/arch/arm64/boot/dts/renesas/r8a7796.dtsi index 822c96601d3c..597c47f3f994 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>;
@@ -2791,10 +2818,12 @@ <&cpg CPG_MOD 723>, <&cpg CPG_MOD 722>; clock-names = "du.0", "du.1", "du.2";
status = "disabled";
renesas,cmms = <&cmm0>, <&cmm1>, <&cmm2>; vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>;status = "disabled";ports { #address-cells = <1>; #size-cells = <0>;diff --git a/arch/arm64/boot/dts/renesas/r8a77965.dtsi b/arch/arm64/boot/dts/renesas/r8a77965.dtsi index 4ae163220f60..c3da8d26ccba 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>;
@@ -2467,10 +2494,12 @@ <&cpg CPG_MOD 723>, <&cpg CPG_MOD 721>; clock-names = "du.0", "du.1", "du.3";
status = "disabled";
renesas,cmms = <&cmm0>, <&cmm1>, <&cmm3>; vsps = <&vspd0 0>, <&vspd1 0>, <&vspd0 1>;status = "disabled";ports { #address-cells = <1>; #size-cells = <0>;diff --git a/arch/arm64/boot/dts/renesas/r8a77990.dtsi b/arch/arm64/boot/dts/renesas/r8a77990.dtsi index 455954c3d98e..bab9b7f96c72 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,7 +1786,10 @@ clock-names = "du.0", "du.1"; resets = <&cpg 724>; reset-names = "du.0";
renesas,cmms = <&cmm0>, <&cmm1>; vsps = <&vspd0 0>, <&vspd1 0>;status = "disabled"; ports {diff --git a/arch/arm64/boot/dts/renesas/r8a77995.dtsi b/arch/arm64/boot/dts/renesas/r8a77995.dtsi index 183fef86cf7c..871c70cc2d2e 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,7 +1021,10 @@ clock-names = "du.0", "du.1"; resets = <&cpg 724>; reset-names = "du.0";
renesas,cmms = <&cmm0>, <&cmm1>; vsps = <&vspd0 0>, <&vspd1 0>;status = "disabled"; ports {-- 2.23.0
Hi Jacopo and Kieran,
On Tue, Oct 15, 2019 at 01:52:29PM +0100, Kieran Bingham wrote:
On 15/10/2019 11:46, 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', 'renesas,cmm' and 'status' properties in the DU unit consistently in all the involved DTS.
Going through this, I think I'm happy, except for a 'future' gotcha detailed below.
The H3-N is possibly going to cause some issues (not supporting/connecting/using the CMM2) ... but as we don't really have that yet ... I'm going to say "la la la " ... and put this here:
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
arch/arm64/boot/dts/renesas/r8a7795.dtsi | 39 +++++++++++++++++++++++ arch/arm64/boot/dts/renesas/r8a7796.dtsi | 31 +++++++++++++++++- arch/arm64/boot/dts/renesas/r8a77965.dtsi | 31 +++++++++++++++++- arch/arm64/boot/dts/renesas/r8a77990.dtsi | 21 ++++++++++++ arch/arm64/boot/dts/renesas/r8a77995.dtsi | 21 ++++++++++++ 5 files changed, 141 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi index 6675462f7585..e16757af8c27 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>;};Yeouch. CMM2 is not available on the H3-N - but as far as I can tell the H3-N is an R8A7795 ...
Geert, How will we differentiate this, or perhaps it just won't matter.
The key part here will be handling it in the DU perhaps anyway.
I think we'll figure it out when we'll have more information about the H3-N, and in particular if DU2 will be present (but not usable) or completely absent. In the latter case we'll need a separate .dtsi.
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
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,7 +3178,10 @@ <&cpg CPG_MOD 722>, <&cpg CPG_MOD 721>; clock-names = "du.0", "du.1", "du.2", "du.3";
renesas,cmms = <&cmm0>, <&cmm1>, <&cmm2>, <&cmm3>; vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>, <&vspd0 1>;status = "disabled"; ports {diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi b/arch/arm64/boot/dts/renesas/r8a7796.dtsi index 822c96601d3c..597c47f3f994 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>;
@@ -2791,10 +2818,12 @@ <&cpg CPG_MOD 723>, <&cpg CPG_MOD 722>; clock-names = "du.0", "du.1", "du.2";
status = "disabled";
renesas,cmms = <&cmm0>, <&cmm1>, <&cmm2>; vsps = <&vspd0 0>, <&vspd1 0>, <&vspd2 0>;status = "disabled";ports { #address-cells = <1>; #size-cells = <0>;diff --git a/arch/arm64/boot/dts/renesas/r8a77965.dtsi b/arch/arm64/boot/dts/renesas/r8a77965.dtsi index 4ae163220f60..c3da8d26ccba 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>;
@@ -2467,10 +2494,12 @@ <&cpg CPG_MOD 723>, <&cpg CPG_MOD 721>; clock-names = "du.0", "du.1", "du.3";
status = "disabled";
renesas,cmms = <&cmm0>, <&cmm1>, <&cmm3>; vsps = <&vspd0 0>, <&vspd1 0>, <&vspd0 1>;status = "disabled";ports { #address-cells = <1>; #size-cells = <0>;diff --git a/arch/arm64/boot/dts/renesas/r8a77990.dtsi b/arch/arm64/boot/dts/renesas/r8a77990.dtsi index 455954c3d98e..bab9b7f96c72 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,7 +1786,10 @@ clock-names = "du.0", "du.1"; resets = <&cpg 724>; reset-names = "du.0";
renesas,cmms = <&cmm0>, <&cmm1>; vsps = <&vspd0 0>, <&vspd1 0>;status = "disabled"; ports {diff --git a/arch/arm64/boot/dts/renesas/r8a77995.dtsi b/arch/arm64/boot/dts/renesas/r8a77995.dtsi index 183fef86cf7c..871c70cc2d2e 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,7 +1021,10 @@ clock-names = "du.0", "du.1"; resets = <&cpg 724>; reset-names = "du.0";
renesas,cmms = <&cmm0>, <&cmm1>; vsps = <&vspd0 0>, <&vspd1 0>;status = "disabled"; ports {
Expand comment in the 'vsps' parsing routine to specify the LIF channel index defaults to 0 in case the second cell of the property is not specified to remain compatible with older DT bindings.
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org --- drivers/gpu/drm/rcar-du/rcar_du_kms.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 7c9fb5860e54..186422ac552b 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -587,7 +587,11 @@ static int rcar_du_vsps_init(struct rcar_du_device *rcdu)
vsps[j].crtcs_mask |= BIT(i);
- /* Store the VSP pointer and pipe index in the CRTC. */ + /* + * Store the VSP pointer and pipe index in the CRTC. If the + * second cell of the 'vsps' specifier isn't present, default + * to 0 to remain compatible with older DT bindings. + */ rcdu->crtcs[i].vsp = &rcdu->vsps[j]; rcdu->crtcs[i].vsp_pipe = cells >= 1 ? args.args[0] : 0; }
Hi Jacopo,
On 15/10/2019 11:46, Jacopo Mondi wrote:
Expand comment in the 'vsps' parsing routine to specify the LIF channel index defaults to 0 in case the second cell of the property is not specified to remain compatible with older DT bindings.
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
drivers/gpu/drm/rcar-du/rcar_du_kms.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 7c9fb5860e54..186422ac552b 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -587,7 +587,11 @@ static int rcar_du_vsps_init(struct rcar_du_device *rcdu)
vsps[j].crtcs_mask |= BIT(i);
/* Store the VSP pointer and pipe index in the CRTC. */
/** Store the VSP pointer and pipe index in the CRTC. If the* second cell of the 'vsps' specifier isn't present, default* to 0 to remain compatible with older DT bindings.*/
Hi Jcopo,
Thank you for the patches.
On Tue, Oct 15, 2019 at 12:46:13PM +0200, Jacopo Mondi wrote:
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... v4: https://lore.kernel.org/linux-renesas-soc/20190906135436.10622-1-jacopo+rene...
Again, quite a consistent changelog, mostly due to the developments happened on Ezequiel's VOP unit following Sean's advices.
I here implemented the same, and moved the CMM handling to the crtc being and enable callbacks. As a result the overall implementation results quite a lot simplified, mostly on the CMM driver side.
I have dropped tags and acks on the CMM driver and CMM enablement patches in DU crtc driver because of the number of changes.
A more detailed change log:
- Rebased on renesas-devel-2019-10-07-v5.4-rc4
- Bindings/DT
- Included Rob's comments on the yaml file license and the use of 'OneOf' in the compatible property description
- Use the bracketed style suggested by Kieran for the 'renesas,cmm' property introduced in patch 2
- Re-order the properties in the SoC DTS files as suggested by Kieran
- CMM/DU
- As anticipated, moved CMM management to the crtc from the atomic commit tail helper where it was implemented in v4 This allow to correctly support resume/suspend and proper ordering of the CMM enable and setup operations (enable -before- setup)
- As a consequence the CMM driver is greatly simplified by removing the need to cache the LUT table entries provided to cmm_setup() and later re-apply them at enable time.
- Better support handling of disabled CMM config option by returning -ENODEV at cmm_init() time as suggested by Kieran.
Could you, for your next series (hopefully not CMM-related :-)) move the changelog to individual patches ? Having it in the cover letter requires going back and forth, and also doesn't provide detailed changelog. I recommend adding below a --- line in the commit message, so that every time you commit --amend to update a patch, you can record the exact detailed change at the same time.
- Testing
I have tested by injecting a color inversion LUT table and enabling/disabling it every 50 displayed frames: https://jmondi.org/cgit/kmsxx/log/?h=gamma_lut
CMM functionalities are retained between suspend/resume cycles (tested with suspend-to-idle) without requiring a re-programming of the LUT tables.
Testing with real world use cases might be beneficial. Rajesh are you still interested in giving this series a spin?
Laurent, Kieran, could we fast-track review of this and hopefully try to have it merged for v5.5 ?
Thanks Ezequiel for having suggested me this solution.
Thanks j
Jacopo Mondi (8): 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: kms: Initialize CMM instances drm: rcar-du: crtc: Control CMM operations drm: rcar-du: crtc: Register GAMMA_LUT properties arm64: dts: renesas: Add CMM units to Gen3 SoCs drm: rcar-du: kms: Expand comment in vsps parsing routine
.../bindings/display/renesas,cmm.yaml | 67 ++++++ .../bindings/display/renesas,du.txt | 5 + arch/arm64/boot/dts/renesas/r8a7795.dtsi | 39 ++++ arch/arm64/boot/dts/renesas/r8a7796.dtsi | 31 ++- arch/arm64/boot/dts/renesas/r8a77965.dtsi | 31 ++- arch/arm64/boot/dts/renesas/r8a77990.dtsi | 21 ++ arch/arm64/boot/dts/renesas/r8a77995.dtsi | 21 ++ drivers/gpu/drm/rcar-du/Kconfig | 7 + drivers/gpu/drm/rcar-du/Makefile | 1 + drivers/gpu/drm/rcar-du/rcar_cmm.c | 198 ++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 60 ++++++ drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 89 ++++++++ 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.c | 5 + drivers/gpu/drm/rcar-du/rcar_du_group.h | 2 + drivers/gpu/drm/rcar-du/rcar_du_kms.c | 82 +++++++- drivers/gpu/drm/rcar-du/rcar_du_regs.h | 5 + 18 files changed, 665 insertions(+), 3 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
Hi Jacopo,
Please find comments below.
Best regards,
Rajesh Kv RBEI/ECF3
-----Original Message----- From: linux-kernel-owner@vger.kernel.org <linux-kernel- owner@vger.kernel.org> On Behalf Of Jacopo Mondi Sent: Tuesday, October 15, 2019 4:16 PM To: laurent.pinchart@ideasonboard.com; kieran.bingham+renesas@ideasonboard.com; geert@linux-m68k.org; horms@verge.net.au; uli+renesas@fpond.eu; Kalakodima Venkata Rajesh (RBEI/ECF3) VenkataRajesh.Kalakodima@in.bosch.com Cc: Jacopo Mondi jacopo+renesas@jmondi.org; airlied@linux.ie; daniel@ffwll.ch; koji.matsuoka.xm@renesas.com; muroya@ksk.co.jp; Harsha Manjula Mallikarjun (RBEI/ECF3) Harsha.ManjulaMallikarjun@in.bosch.com; ezequiel@collabora.com; seanpaul@chromium.org; linux-renesas- soc@vger.kernel.org; dri-devel@lists.freedesktop.org; linux- kernel@vger.kernel.org Subject: [PATCH v5 0/8] drm: rcar-du: Add Color Management Module (CMM)
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+renesas@jmondi.org/ v3: https://lore.kernel.org/linux-renesas-soc/20190825135154.11488-1- jacopo+renesas@jmondi.org/ v4: https://lore.kernel.org/linux-renesas-soc/20190906135436.10622-1- jacopo+renesas@jmondi.org/
Again, quite a consistent changelog, mostly due to the developments happened on Ezequiel's VOP unit following Sean's advices.
I here implemented the same, and moved the CMM handling to the crtc being and enable callbacks. As a result the overall implementation results quite a lot simplified, mostly on the CMM driver side.
I have dropped tags and acks on the CMM driver and CMM enablement patches in DU crtc driver because of the number of changes.
A more detailed change log:
- Rebased on renesas-devel-2019-10-07-v5.4-rc4
- Bindings/DT
- Included Rob's comments on the yaml file license and the use of 'OneOf' in the compatible property description
- Use the bracketed style suggested by Kieran for the 'renesas,cmm' property introduced in patch 2
- Re-order the properties in the SoC DTS files as suggested by Kieran
- CMM/DU
- As anticipated, moved CMM management to the crtc from the atomic commit
tail helper where it was implemented in v4 This allow to correctly support resume/suspend and proper ordering of the CMM enable and setup operations (enable -before- setup)
- As a consequence the CMM driver is greatly simplified by removing the need to cache the LUT table entries provided to cmm_setup() and later re-apply them at enable time.
- Better support handling of disabled CMM config option by returning -ENODEV at cmm_init() time as suggested by Kieran.
- Testing
I have tested by injecting a color inversion LUT table and enabling/disabling it every 50 displayed frames: https://jmondi.org/cgit/kmsxx/log/?h=gamma_lut
CMM functionalities are retained between suspend/resume cycles (tested with suspend-to-idle) without requiring a re-programming of the LUT tables.
Testing with real world use cases might be beneficial. Rajesh are you still interested in giving this series a spin
I have tested version v3 of CMM module with a demo application based on libdrm library. I could successfully test setting of Gamma LUT.
Next step is to test on full featured graphics stack i.e. involving Weston and OpenGL. Weston can set Gamma. I have to stop this work for a while due to other high prio activities. I plan to resume soon.
Laurent, Kieran, could we fast-track review of this and hopefully try to have it merged for v5.5 ?
Thanks Ezequiel for having suggested me this solution.
Thanks j
Jacopo Mondi (8): 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: kms: Initialize CMM instances drm: rcar-du: crtc: Control CMM operations drm: rcar-du: crtc: Register GAMMA_LUT properties arm64: dts: renesas: Add CMM units to Gen3 SoCs drm: rcar-du: kms: Expand comment in vsps parsing routine
.../bindings/display/renesas,cmm.yaml | 67 ++++++ .../bindings/display/renesas,du.txt | 5 + arch/arm64/boot/dts/renesas/r8a7795.dtsi | 39 ++++ arch/arm64/boot/dts/renesas/r8a7796.dtsi | 31 ++- arch/arm64/boot/dts/renesas/r8a77965.dtsi | 31 ++- arch/arm64/boot/dts/renesas/r8a77990.dtsi | 21 ++ arch/arm64/boot/dts/renesas/r8a77995.dtsi | 21 ++ drivers/gpu/drm/rcar-du/Kconfig | 7 + drivers/gpu/drm/rcar-du/Makefile | 1 + drivers/gpu/drm/rcar-du/rcar_cmm.c | 198 ++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 60 ++++++ drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 89 ++++++++ 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.c | 5 + drivers/gpu/drm/rcar-du/rcar_du_group.h | 2 + drivers/gpu/drm/rcar-du/rcar_du_kms.c | 82 +++++++- drivers/gpu/drm/rcar-du/rcar_du_regs.h | 5 + 18 files changed, 665 insertions(+), 3 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
Hello,
On Mon, Nov 11, 2019 at 11:21:28AM +0000, Kalakodima Venkata Rajesh (RBEI/ECF3) wrote:
Hi Jacopo,
Please find comments below.
Best regards,
Rajesh Kv RBEI/ECF3
-----Original Message----- From: linux-kernel-owner@vger.kernel.org <linux-kernel- owner@vger.kernel.org> On Behalf Of Jacopo Mondi Sent: Tuesday, October 15, 2019 4:16 PM To: laurent.pinchart@ideasonboard.com; kieran.bingham+renesas@ideasonboard.com; geert@linux-m68k.org; horms@verge.net.au; uli+renesas@fpond.eu; Kalakodima Venkata Rajesh (RBEI/ECF3) VenkataRajesh.Kalakodima@in.bosch.com Cc: Jacopo Mondi jacopo+renesas@jmondi.org; airlied@linux.ie; daniel@ffwll.ch; koji.matsuoka.xm@renesas.com; muroya@ksk.co.jp; Harsha Manjula Mallikarjun (RBEI/ECF3) Harsha.ManjulaMallikarjun@in.bosch.com; ezequiel@collabora.com; seanpaul@chromium.org; linux-renesas- soc@vger.kernel.org; dri-devel@lists.freedesktop.org; linux- kernel@vger.kernel.org Subject: [PATCH v5 0/8] drm: rcar-du: Add Color Management Module (CMM)
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+renesas@jmondi.org/ v3: https://lore.kernel.org/linux-renesas-soc/20190825135154.11488-1- jacopo+renesas@jmondi.org/ v4: https://lore.kernel.org/linux-renesas-soc/20190906135436.10622-1- jacopo+renesas@jmondi.org/
Again, quite a consistent changelog, mostly due to the developments happened on Ezequiel's VOP unit following Sean's advices.
I here implemented the same, and moved the CMM handling to the crtc being and enable callbacks. As a result the overall implementation results quite a lot simplified, mostly on the CMM driver side.
I have dropped tags and acks on the CMM driver and CMM enablement patches in DU crtc driver because of the number of changes.
A more detailed change log:
- Rebased on renesas-devel-2019-10-07-v5.4-rc4
- Bindings/DT
- Included Rob's comments on the yaml file license and the use of 'OneOf' in the compatible property description
- Use the bracketed style suggested by Kieran for the 'renesas,cmm' property introduced in patch 2
- Re-order the properties in the SoC DTS files as suggested by Kieran
- CMM/DU
- As anticipated, moved CMM management to the crtc from the atomic commit
tail helper where it was implemented in v4 This allow to correctly support resume/suspend and proper ordering of the CMM enable and setup operations (enable -before- setup)
- As a consequence the CMM driver is greatly simplified by removing the need to cache the LUT table entries provided to cmm_setup() and later re-apply them at enable time.
- Better support handling of disabled CMM config option by returning -ENODEV at cmm_init() time as suggested by Kieran.
- Testing
I have tested by injecting a color inversion LUT table and enabling/disabling it every 50 displayed frames: https://jmondi.org/cgit/kmsxx/log/?h=gamma_lut
CMM functionalities are retained between suspend/resume cycles (tested with suspend-to-idle) without requiring a re-programming of the LUT tables.
Testing with real world use cases might be beneficial. Rajesh are you still interested in giving this series a spin
I have tested version v3 of CMM module with a demo application based on libdrm library. I could successfully test setting of Gamma LUT.
\o/
If you want to, please send your Tested-by tag, so that it can be collected, as CMM support will be collected for the v5.6 merge window, as we had a small issue that prevented v6 from being part of the v5.5 one.
Next step is to test on full featured graphics stack i.e. involving Weston and OpenGL. Weston can set Gamma. I have to stop this work for a while due to other high prio activities. I plan to resume soon.
Thanks for testing and please keep us posted!
Thanks j
Laurent, Kieran, could we fast-track review of this and hopefully try to have it merged for v5.5 ?
Thanks Ezequiel for having suggested me this solution.
Thanks j
Jacopo Mondi (8): 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: kms: Initialize CMM instances drm: rcar-du: crtc: Control CMM operations drm: rcar-du: crtc: Register GAMMA_LUT properties arm64: dts: renesas: Add CMM units to Gen3 SoCs drm: rcar-du: kms: Expand comment in vsps parsing routine
.../bindings/display/renesas,cmm.yaml | 67 ++++++ .../bindings/display/renesas,du.txt | 5 + arch/arm64/boot/dts/renesas/r8a7795.dtsi | 39 ++++ arch/arm64/boot/dts/renesas/r8a7796.dtsi | 31 ++- arch/arm64/boot/dts/renesas/r8a77965.dtsi | 31 ++- arch/arm64/boot/dts/renesas/r8a77990.dtsi | 21 ++ arch/arm64/boot/dts/renesas/r8a77995.dtsi | 21 ++ drivers/gpu/drm/rcar-du/Kconfig | 7 + drivers/gpu/drm/rcar-du/Makefile | 1 + drivers/gpu/drm/rcar-du/rcar_cmm.c | 198 ++++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_cmm.h | 60 ++++++ drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 89 ++++++++ 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.c | 5 + drivers/gpu/drm/rcar-du/rcar_du_group.h | 2 + drivers/gpu/drm/rcar-du/rcar_du_kms.c | 82 +++++++- drivers/gpu/drm/rcar-du/rcar_du_regs.h | 5 + 18 files changed, 665 insertions(+), 3 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
Hi Jacopo,
On Tue, Oct 15, 2019 at 12:46:13PM +0200, Jacopo Mondi wrote: ----8<---
- Testing
I have tested by injecting a color inversion LUT table and enabling/disabling it every 50 displayed frames: https://jmondi.org/cgit/kmsxx/log/?h=gamma_lut
Could you kindly share the cross compilation steps for your kmsxx fork?
Just out of curiosity, have you ever tried to pull the display's HDMI cable while reading from CM2_LUT_TBL?
At least with the out-of-tree CMM implementation [*], this sends the R-Car3 reference targets into an unrecoverable freeze, with no lockup reported by the kernel (i.e. looks like an serious HW issue).
CMM functionalities are retained between suspend/resume cycles (tested with suspend-to-idle) without requiring a re-programming of the LUT tables.
Hmm. Is this backed up by any statement in the HW User's manual? This comes in contrast with the original Renesas CMM implementation [**] which does make use of suspend (where the freeze actually happens).
Can we infer, based on your statement, that we could also get rid of the suspend callback in [**]?
[*] https://github.com/renesas-rcar/du_cmm [**] https://github.com/renesas-rcar/du_cmm/blob/c393ed49834bdbc/meta-rcar-gen3/r...
Hi Eugeniu,
On Wed, May 27, 2020 at 9:16 AM Eugeniu Rosca erosca@de.adit-jv.com wrote:
On Tue, Oct 15, 2019 at 12:46:13PM +0200, Jacopo Mondi wrote:
CMM functionalities are retained between suspend/resume cycles (tested with suspend-to-idle) without requiring a re-programming of the LUT tables.
Hmm. Is this backed up by any statement in the HW User's manual? This comes in contrast with the original Renesas CMM implementation [**] which does make use of suspend (where the freeze actually happens).
Can we infer, based on your statement, that we could also get rid of the suspend callback in [**]?
While the CMM state will be retained across suspend-to-idle, I'm quite sure it will be lost by suspend-to-RAM, at least on the Salvator-X(S), ULCB, and Ebisu development boards, as PSCI will ask the BD9571WMV regulator to power down the R-Car SoC.
So IMHO we do need suspend/resume handling.
Gr{oetje,eeting}s,
Geert
Added Hien-san, Michael K. from Renesas.
-----Original Message----- From: Geert Uytterhoeven geert@linux-m68k.org Sent: Mittwoch, 27. Mai 2020 09:35 To: REE erosca@DE.ADIT-JV.COM erosca@DE.ADIT-JV.COM Cc: Jacopo Mondi jacopo+renesas@jmondi.org; Laurent Pinchart laurent.pinchart@ideasonboard.com; Kieran Bingham kieran.bingham+renesas@ideasonboard.com; Simon Horman horms@verge.net.au; Ulrich Hecht uli+renesas@fpond.eu; VenkataRajesh.Kalakodima@in.bosch.com; David Airlie airlied@linux.ie; Daniel Vetter daniel@ffwll.ch; KOJI MATSUOKA koji.matsuoka.xm@renesas.com; muroya@ksk.co.jp; Harsha.ManjulaMallikarjun@in.bosch.com; Ezequiel Garcia ezequiel@collabora.com; Sean Paul seanpaul@chromium.org; Linux-Renesas linux-renesas-soc@vger.kernel.org; DRI Development dri-devel@lists.freedesktop.org; Linux Kernel Mailing List linux-kernel@vger.kernel.org; Michael Dege michael.dege@renesas.com; Gotthard Voellmeke gotthard.voellmeke@renesas.com; REE efriedrich@DE.ADIT-JV.COM efriedrich@DE.ADIT-JV.COM; Michael Rodin mrodin@de.adit-jv.com; ChaitanyaKumar.Borah@in.bosch.com; Eugeniu Rosca roscaeugeniu@gmail.com Subject: Re: [PATCH v5 0/8] drm: rcar-du: Add Color Management Module (CMM)
Hi Eugeniu,
On Wed, May 27, 2020 at 9:16 AM Eugeniu Rosca erosca@de.adit-jv.com wrote:
On Tue, Oct 15, 2019 at 12:46:13PM +0200, Jacopo Mondi wrote:
CMM functionalities are retained between suspend/resume cycles (tested with suspend-to-idle) without requiring a re-programming of the LUT tables.
Hmm. Is this backed up by any statement in the HW User's manual? This comes in contrast with the original Renesas CMM implementation [**] which does make use of suspend (where the freeze actually happens).
Can we infer, based on your statement, that we could also get rid of the suspend callback in [**]?
While the CMM state will be retained across suspend-to-idle, I'm quite sure it will be lost by suspend-to-RAM, at least on the Salvator-X(S), ULCB, and Ebisu development boards, as PSCI will ask the BD9571WMV regulator to power down the R-Car SoC.
So IMHO we do need suspend/resume handling.
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
Hi Geert,
On Wed, May 27, 2020 at 09:34:30AM +0200, Geert Uytterhoeven wrote:
On Wed, May 27, 2020 at 9:16 AM Eugeniu Rosca erosca@de.adit-jv.com wrote:
On Tue, Oct 15, 2019 at 12:46:13PM +0200, Jacopo Mondi wrote:
CMM functionalities are retained between suspend/resume cycles (tested with suspend-to-idle) without requiring a re-programming of the LUT tables.
Hmm. Is this backed up by any statement in the HW User's manual? This comes in contrast with the original Renesas CMM implementation [**] which does make use of suspend (where the freeze actually happens).
Can we infer, based on your statement, that we could also get rid of the suspend callback in [**]?
While the CMM state will be retained across suspend-to-idle, I'm quite sure it will be lost by suspend-to-RAM, at least on the Salvator-X(S), ULCB, and Ebisu development boards, as PSCI will ask the BD9571WMV regulator to power down the R-Car SoC.
So IMHO we do need suspend/resume handling.
That makes sense. I should be more careful about suspend-to-idle vs suspend-to-ram and not alias the two.
Hello Eugeniu, sorry for the late reply
On Wed, May 27, 2020 at 09:15:55AM +0200, Eugeniu Rosca wrote:
Hi Jacopo,
On Tue, Oct 15, 2019 at 12:46:13PM +0200, Jacopo Mondi wrote: ----8<---
- Testing
I have tested by injecting a color inversion LUT table and enabling/disabling it every 50 displayed frames: https://jmondi.org/cgit/kmsxx/log/?h=gamma_lut
Could you kindly share the cross compilation steps for your kmsxx fork?
I usually build it on the target :)
Just out of curiosity, have you ever tried to pull the display's HDMI cable while reading from CM2_LUT_TBL?
Ahem, not really :) Did I get you right, you mean disconnecting the HDMI cable from the board ?
At least with the out-of-tree CMM implementation [*], this sends the R-Car3 reference targets into an unrecoverable freeze, with no lockup reported by the kernel (i.e. looks like an serious HW issue).
CMM functionalities are retained between suspend/resume cycles (tested with suspend-to-idle) without requiring a re-programming of the LUT tables.
Hmm. Is this backed up by any statement in the HW User's manual? This comes in contrast with the original Renesas CMM implementation [**] which does make use of suspend (where the freeze actually happens).
Can we infer, based on your statement, that we could also get rid of the suspend callback in [**]?
As Geert (thanks) explained what I've tested with is suspend-to-idle, which retains the state of the LUT tables (and I assume other not-yet-implemented CMM features, like CLU). I recall the out-of-tree driver has suspend/resume routines but I never really tested that.
Thanks j
[*] https://github.com/renesas-rcar/du_cmm [**] https://github.com/renesas-rcar/du_cmm/blob/c393ed49834bdbc/meta-rcar-gen3/r...
-- Best regards, Eugeniu Rosca
Hi Jacopo,
On Fri, Jun 05, 2020 at 03:29:00PM +0200, Jacopo Mondi wrote:
On Wed, May 27, 2020 at 09:15:55AM +0200, Eugeniu Rosca wrote:
Could you kindly share the cross compilation steps for your kmsxx fork?
I usually build it on the target :)
Interesting approach. With ARM getting more and more potent, why not? :)
Just out of curiosity, have you ever tried to pull the display's HDMI cable while reading from CM2_LUT_TBL?
Ahem, not really :) Did I get you right, you mean disconnecting the HDMI cable from the board ?
Right.
At least with the out-of-tree CMM implementation [*], this sends the R-Car3 reference targets into an unrecoverable freeze, with no lockup reported by the kernel (i.e. looks like an serious HW issue).
CMM functionalities are retained between suspend/resume cycles (tested with suspend-to-idle) without requiring a re-programming of the LUT tables.
Hmm. Is this backed up by any statement in the HW User's manual? This comes in contrast with the original Renesas CMM implementation [**] which does make use of suspend (where the freeze actually happens).
Can we infer, based on your statement, that we could also get rid of the suspend callback in [**]?
As Geert (thanks) explained what I've tested with is suspend-to-idle, which retains the state of the LUT tables (and I assume other not-yet-implemented CMM features, like CLU). I recall the out-of-tree driver has suspend/resume routines but I never really tested that.
I see. JFYI, there is a flaw in the suspend handling in the out-of-tree CMM patch [*], which renders the SoC unresponsive on HDMI hotplug. The fix is currently under review. Hopefully it will make its way to [*] in the nearest future. Just to keep in mind for the moment when CMM s2ram will become a mainline feature.
[*] https://github.com/renesas-rcar/du_cmm [**] https://github.com/renesas-rcar/du_cmm/blob/c393ed49834bdbc/meta-rcar-gen3/r...
Hi Eugeniu
On Fri, Jun 05, 2020 at 03:41:24PM +0200, Eugeniu Rosca wrote:
Hi Jacopo,
On Fri, Jun 05, 2020 at 03:29:00PM +0200, Jacopo Mondi wrote:
On Wed, May 27, 2020 at 09:15:55AM +0200, Eugeniu Rosca wrote:
Could you kindly share the cross compilation steps for your kmsxx fork?
I usually build it on the target :)
Interesting approach. With ARM getting more and more potent, why not? :)
For 'small' utilities like kmsxx it's doable
Just out of curiosity, have you ever tried to pull the display's HDMI cable while reading from CM2_LUT_TBL?
Ahem, not really :) Did I get you right, you mean disconnecting the HDMI cable from the board ?
Right.
So, no, I have not tried. Do you see any intersting failure with the mainline version ?
At least with the out-of-tree CMM implementation [*], this sends the R-Car3 reference targets into an unrecoverable freeze, with no lockup reported by the kernel (i.e. looks like an serious HW issue).
CMM functionalities are retained between suspend/resume cycles (tested with suspend-to-idle) without requiring a re-programming of the LUT tables.
Hmm. Is this backed up by any statement in the HW User's manual? This comes in contrast with the original Renesas CMM implementation [**] which does make use of suspend (where the freeze actually happens).
Can we infer, based on your statement, that we could also get rid of the suspend callback in [**]?
As Geert (thanks) explained what I've tested with is suspend-to-idle, which retains the state of the LUT tables (and I assume other not-yet-implemented CMM features, like CLU). I recall the out-of-tree driver has suspend/resume routines but I never really tested that.
I see. JFYI, there is a flaw in the suspend handling in the out-of-tree CMM patch [*], which renders the SoC unresponsive on HDMI hotplug. The fix is currently under review. Hopefully it will make its way to [*] in the nearest future. Just to keep in mind for the moment when CMM s2ram will become a mainline feature.
Thanks, let's keep this in mind. Next week I'll run a few tests again with s2ram and will get back to you.
Thanks j
[*] https://github.com/renesas-rcar/du_cmm [**] https://github.com/renesas-rcar/du_cmm/blob/c393ed49834bdbc/meta-rcar-gen3/r...
-- Best regards, Eugeniu Rosca
Hello,
On Fri, Jun 05, 2020 at 03:53:15PM +0200, Jacopo Mondi wrote:
On Fri, Jun 05, 2020 at 03:41:24PM +0200, Eugeniu Rosca wrote:
On Fri, Jun 05, 2020 at 03:29:00PM +0200, Jacopo Mondi wrote:
On Wed, May 27, 2020 at 09:15:55AM +0200, Eugeniu Rosca wrote:
Could you kindly share the cross compilation steps for your kmsxx fork?
I usually build it on the target :)
Interesting approach. With ARM getting more and more potent, why not? :)
For 'small' utilities like kmsxx it's doable
Just out of curiosity, have you ever tried to pull the display's HDMI cable while reading from CM2_LUT_TBL?
Ahem, not really :) Did I get you right, you mean disconnecting the HDMI cable from the board ?
Right.
So, no, I have not tried. Do you see any intersting failure with the mainline version ?
Jacopo, would you be able to give this a try ?
At least with the out-of-tree CMM implementation [*], this sends the R-Car3 reference targets into an unrecoverable freeze, with no lockup reported by the kernel (i.e. looks like an serious HW issue).
CMM functionalities are retained between suspend/resume cycles (tested with suspend-to-idle) without requiring a re-programming of the LUT tables.
Hmm. Is this backed up by any statement in the HW User's manual? This comes in contrast with the original Renesas CMM implementation [**] which does make use of suspend (where the freeze actually happens).
Can we infer, based on your statement, that we could also get rid of the suspend callback in [**]?
As Geert (thanks) explained what I've tested with is suspend-to-idle, which retains the state of the LUT tables (and I assume other not-yet-implemented CMM features, like CLU). I recall the out-of-tree driver has suspend/resume routines but I never really tested that.
I see. JFYI, there is a flaw in the suspend handling in the out-of-tree CMM patch [*], which renders the SoC unresponsive on HDMI hotplug. The fix is currently under review. Hopefully it will make its way to [*] in the nearest future. Just to keep in mind for the moment when CMM s2ram will become a mainline feature.
Thanks, let's keep this in mind. Next week I'll run a few tests again with s2ram and will get back to you.
Note that the CMM driver is controlled by the DU driver. As the DU driver will reenable the display during resume, it will call rcar_du_cmm_setup() at resume time, which will reprogram the CMM. There should thus be no need for manual suspend/resume handling in the CMM as far as I can tell, but we need to ensure that the CMM is suspended before and resumed after the DU. I believe this could be implemented using device links.
[*] https://github.com/renesas-rcar/du_cmm [**] https://github.com/renesas-rcar/du_cmm/blob/c393ed49834bdbc/meta-rcar-gen3/r...
Hello,
Many thanks for your comments and involvement.
On Sun, Jun 07, 2020 at 05:41:58AM +0300, Laurent Pinchart wrote:
On Fri, Jun 05, 2020 at 03:53:15PM +0200, Jacopo Mondi wrote:
On Fri, Jun 05, 2020 at 03:41:24PM +0200, Eugeniu Rosca wrote:
On Fri, Jun 05, 2020 at 03:29:00PM +0200, Jacopo Mondi wrote:
On Wed, May 27, 2020 at 09:15:55AM +0200, Eugeniu Rosca wrote:
Could you kindly share the cross compilation steps for your kmsxx fork?
I usually build it on the target :)
Interesting approach. With ARM getting more and more potent, why not? :)
For 'small' utilities like kmsxx it's doable
Just out of curiosity, have you ever tried to pull the display's HDMI cable while reading from CM2_LUT_TBL?
Ahem, not really :) Did I get you right, you mean disconnecting the HDMI cable from the board ?
Right.
So, no, I have not tried. Do you see any intersting failure with the mainline version ?
Jacopo, would you be able to give this a try ?
FWIW, I seem to hit pre-existing issues in vanilla rcar-du, while unplugging HDMI cable during a cyclic suspend-resume:
HW: H3 ES2.0 Salvator-X SW: renesas-drivers-2020-06-02-v5.7 .config: renesas_defconfig +CONFIG_PM_DEBUG +CONFIG_PM_ADVANCED_DEBUG Use-case:
--------8<--------- $ cat s2ram.sh modprobe i2c-dev echo 9 > /proc/sys/kernel/printk i2cset -f -y 7 0x30 0x20 0x0F echo 0 > /sys/module/suspend/parameters/pm_test_delay echo core > /sys/power/pm_test echo deep > /sys/power/mem_sleep echo 1 > /sys/power/pm_debug_messages echo 0 > /sys/power/pm_print_times echo mem > /sys/power/state
$ while true; do sh s2ram.sh ; done $ # unplug HDMI cable several times
[ 55.568051] PM: noirq resume of devices complete after 3.862 msecs [ 55.583253] PM: early resume of devices complete after 8.496 msecs [ 65.757023] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:74:crtc-1] flip_done timed out [ 75.996123] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:74:crtc-1] flip_done timed out [ 86.236112] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CRTC:74:crtc-1] flip_done timed out [ 96.476111] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CONNECTOR:80:HDMI-A-1] flip_done timed out [ 106.716109] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [PLANE:45:plane-5] flip_done timed out [ 116.956111] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:74:crtc-1] flip_done timed out [ 127.196112] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CRTC:74:crtc-1] flip_done timed out [ 137.436116] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CONNECTOR:80:HDMI-A-1] flip_done timed out [ 147.676111] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [PLANE:45:plane-5] flip_done timed out [ 157.916110] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:74:crtc-1] flip_done timed out --------8<---------
This looks to be unrelated to the CMM lockup I initially reported.
JYI, graphics pipelines in production R-Car3 targets are significantly more complex (involving binding/unbinding serializer ICs at runtime during non-trivial shutdown/suspend/resume sequences), as opposed to the relatively straightforward VGA/HDMI outputs present on the reference targets. So, my hope is that Renesas community can take HDMI hot plugging seriously and make it a regular test-case during rcar-du patch review, since this is a precondition for the R-Car3 platform and products to succeed as a whole.
BTW, if you happen to know an affordable programmable HDMI switcher which can do the hot-plugging job in an automated test environment, please let me know.
At least with the out-of-tree CMM implementation [*], this sends the R-Car3 reference targets into an unrecoverable freeze, with no lockup reported by the kernel (i.e. looks like an serious HW issue).
CMM functionalities are retained between suspend/resume cycles (tested with suspend-to-idle) without requiring a re-programming of the LUT tables.
Hmm. Is this backed up by any statement in the HW User's manual? This comes in contrast with the original Renesas CMM implementation [**] which does make use of suspend (where the freeze actually happens).
Can we infer, based on your statement, that we could also get rid of the suspend callback in [**]?
As Geert (thanks) explained what I've tested with is suspend-to-idle, which retains the state of the LUT tables (and I assume other not-yet-implemented CMM features, like CLU). I recall the out-of-tree driver has suspend/resume routines but I never really tested that.
I see. JFYI, there is a flaw in the suspend handling in the out-of-tree CMM patch [*], which renders the SoC unresponsive on HDMI hotplug. The fix is currently under review. Hopefully it will make its way to [*] in the nearest future. Just to keep in mind for the moment when CMM s2ram will become a mainline feature.
Thanks, let's keep this in mind. Next week I'll run a few tests again with s2ram and will get back to you.
Note that the CMM driver is controlled by the DU driver. As the DU driver will reenable the display during resume, it will call rcar_du_cmm_setup() at resume time, which will reprogram the CMM. There should thus be no need for manual suspend/resume handling in the CMM as far as I can tell, but we need to ensure that the CMM is suspended before and resumed after the DU. I believe this could be implemented using device links.
Does this apply to vanilla rcar-du only (where CMM support differs from [*]) or would also be relevant for rcar.9.6 kernel?
[*] https://github.com/renesas-rcar/du_cmm [**] https://github.com/renesas-rcar/du_cmm/blob/c393ed49834bdbc/meta-rcar-gen3/r...
Hi Eugeniu
On Mon, Jun 08, 2020 at 11:44:32AM +0200, Eugeniu Rosca wrote:
Hello,
Many thanks for your comments and involvement.
On Sun, Jun 07, 2020 at 05:41:58AM +0300, Laurent Pinchart wrote:
On Fri, Jun 05, 2020 at 03:53:15PM +0200, Jacopo Mondi wrote:
On Fri, Jun 05, 2020 at 03:41:24PM +0200, Eugeniu Rosca wrote:
On Fri, Jun 05, 2020 at 03:29:00PM +0200, Jacopo Mondi wrote:
On Wed, May 27, 2020 at 09:15:55AM +0200, Eugeniu Rosca wrote:
Could you kindly share the cross compilation steps for your kmsxx fork?
I usually build it on the target :)
Interesting approach. With ARM getting more and more potent, why not? :)
For 'small' utilities like kmsxx it's doable
Just out of curiosity, have you ever tried to pull the display's HDMI cable while reading from CM2_LUT_TBL?
Ahem, not really :) Did I get you right, you mean disconnecting the HDMI cable from the board ?
Right.
So, no, I have not tried. Do you see any intersting failure with the mainline version ?
Jacopo, would you be able to give this a try ?
FWIW, I seem to hit pre-existing issues in vanilla rcar-du, while unplugging HDMI cable during a cyclic suspend-resume:
HW: H3 ES2.0 Salvator-X SW: renesas-drivers-2020-06-02-v5.7 .config: renesas_defconfig +CONFIG_PM_DEBUG +CONFIG_PM_ADVANCED_DEBUG Use-case:
--------8<--------- $ cat s2ram.sh modprobe i2c-dev echo 9 > /proc/sys/kernel/printk i2cset -f -y 7 0x30 0x20 0x0F
According to https://elinux.org/R-Car/Boards/Salvator-X#Suspend-to-RAM this is not needed anymore
echo 0 > /sys/module/suspend/parameters/pm_test_delay echo core > /sys/power/pm_test echo deep > /sys/power/mem_sleep echo 1 > /sys/power/pm_debug_messages echo 0 > /sys/power/pm_print_times echo mem > /sys/power/state
$ while true; do sh s2ram.sh ; done $ # unplug HDMI cable several times
I tried unplugging an plugging the cable while the system was suspended and after resume but I was not able to reproduce anything.
Could you provide more precise instructions on how to reproduce this ? I.e. when to disconnect the cable to trigger the below error.
Thanks j
[ 55.568051] PM: noirq resume of devices complete after 3.862 msecs [ 55.583253] PM: early resume of devices complete after 8.496 msecs [ 65.757023] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:74:crtc-1] flip_done timed out [ 75.996123] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:74:crtc-1] flip_done timed out [ 86.236112] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CRTC:74:crtc-1] flip_done timed out [ 96.476111] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CONNECTOR:80:HDMI-A-1] flip_done timed out [ 106.716109] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [PLANE:45:plane-5] flip_done timed out [ 116.956111] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:74:crtc-1] flip_done timed out [ 127.196112] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CRTC:74:crtc-1] flip_done timed out [ 137.436116] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CONNECTOR:80:HDMI-A-1] flip_done timed out [ 147.676111] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [PLANE:45:plane-5] flip_done timed out [ 157.916110] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:74:crtc-1] flip_done timed out --------8<---------
This looks to be unrelated to the CMM lockup I initially reported.
JYI, graphics pipelines in production R-Car3 targets are significantly more complex (involving binding/unbinding serializer ICs at runtime during non-trivial shutdown/suspend/resume sequences), as opposed to the relatively straightforward VGA/HDMI outputs present on the reference targets. So, my hope is that Renesas community can take HDMI hot plugging seriously and make it a regular test-case during rcar-du patch review, since this is a precondition for the R-Car3 platform and products to succeed as a whole.
BTW, if you happen to know an affordable programmable HDMI switcher which can do the hot-plugging job in an automated test environment, please let me know.
At least with the out-of-tree CMM implementation [*], this sends the R-Car3 reference targets into an unrecoverable freeze, with no lockup reported by the kernel (i.e. looks like an serious HW issue).
> CMM functionalities are retained between suspend/resume cycles (tested with > suspend-to-idle) without requiring a re-programming of the LUT tables.
Hmm. Is this backed up by any statement in the HW User's manual? This comes in contrast with the original Renesas CMM implementation [**] which does make use of suspend (where the freeze actually happens).
Can we infer, based on your statement, that we could also get rid of the suspend callback in [**]?
As Geert (thanks) explained what I've tested with is suspend-to-idle, which retains the state of the LUT tables (and I assume other not-yet-implemented CMM features, like CLU). I recall the out-of-tree driver has suspend/resume routines but I never really tested that.
I see. JFYI, there is a flaw in the suspend handling in the out-of-tree CMM patch [*], which renders the SoC unresponsive on HDMI hotplug. The fix is currently under review. Hopefully it will make its way to [*] in the nearest future. Just to keep in mind for the moment when CMM s2ram will become a mainline feature.
Thanks, let's keep this in mind. Next week I'll run a few tests again with s2ram and will get back to you.
Note that the CMM driver is controlled by the DU driver. As the DU driver will reenable the display during resume, it will call rcar_du_cmm_setup() at resume time, which will reprogram the CMM. There should thus be no need for manual suspend/resume handling in the CMM as far as I can tell, but we need to ensure that the CMM is suspended before and resumed after the DU. I believe this could be implemented using device links.
Does this apply to vanilla rcar-du only (where CMM support differs from [*]) or would also be relevant for rcar.9.6 kernel?
[*] https://github.com/renesas-rcar/du_cmm [**] https://github.com/renesas-rcar/du_cmm/blob/c393ed49834bdbc/meta-rcar-gen3/r...
-- Best regards, Eugeniu Rosca
Hi Jacopo,
On Fri, Jun 12, 2020 at 05:12:09PM +0200, Jacopo Mondi wrote:
On Mon, Jun 08, 2020 at 11:44:32AM +0200, Eugeniu Rosca wrote:
FWIW, I seem to hit pre-existing issues in vanilla rcar-du, while unplugging HDMI cable during a cyclic suspend-resume:
HW: H3 ES2.0 Salvator-X SW: renesas-drivers-2020-06-02-v5.7 .config: renesas_defconfig +CONFIG_PM_DEBUG +CONFIG_PM_ADVANCED_DEBUG Use-case:
--------8<--------- $ cat s2ram.sh modprobe i2c-dev echo 9 > /proc/sys/kernel/printk i2cset -f -y 7 0x30 0x20 0x0F
According to https://elinux.org/R-Car/Boards/Salvator-X#Suspend-to-RAM this is not needed anymore
Good to know. Thanks for the useful remark.
echo 0 > /sys/module/suspend/parameters/pm_test_delay echo core > /sys/power/pm_test echo deep > /sys/power/mem_sleep echo 1 > /sys/power/pm_debug_messages echo 0 > /sys/power/pm_print_times echo mem > /sys/power/state
$ while true; do sh s2ram.sh ; done $ # unplug HDMI cable several times
I tried unplugging an plugging the cable while the system was suspended and after resume but I was not able to reproduce anything.
Your comment sounds like you suspended the system once and resumed it afterwards, while my description mentions "cyclic" :), meaning:
$ while true; do sh s2ram.sh; done $ # connect-disconnect the hdmi display a couple of times $ # NOTE: to avoid this manual step, I am thinking of a USB-controlled HDMI switcher long-term
Could you provide more precise instructions on how to reproduce this ? I.e. when to disconnect the cable to trigger the below error.
See above :)
BTW, using renesas-drivers-2020-06-02-v5.7 as base and performing the use-case just described, I got today (with minimal effort):
[ 459.321733] Enabling non-boot CPUs ... [ 459.331132] Detected PIPT I-cache on CPU1 [ 459.331189] CPU1: Booted secondary processor 0x0000000001 [0x411fd073] [ 459.332312] CPU1 is up [ 459.345635] Detected PIPT I-cache on CPU2 [ 459.345671] CPU2: Booted secondary processor 0x0000000002 [0x411fd073] [ 459.346624] CPU2 is up [ 459.359912] Detected PIPT I-cache on CPU3 [ 459.359942] CPU3: Booted secondary processor 0x0000000003 [0x411fd073] [ 459.360918] CPU3 is up [ 459.374183] Detected VIPT I-cache on CPU4 [ 459.374252] CPU4: Booted secondary processor 0x0000000100 [0x410fd034] [ 459.375875] cpufreq: cpufreq_online: CPU4: Running at unlisted freq: 1199999 KHz [ 459.394204] cpufreq: cpufreq_online: CPU4: Unlisted initial frequency changed to: 1200000 KHz [ 459.403879] CPU4 is up [ 459.406469] Detected VIPT I-cache on CPU5 [ 459.406519] CPU5: Booted secondary processor 0x0000000101 [0x410fd034] [ 459.408520] CPU5 is up [ 459.421762] Detected VIPT I-cache on CPU6 [ 459.421810] CPU6: Booted secondary processor 0x0000000102 [0x410fd034] [ 459.423831] CPU6 is up [ 459.437114] Detected VIPT I-cache on CPU7 [ 459.437164] CPU7: Booted secondary processor 0x0000000103 [0x410fd034] [ 459.439258] CPU7 is up [ 459.456217] PM: noirq resume of devices complete after 3.878 msecs [ 459.471529] PM: early resume of devices complete after 8.590 msecs [ 469.726906] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CRTC:76:crtc-3] flip_done timed out [ 479.966889] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CONNECTOR:78:VGA-1] flip_done timed out [ 490.206887] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [PLANE:34:plane-1] flip_done timed out [ 490.282873] rcar-du feb00000.display: page flip timeout [ 490.288431] ------------[ cut here ]------------ [ 490.293092] WARNING: CPU: 0 PID: 123 at drivers/gpu/drm/rcar-du/rcar_du_crtc.c:787 rcar_du_crtc_atomic_begin+0x64/0x70 [ 490.303854] Modules linked in: [ 490.306933] CPU: 0 PID: 123 Comm: irq/192-fead000 Not tainted 5.7.0-arm64-renesas-00012-gbc5411732961 #131 [ 490.316647] Hardware name: Renesas Salvator-X board based on r8a77951 (DT) [ 490.323568] pstate: 20000005 (nzCv daif -PAN -UAO BTYPE=--) [ 490.329176] pc : rcar_du_crtc_atomic_begin+0x64/0x70 [ 490.334174] lr : drm_atomic_helper_commit_planes+0x70/0x220 [ 490.339781] sp : ffff80001258b960 [ 490.343115] x29: ffff80001258b960 x28: 0000000000000038 [ 490.348463] x27: ffff0006f676a800 x26: ffff8000111c3718 [ 490.353811] x25: ffff0006f5e574c0 x24: ffff0006f6728000 [ 490.359157] x23: 0000000000000038 x22: 0000000000000000 [ 490.364503] x21: 0000000000000001 x20: 0000000000000002 [ 490.369849] x19: ffff0006f5e58e08 x18: ffffffffffffffff [ 490.375195] x17: 0000000000000000 x16: 0000000000000000 [ 490.380541] x15: ffff8000111c09c8 x14: 0000000000000000 [ 490.385887] x13: 0000000000000000 x12: 0000000000000000 [ 490.391233] x11: 0000000000000000 x10: 0000000000000000 [ 490.396579] x9 : 0000000000000000 x8 : 0000000000000000 [ 490.401924] x7 : 0000000000000000 x6 : ffff0006f5e5aab8 [ 490.407270] x5 : 0000000000003a8b x4 : ffff0006f3154438 [ 490.412616] x3 : ffff0006f6728000 x2 : ffff800010649338 [ 490.417962] x1 : ffff0006f228dc00 x0 : 0000000000000000 [ 490.423308] Call trace: [ 490.425771] rcar_du_crtc_atomic_begin+0x64/0x70 [ 490.430418] drm_atomic_helper_commit_planes+0x70/0x220 [ 490.435678] rcar_du_atomic_commit_tail+0xa0/0xd8 [ 490.440412] commit_tail+0x9c/0x160 [ 490.443924] drm_atomic_helper_commit+0x178/0x190 [ 490.448661] drm_atomic_commit+0x48/0x58 [ 490.452611] drm_client_modeset_commit_atomic+0x18c/0x280 [ 490.458045] drm_client_modeset_commit_locked+0x50/0x1e0 [ 490.463390] drm_client_modeset_commit+0x2c/0x50 [ 490.468040] drm_fb_helper_restore_fbdev_mode_unlocked+0x74/0xe8 [ 490.474085] drm_fb_helper_set_par+0x2c/0x60 [ 490.478383] drm_fb_helper_hotplug_event.part.20+0xc0/0xd0 [ 490.483904] drm_fbdev_client_hotplug+0xc8/0x1d0 [ 490.488551] drm_client_dev_hotplug+0x78/0xc0 [ 490.492939] drm_kms_helper_hotplug_event+0x30/0x40 [ 490.497848] drm_helper_hpd_irq_event+0x12c/0x160 [ 490.502583] dw_hdmi_irq+0x118/0x1e0 [ 490.506185] irq_thread_fn+0x28/0x88 [ 490.509783] irq_thread+0x13c/0x1d8 [ 490.513298] kthread+0x150/0x158 [ 490.516549] ret_from_fork+0x10/0x18 [ 490.520147] irq event stamp: 12676 [ 490.523576] hardirqs last enabled at (12675): [<ffff800010a725cc>] _raw_spin_unlock_irqrestore+0x7c/0x90 [ 490.533206] hardirqs last disabled at (12676): [<ffff800010024918>] do_debug_exception+0x1a0/0x234 [ 490.542223] softirqs last enabled at (12634): [<ffff800010001598>] efi_header_end+0x598/0x5ac [ 490.550892] softirqs last disabled at (12625): [<ffff80001007cf9c>] irq_exit+0x13c/0x148 [ 490.559032] ---[ end trace 5cdf3cb912c7ed2d ]--- [ 490.563942] vsp1 fea28000.vsp: vsp1_du_pipeline_setup_inputs: failed to setup BRU source [ 490.572213] vsp1 fea28000.vsp: vsp1_du_pipeline_setup_inputs: failed to setup BRU source [ 490.580410] ------------[ cut here ]------------ [ 490.585087] refcount_t: addition on 0; use-after-free. [ 490.590316] WARNING: CPU: 0 PID: 123 at lib/refcount.c:25 refcount_warn_saturate+0x120/0x140 [ 490.598807] Modules linked in: [ 490.601883] CPU: 0 PID: 123 Comm: irq/192-fead000 Tainted: G W 5.7.0-arm64-renesas-00012-gbc5411732961 #131 [ 490.612994] Hardware name: Renesas Salvator-X board based on r8a77951 (DT) [ 490.619912] pstate: 40000005 (nZcv daif -PAN -UAO BTYPE=--) [ 490.625521] pc : refcount_warn_saturate+0x120/0x140 [ 490.630430] lr : refcount_warn_saturate+0x120/0x140 [ 490.635337] sp : ffff80001258b960 [ 490.638671] x29: ffff80001258b960 x28: 0000000000000038 [ 490.644018] x27: ffff0006f676a800 x26: ffff8000111c3718 [ 490.649364] x25: ffff0006f228c200 x24: ffff0006f228c400 [ 490.654711] x23: 0000000000000038 x22: 0000000000000001 [ 490.660057] x21: ffff0006f3154000 x20: 0000000000000000 [ 490.665403] x19: ffff0006f228ec00 x18: ffffffffffffffff [ 490.670748] x17: 0000000000000000 x16: 000000007a11fc18 [ 490.676095] x15: ffff8000111c09c8 x14: ffff80009258b677 [ 490.681441] x13: ffff80001258b685 x12: ffff8000111e2000 [ 490.686786] x11: 0000000005f5e0ff x10: 00000000000000b8 [ 490.692132] x9 : 0000000000000000 x8 : 0000000000000000 [ 490.697478] x7 : 0000000000000001 x6 : 0000000000000000 [ 490.702824] x5 : 0000000000000000 x4 : 0000000000000000 [ 490.708170] x3 : 0000000000000000 x2 : ffff0006fa12def8 [ 490.713516] x1 : f874420ef852bd00 x0 : 0000000000000000 [ 490.718862] Call trace: [ 490.721325] refcount_warn_saturate+0x120/0x140 [ 490.725884] drm_atomic_helper_commit_hw_done+0x140/0x150 [ 490.731318] rcar_du_atomic_commit_tail+0xb4/0xd8 [ 490.736051] commit_tail+0x9c/0x160 [ 490.739561] drm_atomic_helper_commit+0x178/0x190 [ 490.744296] drm_atomic_commit+0x48/0x58 [ 490.748244] drm_client_modeset_commit_atomic+0x18c/0x280 [ 490.753678] drm_client_modeset_commit_locked+0x50/0x1e0 [ 490.759023] drm_client_modeset_commit+0x2c/0x50 [ 490.763671] drm_fb_helper_restore_fbdev_mode_unlocked+0x74/0xe8 [ 490.769717] drm_fb_helper_set_par+0x2c/0x60 [ 490.774014] drm_fb_helper_hotplug_event.part.20+0xc0/0xd0 [ 490.779535] drm_fbdev_client_hotplug+0xc8/0x1d0 [ 490.784182] drm_client_dev_hotplug+0x78/0xc0 [ 490.788567] drm_kms_helper_hotplug_event+0x30/0x40 [ 490.793476] drm_helper_hpd_irq_event+0x12c/0x160 [ 490.798210] dw_hdmi_irq+0x118/0x1e0 [ 490.801809] irq_thread_fn+0x28/0x88 [ 490.805408] irq_thread+0x13c/0x1d8 [ 490.808919] kthread+0x150/0x158 [ 490.812167] ret_from_fork+0x10/0x18 [ 490.815764] irq event stamp: 12796 [ 490.819189] hardirqs last enabled at (12795): [<ffff8000100eecf0>] console_unlock+0x418/0x638 [ 490.827855] hardirqs last disabled at (12796): [<ffff800010024918>] do_debug_exception+0x1a0/0x234 [ 490.836871] softirqs last enabled at (12792): [<ffff800010001598>] efi_header_end+0x598/0x5ac [ 490.845538] softirqs last disabled at (12785): [<ffff80001007cf9c>] irq_exit+0x13c/0x148 [ 490.853678] ---[ end trace 5cdf3cb912c7ed2e ]--- [ 490.858414] ------------[ cut here ]------------ [ 490.863065] WARNING: CPU: 0 PID: 123 at drivers/gpu/drm/drm_atomic_helper.c:2312 drm_atomic_helper_commit_hw_done+0x130/0x150 [ 490.874438] Modules linked in: [ 490.877513] CPU: 0 PID: 123 Comm: irq/192-fead000 Tainted: G W 5.7.0-arm64-renesas-00012-gbc5411732961 #131 [ 490.888624] Hardware name: Renesas Salvator-X board based on r8a77951 (DT) [ 490.895542] pstate: 00000005 (nzcv daif -PAN -UAO BTYPE=--) [ 490.901150] pc : drm_atomic_helper_commit_hw_done+0x130/0x150 [ 490.906932] lr : drm_atomic_helper_commit_hw_done+0x118/0x150 [ 490.912713] sp : ffff80001258b970 [ 490.916047] x29: ffff80001258b970 x28: 0000000000000038 [ 490.921393] x27: ffff0006f676a800 x26: ffff8000111c3718 [ 490.926739] x25: ffff0006f228dc00 x24: ffff0006f228f000 [ 490.932085] x23: 0000000000000038 x22: 0000000000000001 [ 490.937431] x21: ffff0006f3154000 x20: 0000000000000001 [ 490.942777] x19: ffff0006f228cc00 x18: ffffffffffffffff [ 490.948122] x17: 0000000000000000 x16: 000000007a11fc18 [ 490.953468] x15: ffff8000111c09c8 x14: ffff80009258b677 [ 490.958814] x13: ffff80001258b685 x12: ffff8000111e2000 [ 490.964160] x11: 0000000005f5e0ff x10: 00000000000000b8 [ 490.969506] x9 : 0000000000000080 x8 : 0000000000000000 [ 490.974851] x7 : 0000000000000001 x6 : 0000000000000000 [ 490.980196] x5 : 0000000000000000 x4 : 0000000000006d40 [ 490.985542] x3 : 0000000000000000 x2 : 0000000000000002 [ 490.990887] x1 : 0000000000000001 x0 : ffff0006f5a76580 [ 490.996232] Call trace: [ 490.998694] drm_atomic_helper_commit_hw_done+0x130/0x150 [ 491.004127] rcar_du_atomic_commit_tail+0xb4/0xd8 [ 491.008861] commit_tail+0x9c/0x160 [ 491.012371] drm_atomic_helper_commit+0x178/0x190 [ 491.017105] drm_atomic_commit+0x48/0x58 [ 491.021053] drm_client_modeset_commit_atomic+0x18c/0x280 [ 491.026486] drm_client_modeset_commit_locked+0x50/0x1e0 [ 491.031832] drm_client_modeset_commit+0x2c/0x50 [ 491.036480] drm_fb_helper_restore_fbdev_mode_unlocked+0x74/0xe8 [ 491.042524] drm_fb_helper_set_par+0x2c/0x60 [ 491.046822] drm_fb_helper_hotplug_event.part.20+0xc0/0xd0 [ 491.052343] drm_fbdev_client_hotplug+0xc8/0x1d0 [ 491.056989] drm_client_dev_hotplug+0x78/0xc0 [ 491.061374] drm_kms_helper_hotplug_event+0x30/0x40 [ 491.066283] drm_helper_hpd_irq_event+0x12c/0x160 [ 491.071017] dw_hdmi_irq+0x118/0x1e0 [ 491.074615] irq_thread_fn+0x28/0x88 [ 491.078214] irq_thread+0x13c/0x1d8 [ 491.081724] kthread+0x150/0x158 [ 491.084973] ret_from_fork+0x10/0x18 [ 491.088569] irq event stamp: 12824 [ 491.091996] hardirqs last enabled at (12823): [<ffff800010246bd0>] kfree+0x308/0x430 [ 491.099877] hardirqs last disabled at (12824): [<ffff800010024918>] do_debug_exception+0x1a0/0x234 [ 491.108892] softirqs last enabled at (12818): [<ffff800010001598>] efi_header_end+0x598/0x5ac [ 491.117558] softirqs last disabled at (12799): [<ffff80001007cf9c>] irq_exit+0x13c/0x148 [ 491.125699] ---[ end trace 5cdf3cb912c7ed2f ]--- [ 491.130416] ------------[ cut here ]------------ [ 491.135067] WARNING: CPU: 0 PID: 123 at drivers/gpu/drm/drm_atomic_helper.c:2312 drm_atomic_helper_commit_hw_done+0x130/0x150 [ 491.146440] Modules linked in: [ 491.149515] CPU: 0 PID: 123 Comm: irq/192-fead000 Tainted: G W 5.7.0-arm64-renesas-00012-gbc5411732961 #131 [ 491.160625] Hardware name: Renesas Salvator-X board based on r8a77951 (DT) [ 491.167543] pstate: 00000005 (nzcv daif -PAN -UAO BTYPE=--) [ 491.173150] pc : drm_atomic_helper_commit_hw_done+0x130/0x150 [ 491.178932] lr : drm_atomic_helper_commit_hw_done+0xc8/0x150 [ 491.184625] sp : ffff80001258b970 [ 491.187960] x29: ffff80001258b970 x28: 0000000000000038 [ 491.193306] x27: ffff0006f676a800 x26: ffff8000111c3718 [ 491.198651] x25: ffff0006f228d000 x24: ffff0006f228f600 [ 491.203998] x23: 0000000000000038 x22: 0000000000000001 [ 491.209343] x21: ffff0006f3154000 x20: 0000000000000003 [ 491.214689] x19: ffff0006f228fa00 x18: ffffffffffffffff [ 491.220035] x17: 0000000000000000 x16: 000000007a11fc18 [ 491.225381] x15: ffff8000111c09c8 x14: ffff80009258b677 [ 491.230727] x13: ffff80001258b685 x12: ffff8000111e2000 [ 491.236073] x11: 0000000005f5e0ff x10: 00000000000000b8 [ 491.241418] x9 : 0000000000000080 x8 : 0000000000000000 [ 491.246764] x7 : 0000000000000001 x6 : 0000000000000000 [ 491.252109] x5 : 0000000000000001 x4 : 0000000000000000 [ 491.257455] x3 : 0000000000000000 x2 : 0000000000000002 [ 491.262801] x1 : 0000000000000001 x0 : ffff0006f5a76c80 [ 491.268146] Call trace: [ 491.270607] drm_atomic_helper_commit_hw_done+0x130/0x150 [ 491.276040] rcar_du_atomic_commit_tail+0xb4/0xd8 [ 491.280774] commit_tail+0x9c/0x160 [ 491.284284] drm_atomic_helper_commit+0x178/0x190 [ 491.289018] drm_atomic_commit+0x48/0x58 [ 491.292966] drm_client_modeset_commit_atomic+0x18c/0x280 [ 491.298399] drm_client_modeset_commit_locked+0x50/0x1e0 [ 491.303745] drm_client_modeset_commit+0x2c/0x50 [ 491.308392] drm_fb_helper_restore_fbdev_mode_unlocked+0x74/0xe8 [ 491.314437] drm_fb_helper_set_par+0x2c/0x60 [ 491.318734] drm_fb_helper_hotplug_event.part.20+0xc0/0xd0 [ 491.324254] drm_fbdev_client_hotplug+0xc8/0x1d0 [ 491.328901] drm_client_dev_hotplug+0x78/0xc0 [ 491.333286] drm_kms_helper_hotplug_event+0x30/0x40 [ 491.338194] drm_helper_hpd_irq_event+0x12c/0x160 [ 491.342927] dw_hdmi_irq+0x118/0x1e0 [ 491.346526] irq_thread_fn+0x28/0x88 [ 491.350124] irq_thread+0x13c/0x1d8 [ 491.353635] kthread+0x150/0x158 [ 491.356884] ret_from_fork+0x10/0x18 [ 491.360480] irq event stamp: 12850 [ 491.363904] hardirqs last enabled at (12849): [<ffff800010a725cc>] _raw_spin_unlock_irqrestore+0x7c/0x90 [ 491.373532] hardirqs last disabled at (12850): [<ffff800010024918>] do_debug_exception+0x1a0/0x234 [ 491.382548] softirqs last enabled at (12842): [<ffff800010001598>] efi_header_end+0x598/0x5ac [ 491.391213] softirqs last disabled at (12827): [<ffff80001007cf9c>] irq_exit+0x13c/0x148 [ 491.399354] ---[ end trace 5cdf3cb912c7ed30 ]--- [ 491.404400] sched: RT throttling activated [ 501.470891] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:74:crtc-1] flip_done timed out [ 511.710886] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:76:crtc-3] flip_done timed out [ 511.720062] ------------[ cut here ]------------ [ 511.724740] refcount_t: underflow; use-after-free. [ 511.729624] WARNING: CPU: 0 PID: 123 at lib/refcount.c:28 refcount_warn_saturate+0xcc/0x140 [ 511.738027] Modules linked in: [ 511.741102] CPU: 0 PID: 123 Comm: irq/192-fead000 Tainted: G W 5.7.0-arm64-renesas-00012-gbc5411732961 #131 [ 511.752214] Hardware name: Renesas Salvator-X board based on r8a77951 (DT) [ 511.759132] pstate: 40000005 (nZcv daif -PAN -UAO BTYPE=--) [ 511.764740] pc : refcount_warn_saturate+0xcc/0x140 [ 511.769561] lr : refcount_warn_saturate+0xcc/0x140 [ 511.774381] sp : ffff80001258b9a0 [ 511.777715] x29: ffff80001258b9a0 x28: 0000000000000038 [ 511.783062] x27: ffff0006f676a800 x26: ffff8000111c3718 [ 511.788408] x25: ffff0006f5e574c0 x24: ffff0006f6728440 [ 511.793755] x23: 0000000000000038 x22: 0000000000000001 [ 511.799101] x21: 0000000000000000 x20: 0000000000000000 [ 511.804447] x19: ffff0006f228c200 x18: ffffffffffffffff [ 511.809793] x17: 0000000000000000 x16: 000000007a11fc18 [ 511.815139] x15: ffff8000111c09c8 x14: ffff80009258b6b7 [ 511.820485] x13: ffff80001258b6c5 x12: ffff8000111e2000 [ 511.825830] x11: 0000000005f5e0ff x10: 00000000000000b8 [ 511.831176] x9 : 0000000000000000 x8 : 0000000000000000 [ 511.836522] x7 : 0000000000000001 x6 : 0000000000000000 [ 511.841867] x5 : 0000000000000000 x4 : 0000000000000000 [ 511.847212] x3 : 0000000000000000 x2 : ffff0006fa12def8 [ 511.852558] x1 : f874420ef852bd00 x0 : 0000000000000000 [ 511.857904] Call trace: [ 511.860367] refcount_warn_saturate+0xcc/0x140 [ 511.864841] __drm_atomic_helper_crtc_destroy_state+0xdc/0x100 [ 511.870711] rcar_du_crtc_atomic_destroy_state+0x18/0x30 [ 511.876057] drm_atomic_state_default_clear+0x120/0x2f0 [ 511.881316] drm_atomic_state_clear+0x28/0x30 [ 511.885700] __drm_atomic_state_free+0x18/0x60 [ 511.890173] drm_client_modeset_commit_atomic+0x238/0x280 [ 511.895606] drm_client_modeset_commit_locked+0x50/0x1e0 [ 511.900952] drm_client_modeset_commit+0x2c/0x50 [ 511.905599] drm_fb_helper_restore_fbdev_mode_unlocked+0x74/0xe8 [ 511.911643] drm_fb_helper_set_par+0x2c/0x60 [ 511.915941] drm_fb_helper_hotplug_event.part.20+0xc0/0xd0 [ 511.921462] drm_fbdev_client_hotplug+0xc8/0x1d0 [ 511.926109] drm_client_dev_hotplug+0x78/0xc0 [ 511.930494] drm_kms_helper_hotplug_event+0x30/0x40 [ 511.935403] drm_helper_hpd_irq_event+0x12c/0x160 [ 511.940136] dw_hdmi_irq+0x118/0x1e0 [ 511.943735] irq_thread_fn+0x28/0x88 [ 511.947333] irq_thread+0x13c/0x1d8 [ 511.950845] kthread+0x150/0x158 [ 511.954094] ret_from_fork+0x10/0x18 [ 511.957690] irq event stamp: 13016 [ 511.961114] hardirqs last enabled at (13015): [<ffff8000100eecf0>] console_unlock+0x418/0x638 [ 511.969781] hardirqs last disabled at (13016): [<ffff800010024918>] do_debug_exception+0x1a0/0x234 [ 511.978797] softirqs last enabled at (13012): [<ffff800010001598>] efi_header_end+0x598/0x5ac [ 511.987464] softirqs last disabled at (13005): [<ffff80001007cf9c>] irq_exit+0x13c/0x148 [ 511.995604] ---[ end trace 5cdf3cb912c7ed31 ]--- [ 512.000549] vsp1 fea28000.vsp: vsp1_du_pipeline_setup_inputs: failed to setup BRU source [ 522.206891] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:74:crtc-1] flip_done timed out [ 532.446880] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CRTC:74:crtc-1] flip_done timed out [ 542.686878] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CONNECTOR:80:HDMI-A-1] flip_done timed out [ 552.926877] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [PLANE:45:plane-5] flip_done timed out [ 552.926932] vsp1 fea28000.vsp: vsp1_du_pipeline_setup_inputs: failed to setup BRU source [ 563.166888] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:74:crtc-1] flip_done timed out [ 563.234105] PM: resume of devices complete after 103756.305 msecs [ 563.247877] PM: Finishing wakeup.
Hello Eugeniu,
On Mon, Jun 15, 2020 at 04:17:23PM +0200, Eugeniu Rosca wrote:
Hi Jacopo,
On Fri, Jun 12, 2020 at 05:12:09PM +0200, Jacopo Mondi wrote:
On Mon, Jun 08, 2020 at 11:44:32AM +0200, Eugeniu Rosca wrote:
FWIW, I seem to hit pre-existing issues in vanilla rcar-du, while unplugging HDMI cable during a cyclic suspend-resume:
HW: H3 ES2.0 Salvator-X SW: renesas-drivers-2020-06-02-v5.7 .config: renesas_defconfig +CONFIG_PM_DEBUG +CONFIG_PM_ADVANCED_DEBUG Use-case:
--------8<--------- $ cat s2ram.sh modprobe i2c-dev echo 9 > /proc/sys/kernel/printk i2cset -f -y 7 0x30 0x20 0x0F
According to https://elinux.org/R-Car/Boards/Salvator-X#Suspend-to-RAM this is not needed anymore
Good to know. Thanks for the useful remark.
echo 0 > /sys/module/suspend/parameters/pm_test_delay echo core > /sys/power/pm_test echo deep > /sys/power/mem_sleep echo 1 > /sys/power/pm_debug_messages echo 0 > /sys/power/pm_print_times echo mem > /sys/power/state
$ while true; do sh s2ram.sh ; done $ # unplug HDMI cable several times
I tried unplugging an plugging the cable while the system was suspended and after resume but I was not able to reproduce anything.
Your comment sounds like you suspended the system once and resumed it afterwards, while my description mentions "cyclic" :), meaning:
$ while true; do sh s2ram.sh; done $ # connect-disconnect the hdmi display a couple of times $ # NOTE: to avoid this manual step, I am thinking of a USB-controlled HDMI switcher long-term
Could you provide more precise instructions on how to reproduce this ? I.e. when to disconnect the cable to trigger the below error.
See above :)
BTW, using renesas-drivers-2020-06-02-v5.7 as base and performing the use-case just described, I got today (with minimal effort):
[ 459.321733] Enabling non-boot CPUs ... [ 459.331132] Detected PIPT I-cache on CPU1 [ 459.331189] CPU1: Booted secondary processor 0x0000000001 [0x411fd073] [ 459.332312] CPU1 is up [ 459.345635] Detected PIPT I-cache on CPU2 [ 459.345671] CPU2: Booted secondary processor 0x0000000002 [0x411fd073] [ 459.346624] CPU2 is up [ 459.359912] Detected PIPT I-cache on CPU3 [ 459.359942] CPU3: Booted secondary processor 0x0000000003 [0x411fd073] [ 459.360918] CPU3 is up [ 459.374183] Detected VIPT I-cache on CPU4 [ 459.374252] CPU4: Booted secondary processor 0x0000000100 [0x410fd034] [ 459.375875] cpufreq: cpufreq_online: CPU4: Running at unlisted freq: 1199999 KHz [ 459.394204] cpufreq: cpufreq_online: CPU4: Unlisted initial frequency changed to: 1200000 KHz [ 459.403879] CPU4 is up [ 459.406469] Detected VIPT I-cache on CPU5 [ 459.406519] CPU5: Booted secondary processor 0x0000000101 [0x410fd034] [ 459.408520] CPU5 is up [ 459.421762] Detected VIPT I-cache on CPU6 [ 459.421810] CPU6: Booted secondary processor 0x0000000102 [0x410fd034] [ 459.423831] CPU6 is up [ 459.437114] Detected VIPT I-cache on CPU7 [ 459.437164] CPU7: Booted secondary processor 0x0000000103 [0x410fd034] [ 459.439258] CPU7 is up [ 459.456217] PM: noirq resume of devices complete after 3.878 msecs [ 459.471529] PM: early resume of devices complete after 8.590 msecs [ 469.726906] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CRTC:76:crtc-3] flip_done timed out
I've been able to reproduce this same issue, but I see that errors in drm_atomic_helper_wait_for_dependencies always follow a first failure in drm_atomic_helper_wait_for_flip_done
Looking at the log what I see is that [ 160.488762] PM: late suspend of devices complete after 10.509 msecs [ 171.235584] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:75:crtc-1] flip_done timed out
The 10 second elapsed there matches the timout in drm_atomic_helper_wait_for_flip_done and it seems the issue is related to the first atomic commit after resume not being able to succesfully receive a flip_done event, possibly as the HDMI connector has been disconnected while the system was suspending or suspended and the DRM state was not updated.
Can you confirm you see the same failure sequence ?
Thanks j
Hi Laurent,
On Sun, Jun 07, 2020 at 05:41:58AM +0300, Laurent Pinchart wrote:
Note that the CMM driver is controlled by the DU driver. As the DU driver will reenable the display during resume, it will call rcar_du_cmm_setup() at resume time, which will reprogram the CMM. There should thus be no need for manual suspend/resume handling in the CMM as far as I can tell, but we need to ensure that the CMM is suspended before and resumed after the DU. I believe this could be implemented using device links.
Based on below quote [*] from Jacopo's commit [**], isn't the device link relationship already in place?
[*] Quote from commit [**] Enforce the probe and suspend/resume ordering of DU and CMM by creating a stateless device link between the two.
[**] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i... ("drm: rcar-du: kms: Initialize CMM instances")
Hi Eugeniu,
On Tue, Jun 09, 2020 at 04:29:59PM +0200, Eugeniu Rosca wrote:
Hi Laurent,
On Sun, Jun 07, 2020 at 05:41:58AM +0300, Laurent Pinchart wrote:
Note that the CMM driver is controlled by the DU driver. As the DU driver will reenable the display during resume, it will call rcar_du_cmm_setup() at resume time, which will reprogram the CMM. There should thus be no need for manual suspend/resume handling in the CMM as far as I can tell, but we need to ensure that the CMM is suspended before and resumed after the DU. I believe this could be implemented using device links.
Based on below quote [*] from Jacopo's commit [**], isn't the device link relationship already in place?
Yes, it's in place already.
I added pm_ops to cmm just to be able to printout when suspend/resume happens and the sequence is what comment [*] reports
[ 222.909002] rcar_du_pm_suspend:505 [ 223.145497] rcar_cmm_pm_suspend:193
[ 223.208053] rcar_cmm_pm_resume:200 [ 223.460094] rcar_du_pm_resume:513
However, Laurent mentioned that in his comment here that he expects the opposite sequence to happen (CMM to suspend before and resume after DU).
I still think what is implemented is correct: - CMM is suspended after DU: when CMM is suspended, DU is not feeding it with data - CMM is resumed before: once DU restart operations CMM is ready to receive data.
Laurent, what do you think ?
Thanks j
[*] Quote from commit [**] Enforce the probe and suspend/resume ordering of DU and CMM by creating a stateless device link between the two.
[**] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i... ("drm: rcar-du: kms: Initialize CMM instances")
-- Best regards, Eugeniu Rosca
Hi Jacopo,
On Fri, Jun 12, 2020 at 05:00:32PM +0200, Jacopo Mondi wrote:
On Tue, Jun 09, 2020 at 04:29:59PM +0200, Eugeniu Rosca wrote:
On Sun, Jun 07, 2020 at 05:41:58AM +0300, Laurent Pinchart wrote:
Note that the CMM driver is controlled by the DU driver. As the DU driver will reenable the display during resume, it will call rcar_du_cmm_setup() at resume time, which will reprogram the CMM. There should thus be no need for manual suspend/resume handling in the CMM as far as I can tell, but we need to ensure that the CMM is suspended before and resumed after the DU. I believe this could be implemented using device links.
Based on below quote [*] from Jacopo's commit [**], isn't the device link relationship already in place?
Yes, it's in place already.
I added pm_ops to cmm just to be able to printout when suspend/resume happens and the sequence is what comment [*] reports
[ 222.909002] rcar_du_pm_suspend:505 [ 223.145497] rcar_cmm_pm_suspend:193
[ 223.208053] rcar_cmm_pm_resume:200 [ 223.460094] rcar_du_pm_resume:513
However, Laurent mentioned that in his comment here that he expects the opposite sequence to happen (CMM to suspend before and resume after DU).
I still think what is implemented is correct:
- CMM is suspended after DU: when CMM is suspended, DU is not feeding it with data
- CMM is resumed before: once DU restart operations CMM is ready to receive data.
Laurent, what do you think ?
I think I shouldn't have written the previous e-mail in the middle of the night :-) Suspending CMM after DU is obviously correct.
[*] Quote from commit [**] Enforce the probe and suspend/resume ordering of DU and CMM by creating a stateless device link between the two.
[**] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i... ("drm: rcar-du: kms: Initialize CMM instances")
Hi,
On Fri, Jun 12, 2020 at 06:10:05PM +0300, Laurent Pinchart wrote:
On Fri, Jun 12, 2020 at 05:00:32PM +0200, Jacopo Mondi wrote:
On Tue, Jun 09, 2020 at 04:29:59PM +0200, Eugeniu Rosca wrote:
On Sun, Jun 07, 2020 at 05:41:58AM +0300, Laurent Pinchart wrote:
Note that the CMM driver is controlled by the DU driver. As the DU driver will reenable the display during resume, it will call rcar_du_cmm_setup() at resume time, which will reprogram the CMM. There should thus be no need for manual suspend/resume handling in the CMM as far as I can tell, but we need to ensure that the CMM is suspended before and resumed after the DU. I believe this could be implemented using device links.
Based on below quote [*] from Jacopo's commit [**], isn't the device link relationship already in place?
Yes, it's in place already.
I added pm_ops to cmm just to be able to printout when suspend/resume happens and the sequence is what comment [*] reports
[ 222.909002] rcar_du_pm_suspend:505 [ 223.145497] rcar_cmm_pm_suspend:193
[ 223.208053] rcar_cmm_pm_resume:200 [ 223.460094] rcar_du_pm_resume:513
However, Laurent mentioned that in his comment here that he expects the opposite sequence to happen (CMM to suspend before and resume after DU).
I still think what is implemented is correct:
- CMM is suspended after DU: when CMM is suspended, DU is not feeding it with data
- CMM is resumed before: once DU restart operations CMM is ready to receive data.
Laurent, what do you think ?
I think I shouldn't have written the previous e-mail in the middle of the night :-) Suspending CMM after DU is obviously correct.
Thanks to Renesas team (kudos to Gotthard and Michael), we've figured out that below sequence of clock handling (happening during concurrent suspend and HDMI display unplug) leads to SoC lockup:
cmm1 OFF (caused by HDMI unplug) x21-clock OFF (caused by HDMI unplug) du1 OFF (caused by HDMI unplug) cmm1 ON (caused by suspend to ram, as preparation for CMM register save) # Freeze happens
That seems to be explained by Chapter 35A.4.3 "Restriction of enabling clock signal of the CMM" of HW User's manual (Rev.2.00 Jul 2019):
-----8<----- When the clock signal of the CMM is enabled (RMSTPCR7.CMMn or SMSTPCR7.CMMn = 0), the clock signal of the DU should be also enabled (RMSTPCR7.DUn or SMSTPCR7.DUn = 0). -----8<-----
So, the lesson learned from the above is: do not enable the CMMi clock while the DUi clock is disabled. I expect this to also potentially give some input w.r.t. what to suspend/resume first, CMM or DU.
[*] Quote from commit [**] Enforce the probe and suspend/resume ordering of DU and CMM by creating a stateless device link between the two.
[**] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i... ("drm: rcar-du: kms: Initialize CMM instances")
Hi Eugeniu,
On Fri, Jun 12, 2020 at 05:36:07PM +0200, Eugeniu Rosca wrote:
On Fri, Jun 12, 2020 at 06:10:05PM +0300, Laurent Pinchart wrote:
On Fri, Jun 12, 2020 at 05:00:32PM +0200, Jacopo Mondi wrote:
On Tue, Jun 09, 2020 at 04:29:59PM +0200, Eugeniu Rosca wrote:
On Sun, Jun 07, 2020 at 05:41:58AM +0300, Laurent Pinchart wrote:
Note that the CMM driver is controlled by the DU driver. As the DU driver will reenable the display during resume, it will call rcar_du_cmm_setup() at resume time, which will reprogram the CMM. There should thus be no need for manual suspend/resume handling in the CMM as far as I can tell, but we need to ensure that the CMM is suspended before and resumed after the DU. I believe this could be implemented using device links.
Based on below quote [*] from Jacopo's commit [**], isn't the device link relationship already in place?
Yes, it's in place already.
I added pm_ops to cmm just to be able to printout when suspend/resume happens and the sequence is what comment [*] reports
[ 222.909002] rcar_du_pm_suspend:505 [ 223.145497] rcar_cmm_pm_suspend:193
[ 223.208053] rcar_cmm_pm_resume:200 [ 223.460094] rcar_du_pm_resume:513
However, Laurent mentioned that in his comment here that he expects the opposite sequence to happen (CMM to suspend before and resume after DU).
I still think what is implemented is correct:
- CMM is suspended after DU: when CMM is suspended, DU is not feeding it with data
- CMM is resumed before: once DU restart operations CMM is ready to receive data.
Laurent, what do you think ?
I think I shouldn't have written the previous e-mail in the middle of the night :-) Suspending CMM after DU is obviously correct.
Thanks to Renesas team (kudos to Gotthard and Michael), we've figured out that below sequence of clock handling (happening during concurrent suspend and HDMI display unplug) leads to SoC lockup:
cmm1 OFF (caused by HDMI unplug) x21-clock OFF (caused by HDMI unplug) du1 OFF (caused by HDMI unplug) cmm1 ON (caused by suspend to ram, as preparation for CMM register save) # Freeze happens
That seems to be explained by Chapter 35A.4.3 "Restriction of enabling clock signal of the CMM" of HW User's manual (Rev.2.00 Jul 2019):
-----8<----- When the clock signal of the CMM is enabled (RMSTPCR7.CMMn or SMSTPCR7.CMMn = 0), the clock signal of the DU should be also enabled (RMSTPCR7.DUn or SMSTPCR7.DUn = 0). -----8<-----
So, the lesson learned from the above is: do not enable the CMMi clock while the DUi clock is disabled. I expect this to also potentially give some input w.r.t. what to suspend/resume first, CMM or DU.
This may be an ugly one. The DU driver needs to disable the CMM when suspending, and enabling the CMM when resuming. To do so, it calls functions of the CMM driver, and those functions access CMM registers. We can't do so while the CMM is suspended, so the DU has to suspend before the CMM, and resume after the CMM.
On the other hand, as you state here, we need to make sure the CMM clock is disabled first. The CMM thus needs to suspend before the DU, and resume after the DU.
Those are conflicting requirements.
One option could be to use the .suspend_late() and .resume_early() PM operations for the DU, to turn the DU clock off late and turn it back on early. Integrating it with the DRM suspend/resume helpers will likely be complicated though. I wonder if we could find a more elegant solution.
I the above sequence, you list "cmm1 ON" as a preparation for CMM register save. We don't need to save any register, the CMM driver has no .suspend() handler. The PM core should really skip waking up the device at that point (I recall complaining about the spurious wake ups at suspend time a while ago, but that neevr got addressed). Geert, any opinion on that ?
[*] Quote from commit [**] Enforce the probe and suspend/resume ordering of DU and CMM by creating a stateless device link between the two.
[**] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i... ("drm: rcar-du: kms: Initialize CMM instances")
Hi Laurent,
On Fri, Jun 12, 2020 at 5:51 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
On Fri, Jun 12, 2020 at 05:36:07PM +0200, Eugeniu Rosca wrote:
On Fri, Jun 12, 2020 at 06:10:05PM +0300, Laurent Pinchart wrote:
On Fri, Jun 12, 2020 at 05:00:32PM +0200, Jacopo Mondi wrote:
On Tue, Jun 09, 2020 at 04:29:59PM +0200, Eugeniu Rosca wrote:
On Sun, Jun 07, 2020 at 05:41:58AM +0300, Laurent Pinchart wrote:
Note that the CMM driver is controlled by the DU driver. As the DU driver will reenable the display during resume, it will call rcar_du_cmm_setup() at resume time, which will reprogram the CMM. There should thus be no need for manual suspend/resume handling in the CMM as far as I can tell, but we need to ensure that the CMM is suspended before and resumed after the DU. I believe this could be implemented using device links.
Based on below quote [*] from Jacopo's commit [**], isn't the device link relationship already in place?
Yes, it's in place already.
I added pm_ops to cmm just to be able to printout when suspend/resume happens and the sequence is what comment [*] reports
[ 222.909002] rcar_du_pm_suspend:505 [ 223.145497] rcar_cmm_pm_suspend:193
[ 223.208053] rcar_cmm_pm_resume:200 [ 223.460094] rcar_du_pm_resume:513
However, Laurent mentioned that in his comment here that he expects the opposite sequence to happen (CMM to suspend before and resume after DU).
I still think what is implemented is correct:
- CMM is suspended after DU: when CMM is suspended, DU is not feeding it with data
- CMM is resumed before: once DU restart operations CMM is ready to receive data.
Laurent, what do you think ?
I think I shouldn't have written the previous e-mail in the middle of the night :-) Suspending CMM after DU is obviously correct.
Thanks to Renesas team (kudos to Gotthard and Michael), we've figured out that below sequence of clock handling (happening during concurrent suspend and HDMI display unplug) leads to SoC lockup:
cmm1 OFF (caused by HDMI unplug) x21-clock OFF (caused by HDMI unplug) du1 OFF (caused by HDMI unplug) cmm1 ON (caused by suspend to ram, as preparation for CMM register save) # Freeze happens
That seems to be explained by Chapter 35A.4.3 "Restriction of enabling clock signal of the CMM" of HW User's manual (Rev.2.00 Jul 2019):
-----8<----- When the clock signal of the CMM is enabled (RMSTPCR7.CMMn or SMSTPCR7.CMMn = 0), the clock signal of the DU should be also enabled (RMSTPCR7.DUn or SMSTPCR7.DUn = 0). -----8<-----
So, the lesson learned from the above is: do not enable the CMMi clock while the DUi clock is disabled. I expect this to also potentially give some input w.r.t. what to suspend/resume first, CMM or DU.
This may be an ugly one. The DU driver needs to disable the CMM when suspending, and enabling the CMM when resuming. To do so, it calls functions of the CMM driver, and those functions access CMM registers. We can't do so while the CMM is suspended, so the DU has to suspend before the CMM, and resume after the CMM.
On the other hand, as you state here, we need to make sure the CMM clock is disabled first. The CMM thus needs to suspend before the DU, and resume after the DU.
Those are conflicting requirements.
One option could be to use the .suspend_late() and .resume_early() PM operations for the DU, to turn the DU clock off late and turn it back on early. Integrating it with the DRM suspend/resume helpers will likely be complicated though. I wonder if we could find a more elegant solution.
I the above sequence, you list "cmm1 ON" as a preparation for CMM register save. We don't need to save any register, the CMM driver has no .suspend() handler. The PM core should really skip waking up the device at that point (I recall complaining about the spurious wake ups at suspend time a while ago, but that neevr got addressed). Geert, any opinion on that ?
If there are issues with the PM core, please bring it up with the PM people.
If there's a (too) close integration of the CMM and the DU, perhaps the CMM should list the DU module clock in its clocks property, too? We have a similar construction for USB (module clocks 703 and 704).
Gr{oetje,eeting}s,
Geert
On Fri, Jun 05, 2020 at 03:29:00PM +0200, Jacopo Mondi wrote:
Hello Eugeniu, sorry for the late reply
On Wed, May 27, 2020 at 09:15:55AM +0200, Eugeniu Rosca wrote:
Hi Jacopo,
On Tue, Oct 15, 2019 at 12:46:13PM +0200, Jacopo Mondi wrote: ----8<---
- Testing
I have tested by injecting a color inversion LUT table and enabling/disabling it every 50 displayed frames: https://jmondi.org/cgit/kmsxx/log/?h=gamma_lut
Could you kindly share the cross compilation steps for your kmsxx fork?
I usually build it on the target :)
Otherwise, cross-compilation instructions are available in README.md.
$ mkdir build $ cd build $ cmake -DCMAKE_TOOLCHAIN_FILE=<buildrootpath>/output/host/usr/share/buildroot/toolchainfile.cmake .. $ make -j4
I assume you may use yocto instead of buildroot, you would have to locate the toolchainfile.cmake file accordingly.
Just out of curiosity, have you ever tried to pull the display's HDMI cable while reading from CM2_LUT_TBL?
Ahem, not really :) Did I get you right, you mean disconnecting the HDMI cable from the board ?
At least with the out-of-tree CMM implementation [*], this sends the R-Car3 reference targets into an unrecoverable freeze, with no lockup reported by the kernel (i.e. looks like an serious HW issue).
CMM functionalities are retained between suspend/resume cycles (tested with suspend-to-idle) without requiring a re-programming of the LUT tables.
Hmm. Is this backed up by any statement in the HW User's manual? This comes in contrast with the original Renesas CMM implementation [**] which does make use of suspend (where the freeze actually happens).
Can we infer, based on your statement, that we could also get rid of the suspend callback in [**]?
As Geert (thanks) explained what I've tested with is suspend-to-idle, which retains the state of the LUT tables (and I assume other not-yet-implemented CMM features, like CLU). I recall the out-of-tree driver has suspend/resume routines but I never really tested that.
[*] https://github.com/renesas-rcar/du_cmm [**] https://github.com/renesas-rcar/du_cmm/blob/c393ed49834bdbc/meta-rcar-gen3/r...
dri-devel@lists.freedesktop.org
-
Eugeniu Rosca -
Geert Uytterhoeven -
Gotthard Voellmeke -
Jacopo Mondi -
Jacopo Mondi -
Kalakodima Venkata Rajesh (RBEI/ECF3) -
Kieran Bingham -
Laurent Pinchart