Re: [RFC v5 07/15] media: uapi: Add V4L2_CID_COLOUR_PATTERN for describing colour patterns

Mirela Rabulea
2025-02-09T17:14:45Z

Hi Sakari,

sorry for my late response.

So, this control and the luma-only mbus code is expected to be used for
all raw sensors? Or can raw sensors still use BGGR mbus codes without
the color pattern control?

I noted this is now read-only, is the intention that the user space will
only query the current CFA pattern? In case the user space will want to
change the CFA, is it expected to achieve it via V4L2_CID_HFLIP or/and
V4L2_CID_VFLIP and/or crop?

What exactly is the V4L2_CID_COLOUR_PATTERN expected to report? Is it
the sensor's native CFA pattern, or the current CFA pattern with the
current flip & crop setting?

One more suggestion below.

On 03.02.2025 10:58, Sakari Ailus wrote:
> Add V4L2_CID_COLOUR_PATTERN to tell the camera sensor's native colour
> pattern.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> .../userspace-api/media/v4l/ext-ctrls-image-source.rst | 10 ++++++++++
> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 +
> include/uapi/linux/v4l2-controls.h | 6 ++++++
> 3 files changed, 17 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst
> index 71f23f131f97..fca729512b6f 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst
> @@ -92,3 +92,13 @@ Image Source Control IDs
> representing a gain of exactly 1.0. For example, if this default value
> is reported as being (say) 128, then a value of 192 would represent
> a gain of exactly 1.5.
> +
> +``V4L2_CID_COLOUR_PATTERN (integer)``
> + This control determines the colour components and pixel order in the
> + sensor's CFA (Colour Filter Array) when used in conjunction with
> + :ref:`luma-only mbus codes MEDIA_BUS_FMT_Yx_1Xx (where 'x' is the bit depth)
> + <v4l2-mbus-pixelcode-yuv8>` pixelformats.
> +
> + This control may only be used on a V4L2 sub-device.
> +
> + This is a read-only control.
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 24c9c25e20d1..5b6a4a94f18f 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1155,6 +1155,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> case V4L2_CID_TEST_PATTERN_BLUE: return "Blue Pixel Value";
> case V4L2_CID_TEST_PATTERN_GREENB: return "Green (Blue) Pixel Value";
> case V4L2_CID_NOTIFY_GAINS: return "Notify Gains";
> + case V4L2_CID_COLOUR_PATTERN: return "Colour Pattern";
>
> /* Image processing controls */
> /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 731add75d9ee..8e761c38b995 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1214,6 +1214,12 @@ enum v4l2_jpeg_chroma_subsampling {
> #define V4L2_CID_UNIT_CELL_SIZE (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 8)
> #define V4L2_CID_NOTIFY_GAINS (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 9)
>
> +#define V4L2_CID_COLOUR_PATTERN (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 10)
> +#define V4L2_COLOUR_PATTERN_GRBG 0
> +#define V4L2_COLOUR_PATTERN_RGGB 1
> +#define V4L2_COLOUR_PATTERN_BGGR 2
> +#define V4L2_COLOUR_PATTERN_GBRG 3
> +

I see no pattern for RGB-Ir sensors.

For Omnivision ox05b1s RGB-Ir sensor for example, it has a 4x4 pattern,
we would need something like:

B G R G...

G Ir G Ir...

R G B G...

G Ir G Ir...

V4L2_COLOUR_PATTERN_BGRGGIGIRGBGGIGI

And also 3 other combinations from mirror and flip:

V4L2_COLOUR_PATTERN_GRGBIGIGGBGRIGIG

V4L2_COLOUR_PATTERN_GIGIRGBGGIGIBGRG

V4L2_COLOUR_PATTERN_IGIGGBGRIGIGGRGB

At which point we could have these patterns? Is it possible from the
beginning, or should we add them at the time when there is at least a
driver that needs them?

Also, another question, on which tree does this patch set apply?

Thanks,

Mirela

>
> /* Image processing controls */
>
> --
> 2.39.5
>