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

[PATCH v2 0/9] 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/9 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/9 mimicks patch 2/9 by allowing setting of
color space fields on the source pad.

The next three patches configure the RGB to YUV matrix (in the CSM
module) using the ISP source pad YCbCr encoding. Patch 5/9 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/9 is a small internal API refactoring, and finally
patch 7/9 handles the CSM configuration.

Patches 8/9 and 9/9 mimick 1/9 and 2/9 for the resizer subdevs, allowing
propagation of the color space along the pipeline.

If anyone is curious about how the matrix coefficients of patch 7/9 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 (9):
  media: rkisp1: Initialize color space on ISP 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
  media: rkisp1: Initialize color space on resizer sink and source pads
  media: rkisp1: Allow setting color space on resizer sink pad

 .../platform/rockchip/rkisp1/rkisp1-common.h  |   5 +-
 .../platform/rockchip/rkisp1/rkisp1-isp.c     | 139 ++++++++++++++++--
 .../platform/rockchip/rkisp1/rkisp1-params.c  | 105 +++++++++----
 .../platform/rockchip/rkisp1/rkisp1-resizer.c |  45 +++++-
 4 files changed, 249 insertions(+), 45 deletions(-)

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Initialize the four color space fields on the sink and source video pads
of the ISP 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>
---
Changes since v1:

- Mention ISP in the subject line
---
 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 v2 1/9] media: rkisp1: Initialize color space on ISP sink and source pads

[paul.elder]

On Tue, Aug 23, 2022 at 08:18:32PM +0300, Laurent Pinchart wrote:
> Initialize the four color space fields on the sink and source video pads
> of the ISP 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>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
> Changes since v1:
> 
> - Mention ISP in the subject line
> ---
>  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,

[PATCH v2 2/9] 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>
---
Changes since v1:

- Fix swapped default color spaces for YUV and Bayer
- Improve coherency in usage of ternary operator ? :
---
 .../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..b5bdf427c7e1 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 ? :
+			       (is_yuv ? V4L2_COLORSPACE_SRGB :
+				V4L2_COLORSPACE_RAW);
+	sink_fmt->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 v2 2/9] media: rkisp1: Allow setting color space on ISP sink pad

[paul.elder]

On Tue, Aug 23, 2022 at 08:18:33PM +0300, 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>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
> Changes since v1:
> 
> - Fix swapped default color spaces for YUV and Bayer
> - Improve coherency in usage of ternary operator ? :
> ---
>  .../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..b5bdf427c7e1 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 ? :
> +			       (is_yuv ? V4L2_COLORSPACE_SRGB :
> +				V4L2_COLORSPACE_RAW);
> +	sink_fmt->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 */

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

[Dafna Hirschfeld]

On 23.08.2022 20:18, 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>

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

>---
>Changes since v1:
>
>- Fix swapped default color spaces for YUV and Bayer
>- Improve coherency in usage of ternary operator ? :
>---
> .../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..b5bdf427c7e1 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 ? :
>+			       (is_yuv ? V4L2_COLORSPACE_SRGB :
>+				V4L2_COLORSPACE_RAW);
>+	sink_fmt->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 v2 3/9] 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>
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 b5bdf427c7e1..d34f32271d74 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 v2 3/9] media: rkisp1: Fix source pad format configuration

[paul.elder]

On Tue, Aug 23, 2022 at 08:18:34PM +0300, 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>

Reviewed-by: Paul Elder <paul.elder@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 b5bdf427c7e1..d34f32271d74 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,

[PATCH v2 4/9] 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>
---
Changes since v1:

- Fix quantization setting that was set on sink pad by mistake
---
 .../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 d34f32271d74..7869f0cced62 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)
+			src_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

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

[paul.elder]

On Tue, Aug 23, 2022 at 08:18:35PM +0300, Laurent Pinchart wrote:
> 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>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
> Changes since v1:
> 
> - Fix quantization setting that was set on sink pad by mistake
> ---
>  .../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 d34f32271d74..7869f0cced62 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)
> +			src_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;

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

[Dafna Hirschfeld]

On 23.08.2022 20:18, Laurent Pinchart wrote:
>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>
>---
>Changes since v1:
>
>- Fix quantization setting that was set on sink pad by mistake
>---
> .../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 d34f32271d74..7869f0cced62 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);

the (!!) operator is used to convert boolean to interger rigth?
I think it it not needed here, since 'set_csc' is only used in 'if' conditions

anyway

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

Thanks,
Danfa

>+
>+	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)
>+			src_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
>

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

[Laurent Pinchart]

On Sat, Sep 03, 2022 at 06:35:13AM +0300, Dafna Hirschfeld wrote:
> On 23.08.2022 20:18, Laurent Pinchart wrote:
> > 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>
> > ---
> > Changes since v1:
> > 
> > - Fix quantization setting that was set on sink pad by mistake
> > ---
> >  .../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 d34f32271d74..7869f0cced62 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);
> 
> the (!!) operator is used to convert boolean to interger rigth?

Here it's used to convert an integert to a boolean. The bool type is
stored on one byte, so if the V4L2_MBUS_FRAMEFMT_SET_CSC flag used bit 8
or higher, there would be a risk the result would overflow.

However, given that the bool type is an alias for _Bool, the compiler
will correctly convert any non-zero value to 1. The following code
explains it better:

#include <stdbool.h>
#include <stdint.h>
#include <stdio.h>

int main(int argc __attribute__((__unused__)),
         char *argv[] __attribute__((__unused__)))
{
	bool b;
	uint8_t u8;
	uint8_t u8_notnot;

	b = 0x1ff & 0x100;
	u8 = 0x1ff & 0x100;
	u8_notnot = !!(0x1ff & 0x100);

	printf("b = %u, u8 = %u, u8_notnot = %u\n", b, u8, u8_notnot);

	return 0;
}

$ gcc -W -Wall -o bool bool.c
bool.c: In function ‘main’:
bool.c:13:14: warning: unsigned conversion from ‘int’ to ‘uint8_t’ {aka ‘unsigned char’} changes value from ‘256’ to ‘0’ [-Woverflow]
   13 |         u8 = 0x1ff & 0x100;
      |              ^~~~~
$ ./bool 
b = 1, u8 = 0, u8_notnot = 1


I'll drop the !!.

> I think it it not needed here, since 'set_csc' is only used in 'if' conditions
> 
> anyway
> 
> Reviewed-by: Dafna Hirschfeld <dafna@fastmail.com>
> 
> > +
> > +	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)
> > +			src_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 v2 5/9] 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>
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 7869f0cced62..babf88066c2e 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 v2 5/9] media: rkisp1: Configure quantization using ISP source pad

[paul.elder]

On Tue, Aug 23, 2022 at 08:18:36PM +0300, 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>

Reviewed-by: Paul Elder <paul.elder@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 7869f0cced62..babf88066c2e 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);

[PATCH v2 6/9] 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>
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 v2 6/9] media: rkisp1: Don't pass the quantization to rkisp1_csm_config()

[paul.elder]

On Tue, Aug 23, 2022 at 08:18:37PM +0300, 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>

Reviewed-by: Paul Elder <paul.elder@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);

[PATCH v2 7/9] 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>
Reviewed-by: Dafna Hirschfeld <dafna@fastmail.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 babf88066c2e..20c01e0e2e17 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 v2 7/9] media: rkisp1: Configure CSM based on YCbCr encoding

[paul.elder]

On Tue, Aug 23, 2022 at 08:18:38PM +0300, Laurent Pinchart wrote:
> 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>
> Reviewed-by: Dafna Hirschfeld <dafna@fastmail.com>

Reviewed-by: Paul Elder <paul.elder@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 babf88066c2e..20c01e0e2e17 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);
>  }

[PATCH v2 8/9] media: rkisp1: Initialize color space on resizer sink and source pads

[Laurent Pinchart]

Initialize the four color space fields on the sink and source video pads
of the resizer in the .init_cfg() operation. The resizer can't perform
any color space conversion, so set the sink and source color spaces to
the same defaults, which match the ISP source video pad default.

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

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
index becef04fdf2d..6f6ec00b63b8 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
@@ -430,6 +430,10 @@ static int rkisp1_rsz_init_config(struct v4l2_subdev *sd,
 	sink_fmt->height = RKISP1_DEFAULT_HEIGHT;
 	sink_fmt->field = V4L2_FIELD_NONE;
 	sink_fmt->code = RKISP1_DEF_FMT;
+	sink_fmt->colorspace = V4L2_COLORSPACE_SRGB;
+	sink_fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
+	sink_fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
+	sink_fmt->quantization = V4L2_QUANTIZATION_LIM_RANGE;
 
 	sink_crop = v4l2_subdev_get_try_crop(sd, sd_state,
 					     RKISP1_RSZ_PAD_SINK);
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 8/9] media: rkisp1: Initialize color space on resizer sink and source pads

[paul.elder]

On Tue, Aug 23, 2022 at 08:18:39PM +0300, Laurent Pinchart wrote:
> Initialize the four color space fields on the sink and source video pads
> of the resizer in the .init_cfg() operation. The resizer can't perform
> any color space conversion, so set the sink and source color spaces to
> the same defaults, which match the ISP source video pad default.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
> index becef04fdf2d..6f6ec00b63b8 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
> @@ -430,6 +430,10 @@ static int rkisp1_rsz_init_config(struct v4l2_subdev *sd,
>  	sink_fmt->height = RKISP1_DEFAULT_HEIGHT;
>  	sink_fmt->field = V4L2_FIELD_NONE;
>  	sink_fmt->code = RKISP1_DEF_FMT;
> +	sink_fmt->colorspace = V4L2_COLORSPACE_SRGB;
> +	sink_fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
> +	sink_fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
> +	sink_fmt->quantization = V4L2_QUANTIZATION_LIM_RANGE;
>  
>  	sink_crop = v4l2_subdev_get_try_crop(sd, sd_state,
>  					     RKISP1_RSZ_PAD_SINK);

Re: [PATCH v2 8/9] media: rkisp1: Initialize color space on resizer sink and source pads

[Dafna Hirschfeld]

On 23.08.2022 20:18, Laurent Pinchart wrote:
>Initialize the four color space fields on the sink and source video pads
>of the resizer in the .init_cfg() operation. The resizer can't perform
>any color space conversion, so set the sink and source color spaces to
>the same defaults, which match the ISP source video pad default.
>
>Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>


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

>---
> drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
>index becef04fdf2d..6f6ec00b63b8 100644
>--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
>+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
>@@ -430,6 +430,10 @@ static int rkisp1_rsz_init_config(struct v4l2_subdev *sd,
> 	sink_fmt->height = RKISP1_DEFAULT_HEIGHT;
> 	sink_fmt->field = V4L2_FIELD_NONE;
> 	sink_fmt->code = RKISP1_DEF_FMT;
>+	sink_fmt->colorspace = V4L2_COLORSPACE_SRGB;
>+	sink_fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
>+	sink_fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
>+	sink_fmt->quantization = V4L2_QUANTIZATION_LIM_RANGE;
>
> 	sink_crop = v4l2_subdev_get_try_crop(sd, sd_state,
> 					     RKISP1_RSZ_PAD_SINK);
>-- 
>Regards,
>
>Laurent Pinchart
>

[PATCH v2 9/9] media: rkisp1: Allow setting color space on resizer sink pad

[Laurent Pinchart]

The resizer doesn't deal with color spaces, so it can accept any color
space on its input, and propagates it unchanged to its output. When
operating with a Bayer input format (in pass-through mode) further
restrict the YCbCr encoding and quantization to Rec 601 and full range
respectively, as for raw data the former ought to be ignored and the
latter is always full range.

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

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
index 6f6ec00b63b8..891a622124e2 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
@@ -526,6 +526,7 @@ static void rkisp1_rsz_set_sink_fmt(struct rkisp1_resizer *rsz,
 	const struct rkisp1_mbus_info *mbus_info;
 	struct v4l2_mbus_framefmt *sink_fmt, *src_fmt;
 	struct v4l2_rect *sink_crop;
+	bool is_yuv;
 
 	sink_fmt = rkisp1_rsz_get_pad_fmt(rsz, sd_state, RKISP1_RSZ_PAD_SINK,
 					  which);
@@ -547,9 +548,6 @@ static void rkisp1_rsz_set_sink_fmt(struct rkisp1_resizer *rsz,
 	if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
 		rsz->pixel_enc = mbus_info->pixel_enc;
 
-	/* Propagete to source pad */
-	src_fmt->code = sink_fmt->code;
-
 	sink_fmt->width = clamp_t(u32, format->width,
 				  RKISP1_ISP_MIN_WIDTH,
 				  RKISP1_ISP_MAX_WIDTH);
@@ -557,8 +555,45 @@ static void rkisp1_rsz_set_sink_fmt(struct rkisp1_resizer *rsz,
 				   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 ? :
+			       (is_yuv ? V4L2_COLORSPACE_SRGB :
+				V4L2_COLORSPACE_RAW);
+	sink_fmt->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 the media bus code and color space to the source pad. */
+	src_fmt->code = sink_fmt->code;
+	src_fmt->colorspace = sink_fmt->colorspace;
+	src_fmt->xfer_func = sink_fmt->xfer_func;
+	src_fmt->ycbcr_enc = sink_fmt->ycbcr_enc;
+	src_fmt->quantization = sink_fmt->quantization;
+
 	/* Update sink crop */
 	rkisp1_rsz_set_sink_crop(rsz, sd_state, sink_crop, which);
 }
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 9/9] media: rkisp1: Allow setting color space on resizer sink pad

[paul.elder]

On Tue, Aug 23, 2022 at 08:18:40PM +0300, Laurent Pinchart wrote:
> The resizer doesn't deal with color spaces, so it can accept any color
> space on its input, and propagates it unchanged to its output. When
> operating with a Bayer input format (in pass-through mode) further
> restrict the YCbCr encoding and quantization to Rec 601 and full range
> respectively, as for raw data the former ought to be ignored and the
> latter is always full range.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  .../platform/rockchip/rkisp1/rkisp1-resizer.c | 41 +++++++++++++++++--
>  1 file changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
> index 6f6ec00b63b8..891a622124e2 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
> @@ -526,6 +526,7 @@ static void rkisp1_rsz_set_sink_fmt(struct rkisp1_resizer *rsz,
>  	const struct rkisp1_mbus_info *mbus_info;
>  	struct v4l2_mbus_framefmt *sink_fmt, *src_fmt;
>  	struct v4l2_rect *sink_crop;
> +	bool is_yuv;
>  
>  	sink_fmt = rkisp1_rsz_get_pad_fmt(rsz, sd_state, RKISP1_RSZ_PAD_SINK,
>  					  which);
> @@ -547,9 +548,6 @@ static void rkisp1_rsz_set_sink_fmt(struct rkisp1_resizer *rsz,
>  	if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
>  		rsz->pixel_enc = mbus_info->pixel_enc;
>  
> -	/* Propagete to source pad */
> -	src_fmt->code = sink_fmt->code;
> -
>  	sink_fmt->width = clamp_t(u32, format->width,
>  				  RKISP1_ISP_MIN_WIDTH,
>  				  RKISP1_ISP_MAX_WIDTH);
> @@ -557,8 +555,45 @@ static void rkisp1_rsz_set_sink_fmt(struct rkisp1_resizer *rsz,
>  				   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 ? :
> +			       (is_yuv ? V4L2_COLORSPACE_SRGB :
> +				V4L2_COLORSPACE_RAW);
> +	sink_fmt->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 the media bus code and color space to the source pad. */
> +	src_fmt->code = sink_fmt->code;
> +	src_fmt->colorspace = sink_fmt->colorspace;
> +	src_fmt->xfer_func = sink_fmt->xfer_func;
> +	src_fmt->ycbcr_enc = sink_fmt->ycbcr_enc;
> +	src_fmt->quantization = sink_fmt->quantization;
> +
>  	/* Update sink crop */
>  	rkisp1_rsz_set_sink_crop(rsz, sd_state, sink_crop, which);
>  }

Re: [PATCH v2 9/9] media: rkisp1: Allow setting color space on resizer sink pad

[Dafna Hirschfeld]

On 23.08.2022 20:18, Laurent Pinchart wrote:
>The resizer doesn't deal with color spaces, so it can accept any color
>space on its input, and propagates it unchanged to its output. When
>operating with a Bayer input format (in pass-through mode) further
>restrict the YCbCr encoding and quantization to Rec 601 and full range
>respectively, as for raw data the former ought to be ignored and the
>latter is always full range.
>
>Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>


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

>---
> .../platform/rockchip/rkisp1/rkisp1-resizer.c | 41 +++++++++++++++++--
> 1 file changed, 38 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
>index 6f6ec00b63b8..891a622124e2 100644
>--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
>+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
>@@ -526,6 +526,7 @@ static void rkisp1_rsz_set_sink_fmt(struct rkisp1_resizer *rsz,
> 	const struct rkisp1_mbus_info *mbus_info;
> 	struct v4l2_mbus_framefmt *sink_fmt, *src_fmt;
> 	struct v4l2_rect *sink_crop;
>+	bool is_yuv;
>
> 	sink_fmt = rkisp1_rsz_get_pad_fmt(rsz, sd_state, RKISP1_RSZ_PAD_SINK,
> 					  which);
>@@ -547,9 +548,6 @@ static void rkisp1_rsz_set_sink_fmt(struct rkisp1_resizer *rsz,
> 	if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
> 		rsz->pixel_enc = mbus_info->pixel_enc;
>
>-	/* Propagete to source pad */
>-	src_fmt->code = sink_fmt->code;
>-
> 	sink_fmt->width = clamp_t(u32, format->width,
> 				  RKISP1_ISP_MIN_WIDTH,
> 				  RKISP1_ISP_MAX_WIDTH);
>@@ -557,8 +555,45 @@ static void rkisp1_rsz_set_sink_fmt(struct rkisp1_resizer *rsz,
> 				   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 ? :
>+			       (is_yuv ? V4L2_COLORSPACE_SRGB :
>+				V4L2_COLORSPACE_RAW);
>+	sink_fmt->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 the media bus code and color space to the source pad. */
>+	src_fmt->code = sink_fmt->code;
>+	src_fmt->colorspace = sink_fmt->colorspace;
>+	src_fmt->xfer_func = sink_fmt->xfer_func;
>+	src_fmt->ycbcr_enc = sink_fmt->ycbcr_enc;
>+	src_fmt->quantization = sink_fmt->quantization;
>+
> 	/* Update sink crop */
> 	rkisp1_rsz_set_sink_crop(rsz, sd_state, sink_crop, which);
> }
>-- 
>Regards,
>
>Laurent Pinchart
>