| Message ID | 20200703171019.19270-7-dafna.hirschfeld@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | v4l2: add support for colorspace conversion API (CSC) for video capture and subdevices | expand |
Hi Dafna,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on next-20200703]
[cannot apply to v5.8-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Dafna-Hirschfeld/v4l2-add-support-for-colorspace-conversion-API-CSC-for-video-capture-and-subdevices/20200704-011401
base: git://linuxtv.org/media_tree.git master
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/staging/media/rkisp1/rkisp1-isp.c: In function 'rkisp1_isp_enum_mbus_code':
>> drivers/staging/media/rkisp1/rkisp1-isp.c:597:15: error: 'RKISP1_ISP_SD_SRC' undeclared (first use in this function); did you mean 'RKISP1_RSZ_PAD_SRC'?
597 | dir == RKISP1_ISP_SD_SRC)
| ^~~~~~~~~~~~~~~~~
| RKISP1_RSZ_PAD_SRC
drivers/staging/media/rkisp1/rkisp1-isp.c:597:15: note: each undeclared identifier is reported only once for each function it appears in
vim +597 drivers/staging/media/rkisp1/rkisp1-isp.c
562
563 /* ----------------------------------------------------------------------------
564 * Subdev pad operations
565 */
566
567 static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd,
568 struct v4l2_subdev_pad_config *cfg,
569 struct v4l2_subdev_mbus_code_enum *code)
570 {
571 unsigned int i, dir;
572 int pos = 0;
573
574 if (code->pad == RKISP1_ISP_PAD_SINK_VIDEO) {
575 dir = RKISP1_DIR_SINK;
576 } else if (code->pad == RKISP1_ISP_PAD_SOURCE_VIDEO) {
577 dir = RKISP1_DIR_SRC;
578 } else {
579 if (code->index > 0)
580 return -EINVAL;
581 code->code = MEDIA_BUS_FMT_FIXED;
582 return 0;
583 }
584
585 if (code->index >= ARRAY_SIZE(rkisp1_isp_formats))
586 return -EINVAL;
587
588 for (i = 0; i < ARRAY_SIZE(rkisp1_isp_formats); i++) {
589 const struct rkisp1_isp_mbus_info *fmt = &rkisp1_isp_formats[i];
590
591 if (fmt->direction & dir)
592 pos++;
593
594 if (code->index == pos - 1) {
595 code->code = fmt->mbus_code;
596 if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV &&
> 597 dir == RKISP1_ISP_SD_SRC)
598 code->flags =
599 V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION;
600 return 0;
601 }
602 }
603
604 return -EINVAL;
605 }
606
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Dafna, Thanks for your patch. On 7/3/20 2:10 PM, Dafna Hirschfeld wrote: > The isp entity has a hardware support to force full range quantization > for YUV formats. Use the subdev CSC API to allow userspace to set the > quantization for YUV formats on the isp entity. > > - The flag V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION is set on > YUV formats during enumeration of the isp formats on the source pad. > - The full quantization is set on YUV formats if userspace ask it > on the isp entity using the flag V4L2_MBUS_FRAMEFMT_SET_CSC. > > On the capture and the resizer, the quantization is read-only > and always set to the default quantization. > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > --- > drivers/staging/media/rkisp1/TODO | 2 +- > drivers/staging/media/rkisp1/rkisp1-capture.c | 10 ---------- > drivers/staging/media/rkisp1/rkisp1-isp.c | 18 ++++++++++++++---- > 3 files changed, 15 insertions(+), 15 deletions(-) > > diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO > index c0cbec0a164d..db05288949bd 100644 > --- a/drivers/staging/media/rkisp1/TODO > +++ b/drivers/staging/media/rkisp1/TODO > @@ -2,7 +2,7 @@ > * Use threaded interrupt for rkisp1_stats_isr(), remove work queue. > * Fix checkpatch errors. > * Review and comment every lock > -* Handle quantization > +* Add uapi docs. Remeber to add documentation of how quantization is handled. > * Document rkisp1-common.h > * streaming paths (mainpath and selfpath) check if the other path is streaming > in several places of the code, review this, specially that it doesn't seem it > diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c > index f69235f82c45..93d6846886f2 100644 > --- a/drivers/staging/media/rkisp1/rkisp1-capture.c > +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c > @@ -1085,8 +1085,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap, > const struct v4l2_format_info **fmt_info) > { > const struct rkisp1_capture_config *config = cap->config; > - struct rkisp1_capture *other_cap = > - &cap->rkisp1->capture_devs[cap->id ^ 1]; > const struct rkisp1_capture_fmt_cfg *fmt; > const struct v4l2_format_info *info; > const unsigned int max_widths[] = { RKISP1_RSZ_MP_SRC_MAX_WIDTH, > @@ -1111,14 +1109,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap, > > info = rkisp1_fill_pixfmt(pixm, cap->id); > > - /* can not change quantization when stream-on */ > - if (other_cap->is_streaming) > - pixm->quantization = other_cap->pix.fmt.quantization; > - /* output full range by default, take effect in params */ > - else if (!pixm->quantization || > - pixm->quantization > V4L2_QUANTIZATION_LIM_RANGE) > - pixm->quantization = V4L2_QUANTIZATION_FULL_RANGE; > - > if (fmt_cfg) > *fmt_cfg = fmt; > if (fmt_info) > diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c > index 58c90c67594d..d575c1e4dc4b 100644 > --- a/drivers/staging/media/rkisp1/rkisp1-isp.c > +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c > @@ -589,6 +589,10 @@ static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd, > > if (code->index == pos - 1) { > code->code = fmt->mbus_code; > + if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV && > + dir == RKISP1_ISP_SD_SRC) > + code->flags = > + V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION; > return 0; > } > } > @@ -620,7 +624,6 @@ 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->quantization = V4L2_QUANTIZATION_FULL_RANGE; > > src_crop = v4l2_subdev_get_try_crop(sd, cfg, > RKISP1_ISP_PAD_SOURCE_VIDEO); > @@ -663,10 +666,17 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp, > isp->src_fmt = mbus_info; > src_fmt->width = src_crop->width; > src_fmt->height = src_crop->height; > - src_fmt->quantization = format->quantization; > - /* full range by default */ > - if (!src_fmt->quantization) > + > + /* > + * The CSC API is used to allow userspace to force full > + * quantization on YUV formats. > + */ > + if (format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC && > + format->quantization == V4L2_QUANTIZATION_FULL_RANGE && > + mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV) > src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE; > + else > + src_fmt->quantization = V4L2_QUANTIZATION_DEFAULT; According to the docs [1]: [1] https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/colorspaces-defs.html?highlight=quantization "The default colorspace. This can be used by applications to let the driver fill in the colorspace." So, in my understanding, the driver should never return V4L2_QUANTIZATION_DEFAULT to userspace. We have the same issue in the resizer. I also see an issue in the capture that allows userspace to change quantization, and this should be fixed. In my understanding, since you are leaving all the other pads and video nodes with read-only quantization, changing quantization in the ISP source pad should change the readings in all the other pads and video nodes. So, if all pads are reporting V4L2_QUANTIZATION_LIM_RANGE (which is the default for sRGB colorspace), and userspace sets V4L2_QUANTIZATION_FULL_RANGE to the isp source pad, then all the other pads and video nodes should also report V4L2_QUANTIZATION_FULL_RANGE, since this will be the actual quantization used by the driver. Regards, Helen > > *format = *src_fmt; > } >
Hi Helen, On Fri, Jul 3, 2020 at 10:00 PM Helen Koike <helen.koike@collabora.com> wrote: > > Hi Dafna, > > Thanks for your patch. > > On 7/3/20 2:10 PM, Dafna Hirschfeld wrote: > > The isp entity has a hardware support to force full range quantization > > for YUV formats. Use the subdev CSC API to allow userspace to set the > > quantization for YUV formats on the isp entity. > > > > - The flag V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION is set on > > YUV formats during enumeration of the isp formats on the source pad. > > - The full quantization is set on YUV formats if userspace ask it > > on the isp entity using the flag V4L2_MBUS_FRAMEFMT_SET_CSC. > > > > On the capture and the resizer, the quantization is read-only > > and always set to the default quantization. > > > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > > --- > > drivers/staging/media/rkisp1/TODO | 2 +- > > drivers/staging/media/rkisp1/rkisp1-capture.c | 10 ---------- > > drivers/staging/media/rkisp1/rkisp1-isp.c | 18 ++++++++++++++---- > > 3 files changed, 15 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO > > index c0cbec0a164d..db05288949bd 100644 > > --- a/drivers/staging/media/rkisp1/TODO > > +++ b/drivers/staging/media/rkisp1/TODO > > @@ -2,7 +2,7 @@ > > * Use threaded interrupt for rkisp1_stats_isr(), remove work queue. > > * Fix checkpatch errors. > > * Review and comment every lock > > -* Handle quantization > > +* Add uapi docs. Remeber to add documentation of how quantization is handled. > > * Document rkisp1-common.h > > * streaming paths (mainpath and selfpath) check if the other path is streaming > > in several places of the code, review this, specially that it doesn't seem it > > diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c > > index f69235f82c45..93d6846886f2 100644 > > --- a/drivers/staging/media/rkisp1/rkisp1-capture.c > > +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c > > @@ -1085,8 +1085,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap, > > const struct v4l2_format_info **fmt_info) > > { > > const struct rkisp1_capture_config *config = cap->config; > > - struct rkisp1_capture *other_cap = > > - &cap->rkisp1->capture_devs[cap->id ^ 1]; > > const struct rkisp1_capture_fmt_cfg *fmt; > > const struct v4l2_format_info *info; > > const unsigned int max_widths[] = { RKISP1_RSZ_MP_SRC_MAX_WIDTH, > > @@ -1111,14 +1109,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap, > > > > info = rkisp1_fill_pixfmt(pixm, cap->id); > > > > - /* can not change quantization when stream-on */ > > - if (other_cap->is_streaming) > > - pixm->quantization = other_cap->pix.fmt.quantization; > > - /* output full range by default, take effect in params */ > > - else if (!pixm->quantization || > > - pixm->quantization > V4L2_QUANTIZATION_LIM_RANGE) > > - pixm->quantization = V4L2_QUANTIZATION_FULL_RANGE; > > - > > if (fmt_cfg) > > *fmt_cfg = fmt; > > if (fmt_info) > > diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c > > index 58c90c67594d..d575c1e4dc4b 100644 > > --- a/drivers/staging/media/rkisp1/rkisp1-isp.c > > +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c > > @@ -589,6 +589,10 @@ static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd, > > > > if (code->index == pos - 1) { > > code->code = fmt->mbus_code; > > + if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV && > > + dir == RKISP1_ISP_SD_SRC) > > + code->flags = > > + V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION; > > return 0; > > } > > } > > @@ -620,7 +624,6 @@ 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->quantization = V4L2_QUANTIZATION_FULL_RANGE; > > > > src_crop = v4l2_subdev_get_try_crop(sd, cfg, > > RKISP1_ISP_PAD_SOURCE_VIDEO); > > @@ -663,10 +666,17 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp, > > isp->src_fmt = mbus_info; > > src_fmt->width = src_crop->width; > > src_fmt->height = src_crop->height; > > - src_fmt->quantization = format->quantization; > > - /* full range by default */ > > - if (!src_fmt->quantization) > > + > > + /* > > + * The CSC API is used to allow userspace to force full > > + * quantization on YUV formats. > > + */ > > + if (format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC && > > + format->quantization == V4L2_QUANTIZATION_FULL_RANGE && > > + mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV) > > src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE; > > + else > > + src_fmt->quantization = V4L2_QUANTIZATION_DEFAULT; > > According to the docs [1]: > > [1] https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/colorspaces-defs.html?highlight=quantization > > "The default colorspace. This can be used by applications to let the driver fill in the colorspace." > > So, in my understanding, the driver should never return V4L2_QUANTIZATION_DEFAULT to userspace. > > We have the same issue in the resizer. > > I also see an issue in the capture that allows userspace to change quantization, and this should be fixed. > > In my understanding, since you are leaving all the other pads and video nodes with read-only quantization, > changing quantization in the ISP source pad should change the readings in all the other pads and video nodes. > > So, if all pads are reporting V4L2_QUANTIZATION_LIM_RANGE (which is the default for sRGB colorspace), and userspace sets > V4L2_QUANTIZATION_FULL_RANGE to the isp source pad, then all the other pads and video nodes should also report > V4L2_QUANTIZATION_FULL_RANGE, since this will be the actual quantization used by the driver. That's my interpretation of the spec and I would choose to implement such behavior, however not everyone agrees on that. Please check the discussion over the v3 series [1]. [1] https://lore.kernel.org/linux-media/20200702184324.GP12562@pendragon.ideasonboard.com/ Best regards, Tomasz
Hi Dafna,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on next-20200703]
[cannot apply to v5.8-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Dafna-Hirschfeld/v4l2-add-support-for-colorspace-conversion-API-CSC-for-video-capture-and-subdevices/20200704-011401
base: git://linuxtv.org/media_tree.git master
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project ca464639a1c9dd3944eb055ffd2796e8c2e7639f)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/staging/media/rkisp1/rkisp1-isp.c:597:15: error: use of undeclared identifier 'RKISP1_ISP_SD_SRC'; did you mean 'RKISP1_RSZ_PAD_SRC'?
dir == RKISP1_ISP_SD_SRC)
^~~~~~~~~~~~~~~~~
RKISP1_RSZ_PAD_SRC
drivers/staging/media/rkisp1/rkisp1-common.h:47:2: note: 'RKISP1_RSZ_PAD_SRC' declared here
RKISP1_RSZ_PAD_SRC,
^
1 error generated.
vim +597 drivers/staging/media/rkisp1/rkisp1-isp.c
562
563 /* ----------------------------------------------------------------------------
564 * Subdev pad operations
565 */
566
567 static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd,
568 struct v4l2_subdev_pad_config *cfg,
569 struct v4l2_subdev_mbus_code_enum *code)
570 {
571 unsigned int i, dir;
572 int pos = 0;
573
574 if (code->pad == RKISP1_ISP_PAD_SINK_VIDEO) {
575 dir = RKISP1_DIR_SINK;
576 } else if (code->pad == RKISP1_ISP_PAD_SOURCE_VIDEO) {
577 dir = RKISP1_DIR_SRC;
578 } else {
579 if (code->index > 0)
580 return -EINVAL;
581 code->code = MEDIA_BUS_FMT_FIXED;
582 return 0;
583 }
584
585 if (code->index >= ARRAY_SIZE(rkisp1_isp_formats))
586 return -EINVAL;
587
588 for (i = 0; i < ARRAY_SIZE(rkisp1_isp_formats); i++) {
589 const struct rkisp1_isp_mbus_info *fmt = &rkisp1_isp_formats[i];
590
591 if (fmt->direction & dir)
592 pos++;
593
594 if (code->index == pos - 1) {
595 code->code = fmt->mbus_code;
596 if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV &&
> 597 dir == RKISP1_ISP_SD_SRC)
598 code->flags =
599 V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION;
600 return 0;
601 }
602 }
603
604 return -EINVAL;
605 }
606
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 03/07/2020 19:10, Dafna Hirschfeld wrote: > The isp entity has a hardware support to force full range quantization s/a hardware support/hardware support/ > for YUV formats. Use the subdev CSC API to allow userspace to set the > quantization for YUV formats on the isp entity. > > - The flag V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION is set on s/on/for/ > YUV formats during enumeration of the isp formats on the source pad. > - The full quantization is set on YUV formats if userspace ask it s/ask/asks for/ > on the isp entity using the flag V4L2_MBUS_FRAMEFMT_SET_CSC. > > On the capture and the resizer, the quantization is read-only > and always set to the default quantization. > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > --- > drivers/staging/media/rkisp1/TODO | 2 +- > drivers/staging/media/rkisp1/rkisp1-capture.c | 10 ---------- > drivers/staging/media/rkisp1/rkisp1-isp.c | 18 ++++++++++++++---- > 3 files changed, 15 insertions(+), 15 deletions(-) > > diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO > index c0cbec0a164d..db05288949bd 100644 > --- a/drivers/staging/media/rkisp1/TODO > +++ b/drivers/staging/media/rkisp1/TODO > @@ -2,7 +2,7 @@ > * Use threaded interrupt for rkisp1_stats_isr(), remove work queue. > * Fix checkpatch errors. > * Review and comment every lock > -* Handle quantization > +* Add uapi docs. Remeber to add documentation of how quantization is handled. > * Document rkisp1-common.h > * streaming paths (mainpath and selfpath) check if the other path is streaming > in several places of the code, review this, specially that it doesn't seem it > diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c > index f69235f82c45..93d6846886f2 100644 > --- a/drivers/staging/media/rkisp1/rkisp1-capture.c > +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c > @@ -1085,8 +1085,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap, > const struct v4l2_format_info **fmt_info) > { > const struct rkisp1_capture_config *config = cap->config; > - struct rkisp1_capture *other_cap = > - &cap->rkisp1->capture_devs[cap->id ^ 1]; > const struct rkisp1_capture_fmt_cfg *fmt; > const struct v4l2_format_info *info; > const unsigned int max_widths[] = { RKISP1_RSZ_MP_SRC_MAX_WIDTH, > @@ -1111,14 +1109,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap, > > info = rkisp1_fill_pixfmt(pixm, cap->id); > > - /* can not change quantization when stream-on */ > - if (other_cap->is_streaming) > - pixm->quantization = other_cap->pix.fmt.quantization; > - /* output full range by default, take effect in params */ > - else if (!pixm->quantization || > - pixm->quantization > V4L2_QUANTIZATION_LIM_RANGE) > - pixm->quantization = V4L2_QUANTIZATION_FULL_RANGE; > - > if (fmt_cfg) > *fmt_cfg = fmt; > if (fmt_info) > diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c > index 58c90c67594d..d575c1e4dc4b 100644 > --- a/drivers/staging/media/rkisp1/rkisp1-isp.c > +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c > @@ -589,6 +589,10 @@ static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd, > > if (code->index == pos - 1) { > code->code = fmt->mbus_code; > + if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV && > + dir == RKISP1_ISP_SD_SRC) > + code->flags = > + V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION; > return 0; > } > } > @@ -620,7 +624,6 @@ 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->quantization = V4L2_QUANTIZATION_FULL_RANGE; > > src_crop = v4l2_subdev_get_try_crop(sd, cfg, > RKISP1_ISP_PAD_SOURCE_VIDEO); > @@ -663,10 +666,17 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp, > isp->src_fmt = mbus_info; > src_fmt->width = src_crop->width; > src_fmt->height = src_crop->height; > - src_fmt->quantization = format->quantization; > - /* full range by default */ > - if (!src_fmt->quantization) > + > + /* > + * The CSC API is used to allow userspace to force full > + * quantization on YUV formats. > + */ > + if (format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC && > + format->quantization == V4L2_QUANTIZATION_FULL_RANGE && > + mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV) > src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE; > + else > + src_fmt->quantization = V4L2_QUANTIZATION_DEFAULT; > > *format = *src_fmt; > } > Regards, Hans
On Fri, Jul 03, 2020 at 05:00:40PM -0300, Helen Koike wrote: > Hi Dafna, > > Thanks for your patch. > > On 7/3/20 2:10 PM, Dafna Hirschfeld wrote: > > The isp entity has a hardware support to force full range quantization > > for YUV formats. Use the subdev CSC API to allow userspace to set the > > quantization for YUV formats on the isp entity. > > > > - The flag V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION is set on > > YUV formats during enumeration of the isp formats on the source pad. > > - The full quantization is set on YUV formats if userspace ask it > > on the isp entity using the flag V4L2_MBUS_FRAMEFMT_SET_CSC. > > > > On the capture and the resizer, the quantization is read-only > > and always set to the default quantization. > > > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > > --- > > drivers/staging/media/rkisp1/TODO | 2 +- > > drivers/staging/media/rkisp1/rkisp1-capture.c | 10 ---------- > > drivers/staging/media/rkisp1/rkisp1-isp.c | 18 ++++++++++++++---- > > 3 files changed, 15 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO > > index c0cbec0a164d..db05288949bd 100644 > > --- a/drivers/staging/media/rkisp1/TODO > > +++ b/drivers/staging/media/rkisp1/TODO > > @@ -2,7 +2,7 @@ > > * Use threaded interrupt for rkisp1_stats_isr(), remove work queue. > > * Fix checkpatch errors. > > * Review and comment every lock > > -* Handle quantization > > +* Add uapi docs. Remeber to add documentation of how quantization is handled. > > * Document rkisp1-common.h > > * streaming paths (mainpath and selfpath) check if the other path is streaming > > in several places of the code, review this, specially that it doesn't seem it > > diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c > > index f69235f82c45..93d6846886f2 100644 > > --- a/drivers/staging/media/rkisp1/rkisp1-capture.c > > +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c > > @@ -1085,8 +1085,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap, > > const struct v4l2_format_info **fmt_info) > > { > > const struct rkisp1_capture_config *config = cap->config; > > - struct rkisp1_capture *other_cap = > > - &cap->rkisp1->capture_devs[cap->id ^ 1]; > > const struct rkisp1_capture_fmt_cfg *fmt; > > const struct v4l2_format_info *info; > > const unsigned int max_widths[] = { RKISP1_RSZ_MP_SRC_MAX_WIDTH, > > @@ -1111,14 +1109,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap, > > > > info = rkisp1_fill_pixfmt(pixm, cap->id); > > > > - /* can not change quantization when stream-on */ > > - if (other_cap->is_streaming) > > - pixm->quantization = other_cap->pix.fmt.quantization; > > - /* output full range by default, take effect in params */ > > - else if (!pixm->quantization || > > - pixm->quantization > V4L2_QUANTIZATION_LIM_RANGE) > > - pixm->quantization = V4L2_QUANTIZATION_FULL_RANGE; > > - > > if (fmt_cfg) > > *fmt_cfg = fmt; > > if (fmt_info) > > diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c > > index 58c90c67594d..d575c1e4dc4b 100644 > > --- a/drivers/staging/media/rkisp1/rkisp1-isp.c > > +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c > > @@ -589,6 +589,10 @@ static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd, > > > > if (code->index == pos - 1) { > > code->code = fmt->mbus_code; > > + if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV && > > + dir == RKISP1_ISP_SD_SRC) > > + code->flags = > > + V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION; > > return 0; > > } > > } > > @@ -620,7 +624,6 @@ 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->quantization = V4L2_QUANTIZATION_FULL_RANGE; > > > > src_crop = v4l2_subdev_get_try_crop(sd, cfg, > > RKISP1_ISP_PAD_SOURCE_VIDEO); > > @@ -663,10 +666,17 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp, > > isp->src_fmt = mbus_info; > > src_fmt->width = src_crop->width; > > src_fmt->height = src_crop->height; > > - src_fmt->quantization = format->quantization; > > - /* full range by default */ > > - if (!src_fmt->quantization) > > + > > + /* > > + * The CSC API is used to allow userspace to force full > > + * quantization on YUV formats. > > + */ > > + if (format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC && > > + format->quantization == V4L2_QUANTIZATION_FULL_RANGE && > > + mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV) > > src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE; > > + else > > + src_fmt->quantization = V4L2_QUANTIZATION_DEFAULT; > > According to the docs [1]: > > [1] https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/colorspaces-defs.html?highlight=quantization > > "The default colorspace. This can be used by applications to let the driver fill in the colorspace." > > So, in my understanding, the driver should never return V4L2_QUANTIZATION_DEFAULT to userspace. > The part of the documentation you quoted comes from the description of V4L2_COLORSPACE_DEFAULT and not V4L2_QUANTIZATION_DEFAULT. For the latter it says: "Use the default quantization encoding as defined by the colorspace. This is always full range for R’G’B’ (except for the BT.2020 colorspace) and HSV. It is usually limited range for Y’CbCr." Which, in my understanding, doesn't suggest in any way that it shouldn't be returned by the driver. There is also a valid case when the driver simply doesn't know or care what quantization the captured signal uses. However, in this case, I don't see why we would return DEFAULT here indeed, as the driver has the full knowledge, RGB is full range and YUV is limited range unless full range was requested. > We have the same issue in the resizer. > > I also see an issue in the capture that allows userspace to change quantization, and this should be fixed. > > In my understanding, since you are leaving all the other pads and video nodes with read-only quantization, > changing quantization in the ISP source pad should change the readings in all the other pads and video nodes. > So, if all pads are reporting V4L2_QUANTIZATION_LIM_RANGE (which is the default for sRGB colorspace), and userspace sets > V4L2_QUANTIZATION_FULL_RANGE to the isp source pad, then all the other pads and video nodes should also report > V4L2_QUANTIZATION_FULL_RANGE, since this will be the actual quantization used by the driver. This was extensively discussed at v3: https://patchwork.kernel.org/patch/11493105/ The conclusion is that there is no value from both the kernel and the userspace point of view in propagating this to the end of the pipeline, because the hardware components later in the pipeline don't care and the userspace, given the DEFAULT quantization value, can infer the exact quantziation from the pipeline - here from what it configured at the ISP entity. Best regards, Tomasz
Hi Dafna, Thank you for the patch. On Fri, Jul 03, 2020 at 07:10:18PM +0200, Dafna Hirschfeld wrote: > The isp entity has a hardware support to force full range quantization > for YUV formats. Use the subdev CSC API to allow userspace to set the > quantization for YUV formats on the isp entity. > > - The flag V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION is set on > YUV formats during enumeration of the isp formats on the source pad. > - The full quantization is set on YUV formats if userspace ask it > on the isp entity using the flag V4L2_MBUS_FRAMEFMT_SET_CSC. > > On the capture and the resizer, the quantization is read-only > and always set to the default quantization. > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > --- > drivers/staging/media/rkisp1/TODO | 2 +- > drivers/staging/media/rkisp1/rkisp1-capture.c | 10 ---------- > drivers/staging/media/rkisp1/rkisp1-isp.c | 18 ++++++++++++++---- > 3 files changed, 15 insertions(+), 15 deletions(-) > > diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO > index c0cbec0a164d..db05288949bd 100644 > --- a/drivers/staging/media/rkisp1/TODO > +++ b/drivers/staging/media/rkisp1/TODO > @@ -2,7 +2,7 @@ > * Use threaded interrupt for rkisp1_stats_isr(), remove work queue. > * Fix checkpatch errors. > * Review and comment every lock > -* Handle quantization > +* Add uapi docs. Remeber to add documentation of how quantization is handled. > * Document rkisp1-common.h > * streaming paths (mainpath and selfpath) check if the other path is streaming > in several places of the code, review this, specially that it doesn't seem it > diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c > index f69235f82c45..93d6846886f2 100644 > --- a/drivers/staging/media/rkisp1/rkisp1-capture.c > +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c > @@ -1085,8 +1085,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap, > const struct v4l2_format_info **fmt_info) > { > const struct rkisp1_capture_config *config = cap->config; > - struct rkisp1_capture *other_cap = > - &cap->rkisp1->capture_devs[cap->id ^ 1]; > const struct rkisp1_capture_fmt_cfg *fmt; > const struct v4l2_format_info *info; > const unsigned int max_widths[] = { RKISP1_RSZ_MP_SRC_MAX_WIDTH, > @@ -1111,14 +1109,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap, > > info = rkisp1_fill_pixfmt(pixm, cap->id); > > - /* can not change quantization when stream-on */ > - if (other_cap->is_streaming) > - pixm->quantization = other_cap->pix.fmt.quantization; > - /* output full range by default, take effect in params */ > - else if (!pixm->quantization || > - pixm->quantization > V4L2_QUANTIZATION_LIM_RANGE) > - pixm->quantization = V4L2_QUANTIZATION_FULL_RANGE; > - > if (fmt_cfg) > *fmt_cfg = fmt; > if (fmt_info) > diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c > index 58c90c67594d..d575c1e4dc4b 100644 > --- a/drivers/staging/media/rkisp1/rkisp1-isp.c > +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c > @@ -589,6 +589,10 @@ static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd, > > if (code->index == pos - 1) { > code->code = fmt->mbus_code; > + if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV && > + dir == RKISP1_ISP_SD_SRC) > + code->flags = > + V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION; > return 0; > } > } > @@ -620,7 +624,6 @@ 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->quantization = V4L2_QUANTIZATION_FULL_RANGE; > > src_crop = v4l2_subdev_get_try_crop(sd, cfg, > RKISP1_ISP_PAD_SOURCE_VIDEO); > @@ -663,10 +666,17 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp, > isp->src_fmt = mbus_info; > src_fmt->width = src_crop->width; > src_fmt->height = src_crop->height; > - src_fmt->quantization = format->quantization; > - /* full range by default */ > - if (!src_fmt->quantization) > + > + /* > + * The CSC API is used to allow userspace to force full > + * quantization on YUV formats. > + */ > + if (format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC && > + format->quantization == V4L2_QUANTIZATION_FULL_RANGE && > + mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV) > src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE; > + else > + src_fmt->quantization = V4L2_QUANTIZATION_DEFAULT; Apart from the usage of DEFAULT, as discussed by Helen and Tomasz, this patch looks good to me. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > *format = *src_fmt; > }
Hi, On 7/22/20 10:12 AM, Tomasz Figa wrote: > On Fri, Jul 03, 2020 at 05:00:40PM -0300, Helen Koike wrote: >> Hi Dafna, >> >> Thanks for your patch. >> >> On 7/3/20 2:10 PM, Dafna Hirschfeld wrote: >>> The isp entity has a hardware support to force full range quantization >>> for YUV formats. Use the subdev CSC API to allow userspace to set the >>> quantization for YUV formats on the isp entity. >>> >>> - The flag V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION is set on >>> YUV formats during enumeration of the isp formats on the source pad. >>> - The full quantization is set on YUV formats if userspace ask it >>> on the isp entity using the flag V4L2_MBUS_FRAMEFMT_SET_CSC. >>> >>> On the capture and the resizer, the quantization is read-only >>> and always set to the default quantization. >>> >>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >>> --- >>> drivers/staging/media/rkisp1/TODO | 2 +- >>> drivers/staging/media/rkisp1/rkisp1-capture.c | 10 ---------- >>> drivers/staging/media/rkisp1/rkisp1-isp.c | 18 ++++++++++++++---- >>> 3 files changed, 15 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO >>> index c0cbec0a164d..db05288949bd 100644 >>> --- a/drivers/staging/media/rkisp1/TODO >>> +++ b/drivers/staging/media/rkisp1/TODO >>> @@ -2,7 +2,7 @@ >>> * Use threaded interrupt for rkisp1_stats_isr(), remove work queue. >>> * Fix checkpatch errors. >>> * Review and comment every lock >>> -* Handle quantization >>> +* Add uapi docs. Remeber to add documentation of how quantization is handled. >>> * Document rkisp1-common.h >>> * streaming paths (mainpath and selfpath) check if the other path is streaming >>> in several places of the code, review this, specially that it doesn't seem it >>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c >>> index f69235f82c45..93d6846886f2 100644 >>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c >>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c >>> @@ -1085,8 +1085,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap, >>> const struct v4l2_format_info **fmt_info) >>> { >>> const struct rkisp1_capture_config *config = cap->config; >>> - struct rkisp1_capture *other_cap = >>> - &cap->rkisp1->capture_devs[cap->id ^ 1]; >>> const struct rkisp1_capture_fmt_cfg *fmt; >>> const struct v4l2_format_info *info; >>> const unsigned int max_widths[] = { RKISP1_RSZ_MP_SRC_MAX_WIDTH, >>> @@ -1111,14 +1109,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap, >>> >>> info = rkisp1_fill_pixfmt(pixm, cap->id); >>> >>> - /* can not change quantization when stream-on */ >>> - if (other_cap->is_streaming) >>> - pixm->quantization = other_cap->pix.fmt.quantization; >>> - /* output full range by default, take effect in params */ >>> - else if (!pixm->quantization || >>> - pixm->quantization > V4L2_QUANTIZATION_LIM_RANGE) >>> - pixm->quantization = V4L2_QUANTIZATION_FULL_RANGE; >>> - >>> if (fmt_cfg) >>> *fmt_cfg = fmt; >>> if (fmt_info) >>> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c >>> index 58c90c67594d..d575c1e4dc4b 100644 >>> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c >>> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c >>> @@ -589,6 +589,10 @@ static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd, >>> >>> if (code->index == pos - 1) { >>> code->code = fmt->mbus_code; >>> + if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV && >>> + dir == RKISP1_ISP_SD_SRC) >>> + code->flags = >>> + V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION; >>> return 0; >>> } >>> } >>> @@ -620,7 +624,6 @@ 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->quantization = V4L2_QUANTIZATION_FULL_RANGE; >>> >>> src_crop = v4l2_subdev_get_try_crop(sd, cfg, >>> RKISP1_ISP_PAD_SOURCE_VIDEO); >>> @@ -663,10 +666,17 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp, >>> isp->src_fmt = mbus_info; >>> src_fmt->width = src_crop->width; >>> src_fmt->height = src_crop->height; >>> - src_fmt->quantization = format->quantization; >>> - /* full range by default */ >>> - if (!src_fmt->quantization) >>> + >>> + /* >>> + * The CSC API is used to allow userspace to force full >>> + * quantization on YUV formats. >>> + */ >>> + if (format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC && >>> + format->quantization == V4L2_QUANTIZATION_FULL_RANGE && >>> + mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV) >>> src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE; >>> + else >>> + src_fmt->quantization = V4L2_QUANTIZATION_DEFAULT; >> >> According to the docs [1]: >> >> [1] https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/colorspaces-defs.html?highlight=quantization >> >> "The default colorspace. This can be used by applications to let the driver fill in the colorspace." >> >> So, in my understanding, the driver should never return V4L2_QUANTIZATION_DEFAULT to userspace. >> > > The part of the documentation you quoted comes from the description of > V4L2_COLORSPACE_DEFAULT and not V4L2_QUANTIZATION_DEFAULT. > > For the latter it says: > > "Use the default quantization encoding as defined by the colorspace. > This is always full range for R’G’B’ (except for the BT.2020 colorspace) > and HSV. It is usually limited range for Y’CbCr." > > Which, in my understanding, doesn't suggest in any way that it shouldn't > be returned by the driver. There is also a valid case when the driver > simply doesn't know or care what quantization the captured signal uses. Thanks for this clarification. > > However, in this case, I don't see why we would return DEFAULT here > indeed, as the driver has the full knowledge, RGB is full range and YUV > is limited range unless full range was requested. > >> We have the same issue in the resizer. >> >> I also see an issue in the capture that allows userspace to change quantization, and this should be fixed. >> >> In my understanding, since you are leaving all the other pads and video nodes with read-only quantization, >> changing quantization in the ISP source pad should change the readings in all the other pads and video nodes. >> So, if all pads are reporting V4L2_QUANTIZATION_LIM_RANGE (which is the default for sRGB colorspace), and userspace sets >> V4L2_QUANTIZATION_FULL_RANGE to the isp source pad, then all the other pads and video nodes should also report >> V4L2_QUANTIZATION_FULL_RANGE, since this will be the actual quantization used by the driver. > > This was extensively discussed at v3: > https://patchwork.kernel.org/patch/11493105/ > > The conclusion is that there is no value from both the kernel and the userspace point of > view in propagating this to the end of the pipeline, because the > hardware components later in the pipeline don't care and the userspace, > given the DEFAULT quantization value, can infer the exact quantziation from the > pipeline - here from what it configured at the ISP entity. Let's use DEFAULT then, sounds the simplest solution imho, unless you disagree. Acked-by: Helen Koike <helen.koike@collabora.com> Regards, Helen > > Best regards, > Tomasz >
On Wed, Jul 22, 2020 at 7:13 PM Helen Koike <helen.koike@collabora.com> wrote: > > Hi, > > On 7/22/20 10:12 AM, Tomasz Figa wrote: > > On Fri, Jul 03, 2020 at 05:00:40PM -0300, Helen Koike wrote: > >> Hi Dafna, > >> > >> Thanks for your patch. > >> > >> On 7/3/20 2:10 PM, Dafna Hirschfeld wrote: > >>> The isp entity has a hardware support to force full range quantization > >>> for YUV formats. Use the subdev CSC API to allow userspace to set the > >>> quantization for YUV formats on the isp entity. > >>> > >>> - The flag V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION is set on > >>> YUV formats during enumeration of the isp formats on the source pad. > >>> - The full quantization is set on YUV formats if userspace ask it > >>> on the isp entity using the flag V4L2_MBUS_FRAMEFMT_SET_CSC. > >>> > >>> On the capture and the resizer, the quantization is read-only > >>> and always set to the default quantization. > >>> > >>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > >>> --- > >>> drivers/staging/media/rkisp1/TODO | 2 +- > >>> drivers/staging/media/rkisp1/rkisp1-capture.c | 10 ---------- > >>> drivers/staging/media/rkisp1/rkisp1-isp.c | 18 ++++++++++++++---- > >>> 3 files changed, 15 insertions(+), 15 deletions(-) > >>> > >>> diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO > >>> index c0cbec0a164d..db05288949bd 100644 > >>> --- a/drivers/staging/media/rkisp1/TODO > >>> +++ b/drivers/staging/media/rkisp1/TODO > >>> @@ -2,7 +2,7 @@ > >>> * Use threaded interrupt for rkisp1_stats_isr(), remove work queue. > >>> * Fix checkpatch errors. > >>> * Review and comment every lock > >>> -* Handle quantization > >>> +* Add uapi docs. Remeber to add documentation of how quantization is handled. > >>> * Document rkisp1-common.h > >>> * streaming paths (mainpath and selfpath) check if the other path is streaming > >>> in several places of the code, review this, specially that it doesn't seem it > >>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c > >>> index f69235f82c45..93d6846886f2 100644 > >>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c > >>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c > >>> @@ -1085,8 +1085,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap, > >>> const struct v4l2_format_info **fmt_info) > >>> { > >>> const struct rkisp1_capture_config *config = cap->config; > >>> - struct rkisp1_capture *other_cap = > >>> - &cap->rkisp1->capture_devs[cap->id ^ 1]; > >>> const struct rkisp1_capture_fmt_cfg *fmt; > >>> const struct v4l2_format_info *info; > >>> const unsigned int max_widths[] = { RKISP1_RSZ_MP_SRC_MAX_WIDTH, > >>> @@ -1111,14 +1109,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap, > >>> > >>> info = rkisp1_fill_pixfmt(pixm, cap->id); > >>> > >>> - /* can not change quantization when stream-on */ > >>> - if (other_cap->is_streaming) > >>> - pixm->quantization = other_cap->pix.fmt.quantization; > >>> - /* output full range by default, take effect in params */ > >>> - else if (!pixm->quantization || > >>> - pixm->quantization > V4L2_QUANTIZATION_LIM_RANGE) > >>> - pixm->quantization = V4L2_QUANTIZATION_FULL_RANGE; > >>> - > >>> if (fmt_cfg) > >>> *fmt_cfg = fmt; > >>> if (fmt_info) > >>> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c > >>> index 58c90c67594d..d575c1e4dc4b 100644 > >>> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c > >>> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c > >>> @@ -589,6 +589,10 @@ static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd, > >>> > >>> if (code->index == pos - 1) { > >>> code->code = fmt->mbus_code; > >>> + if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV && > >>> + dir == RKISP1_ISP_SD_SRC) > >>> + code->flags = > >>> + V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION; > >>> return 0; > >>> } > >>> } > >>> @@ -620,7 +624,6 @@ 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->quantization = V4L2_QUANTIZATION_FULL_RANGE; > >>> > >>> src_crop = v4l2_subdev_get_try_crop(sd, cfg, > >>> RKISP1_ISP_PAD_SOURCE_VIDEO); > >>> @@ -663,10 +666,17 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp, > >>> isp->src_fmt = mbus_info; > >>> src_fmt->width = src_crop->width; > >>> src_fmt->height = src_crop->height; > >>> - src_fmt->quantization = format->quantization; > >>> - /* full range by default */ > >>> - if (!src_fmt->quantization) > >>> + > >>> + /* > >>> + * The CSC API is used to allow userspace to force full > >>> + * quantization on YUV formats. > >>> + */ > >>> + if (format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC && > >>> + format->quantization == V4L2_QUANTIZATION_FULL_RANGE && > >>> + mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV) > >>> src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE; > >>> + else > >>> + src_fmt->quantization = V4L2_QUANTIZATION_DEFAULT; > >> > >> According to the docs [1]: > >> > >> [1] https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/colorspaces-defs.html?highlight=quantization > >> > >> "The default colorspace. This can be used by applications to let the driver fill in the colorspace." > >> > >> So, in my understanding, the driver should never return V4L2_QUANTIZATION_DEFAULT to userspace. > >> > > > > The part of the documentation you quoted comes from the description of > > V4L2_COLORSPACE_DEFAULT and not V4L2_QUANTIZATION_DEFAULT. > > > > For the latter it says: > > > > "Use the default quantization encoding as defined by the colorspace. > > This is always full range for R’G’B’ (except for the BT.2020 colorspace) > > and HSV. It is usually limited range for Y’CbCr." > > > > Which, in my understanding, doesn't suggest in any way that it shouldn't > > be returned by the driver. There is also a valid case when the driver > > simply doesn't know or care what quantization the captured signal uses. > > Thanks for this clarification. > > > > > However, in this case, I don't see why we would return DEFAULT here > > indeed, as the driver has the full knowledge, RGB is full range and YUV > > is limited range unless full range was requested. > > > >> We have the same issue in the resizer. > >> > >> I also see an issue in the capture that allows userspace to change quantization, and this should be fixed. > >> > >> In my understanding, since you are leaving all the other pads and video nodes with read-only quantization, > >> changing quantization in the ISP source pad should change the readings in all the other pads and video nodes. > >> So, if all pads are reporting V4L2_QUANTIZATION_LIM_RANGE (which is the default for sRGB colorspace), and userspace sets > >> V4L2_QUANTIZATION_FULL_RANGE to the isp source pad, then all the other pads and video nodes should also report > >> V4L2_QUANTIZATION_FULL_RANGE, since this will be the actual quantization used by the driver. > > > > This was extensively discussed at v3: > > https://patchwork.kernel.org/patch/11493105/ > > > > The conclusion is that there is no value from both the kernel and the userspace point of > > view in propagating this to the end of the pipeline, because the > > hardware components later in the pipeline don't care and the userspace, > > given the DEFAULT quantization value, can infer the exact quantziation from the > > pipeline - here from what it configured at the ISP entity. > > Let's use DEFAULT then, sounds the simplest solution imho, unless you disagree. > > Acked-by: Helen Koike <helen.koike@collabora.com> Yes. Laurent convinced me that it is indeed the right behavior here. Reviewed-by: Tomasz Figa <tfiga@chromium.org> Best regards, Tomasz
diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO index c0cbec0a164d..db05288949bd 100644 --- a/drivers/staging/media/rkisp1/TODO +++ b/drivers/staging/media/rkisp1/TODO @@ -2,7 +2,7 @@ * Use threaded interrupt for rkisp1_stats_isr(), remove work queue. * Fix checkpatch errors. * Review and comment every lock -* Handle quantization +* Add uapi docs. Remeber to add documentation of how quantization is handled. * Document rkisp1-common.h * streaming paths (mainpath and selfpath) check if the other path is streaming in several places of the code, review this, specially that it doesn't seem it diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c index f69235f82c45..93d6846886f2 100644 --- a/drivers/staging/media/rkisp1/rkisp1-capture.c +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c @@ -1085,8 +1085,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap, const struct v4l2_format_info **fmt_info) { const struct rkisp1_capture_config *config = cap->config; - struct rkisp1_capture *other_cap = - &cap->rkisp1->capture_devs[cap->id ^ 1]; const struct rkisp1_capture_fmt_cfg *fmt; const struct v4l2_format_info *info; const unsigned int max_widths[] = { RKISP1_RSZ_MP_SRC_MAX_WIDTH, @@ -1111,14 +1109,6 @@ static void rkisp1_try_fmt(const struct rkisp1_capture *cap, info = rkisp1_fill_pixfmt(pixm, cap->id); - /* can not change quantization when stream-on */ - if (other_cap->is_streaming) - pixm->quantization = other_cap->pix.fmt.quantization; - /* output full range by default, take effect in params */ - else if (!pixm->quantization || - pixm->quantization > V4L2_QUANTIZATION_LIM_RANGE) - pixm->quantization = V4L2_QUANTIZATION_FULL_RANGE; - if (fmt_cfg) *fmt_cfg = fmt; if (fmt_info) diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c index 58c90c67594d..d575c1e4dc4b 100644 --- a/drivers/staging/media/rkisp1/rkisp1-isp.c +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c @@ -589,6 +589,10 @@ static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd, if (code->index == pos - 1) { code->code = fmt->mbus_code; + if (fmt->pixel_enc == V4L2_PIXEL_ENC_YUV && + dir == RKISP1_ISP_SD_SRC) + code->flags = + V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION; return 0; } } @@ -620,7 +624,6 @@ 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->quantization = V4L2_QUANTIZATION_FULL_RANGE; src_crop = v4l2_subdev_get_try_crop(sd, cfg, RKISP1_ISP_PAD_SOURCE_VIDEO); @@ -663,10 +666,17 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp, isp->src_fmt = mbus_info; src_fmt->width = src_crop->width; src_fmt->height = src_crop->height; - src_fmt->quantization = format->quantization; - /* full range by default */ - if (!src_fmt->quantization) + + /* + * The CSC API is used to allow userspace to force full + * quantization on YUV formats. + */ + if (format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC && + format->quantization == V4L2_QUANTIZATION_FULL_RANGE && + mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV) src_fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE; + else + src_fmt->quantization = V4L2_QUANTIZATION_DEFAULT; *format = *src_fmt; }
The isp entity has a hardware support to force full range quantization for YUV formats. Use the subdev CSC API to allow userspace to set the quantization for YUV formats on the isp entity. - The flag V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION is set on YUV formats during enumeration of the isp formats on the source pad. - The full quantization is set on YUV formats if userspace ask it on the isp entity using the flag V4L2_MBUS_FRAMEFMT_SET_CSC. On the capture and the resizer, the quantization is read-only and always set to the default quantization. Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> --- drivers/staging/media/rkisp1/TODO | 2 +- drivers/staging/media/rkisp1/rkisp1-capture.c | 10 ---------- drivers/staging/media/rkisp1/rkisp1-isp.c | 18 ++++++++++++++---- 3 files changed, 15 insertions(+), 15 deletions(-)