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

[Jacopo Mondi]

Hi Niklas

On Wed, Jun 03, 2026 at 05:17:15PM +0200, Niklas Söderlund wrote:
> 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.

I had completely missed this is the 'csm' and this module handles both
the CCM and the color space conversion o_0

I thought only CCM was handled here.

>
> 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.
>

Oook I was not expecting this


> 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.
>

I completely missed the 'csm' part. I guess this is ok, even in a
single file.

Sorry for the noise and thanks for the time taken.

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