Re: [v8 02/14] media: rppx1: Add framework to support Dreamchip RPPX1 ISP

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

Hi Niklas

On Sat, May 16, 2026 at 05:17:18PM +0200, Niklas Söderlund wrote:
> Hello Jacopo,
>

[snip]

> > > diff --git a/drivers/media/platform/dreamchip/rppx1/rppx1_shrp.c b/drivers/media/platform/dreamchip/rppx1/rppx1_shrp.c
> > > new file mode 100644
> > > index 000000000000..5bec022e8f05
> > > --- /dev/null
> > > +++ b/drivers/media/platform/dreamchip/rppx1/rppx1_shrp.c
> > > @@ -0,0 +1,64 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright 2025 Renesas Electronics Corp.
> > > + * Copyright 2025 Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > + */
> > > +
> > > +#include "rpp_module.h"
> > > +
> > > +#define SHRPCNR_VERSION_REG				0x0000
> > > +
> > > +#define SHRPCNR_CTRL_REG				0x0004
> > > +#define SHRPCNR_CTRL_CAD_EN				BIT(3)
> > > +#define SHRPCNR_CTRL_DESAT_EN				BIT(2)
> > > +#define SHRPCNR_CTRL_CNR_EN				BIT(1)
> > > +#define SHRPCNR_CTRL_SHARPEN_EN				BIT(0)
> > > +
> > > +#define SHRPCNR_PARAM_REG				0x0008
> > > +#define SHRPCNR_PARAM_SHARP_FACTOR_MASK			GENMASK(19, 12)
> > > +#define SHRPCNR_PARAM_CORING_THR_MASK			GENMASK(11, 0)
> > > +
> > > +#define SHRPCNR_MAT_1_REG				0x000c
> > > +#define SHRPCNR_MAT_2_REG				0x0010
> > > +#define SHRPCNR_CLB_LINESIZE_REG			0x0014
> > > +#define SHRPCNR_YUV2RGB_CCOR_COEFF_0_REG		0x0018
> > > +#define SHRPCNR_YUV2RGB_CCOR_COEFF_1_REG		0x001c
> > > +#define SHRPCNR_YUV2RGB_CCOR_COEFF_2_REG		0x0020
> > > +#define SHRPCNR_YUV2RGB_CCOR_COEFF_3_REG		0x0024
> > > +#define SHRPCNR_YUV2RGB_CCOR_COEFF_4_REG		0x0028
> > > +#define SHRPCNR_YUV2RGB_CCOR_COEFF_5_REG		0x002c
> > > +#define SHRPCNR_YUV2RGB_CCOR_COEFF_6_REG		0x0030
> > > +#define SHRPCNR_YUV2RGB_CCOR_COEFF_7_REG		0x0034
> > > +#define SHRPCNR_YUV2RGB_CCOR_COEFF_8_REG		0x0038
> > > +#define SHRPCNR_YUV2RGB_CCOR_OFFSET_R_REG		0x003c
> > > +#define SHRPCNR_YUV2RGB_CCOR_OFFSET_G_REG		0x0040
> > > +#define SHRPCNR_YUV2RGB_CCOR_OFFSET_B_REG		0x0044
> > > +
> > > +#define SHRPCNR_CNR_THRES_REG				0x0048
> > > +#define SHRPCNR_CNR_THRES_CNR_THRES_CR_MASK		GENMASK(27, 16)
> > > +#define SHRPCNR_CNR_THRES_CNR_THRES_CB_MASK		GENMASK(11, 0)
> > > +
> > > +#define SHRPCNR_CRED_THRES_REG				0x004c
> > > +#define SHRPCNR_CRED_SLOPE_REG				0x0050
> > > +#define SHRPCNR_CAD_RESTORE_LVL_REG			0x0054
> > > +#define SHRPCNR_CAD_THRESH_V_UNEG_REG			0x0058
> > > +#define SHRPCNR_CAD_THRESH_V_UPOS_REG			0x005c
> > > +#define SHRPCNR_CAD_THRESH_U_REG			0x0060
> > > +
> > > +static int rppx1_shrp_probe(struct rpp_module *mod)
> > > +{
> > > +	/* Version check. */
> > > +	switch (rpp_module_read(mod, SHRPCNR_VERSION_REG)) {
> > > +	case 2:
> > > +		mod->info.shrp.colorbits = 12;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +const struct rpp_module_ops rppx1_shrp_ops = {
> > > +	.probe = rppx1_shrp_probe,
> > > +};
> >
> > There are quite some modules here that have no corresponding user in
> > libcamera and which are not exercized.
> >
> > As long as we don't have a userspace user, I would refrein from adding
> > them to the driver to avoid committing to a uAPI before anyone
> > actually uses it.
>
> I do have user-space that exercise each uAPI exposed by this series in
> the form of unit-tests. But I know you feel strongly about libcamera
> support so I will drop the uAPI from the ones not used by libcamera from
> this series.
>

More than libcamera itself, I think it's safer to make sure we can
exercize the user API before committing to them. The best way to do
that is to have an algorithm in libcamera indeed

> Before the switch to the RPPX1 dedicated format the RkISP1 IPA in
> libcamera did use more of the enabled blocks together with some sensors.

yeah, but thinking about denoise in example, I wouldn't fully commit
to that yet as we know that algorithm is just very basic


> So for my core use-cases dropping them will not lose me much
> functionality. And as this series now structures uAPI together with the
> code moving modules in out is easy :-)
>
> I will drop the following blocks:
>
> - BLS - statistics reporting.
> - DB
> - BD
>
> >
> > If you want to keep the kernel module, fine, but no ABI for blocks
> > without a userspace user.
>
> The probe and static init config (that instructs the ISP pipeline to
> bypass the module) I will keep. I will drop all logic and uAPU
> structures.

Thank you!

>
> >
> > > diff --git a/drivers/media/platform/dreamchip/rppx1/rppx1_wbmeas.c b/drivers/media/platform/dreamchip/rppx1/rppx1_wbmeas.c
> > > new file mode 100644
> > > index 000000000000..3d197d914d07
> > > --- /dev/null
> > > +++ b/drivers/media/platform/dreamchip/rppx1/rppx1_wbmeas.c
> > > @@ -0,0 +1,61 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright 2025 Renesas Electronics Corp.
> > > + * Copyright 2025 Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > + */
> > > +
> > > +#include "rpp_module.h"
> > > +
> > > +#define AWB_MEAS_VERSION_REG			0x0000
> > > +
> > > +#define AWB_MEAS_PROP_REG			0x0004
> > > +#define AWB_MEAS_PROP_MEAS_MODE_RGB		BIT(16) /* 0: YCbCr 1: RGB */
> > > +#define AWB_MEAS_PROP_YMAX			BIT(2)
> > > +#define AWB_MEAS_PROP_AWB_MODE_ON		BIT(1)
> > > +
> > > +#define AWB_MEAS_H_OFFS_REG			0x0008
> > > +#define AWB_MEAS_V_OFFS_REG			0x000c
> > > +#define AWB_MEAS_H_SIZE_REG			0x0010
> > > +#define AWB_MEAS_V_SIZE_REG			0x0014
> > > +#define AWB_MEAS_FRAMES_REG			0x0018
> > > +#define AWB_MEAS_REF_CB_MAX_B_REG		0x001c
> > > +#define AWB_MEAS_REF_CR_MAX_R_REG		0x0020
> > > +#define AWB_MEAS_MAX_Y_REG			0x0024
> > > +#define AWB_MEAS_MIN_Y_MAX_G_REG		0x0028
> > > +#define AWB_MEAS_MAX_CSUM_REG			0x002c
> > > +#define AWB_MEAS_MIN_C_REG			0x0030
> > > +#define AWB_MEAS_WHITE_CNT_REG			0x0034
> > > +#define AWB_MEAS_MEAN_Y_G_REG			0x0038
> > > +#define AWB_MEAS_MEAN_CB_B_REG			0x003c
> > > +#define AWB_MEAS_MEAN_CR_R_REG			0x0040
> > > +
> > > +#define AWB_MEAS_CCOR_COEFF_NUM			9
> > > +#define AWB_MEAS_CCOR_COEFF_REG(n)		(0x0044 + (4 * (n)))
> > > +
> > > +#define AWB_MEAS_CCOR_OFFSET_R_REG		0x0068
> > > +#define AWB_MEAS_CCOR_OFFSET_G_REG		0x006c
> > > +#define AWB_MEAS_CCOR_OFFSET_B_REG		0x0070
> > > +
> > > +static int rppx1_wbmeas_probe(struct rpp_module *mod)
> > > +{
> > > +	/* Version check. */
> > > +	switch (rpp_module_read(mod, AWB_MEAS_VERSION_REG)) {
> > > +	case 1:
> > > +		mod->info.wbmeas.colorbits = 8;
> > > +		break;
> > > +	case 2:
> > > +		mod->info.wbmeas.colorbits = 20;
> > > +		break;
> > > +	case 3:
> > > +		mod->info.wbmeas.colorbits = 24;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +const struct rpp_module_ops rppx1_wbmeas_ops = {
> > > +	.probe = rppx1_wbmeas_probe,
> > > +};
> > > diff --git a/drivers/media/platform/dreamchip/rppx1/rppx1_xyz2luv.c b/drivers/media/platform/dreamchip/rppx1/rppx1_xyz2luv.c
> > > new file mode 100644
> > > index 000000000000..73789c48c057
> > > --- /dev/null
> > > +++ b/drivers/media/platform/dreamchip/rppx1/rppx1_xyz2luv.c
> > > @@ -0,0 +1,26 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright 2025 Renesas Electronics Corp.
> > > + * Copyright 2025 Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > + */
> > > +
> > > +#include "rpp_module.h"
> > > +
> > > +#define XYZ2LUV_VERSION_REG			0x0000
> > > +#define XYZ2LUV_U_REF_REG			0x0004
> > > +#define XYZ2LUV_V_REF_REG			0x0008
> > > +#define XYZ2LUV_LUMA_OUT_FAC_REG		0x000c
> > > +#define XYZ2LUV_CHROMA_OUT_FAC_REG		0x0010
> > > +
> > > +static int rppx1_xyz2luv_probe(struct rpp_module *mod)
> > > +{
> > > +	/* Version check. */
> > > +	if (rpp_module_read(mod, XYZ2LUV_VERSION_REG) != 4)
> > > +		return -EINVAL;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +const struct rpp_module_ops rppx1_xyz2luv_ops = {
> > > +	.probe = rppx1_xyz2luv_probe,
> > > +};
> > > diff --git a/include/media/rppx1.h b/include/media/rppx1.h
> > > new file mode 100644
> > > index 000000000000..cb3470c27ceb
> > > --- /dev/null
> > > +++ b/include/media/rppx1.h
> > > @@ -0,0 +1,34 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * Copyright 2025 Renesas Electronics Corp.
> > > + * Copyright 2025 Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > + */
> > > +#ifndef __MEDIA_DCT_RPPX1_H__
> > > +#define __MEDIA_DCT_RPPX1_H__
> > > +
> > > +#include <linux/v4l2-mediabus.h>
> > > +#include <linux/media/dreamchip/rppx1-config.h>
> > > +
> > > +#include <media/videobuf2-core.h>
> > > +
> > > +struct rppx1;
> > > +
> > > +struct rppx1 *rppx1_create(void __iomem *base, struct device *dev);
> > > +
> > > +void rppx1_destroy(struct rppx1 *rpp);
> > > +
> > > +int rppx1_start(struct rppx1 *rpp, const struct v4l2_mbus_framefmt *input,
> > > +		const struct v4l2_mbus_framefmt *hv,
> > > +		const struct v4l2_mbus_framefmt *mv);
> > > +
> > > +int rppx1_stop(struct rppx1 *rpp);
> > > +
> > > +bool rppx1_interrupt(struct rppx1 *rpp, u32 *isc);
> > > +
> > > +typedef int (*rppx1_reg_write)(void *priv, u32 offset, u32 value);
> > > +int rppx1_params(struct rppx1 *rpp, struct vb2_buffer *vb, size_t max_size,
> > > +		 rppx1_reg_write write, void *priv);
> > > +
> > > +void rppx1_stats_fill_isr(struct rppx1 *rpp, u32 isc, void *buf);
> > > +
> > > +#endif /* __MEDIA_DCT_RPPX1_H__ */
> > > diff --git a/include/uapi/linux/media/dreamchip/rppx1-config.h b/include/uapi/linux/media/dreamchip/rppx1-config.h
> > > new file mode 100644
> > > index 000000000000..26627be6f483
> > > --- /dev/null
> > > +++ b/include/uapi/linux/media/dreamchip/rppx1-config.h
> >
> > As said, I don't think it makes much sense to add an almost empty uapi
> > file.
> >
> > I would add it in one commit.
> >
> > Please retain Jai's authorship and add my Co-developed-by tag as I
> > rewrote most of the documentation and reworked most blocks.
>
> As discussed above, I really like adding the uAPI together with the
> logic. I will retain the scaffolding as a separate commit from Jai and
> add the Co-developed tags.

Fine with me then!

Thanks
  j

>
> >
> > > @@ -0,0 +1,66 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > +/*
> > > + * Dreamchip RPP-X1 ISP Driver - Userspace API
> > > + *
> > > + * Copyright (C) 2026 Renesas Electronics Corp.
> > > + * Copyright (C) 2026 Ideas on Board Oy
> > > + * Copyright (C) 2026 Ragnatech AB
> > > + */
> > > +
> > > +#ifndef __UAPI_RPP_X1_CONFIG_H
> > > +#define __UAPI_RPP_X1_CONFIG_H
> > > +
> > > +#include <linux/types.h>
> > > +#include <linux/media/v4l2-isp.h>
> > > +
> > > +/**
> > > + * struct rppx1_window - Measurement window
> > > + *
> > > + * RPP-X1 measurement window. Different blocks use a window or multiple
> > > + * windows for measurement purposes. This defines a common type for all of
> > > + * them. The number of relevant bits depends on the block where the window is
> > > + * used and is specified in the per-block description
> > > + *
> > > + * @h_offs: horizontal offset from the left of the frame in pixels
> > > + * @v_offs: vertical offset from the top of the frame in pixels
> > > + * @h_size: horizontal size of the window in pixels
> > > + * @v_size: vertical size of the window in pixels
> > > + */
> > > +struct rppx1_window {
> > > +	__u16 h_offs;
> > > +	__u16 v_offs;
> > > +	__u16 h_size;
> > > +	__u16 v_size;
> > > +};
> > > +
> > > +/* ---------------------------------------------------------------------------
> > > + * Parameter Structures
> > > + *
> > > + * Native RPP-X1 precision. Fields use __u32 where the hardware provides
> > > + * wider-than-8-bit results.
> >
> > I think you could drop the first part: of course the RPP-X1 uAPI
> > header uses the RPP-X1 precision. You could add:
> >
> >       The same ISP block might be instantiated in multiple pipeliness
> >       and operate on a different bitdepth/precision. For fields of
> >       varying length among different instances of the same block, use
> >       a data type that can accommodate the larger bitdepth/precision.
> >
> > Or something similar
>
> Thanks, this should of course be updated now that we have a native RPPX1
> format.
>
> >
> > > + */
> > > +
> > > +/**
> > > + * RPPX1_PARAMS_MAX_SIZE - Maximum size of all RPP-X1 parameter blocks
> > > + *
> > > + * Some types are reported twice as the same block might be instantiated in
> > > + * multiple pipes.
> > > + */
> > > +#define RPPX1_PARAMS_MAX_SIZE 0
> > > +
> > > +/* ---------------------------------------------------------------------------
> > > + * Statistics Structures
> > > + *
> > > + * Native RPP-X1 precision. Fields use __u32 where the hardware provides
> > > + * wider-than-8-bit results.
> >
> > Same here
>
> Ditto.
>
> >
> > > + */
> > > +
> > > +/**
> > > + * RPPX1_STATS_MAX_SIZE - Maximum size of all RPP-X1 statistics
> > > + *
> > > + * Some types are reported twice as the same block might be instantiated in
> > > + * multiple pipes.
> > > + */
> > > +#define RPPX1_STATS_MAX_SIZE 0
> > > +
> > > +#endif /* __UAPI_RPP_X1_CONFIG_H */
> > > --
> > > 2.54.0
> > >
>
> --
> Kind Regards,
> Niklas Söderlund
>