[v5,6/7] media: staging: rkisp1: allow quantization setting by userspace on the isp source pad
diff mbox series

Message ID 20200703171019.19270-7-dafna.hirschfeld@collabora.com
State New
Headers show
Series
  • v4l2: add support for colorspace conversion API (CSC) for video capture and subdevices
Related show

Commit Message

Dafna Hirschfeld July 3, 2020, 5:10 p.m. UTC
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(-)

Comments

kernel test robot July 3, 2020, 7:13 p.m. UTC | #1
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
Helen Koike July 3, 2020, 8 p.m. UTC | #2
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;
>  }
>
Tomasz Figa July 3, 2020, 9:21 p.m. UTC | #3
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
kernel test robot July 3, 2020, 10:35 p.m. UTC | #4
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
Hans Verkuil July 21, 2020, 2:02 p.m. UTC | #5
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
Tomasz Figa July 22, 2020, 1:12 p.m. UTC | #6
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
Laurent Pinchart July 22, 2020, 4:45 p.m. UTC | #7
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;
>  }
Helen Koike July 22, 2020, 5:13 p.m. UTC | #8
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
>
Tomasz Figa July 22, 2020, 5:20 p.m. UTC | #9
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

Patch
diff mbox series

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;
 }