Re: [PATCH v11 19/66] media: uapi: Add V4L2_CID_COLOR_PATTERN for describing color patterns

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Laurent,

On Wed, Sep 03, 2025 at 05:05:07PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Mon, Aug 25, 2025 at 12:50:20PM +0300, Sakari Ailus wrote:
> > Add V4L2_CID_COLOR_PATTERN to tell the camera sensor's native color
> > pattern.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  .../media/v4l/ext-ctrls-image-source.rst      | 39 +++++++++++++++++++
> >  .../media/v4l/subdev-formats.rst              |  2 +
> >  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  1 +
> >  include/uapi/linux/v4l2-controls.h            |  6 +++
> >  4 files changed, 48 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..b19aaaffbce0 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,42 @@ 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_COLOR_PATTERN (integer)``
> > +    This control determines the color components and native pixel order in the
> > +    sensor's CFA (Color Filter Array) when used in conjunction with
> > +    :ref:`generic raw mbus codes MEDIA_BUS_FMT_RAW_x (where 'x' is the bit
> > +    depth) <v4l2-mbus-pixelcode-generic-raw>` pixelcodes. The native pixel
> > +    order is constant for a given device, it is not affected by cropping or
> > +    flipping.
> > +
> > +    This control may only be used on V4L2 sub-devices.
> > +
> > +    This is a read-only control.
> > +
> > +    Available patterns:
> > +
> > +.. flat-table:: V4L2 color patterns
> > +    :header-rows:  1
> > +    :stub-columns: 0
> > +    :widths:       1 2 1
> > +
> > +    * - Macro name
> > +      - Synopsis
> > +      - Value
> > +    * - V4L2_COLOR_PATTERN_GRBG
> > +      - Raw Bayer, with alternating lines beginning with green, red pixels and
> > +        blue, green pixels.
> 
> I know what you mean, but it's a bit vague. Let's try to be more
> precise:
> 
>       - Bayer pattern with 2x2 macroblocks. Each macroblock stores pixels in
>         green, red, blue, green order.
> 
> Or even more precise
> 
>       - Bayer pattern with 2x2 macroblocks. Each macroblock has a green filter
>         in the top-left and bottom-right pixels, a red filter in the top-right
> 	pixel, and a blue filter in the bottom-left pixel.

The Bayer pattern indeed uses 2x2 blocks. Seems good to me. I modified it
slightly:

      - Bayer pattern (2x2 macroblocks). Each macroblock has a green filter in
        the top-left and bottom-right pixels, a red filter in the top-right
        pixel, and a blue filter in the bottom-left pixel.

> 
> > +      - 0
> > +    * - V4L2_COLOR_PATTERN_RGGB
> > +      - Raw Bayer, with alternating lines beginning with red, green pixels and
> > +        green, blue pixels.
> > +      - 1
> > +    * - V4L2_COLOR_PATTERN_BGGR
> > +      - Raw Bayer, with alternating lines beginning with blue, green pixels and
> > +        green, red pixels.
> > +      - 2
> > +    * - V4L2_COLOR_PATTERN_GBRG
> > +      - Raw Bayer, with alternating lines beginning with green, blue pixels and
> > +        red, green pixels.
> > +      - 3
> > diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> > index 1a51b5c817f1..e77b4ad19737 100644
> > --- a/Documentation/userspace-api/media/v4l/subdev-formats.rst
> > +++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> > @@ -3434,6 +3434,8 @@ organization is given as an example for the first pixel only.
> >  
> >      \endgroup
> >  
> > +.. _v4l2-mbus-pixelcode-generic-raw:
> > +
> 
> I'd move this to the patch that adds the raw formats. No big deal
> though.
> 
> >  Generic raw formats on serial interfaces
> >  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >  
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > index dd5f06546773..8119012c2638 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_COLOR_PATTERN:		return "Color Pattern";
> >  
> 
> Shouldn't you also patch v4l2_ctrl_fill() ?

Yes.

> 
> >  	/* 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 3c0f6afe7500..e552f6655fd0 100644
> > --- a/include/uapi/linux/v4l2-controls.h
> > +++ b/include/uapi/linux/v4l2-controls.h
> > @@ -1227,6 +1227,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_COLOR_PATTERN			(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 10)
> > +#define V4L2_COLOR_PATTERN_GRBG			0U
> > +#define V4L2_COLOR_PATTERN_RGGB			1U
> > +#define V4L2_COLOR_PATTERN_BGGR			2U
> > +#define V4L2_COLOR_PATTERN_GBRG			3U
> 
> How about V4L2_CID_CFA_PATTERN and V4L2_CFA_PATTERN_* ?

We've discussed this before. I think I'll switch, it makes sense for
consistency.

> 
> > +
> >  
> >  /* Image processing controls */
> >  
> 

-- 
Regards,

Sakari Ailus