Re: [PATCH v9 03/13] media: rppx1: Add framework to support Dreamchip RPPX1 ISP

[Niklas Söderlund]

Hi Jacopo,

Thanks for your feedback.

On 2026-06-03 16:26:45 +0200, Jacopo Mondi wrote:

[ snip ]

> > > > diff --git a/drivers/media/platform/dreamchip/rppx1/rppx1_ccor.c 
> > > > b/drivers/media/platform/dreamchip/rppx1/rppx1_ccor.c
> > > > new file mode 100644
> > > > index 000000000000..3bfad3ba12e6
> > > > --- /dev/null
> > > > +++ b/drivers/media/platform/dreamchip/rppx1/rppx1_ccor.c
> > > > @@ -0,0 +1,105 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright (C) 2026 Renesas Electronics Corp.
> > > > + * Copyright (C) 2026 Ideas on Board Oy
> > > > + * Copyright (C) 2026 Ragnatech AB
> > > > + */
> > > > +
> > > > +#include "rpp_module.h"
> > > > +
> > > > +#define CCOR_VERSION_REG				0x0000
> > > > +
> > > > +#define CCOR_COEFF_REG_NUM				9
> > > > +#define CCOR_COEFF_REG(n)				(0x0004 + (4 * (n)))
> > > > +
> > > > +#define CCOR_OFFSET_R_REG				0x0028
> > > > +#define CCOR_OFFSET_G_REG				0x002c
> > > > +#define CCOR_OFFSET_B_REG				0x0030
> > > > +
> > > > +#define CCOR_CONFIG_TYPE_REG				0x0034
> > > > +#define CCOR_CONFIG_TYPE_USE_OFFSETS_AS_PRE_OFFSETS	BIT(1)
> > > > +#define CCOR_CONFIG_TYPE_CCOR_RANGE_AVAILABLE		BIT(0)
> > > > +
> > > > +#define CCOR_RANGE_REG					0x0038
> > > > +#define CCOR_RANGE_CCOR_C_RANGE				BIT(1)
> > > > +#define CCOR_RANGE_CCOR_Y_RANGE				BIT(0)
> > > > +
> > > > +static int rppx1_ccor_probe(struct rpp_module *mod)
> > > > +{
> > > > +	/* Version check. */
> > > > +	switch (rpp_module_read(mod, CCOR_VERSION_REG)) {
> > > > +	case 3:
> > > > +		/* 12-bit. */
> > > > +		break;
> > > > +	case 4:
> > > > +		/* 20-bit. */
> > > > +		break;
> > > > +	case 5:
> > > > +		/* 24-bit. */
> > > > +		break;
> > > > +	default:
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int rppx1_ccor_start(struct rpp_module *mod,
> > > > +			    const struct v4l2_mbus_framefmt *fmt)
> > > > +{
> > > > +	/* Configure matrix in bypass mode. */
> > > > +	rpp_module_write(mod, CCOR_COEFF_REG(0), 0x1000);
> > > > +	rpp_module_write(mod, CCOR_COEFF_REG(1), 0x0000);
> > > > +	rpp_module_write(mod, CCOR_COEFF_REG(2), 0x0000);
> > > > +
> > > > +	rpp_module_write(mod, CCOR_COEFF_REG(3), 0x0000);
> > > > +	rpp_module_write(mod, CCOR_COEFF_REG(4), 0x1000);
> > > > +	rpp_module_write(mod, CCOR_COEFF_REG(5), 0x0000);
> > > > +
> > > > +	rpp_module_write(mod, CCOR_COEFF_REG(6), 0x0000);
> > > > +	rpp_module_write(mod, CCOR_COEFF_REG(7), 0x0000);
> > > > +	rpp_module_write(mod, CCOR_COEFF_REG(8), 0x1000);
> > > > +
> > > > +	rpp_module_write(mod, CCOR_OFFSET_R_REG, 0x00000000);
> > > > +	rpp_module_write(mod, CCOR_OFFSET_G_REG, 0x00000000);
> > > > +	rpp_module_write(mod, CCOR_OFFSET_B_REG, 0x00000000);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +const struct rpp_module_ops rppx1_ccor_ops = {
> > > > +	.probe = rppx1_ccor_probe,
> > > > +	.start = rppx1_ccor_start,
> > > > +};
> > > > +
> > > > +static int rppx1_ccor_csm_start(struct rpp_module *mod,
> > > > +				const struct v4l2_mbus_framefmt *fmt)
> > > > +{
> > > > +	/* Reuse bypass matrix setup. */
> > > > +	if (fmt->code == MEDIA_BUS_FMT_RGB888_1X24)
> > > > +		return rppx1_ccor_start(mod, fmt);
> > > > +
> > > > +	/* Color Transformation RGB to YUV according to ITU-R BT.709. */
> > > > +	rpp_module_write(mod, CCOR_COEFF_REG(0), 0x0367);
> > > > +	rpp_module_write(mod, CCOR_COEFF_REG(1), 0x0b71);
> > > > +	rpp_module_write(mod, CCOR_COEFF_REG(2), 0x0128);
> > > > +
> > > > +	rpp_module_write(mod, CCOR_COEFF_REG(3), 0xfe2b);
> > > > +	rpp_module_write(mod, CCOR_COEFF_REG(4), 0xf9d5);
> > > > +	rpp_module_write(mod, CCOR_COEFF_REG(5), 0x0800);
> > > > +
> > > > +	rpp_module_write(mod, CCOR_COEFF_REG(6), 0x0800);
> > > > +	rpp_module_write(mod, CCOR_COEFF_REG(7), 0xf8bc);
> > > > +	rpp_module_write(mod, CCOR_COEFF_REG(8), 0xff44);
> > > > +
> > > > +	rpp_module_write(mod, CCOR_OFFSET_R_REG, 0x00000000);
> > > > +	rpp_module_write(mod, CCOR_OFFSET_G_REG, 0x00000800);
> > > > +	rpp_module_write(mod, CCOR_OFFSET_B_REG, 0x00000800);
> > >
> > > Is this a leftover or is it intetional ?
> >
> > Intentional.
> >
> > Most of the _start callbacks just disables or configure pass-thru mode
> > of each modules that are not used (read not yet configured by
> > user-space). And this is what is done here, configure the CCOR in
> > pass-thru mode.
> >
> > The RGB case is easiest as it's just configure the identity matrix.
> > While for the YUV case we need to pick something as a default and I
> > picked ITU-R BT.709 as IIRC this was the default for RkISP1.
> 
> Sorry, I don't think I'm following here.
> 
> Color correction is applied in RGB space, what's the YUV case and
> why should you use the CCM for color-space conversion ?
> 
> Are you confusing this block with the colorspace conversion matrix in
> the AWB stats engine that is used to return stats in YUV mode ?

I thought I had it clear in my mind, but after reading this I'm confused 
too ;-) Let's start from scratch and hash this out.

In rppx1_ccor.c there is code for a color correction module. A bit 
simplified this module is a color matrix multiplication and an offset 
for each color component. However this module is used in three 
different places,

- In the POST pipeline as the CCOR module used for color correction 
  control. See CCOR_BASE in datasheet. In this driver this module is 
  implemented as struct rpp_module_ops rppx1_ccor_ops.

- In each of the two OUTPUT pipelines (Human Vision and Machine Vision) 
  as a CSM color space matrix where it is used for conversion from 
  linear RGB to YcbCr. See CSM_BASE in the datasheet. In this driver 
  this module is implemented as struct rpp_module_ops 
  rppx1_ccor_csm_ops.

Both struct rppx1_ccor_ops and struct rppx1_ccor_csm_ops are implemented 
in this rppx1_ccor.c file as they module is the same only the 
configuration of them differ as they have different functions in the 
pipeline.

For the CCOR module in the POST pipeline it can be used to correct 
colors. As we have no disable bit for this module we configure it in 
pass-thru mode in rppx1_ccor_start() by just programming the identity 
matrix and no offsets.

For the CSM module in each of the two OUTPUT pipelines (only Human 
Vision is supported) the function is to do color space conversion. And 
this is what we see here in rppx1_ccor_csm_start().

If the output format of the RPPX1 is to be RGB we do "nothing" and just 
program the identity matrix with no offsets by calling 
rppx1_ccor_start(). However if the output format is to be YUYV we need 
convert it, and that is what you see here. IIRC I picked RGB to YUV 
according to ITU-R BT.709 as this is what RkISP1 do.

Without this we are not able to support both output formats. So for SCM 
this can't really be configured at runtime as it depends on the output 
format. While for CCOR it can be configured at runtime but we need some 
default setting to start with, else I have seen either complete black 
images or a lockup of the pipeline.

Does it make sens? It is a tad confusing as the same code is used for 
different functions at different stages in the pipeline.

> 
> >
> > >
> > > Userspace is expected to fully configure the block, I'm not sure this
> > > default initialization is useful.
> >
> > If this is not configured I have been able to lockup the whole
> > processing pipeline during development.
> >
> 
> Indeed, if there's no disable bit, the ccm shall be programmed with an
> identity matrix
> 
>  write(priv, mod->base + CCOR_COEFF_REG(0), 0x1000);
>  write(priv, mod->base + CCOR_COEFF_REG(1), 0x0000);
>  write(priv, mod->base + CCOR_COEFF_REG(2), 0x0000);
> 
>  write(priv, mod->base + CCOR_COEFF_REG(3), 0x0000);
>  write(priv, mod->base + CCOR_COEFF_REG(4), 0x1000);
>  write(priv, mod->base + CCOR_COEFF_REG(5), 0x0000);
> 
>  write(priv, mod->base + CCOR_COEFF_REG(6), 0x0000);
>  write(priv, mod->base + CCOR_COEFF_REG(7), 0x0000);
>  write(priv, mod->base + CCOR_COEFF_REG(8), 0x1000);
> 
>  write(priv, mod->base + CCOR_OFFSET_R_REG, 0x00000000);
>  write(priv, mod->base + CCOR_OFFSET_G_REG, 0x00000000);
>  write(priv, mod->base + CCOR_OFFSET_B_REG, 0x00000000);
> 
> ?

Yes and that is what we do in CCOR module (always) and in CSM if output 
is RGB.

> 
> > >
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +const struct rpp_module_ops rppx1_ccor_csm_ops = {
> > > > +	.probe = rppx1_ccor_probe,
> > > > +	.start = rppx1_ccor_csm_start,
> > > > +};
> > > > diff --git a/drivers/media/platform/dreamchip/rppx1/rppx1_db.c b/drivers/media/platform/dreamchip/rppx1/rppx1_db.c
> > > > new file mode 100644

[snip]

-- 
Kind Regards,
Niklas Söderlund