[PATCH 0/7] media: rkisp1: Fix and improve color space support

[PATCH 0/7] media: rkisp1: Fix and improve color space support

[Laurent Pinchart]

Hello,

This patch series fixes and improves color space support in the rkisp1
driver.

The first two patches initialize the color space fields to default
values on the ISP subdev video sink and source pads, and allow setting
the color space on the ISP sink pad (this has no influence on the ISP
configuration, and only serves to propagate the correct color space on
the pipeline).

Patch 3/7 fixes a bug in the ISP source pad configuration, which allowed
setting a Bayer output format with a YUV input format, a clearly invalid
configuration. Patch then 4/7 mimicks patch 2/7 by allowing setting of
color space fields on the source pad.

The last three patches configure the RGB to YUV matrix (in the CSM
module) using the ISP source pad YCbCr encoding. Patch 5/7 fixes a small
bug in color space handling that resulted in the sink pad quantization
controlling the ISP output, instead of using the source pad
quantization. Patch 6/7 is a small internal API refactoring, and finally
patch 7/7 handles the CSM configuration.

If anyone is curious about how the matrix coefficients of patch 7/7 have
been computed, see the script posted in [1] that will be merged in
libcamera.

[1] https://lists.libcamera.org/pipermail/libcamera-devel/2022-August/033183.html

Laurent Pinchart (7):
  media: rkisp1: Initialize color space on sink and source pads
  media: rkisp1: Allow setting color space on ISP sink pad
  media: rkisp1: Fix source pad format configuration
  media: rkisp1: Allow setting all color space fields on ISP source pad
  media: rkisp1: Configure quantization using ISP source pad
  media: rkisp1: Don't pass the quantization to rkisp1_csm_config()
  media: rkisp1: Configure CSM based on YCbCr encoding

 .../platform/rockchip/rkisp1/rkisp1-common.h  |   5 +-
 .../platform/rockchip/rkisp1/rkisp1-isp.c     | 139 ++++++++++++++++--
 .../platform/rockchip/rkisp1/rkisp1-params.c  | 105 +++++++++----
 3 files changed, 207 insertions(+), 42 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 1/7] media: rkisp1: Initialize color space on sink and source pads

[Laurent Pinchart]

Initialize the four color space fields on the sink and source
video pads in the .init_cfg() operation. As the main use case for the
ISP is to convert Bayer data to YUV, select a raw color space on the
sink pad and a limited range quantization of SYCC on the source pad by
default.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
index a3c7d4d88387..a52b22824739 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
@@ -553,12 +553,17 @@ static int rkisp1_isp_init_config(struct v4l2_subdev *sd,
 	struct v4l2_mbus_framefmt *sink_fmt, *src_fmt;
 	struct v4l2_rect *sink_crop, *src_crop;
 
+	/* Video. */
 	sink_fmt = v4l2_subdev_get_try_format(sd, sd_state,
 					      RKISP1_ISP_PAD_SINK_VIDEO);
 	sink_fmt->width = RKISP1_DEFAULT_WIDTH;
 	sink_fmt->height = RKISP1_DEFAULT_HEIGHT;
 	sink_fmt->field = V4L2_FIELD_NONE;
 	sink_fmt->code = RKISP1_DEF_SINK_PAD_FMT;
+	sink_fmt->colorspace = V4L2_COLORSPACE_RAW;
+	sink_fmt->xfer_func = V4L2_XFER_FUNC_NONE;
+	sink_fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
+	sink_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
 
 	sink_crop = v4l2_subdev_get_try_crop(sd, sd_state,
 					     RKISP1_ISP_PAD_SINK_VIDEO);
@@ -571,11 +576,16 @@ static int rkisp1_isp_init_config(struct v4l2_subdev *sd,
 					     RKISP1_ISP_PAD_SOURCE_VIDEO);
 	*src_fmt = *sink_fmt;
 	src_fmt->code = RKISP1_DEF_SRC_PAD_FMT;
+	src_fmt->colorspace = V4L2_COLORSPACE_SRGB;
+	src_fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
+	src_fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
+	src_fmt->quantization = V4L2_QUANTIZATION_LIM_RANGE;
 
 	src_crop = v4l2_subdev_get_try_crop(sd, sd_state,
 					    RKISP1_ISP_PAD_SOURCE_VIDEO);
 	*src_crop = *sink_crop;
 
+	/* Parameters and statistics. */
 	sink_fmt = v4l2_subdev_get_try_format(sd, sd_state,
 					      RKISP1_ISP_PAD_SINK_PARAMS);
 	src_fmt = v4l2_subdev_get_try_format(sd, sd_state,
-- 
Regards,

Laurent Pinchart

[PATCH 2/7] media: rkisp1: Allow setting color space on ISP sink pad

[Laurent Pinchart]

The ISP accepts different color spaces on its input: for YUV input, it
doesn't set any restrictions, and for Bayer inputs, any color primaries
or transfer function can be accepted (YCbCr encoding isn't applicable
there, and quantization range can only be full).

Allow setting a color space on the ISP sink pad, with the aforementioned
restrictions. The settings don't influence hardware yet (only the YUV
quantization range will, anything else has no direct effect on the ISP
configuration), but can already be set to allow color space information
to be coherent across the ISP sink link.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../platform/rockchip/rkisp1/rkisp1-isp.c     | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
index a52b22824739..32114d1e8ad1 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
@@ -705,6 +705,7 @@ static void rkisp1_isp_set_sink_fmt(struct rkisp1_isp *isp,
 	const struct rkisp1_mbus_info *mbus_info;
 	struct v4l2_mbus_framefmt *sink_fmt;
 	struct v4l2_rect *sink_crop;
+	bool is_yuv;
 
 	sink_fmt = rkisp1_isp_get_pad_fmt(isp, sd_state,
 					  RKISP1_ISP_PAD_SINK_VIDEO,
@@ -725,6 +726,36 @@ static void rkisp1_isp_set_sink_fmt(struct rkisp1_isp *isp,
 				   RKISP1_ISP_MIN_HEIGHT,
 				   RKISP1_ISP_MAX_HEIGHT);
 
+	/*
+	 * Adjust the color space fields. Accept any color primaries and
+	 * transfer function for both YUV and Bayer. For YUV any YCbCr encoding
+	 * and quantization range is also accepted. For Bayer formats, the YCbCr
+	 * encoding isn't applicable, and the quantization range can only be
+	 * full.
+	 */
+	is_yuv = mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV;
+
+	sink_fmt->colorspace = format->colorspace ? format->colorspace :
+			       (is_yuv ? V4L2_COLORSPACE_RAW :
+				V4L2_COLORSPACE_SRGB);
+	sink_fmt->xfer_func = format->xfer_func ? format->xfer_func :
+			      V4L2_MAP_XFER_FUNC_DEFAULT(sink_fmt->colorspace);
+	if (is_yuv) {
+		sink_fmt->ycbcr_enc = format->ycbcr_enc ? :
+			V4L2_MAP_YCBCR_ENC_DEFAULT(sink_fmt->colorspace);
+		sink_fmt->quantization = format->quantization ? :
+			V4L2_MAP_QUANTIZATION_DEFAULT(false, sink_fmt->colorspace,
+						      sink_fmt->ycbcr_enc);
+	} else {
+		/*
+		 * The YCbCr encoding isn't applicable for non-YUV formats, but
+		 * V4L2 has no "no encoding" value. Hardcode it to Rec. 601, it
+		 * should be ignored by userspace.
+		 */
+		sink_fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
+		sink_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	}
+
 	*format = *sink_fmt;
 
 	/* Propagate to in crop */
-- 
Regards,

Laurent Pinchart

[PATCH 3/7] media: rkisp1: Fix source pad format configuration

[Laurent Pinchart]

The ISP converts Bayer data to YUV when operating normally, and can also
operate in pass-through mode where the input and output formats must
match. Converting from YUV to Bayer isn't possible. If such an invalid
configuration is attempted, adjust it by copying the sink pad media bus
code to the source pad.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../platform/rockchip/rkisp1/rkisp1-isp.c     | 40 +++++++++++++++----
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
index 32114d1e8ad1..0441ccbc01a9 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
@@ -604,23 +604,43 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
 				   struct v4l2_mbus_framefmt *format,
 				   unsigned int which)
 {
-	const struct rkisp1_mbus_info *mbus_info;
+	const struct rkisp1_mbus_info *sink_info;
+	const struct rkisp1_mbus_info *src_info;
+	struct v4l2_mbus_framefmt *sink_fmt;
 	struct v4l2_mbus_framefmt *src_fmt;
 	const struct v4l2_rect *src_crop;
 
+	sink_fmt = rkisp1_isp_get_pad_fmt(isp, sd_state,
+					  RKISP1_ISP_PAD_SINK_VIDEO, which);
 	src_fmt = rkisp1_isp_get_pad_fmt(isp, sd_state,
 					 RKISP1_ISP_PAD_SOURCE_VIDEO, which);
 	src_crop = rkisp1_isp_get_pad_crop(isp, sd_state,
 					   RKISP1_ISP_PAD_SOURCE_VIDEO, which);
 
+	/*
+	 * Media bus code. The ISP can operate in pass-through mode (Bayer in,
+	 * Bayer out or YUV in, YUV out) or process Bayer data to YUV, but
+	 * can't convert from YUV to Bayer.
+	 */
+	sink_info = rkisp1_mbus_info_get_by_code(sink_fmt->code);
+
 	src_fmt->code = format->code;
-	mbus_info = rkisp1_mbus_info_get_by_code(src_fmt->code);
-	if (!mbus_info || !(mbus_info->direction & RKISP1_ISP_SD_SRC)) {
+	src_info = rkisp1_mbus_info_get_by_code(src_fmt->code);
+	if (!src_info || !(src_info->direction & RKISP1_ISP_SD_SRC)) {
 		src_fmt->code = RKISP1_DEF_SRC_PAD_FMT;
-		mbus_info = rkisp1_mbus_info_get_by_code(src_fmt->code);
+		src_info = rkisp1_mbus_info_get_by_code(src_fmt->code);
 	}
-	if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
-		isp->src_fmt = mbus_info;
+
+	if (sink_info->pixel_enc == V4L2_PIXEL_ENC_YUV &&
+	    src_info->pixel_enc == V4L2_PIXEL_ENC_BAYER) {
+		src_fmt->code = sink_fmt->code;
+		src_info = sink_info;
+	}
+
+	/*
+	 * The source width and height must be identical to the source crop
+	 * size.
+	 */
 	src_fmt->width  = src_crop->width;
 	src_fmt->height = src_crop->height;
 
@@ -630,14 +650,18 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
 	 */
 	if (format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC &&
 	    format->quantization == V4L2_QUANTIZATION_FULL_RANGE &&
-	    mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV)
+	    src_info->pixel_enc == V4L2_PIXEL_ENC_YUV)
 		src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
-	else if (mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV)
+	else if (src_info->pixel_enc == V4L2_PIXEL_ENC_YUV)
 		src_fmt->quantization = V4L2_QUANTIZATION_LIM_RANGE;
 	else
 		src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
 
 	*format = *src_fmt;
+
+	/* Store the source format info when setting the active format. */
+	if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
+		isp->src_fmt = src_info;
 }
 
 static void rkisp1_isp_set_src_crop(struct rkisp1_isp *isp,
-- 
Regards,

Laurent Pinchart

[PATCH 4/7] media: rkisp1: Allow setting all color space fields on ISP source pad

[Laurent Pinchart]

The ISP output color space is configured through the ISP source pad. At
the moment, only the quantization can be set. Extend it to the three
other color space fields:

- The ycbcr_enc field will be used to configure the RGB to YUV matrix
  (currently hardcoded to Rec. 601).

- The colorspace (which controls the color primaries) and xfer_func
  fields will not be used to configure the ISP, as the corresponding
  hardware blocks (the cross-talk RGB to RGB matrix and the tone mapping
  curve) are programmed directly by applications through ISP parameters.
  Nonetheless, those two fields should be set by applications to match
  the ISP configuration, in order to propagate the correct color space
  down the pipeline up to the capture video nodes.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../platform/rockchip/rkisp1/rkisp1-isp.c     | 55 ++++++++++++++++---
 1 file changed, 48 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
index 0441ccbc01a9..8b93b5c03bce 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
@@ -609,6 +609,7 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
 	struct v4l2_mbus_framefmt *sink_fmt;
 	struct v4l2_mbus_framefmt *src_fmt;
 	const struct v4l2_rect *src_crop;
+	bool set_csc;
 
 	sink_fmt = rkisp1_isp_get_pad_fmt(isp, sd_state,
 					  RKISP1_ISP_PAD_SINK_VIDEO, which);
@@ -645,20 +646,60 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
 	src_fmt->height = src_crop->height;
 
 	/*
-	 * The CSC API is used to allow userspace to force full
-	 * quantization on YUV formats.
+	 * Copy the color space for the sink pad. When converting from Bayer to
+	 * YUV, default to a limited quantization range.
 	 */
-	if (format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC &&
-	    format->quantization == V4L2_QUANTIZATION_FULL_RANGE &&
+	src_fmt->colorspace = sink_fmt->colorspace;
+	src_fmt->xfer_func = sink_fmt->xfer_func;
+	src_fmt->ycbcr_enc = sink_fmt->ycbcr_enc;
+
+	if (sink_info->pixel_enc == V4L2_PIXEL_ENC_BAYER &&
 	    src_info->pixel_enc == V4L2_PIXEL_ENC_YUV)
-		src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
-	else if (src_info->pixel_enc == V4L2_PIXEL_ENC_YUV)
 		src_fmt->quantization = V4L2_QUANTIZATION_LIM_RANGE;
 	else
-		src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+		src_fmt->quantization = sink_fmt->quantization;
+
+	/*
+	 * Allow setting the source color space fields when the SET_CSC flag is
+	 * set and the source format is YUV. If the sink format is YUV, don't
+	 * set the color primaries, transfer function or YCbCr encoding as the
+	 * ISP is bypassed in that case and passes YUV data through without
+	 * modifications.
+	 *
+	 * The color primaries and transfer function are configured through the
+	 * cross-talk matrix and tone curve respectively. Settings for those
+	 * hardware blocks are conveyed through the ISP parameters buffer, as
+	 * they need to combine color space information with other image tuning
+	 * characteristics and can't thus be computed by the kernel based on the
+	 * color space. The source pad colorspace and xfer_func fields are thus
+	 * ignored by the driver, but can be set by userspace to propagate
+	 * accurate color space information down the pipeline.
+	 */
+	set_csc = !!(format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC);
+
+	if (set_csc && src_info->pixel_enc == V4L2_PIXEL_ENC_YUV) {
+		if (sink_info->pixel_enc == V4L2_PIXEL_ENC_BAYER) {
+			if (format->colorspace != V4L2_COLORSPACE_DEFAULT)
+				src_fmt->colorspace = format->colorspace;
+			if (format->xfer_func != V4L2_XFER_FUNC_DEFAULT)
+				src_fmt->xfer_func = format->xfer_func;
+			if (format->ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT)
+				src_fmt->ycbcr_enc = format->ycbcr_enc;
+		}
+
+		if (format->quantization != V4L2_QUANTIZATION_DEFAULT)
+			sink_fmt->quantization = format->quantization;
+	}
 
 	*format = *src_fmt;
 
+	/*
+	 * Restore the SET_CSC flag if it was set to indicate support for the
+	 * CSC setting API.
+	 */
+	if (set_csc)
+		format->flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;
+
 	/* Store the source format info when setting the active format. */
 	if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
 		isp->src_fmt = src_info;
-- 
Regards,

Laurent Pinchart

[PATCH 5/7] media: rkisp1: Configure quantization using ISP source pad

[Laurent Pinchart]

The rkisp1_config_isp() function uses the format on the sink pad of the
ISP to configure quantization at the output of the ISP. This is
incorrect, as hinted by the src_frm variable name that stores the
format. Fix it by using the source pad.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
index 8b93b5c03bce..9d4d018d58b6 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
@@ -341,7 +341,7 @@ static int rkisp1_config_isp(struct rkisp1_isp *isp,
 		struct v4l2_mbus_framefmt *src_frm;
 
 		src_frm = rkisp1_isp_get_pad_fmt(isp, NULL,
-						 RKISP1_ISP_PAD_SINK_VIDEO,
+						 RKISP1_ISP_PAD_SOURCE_VIDEO,
 						 V4L2_SUBDEV_FORMAT_ACTIVE);
 		rkisp1_params_configure(&rkisp1->params, sink_fmt->bayer_pat,
 					src_frm->quantization);
-- 
Regards,

Laurent Pinchart

[PATCH 6/7] media: rkisp1: Don't pass the quantization to rkisp1_csm_config()

[Laurent Pinchart]

The rkisp1_csm_config() function takes a pointer to the rkisp1_params
structure which contains the quantization value. There's no need to pass
it separately to the function. Drop it from the function parameters.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/rockchip/rkisp1/rkisp1-params.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
index 8b4eea77af0d..163419624370 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
@@ -1076,7 +1076,7 @@ static void rkisp1_ie_enable(struct rkisp1_params *params, bool en)
 	}
 }
 
-static void rkisp1_csm_config(struct rkisp1_params *params, bool full_range)
+static void rkisp1_csm_config(struct rkisp1_params *params)
 {
 	static const u16 full_range_coeff[] = {
 		0x0026, 0x004b, 0x000f,
@@ -1090,7 +1090,7 @@ static void rkisp1_csm_config(struct rkisp1_params *params, bool full_range)
 	};
 	unsigned int i;
 
-	if (full_range) {
+	if (params->quantization == V4L2_QUANTIZATION_FULL_RANGE) {
 		for (i = 0; i < ARRAY_SIZE(full_range_coeff); i++)
 			rkisp1_write(params->rkisp1,
 				     RKISP1_CIF_ISP_CC_COEFF_0 + i * 4,
@@ -1562,11 +1562,7 @@ static void rkisp1_params_config_parameter(struct rkisp1_params *params)
 	rkisp1_param_set_bits(params, RKISP1_CIF_ISP_HIST_PROP_V10,
 			      rkisp1_hst_params_default_config.mode);
 
-	/* set the  range */
-	if (params->quantization == V4L2_QUANTIZATION_FULL_RANGE)
-		rkisp1_csm_config(params, true);
-	else
-		rkisp1_csm_config(params, false);
+	rkisp1_csm_config(params);
 
 	spin_lock_irq(&params->config_lock);
 
-- 
Regards,

Laurent Pinchart

[PATCH 7/7] media: rkisp1: Configure CSM based on YCbCr encoding

[Laurent Pinchart]

The driver currently only implements the Rec. 601 YCbCr encoding, extend
it with support for the other encodings defined by V4L2 (Rec. 709, Rec.
2020 and SMPTE240m). The coefficients have been calculated by rounding
the floating point values to the nearest Q1.7 fixed-point value,
adjusting the rounding to ensure that the sum of each line in the matrix
is preserved to avoid overflows.

At the hardware level, the RGB to YUV conversion matrix is fully
configurable, custom encoding could be supported by extending the ISP
parameters if desired.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../platform/rockchip/rkisp1/rkisp1-common.h  |  5 +-
 .../platform/rockchip/rkisp1/rkisp1-isp.c     |  3 +-
 .../platform/rockchip/rkisp1/rkisp1-params.c  | 97 +++++++++++++++----
 3 files changed, 84 insertions(+), 21 deletions(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
index 589999020a16..1383c13e22b8 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
@@ -393,6 +393,7 @@ struct rkisp1_params {
 	struct v4l2_format vdev_fmt;
 
 	enum v4l2_quantization quantization;
+	enum v4l2_ycbcr_encoding ycbcr_encoding;
 	enum rkisp1_fmt_raw_pat_type raw_type;
 };
 
@@ -595,10 +596,12 @@ const struct rkisp1_mbus_info *rkisp1_mbus_info_get_by_code(u32 mbus_code);
  * @params:	  pointer to rkisp1_params.
  * @bayer_pat:	  the bayer pattern on the isp video sink pad
  * @quantization: the quantization configured on the isp's src pad
+ * @ycbcr_encoding: the ycbcr_encoding configured on the isp's src pad
  */
 void rkisp1_params_configure(struct rkisp1_params *params,
 			     enum rkisp1_fmt_raw_pat_type bayer_pat,
-			     enum v4l2_quantization quantization);
+			     enum v4l2_quantization quantization,
+			     enum v4l2_ycbcr_encoding ycbcr_encoding);
 
 /* rkisp1_params_disable - disable all parameters.
  *			   This function is called by the isp entity upon stream start
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
index 9d4d018d58b6..c029d2e86717 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
@@ -344,7 +344,8 @@ static int rkisp1_config_isp(struct rkisp1_isp *isp,
 						 RKISP1_ISP_PAD_SOURCE_VIDEO,
 						 V4L2_SUBDEV_FORMAT_ACTIVE);
 		rkisp1_params_configure(&rkisp1->params, sink_fmt->bayer_pat,
-					src_frm->quantization);
+					src_frm->quantization,
+					src_frm->ycbcr_enc);
 	}
 
 	return 0;
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
index 163419624370..246a6faa1fc1 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
@@ -1078,37 +1078,94 @@ static void rkisp1_ie_enable(struct rkisp1_params *params, bool en)
 
 static void rkisp1_csm_config(struct rkisp1_params *params)
 {
-	static const u16 full_range_coeff[] = {
-		0x0026, 0x004b, 0x000f,
-		0x01ea, 0x01d6, 0x0040,
-		0x0040, 0x01ca, 0x01f6
+	struct csm_coeffs {
+		u16 limited[9];
+		u16 full[9];
 	};
-	static const u16 limited_range_coeff[] = {
-		0x0021, 0x0040, 0x000d,
-		0x01ed, 0x01db, 0x0038,
-		0x0038, 0x01d1, 0x01f7,
+	static const struct csm_coeffs rec601_coeffs = {
+		.limited = {
+			0x0021, 0x0042, 0x000d,
+			0x01ed, 0x01db, 0x0038,
+			0x0038, 0x01d1, 0x01f7,
+		},
+		.full = {
+			0x0026, 0x004b, 0x000f,
+			0x01ea, 0x01d6, 0x0040,
+			0x0040, 0x01ca, 0x01f6,
+		},
 	};
+	static const struct csm_coeffs rec709_coeffs = {
+		.limited = {
+			0x0018, 0x0050, 0x0008,
+			0x01f3, 0x01d5, 0x0038,
+			0x0038, 0x01cd, 0x01fb,
+		},
+		.full = {
+			0x001b, 0x005c, 0x0009,
+			0x01f1, 0x01cf, 0x0040,
+			0x0040, 0x01c6, 0x01fa,
+		},
+	};
+	static const struct csm_coeffs rec2020_coeffs = {
+		.limited = {
+			0x001d, 0x004c, 0x0007,
+			0x01f0, 0x01d8, 0x0038,
+			0x0038, 0x01cd, 0x01fb,
+		},
+		.full = {
+			0x0022, 0x0057, 0x0008,
+			0x01ee, 0x01d2, 0x0040,
+			0x0040, 0x01c5, 0x01fb,
+		},
+	};
+	static const struct csm_coeffs smpte240m_coeffs = {
+		.limited = {
+			0x0018, 0x004f, 0x000a,
+			0x01f3, 0x01d5, 0x0038,
+			0x0038, 0x01ce, 0x01fa,
+		},
+		.full = {
+			0x001b, 0x005a, 0x000b,
+			0x01f1, 0x01cf, 0x0040,
+			0x0040, 0x01c7, 0x01f9,
+		},
+	};
+
+	const struct csm_coeffs *coeffs;
+	const u16 *csm;
 	unsigned int i;
 
+	switch (params->ycbcr_encoding) {
+	case V4L2_YCBCR_ENC_601:
+	default:
+		coeffs = &rec601_coeffs;
+		break;
+	case V4L2_YCBCR_ENC_709:
+		coeffs = &rec709_coeffs;
+		break;
+	case V4L2_YCBCR_ENC_BT2020:
+		coeffs = &rec2020_coeffs;
+		break;
+	case V4L2_YCBCR_ENC_SMPTE240M:
+		coeffs = &smpte240m_coeffs;
+		break;
+	}
+
 	if (params->quantization == V4L2_QUANTIZATION_FULL_RANGE) {
-		for (i = 0; i < ARRAY_SIZE(full_range_coeff); i++)
-			rkisp1_write(params->rkisp1,
-				     RKISP1_CIF_ISP_CC_COEFF_0 + i * 4,
-				     full_range_coeff[i]);
-
+		csm = coeffs->full;
 		rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL,
 				      RKISP1_CIF_ISP_CTRL_ISP_CSM_Y_FULL_ENA |
 				      RKISP1_CIF_ISP_CTRL_ISP_CSM_C_FULL_ENA);
 	} else {
-		for (i = 0; i < ARRAY_SIZE(limited_range_coeff); i++)
-			rkisp1_write(params->rkisp1,
-				     RKISP1_CIF_ISP_CC_COEFF_0 + i * 4,
-				     limited_range_coeff[i]);
-
+		csm = coeffs->limited;
 		rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL,
 					RKISP1_CIF_ISP_CTRL_ISP_CSM_Y_FULL_ENA |
 					RKISP1_CIF_ISP_CTRL_ISP_CSM_C_FULL_ENA);
 	}
+
+	for (i = 0; i < 9; i++)
+		rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_CC_COEFF_0 + i * 4,
+			     csm[i]);
 }
 
 /* ISP De-noise Pre-Filter(DPF) function */
@@ -1574,9 +1631,11 @@ static void rkisp1_params_config_parameter(struct rkisp1_params *params)
 
 void rkisp1_params_configure(struct rkisp1_params *params,
 			     enum rkisp1_fmt_raw_pat_type bayer_pat,
-			     enum v4l2_quantization quantization)
+			     enum v4l2_quantization quantization,
+			     enum v4l2_ycbcr_encoding ycbcr_encoding)
 {
 	params->quantization = quantization;
+	params->ycbcr_encoding = ycbcr_encoding;
 	params->raw_type = bayer_pat;
 	rkisp1_params_config_parameter(params);
 }
-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/7] media: rkisp1: Initialize color space on sink and source pads

[Dafna Hirschfeld]

On 15.08.2022 09:52, Laurent Pinchart wrote:
>Initialize the four color space fields on the sink and source
>video pads in the .init_cfg() operation. As the main use case for the
>ISP is to convert Bayer data to YUV, select a raw color space on the
>sink pad and a limited range quantization of SYCC on the source pad by
>default.
>
>Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Dafna Hirschfeld <dafna@fastmail.com>

>---
> drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
>diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>index a3c7d4d88387..a52b22824739 100644
>--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>@@ -553,12 +553,17 @@ static int rkisp1_isp_init_config(struct v4l2_subdev *sd,
> 	struct v4l2_mbus_framefmt *sink_fmt, *src_fmt;
> 	struct v4l2_rect *sink_crop, *src_crop;
>
>+	/* Video. */
> 	sink_fmt = v4l2_subdev_get_try_format(sd, sd_state,
> 					      RKISP1_ISP_PAD_SINK_VIDEO);
> 	sink_fmt->width = RKISP1_DEFAULT_WIDTH;
> 	sink_fmt->height = RKISP1_DEFAULT_HEIGHT;
> 	sink_fmt->field = V4L2_FIELD_NONE;
> 	sink_fmt->code = RKISP1_DEF_SINK_PAD_FMT;
>+	sink_fmt->colorspace = V4L2_COLORSPACE_RAW;
>+	sink_fmt->xfer_func = V4L2_XFER_FUNC_NONE;
>+	sink_fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
>+	sink_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>
> 	sink_crop = v4l2_subdev_get_try_crop(sd, sd_state,
> 					     RKISP1_ISP_PAD_SINK_VIDEO);
>@@ -571,11 +576,16 @@ static int rkisp1_isp_init_config(struct v4l2_subdev *sd,
> 					     RKISP1_ISP_PAD_SOURCE_VIDEO);
> 	*src_fmt = *sink_fmt;
> 	src_fmt->code = RKISP1_DEF_SRC_PAD_FMT;
>+	src_fmt->colorspace = V4L2_COLORSPACE_SRGB;
>+	src_fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
>+	src_fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
>+	src_fmt->quantization = V4L2_QUANTIZATION_LIM_RANGE;
>
> 	src_crop = v4l2_subdev_get_try_crop(sd, sd_state,
> 					    RKISP1_ISP_PAD_SOURCE_VIDEO);
> 	*src_crop = *sink_crop;
>
>+	/* Parameters and statistics. */
> 	sink_fmt = v4l2_subdev_get_try_format(sd, sd_state,
> 					      RKISP1_ISP_PAD_SINK_PARAMS);
> 	src_fmt = v4l2_subdev_get_try_format(sd, sd_state,
>-- 
>Regards,
>
>Laurent Pinchart
>

Re: [PATCH 2/7] media: rkisp1: Allow setting color space on ISP sink pad

[Dafna Hirschfeld]

On 15.08.2022 09:52, Laurent Pinchart wrote:
>The ISP accepts different color spaces on its input: for YUV input, it
>doesn't set any restrictions, and for Bayer inputs, any color primaries
>or transfer function can be accepted (YCbCr encoding isn't applicable
>there, and quantization range can only be full).
>
>Allow setting a color space on the ISP sink pad, with the aforementioned
>restrictions. The settings don't influence hardware yet (only the YUV
>quantization range will, anything else has no direct effect on the ISP
>configuration), but can already be set to allow color space information
>to be coherent across the ISP sink link.
>
>Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>---
> .../platform/rockchip/rkisp1/rkisp1-isp.c     | 31 +++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
>diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>index a52b22824739..32114d1e8ad1 100644
>--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>@@ -705,6 +705,7 @@ static void rkisp1_isp_set_sink_fmt(struct rkisp1_isp *isp,
> 	const struct rkisp1_mbus_info *mbus_info;
> 	struct v4l2_mbus_framefmt *sink_fmt;
> 	struct v4l2_rect *sink_crop;
>+	bool is_yuv;
>
> 	sink_fmt = rkisp1_isp_get_pad_fmt(isp, sd_state,
> 					  RKISP1_ISP_PAD_SINK_VIDEO,
>@@ -725,6 +726,36 @@ static void rkisp1_isp_set_sink_fmt(struct rkisp1_isp *isp,
> 				   RKISP1_ISP_MIN_HEIGHT,
> 				   RKISP1_ISP_MAX_HEIGHT);
>
>+	/*
>+	 * Adjust the color space fields. Accept any color primaries and
>+	 * transfer function for both YUV and Bayer. For YUV any YCbCr encoding
>+	 * and quantization range is also accepted. For Bayer formats, the YCbCr
>+	 * encoding isn't applicable, and the quantization range can only be
>+	 * full.
>+	 */
>+	is_yuv = mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV;
>+
>+	sink_fmt->colorspace = format->colorspace ? format->colorspace :
>+			       (is_yuv ? V4L2_COLORSPACE_RAW :

I don't understand enough of the different colorspaces, why is 'raw' chosen for yuv input?

Thanks,
Dafna

>+				V4L2_COLORSPACE_SRGB);
>+	sink_fmt->xfer_func = format->xfer_func ? format->xfer_func :
>+			      V4L2_MAP_XFER_FUNC_DEFAULT(sink_fmt->colorspace);
>+	if (is_yuv) {
>+		sink_fmt->ycbcr_enc = format->ycbcr_enc ? :
>+			V4L2_MAP_YCBCR_ENC_DEFAULT(sink_fmt->colorspace);
>+		sink_fmt->quantization = format->quantization ? :
>+			V4L2_MAP_QUANTIZATION_DEFAULT(false, sink_fmt->colorspace,
>+						      sink_fmt->ycbcr_enc);
>+	} else {
>+		/*
>+		 * The YCbCr encoding isn't applicable for non-YUV formats, but
>+		 * V4L2 has no "no encoding" value. Hardcode it to Rec. 601, it
>+		 * should be ignored by userspace.
>+		 */
>+		sink_fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
>+		sink_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>+	}
>+
> 	*format = *sink_fmt;
>
> 	/* Propagate to in crop */
>-- 
>Regards,
>
>Laurent Pinchart
>

Re: [PATCH 3/7] media: rkisp1: Fix source pad format configuration

[Dafna Hirschfeld]

On 15.08.2022 09:52, Laurent Pinchart wrote:
>The ISP converts Bayer data to YUV when operating normally, and can also
>operate in pass-through mode where the input and output formats must
>match. Converting from YUV to Bayer isn't possible. If such an invalid
>configuration is attempted, adjust it by copying the sink pad media bus
>code to the source pad.
>
>Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Dafna Hirschfeld <dafna@fastmail.com>

>---
> .../platform/rockchip/rkisp1/rkisp1-isp.c     | 40 +++++++++++++++----
> 1 file changed, 32 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>index 32114d1e8ad1..0441ccbc01a9 100644
>--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>@@ -604,23 +604,43 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
> 				   struct v4l2_mbus_framefmt *format,
> 				   unsigned int which)
> {
>-	const struct rkisp1_mbus_info *mbus_info;
>+	const struct rkisp1_mbus_info *sink_info;
>+	const struct rkisp1_mbus_info *src_info;
>+	struct v4l2_mbus_framefmt *sink_fmt;
> 	struct v4l2_mbus_framefmt *src_fmt;
> 	const struct v4l2_rect *src_crop;
>
>+	sink_fmt = rkisp1_isp_get_pad_fmt(isp, sd_state,
>+					  RKISP1_ISP_PAD_SINK_VIDEO, which);
> 	src_fmt = rkisp1_isp_get_pad_fmt(isp, sd_state,
> 					 RKISP1_ISP_PAD_SOURCE_VIDEO, which);
> 	src_crop = rkisp1_isp_get_pad_crop(isp, sd_state,
> 					   RKISP1_ISP_PAD_SOURCE_VIDEO, which);
>
>+	/*
>+	 * Media bus code. The ISP can operate in pass-through mode (Bayer in,
>+	 * Bayer out or YUV in, YUV out) or process Bayer data to YUV, but
>+	 * can't convert from YUV to Bayer.
>+	 */
>+	sink_info = rkisp1_mbus_info_get_by_code(sink_fmt->code);
>+
> 	src_fmt->code = format->code;
>-	mbus_info = rkisp1_mbus_info_get_by_code(src_fmt->code);
>-	if (!mbus_info || !(mbus_info->direction & RKISP1_ISP_SD_SRC)) {
>+	src_info = rkisp1_mbus_info_get_by_code(src_fmt->code);
>+	if (!src_info || !(src_info->direction & RKISP1_ISP_SD_SRC)) {
> 		src_fmt->code = RKISP1_DEF_SRC_PAD_FMT;
>-		mbus_info = rkisp1_mbus_info_get_by_code(src_fmt->code);
>+		src_info = rkisp1_mbus_info_get_by_code(src_fmt->code);
> 	}
>-	if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
>-		isp->src_fmt = mbus_info;
>+
>+	if (sink_info->pixel_enc == V4L2_PIXEL_ENC_YUV &&
>+	    src_info->pixel_enc == V4L2_PIXEL_ENC_BAYER) {
>+		src_fmt->code = sink_fmt->code;
>+		src_info = sink_info;
>+	}
>+
>+	/*
>+	 * The source width and height must be identical to the source crop
>+	 * size.
>+	 */
> 	src_fmt->width  = src_crop->width;
> 	src_fmt->height = src_crop->height;
>
>@@ -630,14 +650,18 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
> 	 */
> 	if (format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC &&
> 	    format->quantization == V4L2_QUANTIZATION_FULL_RANGE &&
>-	    mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV)
>+	    src_info->pixel_enc == V4L2_PIXEL_ENC_YUV)
> 		src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>-	else if (mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV)
>+	else if (src_info->pixel_enc == V4L2_PIXEL_ENC_YUV)
> 		src_fmt->quantization = V4L2_QUANTIZATION_LIM_RANGE;
> 	else
> 		src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>
> 	*format = *src_fmt;
>+
>+	/* Store the source format info when setting the active format. */
>+	if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
>+		isp->src_fmt = src_info;
> }
>
> static void rkisp1_isp_set_src_crop(struct rkisp1_isp *isp,
>-- 
>Regards,
>
>Laurent Pinchart
>

Re: [PATCH 5/7] media: rkisp1: Configure quantization using ISP source pad

[Dafna Hirschfeld]

On 15.08.2022 09:52, Laurent Pinchart wrote:
>The rkisp1_config_isp() function uses the format on the sink pad of the
>ISP to configure quantization at the output of the ISP. This is
>incorrect, as hinted by the src_frm variable name that stores the
>format. Fix it by using the source pad.
>
>Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Dafna Hirschfeld <dafna@fastmail.com>

>---
> drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>index 8b93b5c03bce..9d4d018d58b6 100644
>--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>@@ -341,7 +341,7 @@ static int rkisp1_config_isp(struct rkisp1_isp *isp,
> 		struct v4l2_mbus_framefmt *src_frm;
>
> 		src_frm = rkisp1_isp_get_pad_fmt(isp, NULL,
>-						 RKISP1_ISP_PAD_SINK_VIDEO,
>+						 RKISP1_ISP_PAD_SOURCE_VIDEO,
> 						 V4L2_SUBDEV_FORMAT_ACTIVE);
> 		rkisp1_params_configure(&rkisp1->params, sink_fmt->bayer_pat,
> 					src_frm->quantization);
>-- 
>Regards,
>
>Laurent Pinchart
>

Re: [PATCH 6/7] media: rkisp1: Don't pass the quantization to rkisp1_csm_config()

[Dafna Hirschfeld]

On 15.08.2022 09:52, Laurent Pinchart wrote:
>The rkisp1_csm_config() function takes a pointer to the rkisp1_params
>structure which contains the quantization value. There's no need to pass
>it separately to the function. Drop it from the function parameters.
>
>Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Dafna Hirschfeld <dafna@fastmail.com>

>---
> drivers/media/platform/rockchip/rkisp1/rkisp1-params.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
>index 8b4eea77af0d..163419624370 100644
>--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
>+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
>@@ -1076,7 +1076,7 @@ static void rkisp1_ie_enable(struct rkisp1_params *params, bool en)
> 	}
> }
>
>-static void rkisp1_csm_config(struct rkisp1_params *params, bool full_range)
>+static void rkisp1_csm_config(struct rkisp1_params *params)
> {
> 	static const u16 full_range_coeff[] = {
> 		0x0026, 0x004b, 0x000f,
>@@ -1090,7 +1090,7 @@ static void rkisp1_csm_config(struct rkisp1_params *params, bool full_range)
> 	};
> 	unsigned int i;
>
>-	if (full_range) {
>+	if (params->quantization == V4L2_QUANTIZATION_FULL_RANGE) {
> 		for (i = 0; i < ARRAY_SIZE(full_range_coeff); i++)
> 			rkisp1_write(params->rkisp1,
> 				     RKISP1_CIF_ISP_CC_COEFF_0 + i * 4,
>@@ -1562,11 +1562,7 @@ static void rkisp1_params_config_parameter(struct rkisp1_params *params)
> 	rkisp1_param_set_bits(params, RKISP1_CIF_ISP_HIST_PROP_V10,
> 			      rkisp1_hst_params_default_config.mode);
>
>-	/* set the  range */
>-	if (params->quantization == V4L2_QUANTIZATION_FULL_RANGE)
>-		rkisp1_csm_config(params, true);
>-	else
>-		rkisp1_csm_config(params, false);
>+	rkisp1_csm_config(params);
>
> 	spin_lock_irq(&params->config_lock);
>
>-- 
>Regards,
>
>Laurent Pinchart
>

Re: [PATCH 2/7] media: rkisp1: Allow setting color space on ISP sink pad

[Laurent Pinchart]

Hi Dafna,

On Thu, Aug 18, 2022 at 07:00:27AM +0300, Dafna Hirschfeld wrote:
> On 15.08.2022 09:52, Laurent Pinchart wrote:
> > The ISP accepts different color spaces on its input: for YUV input, it
> > doesn't set any restrictions, and for Bayer inputs, any color primaries
> > or transfer function can be accepted (YCbCr encoding isn't applicable
> > there, and quantization range can only be full).
> > 
> > Allow setting a color space on the ISP sink pad, with the aforementioned
> > restrictions. The settings don't influence hardware yet (only the YUV
> > quantization range will, anything else has no direct effect on the ISP
> > configuration), but can already be set to allow color space information
> > to be coherent across the ISP sink link.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  .../platform/rockchip/rkisp1/rkisp1-isp.c     | 31 +++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> > 
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > index a52b22824739..32114d1e8ad1 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > @@ -705,6 +705,7 @@ static void rkisp1_isp_set_sink_fmt(struct rkisp1_isp *isp,
> >  	const struct rkisp1_mbus_info *mbus_info;
> >  	struct v4l2_mbus_framefmt *sink_fmt;
> >  	struct v4l2_rect *sink_crop;
> > +	bool is_yuv;
> > 
> >  	sink_fmt = rkisp1_isp_get_pad_fmt(isp, sd_state,
> >  					  RKISP1_ISP_PAD_SINK_VIDEO,
> > @@ -725,6 +726,36 @@ static void rkisp1_isp_set_sink_fmt(struct rkisp1_isp *isp,
> >  				   RKISP1_ISP_MIN_HEIGHT,
> >  				   RKISP1_ISP_MAX_HEIGHT);
> > 
> > +	/*
> > +	 * Adjust the color space fields. Accept any color primaries and
> > +	 * transfer function for both YUV and Bayer. For YUV any YCbCr encoding
> > +	 * and quantization range is also accepted. For Bayer formats, the YCbCr
> > +	 * encoding isn't applicable, and the quantization range can only be
> > +	 * full.
> > +	 */
> > +	is_yuv = mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV;
> > +
> > +	sink_fmt->colorspace = format->colorspace ? format->colorspace :
> > +			       (is_yuv ? V4L2_COLORSPACE_RAW :
> 
> I don't understand enough of the different colorspaces, why is 'raw'
> chosen for yuv input?

You clearly understand the topic as you've spotted a bug :-) It should
be the other way around, for Bayer input the driver should default to
RAW, and for YUV input, to SRGB (which in V4L2 is used to describe
limited-range, Rec. 601 encoded sRGB RGB data).

> > +				V4L2_COLORSPACE_SRGB);
> > +	sink_fmt->xfer_func = format->xfer_func ? format->xfer_func :
> > +			      V4L2_MAP_XFER_FUNC_DEFAULT(sink_fmt->colorspace);
> > +	if (is_yuv) {
> > +		sink_fmt->ycbcr_enc = format->ycbcr_enc ? :
> > +			V4L2_MAP_YCBCR_ENC_DEFAULT(sink_fmt->colorspace);
> > +		sink_fmt->quantization = format->quantization ? :
> > +			V4L2_MAP_QUANTIZATION_DEFAULT(false, sink_fmt->colorspace,
> > +						      sink_fmt->ycbcr_enc);
> > +	} else {
> > +		/*
> > +		 * The YCbCr encoding isn't applicable for non-YUV formats, but
> > +		 * V4L2 has no "no encoding" value. Hardcode it to Rec. 601, it
> > +		 * should be ignored by userspace.
> > +		 */
> > +		sink_fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
> > +		sink_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > +	}
> > +
> >  	*format = *sink_fmt;
> > 
> >  	/* Propagate to in crop */

-- 
Regards,

Laurent Pinchart