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@xxxxxxxxxxxxxxxx> > > --- > > 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@xxxxxxxxxxxx> > > > + > > + 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