by Kieran Bingham

[permalink] [raw]

Subject: Re: [PATCH v5 3/8] drm: rcar-du: Add support for CMM

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

2019-10-15 13:32:17

by jacopo mondi

[permalink] [raw]

Subject: Re: [PATCH v5 3/8] drm: rcar-du: Add support for CMM

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 <[email protected]>
>
>
> > Signed-off-by: Jacopo Mondi <[email protected]>
> > ---
> > drivers/gpu/drm/rcar-du/Kconfig | 7 +
> > drivers/gpu/drm/rcar-du/Makefile | 1 +
> > drivers/gpu/drm/rcar-du/rcar_cmm.c | 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 <[email protected]>
> > + */
> > +
> > +#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 <[email protected]>");
> > +MODULE_DESCRIPTION("Renesas R-Car CMM Driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.h b/drivers/gpu/drm/rcar-du/rcar_cmm.h
> > new file mode 100644
> > index 000000000000..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 <[email protected]>
> > + */
> > +
> > +#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__ */
> >
>

Attachments:

(No filename) (11.71 kB)
signature.asc (849.00 B)
Download all attachments

2019-10-15 13:37:15

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 <[email protected]>
> > >
> > > > Signed-off-by: Jacopo Mondi <[email protected]>
> > > > ---
> > > > 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

Attachments:

(No filename) (11.62 kB)
signature.asc (849.00 B)
Download all attachments

2019-10-16 02:28:51

by Laurent Pinchart

[permalink] [raw]

Subject: Re: [PATCH v5 5/8] drm: rcar-du: crtc: Control CMM operations

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 <[email protected]>
> >>>
> >>>> Signed-off-by: Jacopo Mondi <[email protected]>
> >>>> ---
> >>>> 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 ?

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

--
Regards,

Laurent Pinchart

2019-11-11 13:07:51

Hi Laurent,

On Fri, Jun 12, 2020 at 5:51 PM Laurent Pinchart
<[email protected]> 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

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds