[PATCH 0/3] Fix full range quantization on rkisp1 based devices

[PATCH 0/3] Fix full range quantization on rkisp1 based devices

[Stefan Klug]

Hi all,

After I sent a patch too early yesterday, I'm sending the corrected
version now. This series fixes two issues with the rkisp1 driver and
full range quantization. It was developed and tested on a imx8mp board
(Debix Som). With the current code it is impossible to get full range
YUV data by selecting color space JPEG (fixed by patch 1). But even
explicitly setting the range to full range results in image artifacts
due to incorrect range handling in CPROC (fixed by patch 2).

Please see the individual patches for more details.

Best regards,
Stefan

Stefan Klug (3):
  media: rkisp1: Set format defaults based on requested color space
  media: rkisp1: Fix the quantization settings of CPROC
  media: rkisp1: Remove unnecessary defines

 .../media/platform/rockchip/rkisp1/rkisp1-isp.c   | 15 ++++++++++++++-
 .../platform/rockchip/rkisp1/rkisp1-params.c      |  8 +-------
 .../media/platform/rockchip/rkisp1/rkisp1-regs.h  |  7 -------
 3 files changed, 15 insertions(+), 15 deletions(-)

-- 
2.43.0

[PATCH 1/3] media: rkisp1: Set format defaults based on requested color space

[Stefan Klug]

When color space JPEG is requested, the ISP sets the quantization
incorrectly to limited range. To fix that, set the xfer_func, ycbcr_enc
and quantization to the defaults for the requested color space if they
are not specified explicitly. Do this only in case we are converting
from RAW to YUV.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 .../media/platform/rockchip/rkisp1/rkisp1-isp.c   | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
index d94917211828..468f5a7d03c7 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
@@ -680,10 +680,23 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
 		if (sink_info->pixel_enc == V4L2_PIXEL_ENC_BAYER) {
 			if (format->colorspace != V4L2_COLORSPACE_DEFAULT)
 				src_fmt->colorspace = format->colorspace;
-			if (format->xfer_func != V4L2_XFER_FUNC_DEFAULT)
+
+			if (format->xfer_func == V4L2_XFER_FUNC_DEFAULT)
 				src_fmt->xfer_func = format->xfer_func;
+			else
+				src_fmt->xfer_func =
+					V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
+
 			if (format->ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT)
 				src_fmt->ycbcr_enc = format->ycbcr_enc;
+			else
+				src_fmt->ycbcr_enc =
+					V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
+
+			if (format->quantization == V4L2_QUANTIZATION_DEFAULT)
+				src_fmt->quantization =
+					V4L2_MAP_QUANTIZATION_DEFAULT(false,
+						format->colorspace, format->ycbcr_enc);
 		}
 
 		if (format->quantization != V4L2_QUANTIZATION_DEFAULT)
-- 
2.43.0

Re: [PATCH 1/3] media: rkisp1: Set format defaults based on requested color space

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Thu, Feb 27, 2025 at 12:44:59PM +0100, Stefan Klug wrote:
> When color space JPEG is requested, the ISP sets the quantization
> incorrectly to limited range. To fix that, set the xfer_func, ycbcr_enc
> and quantization to the defaults for the requested color space if they
> are not specified explicitly.

The commit message fails to explain why you're addressing xfer_func and
ycbcr_enc to fix the quantization issue.

> Do this only in case we are converting
> from RAW to YUV.

And this should explain why.

> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  .../media/platform/rockchip/rkisp1/rkisp1-isp.c   | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> index d94917211828..468f5a7d03c7 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> @@ -680,10 +680,23 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,

Adding a bit more context:

	set_csc = format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC;

	if (set_csc && src_info->pixel_enc == V4L2_PIXEL_ENC_YUV) {

If V4L2_MBUS_FRAMEFMT_SET_CSC isn't set, the colorspace fields on the
source pad will be copied from the sink pad, which doesn't seem right.
It's a separate issue, but fixing both together may lead to better code.

>  		if (sink_info->pixel_enc == V4L2_PIXEL_ENC_BAYER) {
>  			if (format->colorspace != V4L2_COLORSPACE_DEFAULT)
>  				src_fmt->colorspace = format->colorspace;
> -			if (format->xfer_func != V4L2_XFER_FUNC_DEFAULT)
> +
> +			if (format->xfer_func == V4L2_XFER_FUNC_DEFAULT)

Are you sure the condition should be inverted ?

>  				src_fmt->xfer_func = format->xfer_func;
> +			else
> +				src_fmt->xfer_func =
> +					V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
> +
>  			if (format->ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT)
>  				src_fmt->ycbcr_enc = format->ycbcr_enc;
> +			else
> +				src_fmt->ycbcr_enc =
> +					V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
> +
> +			if (format->quantization == V4L2_QUANTIZATION_DEFAULT)
> +				src_fmt->quantization =
> +					V4L2_MAP_QUANTIZATION_DEFAULT(false,
> +						format->colorspace, format->ycbcr_enc);

Shouldn't this use src_fmt instead of format ?

I think quantization handling could be moved below.

>  		}
>  
>  		if (format->quantization != V4L2_QUANTIZATION_DEFAULT)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] media: rkisp1: Set format defaults based on requested color space

[Laurent Pinchart]

On Sat, Mar 01, 2025 at 03:02:54AM +0200, Laurent Pinchart wrote:
> On Thu, Feb 27, 2025 at 12:44:59PM +0100, Stefan Klug wrote:
> > When color space JPEG is requested, the ISP sets the quantization
> > incorrectly to limited range. To fix that, set the xfer_func, ycbcr_enc
> > and quantization to the defaults for the requested color space if they
> > are not specified explicitly.
> 
> The commit message fails to explain why you're addressing xfer_func and
> ycbcr_enc to fix the quantization issue.
> 
> > Do this only in case we are converting
> > from RAW to YUV.
> 
> And this should explain why.
> 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  .../media/platform/rockchip/rkisp1/rkisp1-isp.c   | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > index d94917211828..468f5a7d03c7 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > @@ -680,10 +680,23 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
> 
> Adding a bit more context:
> 
> 	set_csc = format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC;
> 
> 	if (set_csc && src_info->pixel_enc == V4L2_PIXEL_ENC_YUV) {
> 
> If V4L2_MBUS_FRAMEFMT_SET_CSC isn't set, the colorspace fields on the
> source pad will be copied from the sink pad, which doesn't seem right.

Thinking some more about it, it's not wrong either. The colorspace and
xfer_func fields are not used by the driver, as the related ISP
processing blocks are configured through ISP parameters. Without
userspace providing the value of the fields on the source pad, the
driver can't know what colorspace and xfer_func is produced. Copying the
values from the sink pad is as good of a guess as we can make.

The ycbcr_enc and quantization fields are different, as they are taken
into account by the driver to configure the ISP. Copying ycbcr_enc from
the sink pad means that it will be set to V4L2_YCBCR_ENC_601 when the
sink format is bayer and the source format is YUV. As the sink
colorspace is most likely going to be V4L2_COLORSPACE_RAW in that case,
that's a fine default, and is identical to what we would get from
V4L2_MAP_YCBCR_ENC_DEFAULT(). Setting the quantization to
V4L2_QUANTIZATION_LIM_RANGE also seems fine as a default, and it what
V4L2_MAP_QUANTIZATION_DEFAULT() would give us.

TL;DR: there's probably no need to change the current behaviour when
V4L2_MBUS_FRAMEFMT_SET_CSC isn't set.

> It's a separate issue, but fixing both together may lead to better code.
> 
> >  		if (sink_info->pixel_enc == V4L2_PIXEL_ENC_BAYER) {
> >  			if (format->colorspace != V4L2_COLORSPACE_DEFAULT)
> >  				src_fmt->colorspace = format->colorspace;
> > -			if (format->xfer_func != V4L2_XFER_FUNC_DEFAULT)
> > +
> > +			if (format->xfer_func == V4L2_XFER_FUNC_DEFAULT)
> 
> Are you sure the condition should be inverted ?
> 
> >  				src_fmt->xfer_func = format->xfer_func;
> > +			else
> > +				src_fmt->xfer_func =
> > +					V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
> > +
> >  			if (format->ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT)
> >  				src_fmt->ycbcr_enc = format->ycbcr_enc;
> > +			else
> > +				src_fmt->ycbcr_enc =
> > +					V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
> > +
> > +			if (format->quantization == V4L2_QUANTIZATION_DEFAULT)
> > +				src_fmt->quantization =
> > +					V4L2_MAP_QUANTIZATION_DEFAULT(false,
> > +						format->colorspace, format->ycbcr_enc);
> 
> Shouldn't this use src_fmt instead of format ?
> 
> I think quantization handling could be moved below.
> 
> >  		}
> >  
> >  		if (format->quantization != V4L2_QUANTIZATION_DEFAULT)

Now I'm wondering if this is right. As far as I can tell, the
quantization isn't taken into account by the driver when the ISP is
bypassed (capturing raw bayer data, or capturing YUV data from a YUV
sensor).

How about something like this ?

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
index d94917211828..9c215c9bb30f 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
@@ -659,11 +659,10 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
 		src_fmt->quantization = sink_fmt->quantization;

 	/*
-	 * Allow setting the source color space fields when the SET_CSC flag is
-	 * set and the source format is YUV. If the sink format is YUV, don't
-	 * set the color primaries, transfer function or YCbCr encoding as the
-	 * ISP is bypassed in that case and passes YUV data through without
-	 * modifications.
+	 * Allow setting the source color space fields when the SET_CSC flag.
+	 * This is restricted to the case where the sink format is raw and the
+	 * source format is YUV, as in other cases the ISP is bypassed and the
+	 * input data is passed through without modifications.
 	 *
 	 * The color primaries and transfer function are configured through the
 	 * cross-talk matrix and tone curve respectively. Settings for those
@@ -676,18 +675,30 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
 	 */
 	set_csc = format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC;

-	if (set_csc && src_info->pixel_enc == V4L2_PIXEL_ENC_YUV) {
-		if (sink_info->pixel_enc == V4L2_PIXEL_ENC_BAYER) {
-			if (format->colorspace != V4L2_COLORSPACE_DEFAULT)
-				src_fmt->colorspace = format->colorspace;
-			if (format->xfer_func != V4L2_XFER_FUNC_DEFAULT)
-				src_fmt->xfer_func = format->xfer_func;
-			if (format->ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT)
-				src_fmt->ycbcr_enc = format->ycbcr_enc;
-		}
+	if (set_csc && sink_info->pixel_enc == V4L2_PIXEL_ENC_BAYER &&
+	    src_info->pixel_enc == V4L2_PIXEL_ENC_YUV) {
+		if (format->colorspace != V4L2_COLORSPACE_DEFAULT)
+			src_fmt->colorspace = format->colorspace;
+
+		if (format->xfer_func != V4L2_XFER_FUNC_DEFAULT)
+			src_fmt->xfer_func = format->xfer_func;
+		else
+			src_fmt->xfer_func =
+				V4L2_MAP_XFER_FUNC_DEFAULT(src_fmt->colorspace);
+
+		if (format->ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT)
+			src_fmt->ycbcr_enc = format->ycbcr_enc;
+		else
+			src_fmt->ycbcr_enc =
+				V4L2_MAP_YCBCR_ENC_DEFAULT(src_fmt->colorspace);

 		if (format->quantization != V4L2_QUANTIZATION_DEFAULT)
 			src_fmt->quantization = format->quantization;
+		else
+			src_fmt->quantization =
+				V4L2_MAP_QUANTIZATION_DEFAULT(false,
+							      src_fmt->colorspace,
+							      src_fmt->ycbcr_enc);
 	}

 	*format = *src_fmt;

Can I let you write a commit message ? :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] media: rkisp1: Set format defaults based on requested color space

[Stefan Klug]

Hi Laurent,

Thank you for the (loong ago) review. I finally came around to wrap my
head around that again.

Quoting Laurent Pinchart (2025-03-01 15:34:53)
> On Sat, Mar 01, 2025 at 03:02:54AM +0200, Laurent Pinchart wrote:
> > On Thu, Feb 27, 2025 at 12:44:59PM +0100, Stefan Klug wrote:
> > > When color space JPEG is requested, the ISP sets the quantization
> > > incorrectly to limited range. To fix that, set the xfer_func, ycbcr_enc
> > > and quantization to the defaults for the requested color space if they
> > > are not specified explicitly.
> > 
> > The commit message fails to explain why you're addressing xfer_func and
> > ycbcr_enc to fix the quantization issue.
> > 
> > > Do this only in case we are converting
> > > from RAW to YUV.
> > 
> > And this should explain why.
> > 
> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > ---
> > >  .../media/platform/rockchip/rkisp1/rkisp1-isp.c   | 15 ++++++++++++++-
> > >  1 file changed, 14 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > > index d94917211828..468f5a7d03c7 100644
> > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > > @@ -680,10 +680,23 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
> > 
> > Adding a bit more context:
> > 
> >       set_csc = format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC;
> > 
> >       if (set_csc && src_info->pixel_enc == V4L2_PIXEL_ENC_YUV) {
> > 
> > If V4L2_MBUS_FRAMEFMT_SET_CSC isn't set, the colorspace fields on the
> > source pad will be copied from the sink pad, which doesn't seem right.
> 
> Thinking some more about it, it's not wrong either. The colorspace and
> xfer_func fields are not used by the driver, as the related ISP
> processing blocks are configured through ISP parameters. Without
> userspace providing the value of the fields on the source pad, the
> driver can't know what colorspace and xfer_func is produced. Copying the
> values from the sink pad is as good of a guess as we can make.
> 
> The ycbcr_enc and quantization fields are different, as they are taken
> into account by the driver to configure the ISP. Copying ycbcr_enc from
> the sink pad means that it will be set to V4L2_YCBCR_ENC_601 when the
> sink format is bayer and the source format is YUV. As the sink
> colorspace is most likely going to be V4L2_COLORSPACE_RAW in that case,
> that's a fine default, and is identical to what we would get from
> V4L2_MAP_YCBCR_ENC_DEFAULT(). Setting the quantization to
> V4L2_QUANTIZATION_LIM_RANGE also seems fine as a default, and it what
> V4L2_MAP_QUANTIZATION_DEFAULT() would give us.
> 
> TL;DR: there's probably no need to change the current behaviour when
> V4L2_MBUS_FRAMEFMT_SET_CSC isn't set.

I partially agree. Basically we don't care about colorspace and
xfer_func because we know that it is not used by the driver. But
src_fmt->colorspace is now V4L2_COLORSPACE_RAW and that could also be
queried by the user if I'm not mistaken. Wouldn't it be better (and
clearer code wise) to explicitly set the defaults in case we are
converting from RAW to YUV? Something like

	/*
	 * Copy the color space for the sink pad. When converting from Bayer to
	 * YUV, set proper defaults.
	 */
	if (sink_info->pixel_enc == V4L2_PIXEL_ENC_BAYER &&
	    src_info->pixel_enc == V4L2_PIXEL_ENC_YUV) {
		src_fmt->colorspace = V4L2_COLORSPACE_SRGB;
		src_fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
		src_fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
		src_fmt->quantization = V4L2_QUANTIZATION_LIM_RANGE;
	} else {
		src_fmt->colorspace = sink_fmt->colorspace;
		src_fmt->xfer_func = sink_fmt->xfer_func;
		src_fmt->ycbcr_enc = sink_fmt->ycbcr_enc;
		src_fmt->quantization = sink_fmt->quantization;
	}

Functionality wise it is the same, but it makes the following code easier
to understand without the need to remember that we can safely copy
colorspace as it won't be used anyways.

> 
> > It's a separate issue, but fixing both together may lead to better code.
> > 
> > >             if (sink_info->pixel_enc == V4L2_PIXEL_ENC_BAYER) {
> > >                     if (format->colorspace != V4L2_COLORSPACE_DEFAULT)
> > >                             src_fmt->colorspace = format->colorspace;
> > > -                   if (format->xfer_func != V4L2_XFER_FUNC_DEFAULT)
> > > +
> > > +                   if (format->xfer_func == V4L2_XFER_FUNC_DEFAULT)
> > 
> > Are you sure the condition should be inverted ?

That really looks quite wrong.

> > 
> > >                             src_fmt->xfer_func = format->xfer_func;
> > > +                   else
> > > +                           src_fmt->xfer_func =
> > > +                                   V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
> > > +
> > >                     if (format->ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT)
> > >                             src_fmt->ycbcr_enc = format->ycbcr_enc;
> > > +                   else
> > > +                           src_fmt->ycbcr_enc =
> > > +                                   V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
> > > +
> > > +                   if (format->quantization == V4L2_QUANTIZATION_DEFAULT)
> > > +                           src_fmt->quantization =
> > > +                                   V4L2_MAP_QUANTIZATION_DEFAULT(false,
> > > +                                           format->colorspace, format->ycbcr_enc);
> > 
> > Shouldn't this use src_fmt instead of format ?

The outcome is the same. It felt more symmetrical to base the default
value on format->... as we do for the other fields.

> > 
> > I think quantization handling could be moved below.
> > 
> > >             }
> > >  
> > >             if (format->quantization != V4L2_QUANTIZATION_DEFAULT)
> 
> Now I'm wondering if this is right. As far as I can tell, the
> quantization isn't taken into account by the driver when the ISP is
> bypassed (capturing raw bayer data, or capturing YUV data from a YUV
> sensor).

Are you sure about the YUV to YUV case? We pass src_fmt->quatization
into rkisp1_params_pre_configure() which is then used to initializ the
remaining blocks. Iam however unsure if *any* of these blocks is active
in YUV to YUV mode.

> 
> How about something like this ?
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> index d94917211828..9c215c9bb30f 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> @@ -659,11 +659,10 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
>                 src_fmt->quantization = sink_fmt->quantization;
> 
>         /*
> -        * Allow setting the source color space fields when the SET_CSC flag is
> -        * set and the source format is YUV. If the sink format is YUV, don't
> -        * set the color primaries, transfer function or YCbCr encoding as the
> -        * ISP is bypassed in that case and passes YUV data through without
> -        * modifications.
> +        * Allow setting the source color space fields when the SET_CSC flag.
> +        * This is restricted to the case where the sink format is raw and the
> +        * source format is YUV, as in other cases the ISP is bypassed and the
> +        * input data is passed through without modifications.

Yes, that seems legit and makes the logic easier to follow. Only concern
is the YUV to YUV mode.

>          *
>          * The color primaries and transfer function are configured through the
>          * cross-talk matrix and tone curve respectively. Settings for those
> @@ -676,18 +675,30 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
>          */
>         set_csc = format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC;
> 
> -       if (set_csc && src_info->pixel_enc == V4L2_PIXEL_ENC_YUV) {
> -               if (sink_info->pixel_enc == V4L2_PIXEL_ENC_BAYER) {
> -                       if (format->colorspace != V4L2_COLORSPACE_DEFAULT)
> -                               src_fmt->colorspace = format->colorspace;
> -                       if (format->xfer_func != V4L2_XFER_FUNC_DEFAULT)
> -                               src_fmt->xfer_func = format->xfer_func;
> -                       if (format->ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT)
> -                               src_fmt->ycbcr_enc = format->ycbcr_enc;
> -               }
> +       if (set_csc && sink_info->pixel_enc == V4L2_PIXEL_ENC_BAYER &&
> +           src_info->pixel_enc == V4L2_PIXEL_ENC_YUV) {
> +               if (format->colorspace != V4L2_COLORSPACE_DEFAULT)
> +                       src_fmt->colorspace = format->colorspace;
> +
> +               if (format->xfer_func != V4L2_XFER_FUNC_DEFAULT)
> +                       src_fmt->xfer_func = format->xfer_func;
> +               else
> +                       src_fmt->xfer_func =
> +                               V4L2_MAP_XFER_FUNC_DEFAULT(src_fmt->colorspace);
> +
> +               if (format->ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT)
> +                       src_fmt->ycbcr_enc = format->ycbcr_enc;
> +               else
> +                       src_fmt->ycbcr_enc =
> +                               V4L2_MAP_YCBCR_ENC_DEFAULT(src_fmt->colorspace);
> 
>                 if (format->quantization != V4L2_QUANTIZATION_DEFAULT)
>                         src_fmt->quantization = format->quantization;
> +               else
> +                       src_fmt->quantization =
> +                               V4L2_MAP_QUANTIZATION_DEFAULT(false,
> +                                                             src_fmt->colorspace,
> +                                                             src_fmt->ycbcr_enc);
>         }
> 
>         *format = *src_fmt;
> 
> Can I let you write a commit message ? :-)

I try to get bak to it :-)

Best regards,
Stefan

> 
> -- 
> Regards,
> 
> Laurent Pinchart
>

Re: [PATCH 1/3] media: rkisp1: Set format defaults based on requested color space

[Laurent Pinchart]

On Wed, Feb 25, 2026 at 01:21:20PM +0100, Stefan Klug wrote:
> Hi Laurent,
> 
> Thank you for the (loong ago) review. I finally came around to wrap my
> head around that again.
> 
> Quoting Laurent Pinchart (2025-03-01 15:34:53)
> > On Sat, Mar 01, 2025 at 03:02:54AM +0200, Laurent Pinchart wrote:
> > > On Thu, Feb 27, 2025 at 12:44:59PM +0100, Stefan Klug wrote:
> > > > When color space JPEG is requested, the ISP sets the quantization
> > > > incorrectly to limited range. To fix that, set the xfer_func, ycbcr_enc
> > > > and quantization to the defaults for the requested color space if they
> > > > are not specified explicitly.
> > > 
> > > The commit message fails to explain why you're addressing xfer_func and
> > > ycbcr_enc to fix the quantization issue.
> > > 
> > > > Do this only in case we are converting
> > > > from RAW to YUV.
> > > 
> > > And this should explain why.
> > > 
> > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > > ---
> > > >  .../media/platform/rockchip/rkisp1/rkisp1-isp.c   | 15 ++++++++++++++-
> > > >  1 file changed, 14 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > > > index d94917211828..468f5a7d03c7 100644
> > > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > > > @@ -680,10 +680,23 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
> > > 
> > > Adding a bit more context:
> > > 
> > >       set_csc = format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC;
> > > 
> > >       if (set_csc && src_info->pixel_enc == V4L2_PIXEL_ENC_YUV) {
> > > 
> > > If V4L2_MBUS_FRAMEFMT_SET_CSC isn't set, the colorspace fields on the
> > > source pad will be copied from the sink pad, which doesn't seem right.
> > 
> > Thinking some more about it, it's not wrong either. The colorspace and
> > xfer_func fields are not used by the driver, as the related ISP
> > processing blocks are configured through ISP parameters. Without
> > userspace providing the value of the fields on the source pad, the
> > driver can't know what colorspace and xfer_func is produced. Copying the
> > values from the sink pad is as good of a guess as we can make.
> > 
> > The ycbcr_enc and quantization fields are different, as they are taken
> > into account by the driver to configure the ISP. Copying ycbcr_enc from
> > the sink pad means that it will be set to V4L2_YCBCR_ENC_601 when the
> > sink format is bayer and the source format is YUV. As the sink
> > colorspace is most likely going to be V4L2_COLORSPACE_RAW in that case,
> > that's a fine default, and is identical to what we would get from
> > V4L2_MAP_YCBCR_ENC_DEFAULT(). Setting the quantization to
> > V4L2_QUANTIZATION_LIM_RANGE also seems fine as a default, and it what
> > V4L2_MAP_QUANTIZATION_DEFAULT() would give us.
> > 
> > TL;DR: there's probably no need to change the current behaviour when
> > V4L2_MBUS_FRAMEFMT_SET_CSC isn't set.
> 
> I partially agree. Basically we don't care about colorspace and
> xfer_func because we know that it is not used by the driver. But
> src_fmt->colorspace is now V4L2_COLORSPACE_RAW and that could also be
> queried by the user if I'm not mistaken. Wouldn't it be better (and
> clearer code wise) to explicitly set the defaults in case we are
> converting from RAW to YUV? Something like
> 
> 	/*
> 	 * Copy the color space for the sink pad. When converting from Bayer to
> 	 * YUV, set proper defaults.
> 	 */
> 	if (sink_info->pixel_enc == V4L2_PIXEL_ENC_BAYER &&
> 	    src_info->pixel_enc == V4L2_PIXEL_ENC_YUV) {
> 		src_fmt->colorspace = V4L2_COLORSPACE_SRGB;
> 		src_fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
> 		src_fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
> 		src_fmt->quantization = V4L2_QUANTIZATION_LIM_RANGE;
> 	} else {
> 		src_fmt->colorspace = sink_fmt->colorspace;
> 		src_fmt->xfer_func = sink_fmt->xfer_func;
> 		src_fmt->ycbcr_enc = sink_fmt->ycbcr_enc;
> 		src_fmt->quantization = sink_fmt->quantization;
> 	}
> 
> Functionality wise it is the same, but it makes the following code easier
> to understand without the need to remember that we can safely copy
> colorspace as it won't be used anyways.
> 
> > > It's a separate issue, but fixing both together may lead to better code.
> > > 
> > > >             if (sink_info->pixel_enc == V4L2_PIXEL_ENC_BAYER) {
> > > >                     if (format->colorspace != V4L2_COLORSPACE_DEFAULT)
> > > >                             src_fmt->colorspace = format->colorspace;
> > > > -                   if (format->xfer_func != V4L2_XFER_FUNC_DEFAULT)
> > > > +
> > > > +                   if (format->xfer_func == V4L2_XFER_FUNC_DEFAULT)
> > > 
> > > Are you sure the condition should be inverted ?
> 
> That really looks quite wrong.
> 
> > > >                             src_fmt->xfer_func = format->xfer_func;
> > > > +                   else
> > > > +                           src_fmt->xfer_func =
> > > > +                                   V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
> > > > +
> > > >                     if (format->ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT)
> > > >                             src_fmt->ycbcr_enc = format->ycbcr_enc;
> > > > +                   else
> > > > +                           src_fmt->ycbcr_enc =
> > > > +                                   V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
> > > > +
> > > > +                   if (format->quantization == V4L2_QUANTIZATION_DEFAULT)
> > > > +                           src_fmt->quantization =
> > > > +                                   V4L2_MAP_QUANTIZATION_DEFAULT(false,
> > > > +                                           format->colorspace, format->ycbcr_enc);
> > > 
> > > Shouldn't this use src_fmt instead of format ?
> 
> The outcome is the same. It felt more symmetrical to base the default
> value on format->... as we do for the other fields.

If format->colorspace is V4L2_COLORSPACE_DEFAULT then I think it won't
be the same. the V4L2_MAP_*_DEFAULT macros don't expect their argument
to be a *_DEFAULT value.

> > > I think quantization handling could be moved below.
> > > 
> > > >             }
> > > >  
> > > >             if (format->quantization != V4L2_QUANTIZATION_DEFAULT)
> > 
> > Now I'm wondering if this is right. As far as I can tell, the
> > quantization isn't taken into account by the driver when the ISP is
> > bypassed (capturing raw bayer data, or capturing YUV data from a YUV
> > sensor).
> 
> Are you sure about the YUV to YUV case? We pass src_fmt->quatization
> into rkisp1_params_pre_configure() which is then used to initializ the
> remaining blocks. Iam however unsure if *any* of these blocks is active
> in YUV to YUV mode.

That's a good question. It should be tested. Isaac may have checked when
working on YUV passthrough mode.

> > How about something like this ?
> > 
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > index d94917211828..9c215c9bb30f 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > @@ -659,11 +659,10 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
> >                 src_fmt->quantization = sink_fmt->quantization;
> > 
> >         /*
> > -        * Allow setting the source color space fields when the SET_CSC flag is
> > -        * set and the source format is YUV. If the sink format is YUV, don't
> > -        * set the color primaries, transfer function or YCbCr encoding as the
> > -        * ISP is bypassed in that case and passes YUV data through without
> > -        * modifications.
> > +        * Allow setting the source color space fields when the SET_CSC flag.
> > +        * This is restricted to the case where the sink format is raw and the
> > +        * source format is YUV, as in other cases the ISP is bypassed and the
> > +        * input data is passed through without modifications.
> 
> Yes, that seems legit and makes the logic easier to follow. Only concern
> is the YUV to YUV mode.
> 
> >          *
> >          * The color primaries and transfer function are configured through the
> >          * cross-talk matrix and tone curve respectively. Settings for those
> > @@ -676,18 +675,30 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
> >          */
> >         set_csc = format->flags & V4L2_MBUS_FRAMEFMT_SET_CSC;
> > 
> > -       if (set_csc && src_info->pixel_enc == V4L2_PIXEL_ENC_YUV) {
> > -               if (sink_info->pixel_enc == V4L2_PIXEL_ENC_BAYER) {
> > -                       if (format->colorspace != V4L2_COLORSPACE_DEFAULT)
> > -                               src_fmt->colorspace = format->colorspace;
> > -                       if (format->xfer_func != V4L2_XFER_FUNC_DEFAULT)
> > -                               src_fmt->xfer_func = format->xfer_func;
> > -                       if (format->ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT)
> > -                               src_fmt->ycbcr_enc = format->ycbcr_enc;
> > -               }
> > +       if (set_csc && sink_info->pixel_enc == V4L2_PIXEL_ENC_BAYER &&
> > +           src_info->pixel_enc == V4L2_PIXEL_ENC_YUV) {
> > +               if (format->colorspace != V4L2_COLORSPACE_DEFAULT)
> > +                       src_fmt->colorspace = format->colorspace;
> > +
> > +               if (format->xfer_func != V4L2_XFER_FUNC_DEFAULT)
> > +                       src_fmt->xfer_func = format->xfer_func;
> > +               else
> > +                       src_fmt->xfer_func =
> > +                               V4L2_MAP_XFER_FUNC_DEFAULT(src_fmt->colorspace);
> > +
> > +               if (format->ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT)
> > +                       src_fmt->ycbcr_enc = format->ycbcr_enc;
> > +               else
> > +                       src_fmt->ycbcr_enc =
> > +                               V4L2_MAP_YCBCR_ENC_DEFAULT(src_fmt->colorspace);
> > 
> >                 if (format->quantization != V4L2_QUANTIZATION_DEFAULT)
> >                         src_fmt->quantization = format->quantization;
> > +               else
> > +                       src_fmt->quantization =
> > +                               V4L2_MAP_QUANTIZATION_DEFAULT(false,
> > +                                                             src_fmt->colorspace,
> > +                                                             src_fmt->ycbcr_enc);
> >         }
> > 
> >         *format = *src_fmt;
> > 
> > Can I let you write a commit message ? :-)
> 
> I try to get bak to it :-)

I'd like to send a pull request tomorrow. It's a bit of a short notice,
we can also merge this for v7.2.

-- 
Regards,

Laurent Pinchart

[PATCH 2/3] media: rkisp1: Fix the quantization settings of CPROC

[Stefan Klug]

On the imx8mp the Image Effect module is not supported. In my case the
effect variable had an uninitialized default value of 0x700 leading to
limited YUV range in all cases (effects were never touched from user
space). The effects configuration is as far is I understand completely
independent of CPROC. Completely remove that check. This fixes full
range mode on imx8mp and possibly others.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 drivers/media/platform/rockchip/rkisp1/rkisp1-params.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
index b28f4140c8a3..8d61e21ad475 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
@@ -764,11 +764,6 @@ static void rkisp1_aec_config_v12(struct rkisp1_params *params,
 static void rkisp1_cproc_config(struct rkisp1_params *params,
 				const struct rkisp1_cif_isp_cproc_config *arg)
 {
-	struct rkisp1_cif_isp_isp_other_cfg *cur_other_cfg =
-		container_of(arg, struct rkisp1_cif_isp_isp_other_cfg, cproc_config);
-	struct rkisp1_cif_isp_ie_config *cur_ie_config =
-						&cur_other_cfg->ie_config;
-	u32 effect = cur_ie_config->effect;
 	u32 quantization = params->quantization;
 
 	rkisp1_write(params->rkisp1, RKISP1_CIF_C_PROC_CONTRAST,
@@ -778,8 +773,7 @@ static void rkisp1_cproc_config(struct rkisp1_params *params,
 	rkisp1_write(params->rkisp1, RKISP1_CIF_C_PROC_BRIGHTNESS,
 		     arg->brightness);
 
-	if (quantization != V4L2_QUANTIZATION_FULL_RANGE ||
-	    effect != V4L2_COLORFX_NONE) {
+	if (quantization != V4L2_QUANTIZATION_FULL_RANGE) {
 		rkisp1_param_clear_bits(params, RKISP1_CIF_C_PROC_CTRL,
 					RKISP1_CIF_C_PROC_YOUT_FULL |
 					RKISP1_CIF_C_PROC_YIN_FULL |
-- 
2.43.0

Re: [PATCH 2/3] media: rkisp1: Fix the quantization settings of CPROC

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Thu, Feb 27, 2025 at 12:45:00PM +0100, Stefan Klug wrote:
> On the imx8mp the Image Effect module is not supported.

Could you please include a patch that adds a feature flag for this, and
disables the feature on i.MX8MP ? That requires:

- Clearing the module update bits based on the feature flag in
  rkisp1_isp_isr_other_config(), as done for BLS.
- Marking the RKISP1_EXT_PARAMS_BLOCK_TYPE_IE entry in
  rkisp1_ext_params_handler with the new feature flag.

> In my case the
> effect variable had an uninitialized default value of 0x700 leading to
> limited YUV range in all cases (effects were never touched from user
> space).

I don't think the value of an uninitialized variable is relevant :-)

> The effects configuration is as far is I understand completely
> independent of CPROC.

I agree the CPROC and IMGEFF blocks are most likely separate, but they
both need to be configured according to the selected quantization.

Quantization is applied by the CSM (CSC) block, configured in
rkisp1_csm_config(). The CPROC module follows, and its
RKISP1_CIF_C_PROC_CTRL register needs to take quantization into account.
It has one bit to indicate whether its input data (produced by the CSM
block) is in full or limited range, and two bits to set the range of
output data (separately for luma and chroma, I have no idea why).

The rkisp1_cproc_config() function assumes that the input and output
quantization of the CPROC are always the same. It sets or clears all
three configuration bits based on the quantization value. As you've
noticed, it will also hardcode limited range when an image effect is
selected. That seems wrong indeed.

Note that the CPROC quantization bits only controls the Y offset
(subtracted on the input side and added on the output side) and the Y
and C clipping (on the output side). If we were to have different
quantization on the input and output, the scaling factor to adjust the
range seems to be something that userspace needs to take into account
when calculating values for the other CPROC registers.

Then, the IE (IMGEFF) block processes the data, and also needs to be
configured based on the quantization range. The control register has a
single bit that selects limited or full range. It is set in
rkisp1_ie_config():

	if (params->quantization == V4L2_QUANTIZATION_FULL_RANGE)
		eff_ctrl |= RKISP1_CIF_IMG_EFF_CTRL_YCBCR_FULL;

There's a note in the documentation of the RKISP1_CIF_IMG_EFF_CTRL
register in the RK3399 registers manual that states

  Note: full_range for image effects is supported in ISP M5_v6, M5_v7 only

The ISP version in the RK3399 seems to be M14_v2. This may be why the
code disables full range quantization in rkisp1_cproc_config() when a
image effect is selected.

All of this is a mess, and to make it worse, the implementation in the
driver is quite broken. The effect is selected by the
RKISP1_CIF_IMG_EFF_CTRL register, written in rkisp1_ie_config(), and the
rkisp1_ie_enable() function then rewrites the whole register to enable
the module, overwriting the selected effect. Only when userspace sets
the effect without updating the module enable bit does the driver seem
to handle things correctly.

Now, how do we handle this ? I think this patch is fine, but the commit
message needs a rewrite to summarize the above, and explain that the
CPROC never changes quantization. A corresponding comment in
rkisp1_cproc_config() would be useful.

Regarding the image effect configuration, I think we can consider that
it should only be enabled by userspace when using limited range, if
running on a device that doesn't support full range for image effects.
The driver doesn't need to protect against this in my opinion. A comment
in rkisp1_ie_config() could also be useful to explain that.

> Completely remove that check. This fixes full
> range mode on imx8mp and possibly others.
>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  drivers/media/platform/rockchip/rkisp1/rkisp1-params.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> index b28f4140c8a3..8d61e21ad475 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> @@ -764,11 +764,6 @@ static void rkisp1_aec_config_v12(struct rkisp1_params *params,
>  static void rkisp1_cproc_config(struct rkisp1_params *params,
>  				const struct rkisp1_cif_isp_cproc_config *arg)
>  {
> -	struct rkisp1_cif_isp_isp_other_cfg *cur_other_cfg =
> -		container_of(arg, struct rkisp1_cif_isp_isp_other_cfg, cproc_config);
> -	struct rkisp1_cif_isp_ie_config *cur_ie_config =
> -						&cur_other_cfg->ie_config;
> -	u32 effect = cur_ie_config->effect;
>  	u32 quantization = params->quantization;
>  
>  	rkisp1_write(params->rkisp1, RKISP1_CIF_C_PROC_CONTRAST,
> @@ -778,8 +773,7 @@ static void rkisp1_cproc_config(struct rkisp1_params *params,
>  	rkisp1_write(params->rkisp1, RKISP1_CIF_C_PROC_BRIGHTNESS,
>  		     arg->brightness);
>  
> -	if (quantization != V4L2_QUANTIZATION_FULL_RANGE ||
> -	    effect != V4L2_COLORFX_NONE) {
> +	if (quantization != V4L2_QUANTIZATION_FULL_RANGE) {
>  		rkisp1_param_clear_bits(params, RKISP1_CIF_C_PROC_CTRL,
>  					RKISP1_CIF_C_PROC_YOUT_FULL |
>  					RKISP1_CIF_C_PROC_YIN_FULL |

-- 
Regards,

Laurent Pinchart

[PATCH 3/3] media: rkisp1: Remove unnecessary defines

[Stefan Klug]

The effect modes are not shifts but numbers which are already defined a
few lines above. Remove the misleading defines.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
index bf0260600a19..139177db9c6d 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
@@ -327,13 +327,6 @@
 #define RKISP1_CIF_IMG_EFF_CTRL_CFG_UPD			BIT(4)
 #define RKISP1_CIF_IMG_EFF_CTRL_YCBCR_FULL		BIT(5)
 
-#define RKISP1_CIF_IMG_EFF_CTRL_MODE_BLACKWHITE_SHIFT	0
-#define RKISP1_CIF_IMG_EFF_CTRL_MODE_NEGATIVE_SHIFT	1
-#define RKISP1_CIF_IMG_EFF_CTRL_MODE_SEPIA_SHIFT	2
-#define RKISP1_CIF_IMG_EFF_CTRL_MODE_COLOR_SEL_SHIFT	3
-#define RKISP1_CIF_IMG_EFF_CTRL_MODE_EMBOSS_SHIFT	4
-#define RKISP1_CIF_IMG_EFF_CTRL_MODE_SKETCH_SHIFT	5
-#define RKISP1_CIF_IMG_EFF_CTRL_MODE_SHARPEN_SHIFT	6
 #define RKISP1_CIF_IMG_EFF_CTRL_MODE_MASK		0xe
 
 /* IMG_EFF_COLOR_SEL */
-- 
2.43.0

Re: [PATCH 3/3] media: rkisp1: Remove unnecessary defines

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Thu, Feb 27, 2025 at 12:45:01PM +0100, Stefan Klug wrote:
> The effect modes are not shifts but numbers which are already defined a
> few lines above. Remove the misleading defines.

s/misleading/misleading and unused/ (I didn't expect you to
intentionally break the build, and I'm sure it would be caught by my
build tests, but making it clear for reviewers is always nice)

> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

No need to resubmit for this, I can update the commit message.

> ---
>  drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
> index bf0260600a19..139177db9c6d 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
> @@ -327,13 +327,6 @@
>  #define RKISP1_CIF_IMG_EFF_CTRL_CFG_UPD			BIT(4)
>  #define RKISP1_CIF_IMG_EFF_CTRL_YCBCR_FULL		BIT(5)
>  
> -#define RKISP1_CIF_IMG_EFF_CTRL_MODE_BLACKWHITE_SHIFT	0
> -#define RKISP1_CIF_IMG_EFF_CTRL_MODE_NEGATIVE_SHIFT	1
> -#define RKISP1_CIF_IMG_EFF_CTRL_MODE_SEPIA_SHIFT	2
> -#define RKISP1_CIF_IMG_EFF_CTRL_MODE_COLOR_SEL_SHIFT	3
> -#define RKISP1_CIF_IMG_EFF_CTRL_MODE_EMBOSS_SHIFT	4
> -#define RKISP1_CIF_IMG_EFF_CTRL_MODE_SKETCH_SHIFT	5
> -#define RKISP1_CIF_IMG_EFF_CTRL_MODE_SHARPEN_SHIFT	6
>  #define RKISP1_CIF_IMG_EFF_CTRL_MODE_MASK		0xe
>  
>  /* IMG_EFF_COLOR_SEL */

-- 
Regards,

Laurent Pinchart