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

2019-08-22 Thread Jacopo Mondi 
Hi Ulrich,

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

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

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

2019-08-22 Thread Laurent Pinchart 
Hi Jacopo,

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

I think it's important to document all fields.

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

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

2019-08-22 Thread Jacopo Mondi 
Hi Laurent,
   thanks for review

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

Ack on all of these

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

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

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

I'm a bit bothered that we would have
rcmm.en

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

2019-08-20 Thread Laurent Pinchart 
Hello,

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

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

2019-08-20 Thread Laurent Pinchart 
Hi Jacopo,

Thank you for the patch.

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

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

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

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

> +#define CM2_LUT_TBLA_BASE0x0600
> +#define CM2_LUT_TBLA(__i)(CM2_LUT_TBLA_BASE + (__i) * 4)

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

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

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

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

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

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

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

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

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

s/unsigned int/size_t/ ?

> +struct drm_color_lut *lut)

You can make this pointer const.

> +{
> + unsigned int i;
> +
> + for (

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

2019-07-16 Thread Ulrich Hecht 
Thanks for your patch!

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

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

> + rcar_cmm_write(rcmm, CM2_LUT_TBLA(i), val);
> + }
> +}
> +
> +/**
> + * rcar_cmm_setup() - configure the CMM unit
> + *
> + * @pdev: The platform device associated with the CMM instance
> + * @config: The CRTC provided configuration.
> + *
> + * Configure the CMM unit with the CRTC provided configuration.
> + * Currently enabling, disabling and programming of the 1-D LUT unit is
> + * supported.
> + */
> +int rcar_cmm_setup(struct platform_device *pdev,
> +const struct rcar_cmm_config *config)
> +{
> + struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> + unsigned int i;
> +
> + if (config

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

2019-07-06 Thread Jacopo Mondi 
Add a driver for the R-Car Display Unit Color Correction Module.

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

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

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

diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
index 1529849e217e..539d232790d1 100644
--- a/drivers/gpu/drm/rcar-du/Kconfig
+++ b/drivers/gpu/drm/rcar-du/Kconfig
@@ -13,6 +13,13 @@ config DRM_RCAR_DU
  Choose this option if you have an R-Car chipset.
  If M is selected the module will be called rcar-du-drm.
 
+config DRM_RCAR_CMM
+   bool "R-Car DU Color Management Module (CMM) Support"
+   depends on DRM && OF
+   depends on DRM_RCAR_DU
+   help
+ Enable support for R-Car Color Management Module (CMM).
+
 config DRM_RCAR_DW_HDMI
tristate "R-Car DU Gen3 HDMI Encoder Support"
depends on DRM && OF
diff --git a/drivers/gpu/drm/rcar-du/Makefile b/drivers/gpu/drm/rcar-du/Makefile
index 6c2ed9c46467..4d1187ccc3e5 100644
--- a/drivers/gpu/drm/rcar-du/Makefile
+++ b/drivers/gpu/drm/rcar-du/Makefile
@@ -15,6 +15,7 @@ rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS)   += rcar_du_of.o \
 rcar-du-drm-$(CONFIG_DRM_RCAR_VSP) += rcar_du_vsp.o
 rcar-du-drm-$(CONFIG_DRM_RCAR_WRITEBACK) += rcar_du_writeback.o
 
+obj-$(CONFIG_DRM_RCAR_CMM) += rcar_cmm.o
 obj-$(CONFIG_DRM_RCAR_DU)  += rcar-du-drm.o
 obj-$(CONFIG_DRM_RCAR_DW_HDMI) += rcar_dw_hdmi.o
 obj-$(CONFIG_DRM_RCAR_LVDS)+= rcar_lvds.o
diff --git a/drivers/gpu/drm/rcar-du/rcar_cmm.c 
b/drivers/gpu/drm/rcar-du/rcar_cmm.c
new file mode 100644
index ..76ed3fce2b33
--- /dev/null
+++ b/drivers/gpu/drm/rcar-du/rcar_cmm.c
@@ -0,0 +1,292 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * rcar_cmm.c -- R-Car Display Unit Color Management Module
+ *
+ * Copyright (C) 2019 Jacopo Mondi 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#include "rcar_cmm.h"
+
+#define CM2_LUT_CTRL   0x
+#define CM2_LUT_CTRL_ENBIT(0)
+#define CM2_LUT_TBLA_BASE  0x0600
+#define CM2_LUT_TBLA(__i)  (CM2_LUT_TBLA_BASE + (__i) * 4)
+
+struct rcar_cmm {
+   struct clk *clk;
+   void __iomem *base;
+   bool enabled;
+
+   /*
+* restore: LUT restore flag
+* running: LUT operating flag
+* size: Number of programmed entries in the LUT table
+* table: Scratch buffer where to store the LUT table entries to be
+*later restored.
+*/
+   struct {
+   bool restore;
+   bool running;
+   unsigned int size;
+   struct drm_color_lut table[CMM_GAMMA_LUT_SIZE];
+   } lut;
+};
+
+static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg)
+{
+   return ioread32(rcmm->base + reg);
+}
+
+static inline void rcar_cmm_write(struct rcar_cmm *rcmm, u32 reg, u32 data)
+{
+   iowrite32(data, rcmm->base + reg);
+}
+
+static void rcar_cmm_lut_write(struct rcar_cmm *rcmm, unsigned int size,
+  struct drm_color_lut *lut)
+{
+   unsigned int i;
+
+   for (i = 0; i < size; ++i) {
+   struct drm_color_lut *entry = &lut[i];
+
+   u32 val = (entry->red & 0xff) << 16 |
+ (entry->green & 0xff) << 8 |
+ (entry->blue & 0xff);
+   rcar_cmm_write(rcmm, CM2_LUT_TBLA(i), val);
+   }
+}
+
+/**
+ * rcar_cmm_setup() - configure the CMM unit
+ *
+ * @pdev: The platform device associated with the CMM instance
+ * @config: The CRTC provided configuration.
+ *
+ * Configure the CMM unit with the CRTC provided configuration.
+ * Currently enabling, disabling and programming of the 1-D LUT unit is
+ * supported.
+ */
+int rcar_cmm_setup(struct platform_device *pdev,
+  const struct rcar_cmm_config *config)
+{
+   struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
+   unsigned int i;
+
+   if (config->lut.size > CMM_GAMMA_LUT_SIZE)
+   return -EINVAL;
+
+   /*
+* As cmm_setup is called by atomic commit tail helper, it might be
+* called before enabling the CRTC (which calls cmm_enable()).
+*/
+   if (!rcmm->enabled) {
+   if (!config->lut.enable)
+   return 0;
+
+   /*
+* Store the LUT table entries in the scratch buffer to be la