| Message ID | 20220823171840.8958-5-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | c1ec5efba08079820b0bfc5149891f364934440d |
| Headers | show |
| Series | media: rkisp1: Fix and improve color space support | expand |
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;
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 >
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;
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;
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(-)