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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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