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)

quoted

On Sat, Mar 01, 2025 at 03:02:54AM +0200, Laurent Pinchart wrote:

quoted

On Thu, Feb 27, 2025 at 12:44:59PM +0100, Stefan Klug wrote:

quoted

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.

quoted

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

And this should explain why.

quoted

Signed-off-by: Stefan Klug <redacted>
---
 .../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.

quoted

quoted

It's a separate issue, but fixing both together may lead to better code.

quoted

            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.

quoted

quoted

quoted

                            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.

quoted

quoted

I think quantization handling could be moved below.

quoted

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

quoted

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.

quoted

         *
         * 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