Hi Mirela,
Thank you for the review.
On Sun, Feb 09, 2025 at 07:14:37PM +0200, Mirela Rabulea wrote:
> 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?
Once we have these merged, I'd expect new drivers to use common raw sensor
model (as long as it's suitable for the device) and the Yxx mbus codes and
these controls. I think it's fine to keep using the old pattern specific
mbus codes in addition to that.
>
> 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?
Correct. The CFA pattern isn't directly changeable by the user space so
this is similar to existing pattern specific mbus codes.
>
> 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?
The native pattern which isn't affected by flipping or cropping. I'll add
it to the documentation here, too.
>
> 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@xxxxxxxxxxxxxxx>
> > ---
> > .../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.
Correct. Could you write a patch for that, please? :-)
I'll add more documentation to the above four patterns, that should be a
better starting point than this version.
>
> 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?
They're all here
<URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=metadata-raw>.
It seems I forgot this from the cover page.
--
Kind regards,
Sakari Ailus
