Hi Hans,
On Sat, Jan 25, 2025 at 11:12:16AM +0100, Hans de Goede wrote:
> Hi Laurent,
>
> On 25-Jan-25 1:14 AM, Laurent Pinchart wrote:
> > Hi Hans,
> >
> > On Fri, Jan 24, 2025 at 06:17:40PM +0100, Hans de Goede wrote:
> >> On 26-Nov-24 4:50 PM, Mirela Rabulea wrote:
> >>> Add a v4l2 subdevice driver for the Omnivision OX05B1S RGB-IR sensor.
> >>>
> >>> The Omnivision OX05B1S is a 1/2.5-Inch CMOS image sensor with an
> >>> active array size of 2592 x 1944.
> >>>
> >>> The following features are supported for OX05B1S:
> >>> - Manual exposure an gain control support
> >>> - vblank/hblank control support
> >>> - Supported resolution: 2592 x 1944 @ 30fps (SGRBG10)
> >>>
> >>> Signed-off-by: Mirela Rabulea <mirela.rabulea@xxxxxxx>
> >>
> >> Thank you for your contribution, I noticed in $subject
> >> that the model-nr ends in a "S" and that you describe
> >> this as a RGB-IR sensor.
> >>
> >> I've been working on OV01A1S support myself and one of
> >> the issues is how to communicate the RGB-IR color-filter
> >> to userspace.
> >>
> >> <snip>
> >>
> >> I see here:
> >>
> >>> +static const struct ox05b1s_sizes ox05b1s_supported_codes[] = {
> >>> + {
> >>> + .code = MEDIA_BUS_FMT_SGRBG10_1X10,
> >>> + .sizes = ox05b1s_sgrbg10_sizes,
> >>> + }, {
> >>> + /* sentinel */
> >>> + }
> >>> +};
> >>
> >> That you are using MEDIA_BUS_FMT_SGRBG10_1X10, but that suggests
> >> this sensor is using a plain Bayer color filter which is not correct.
> >>
> >> Here is what I have come up with:
> >>
> >> diff --git a/include/linux/drm_fourcc.h b/include/linux/drm_fourcc.h
> >> index db679877..68ed65c5 100644
> >> --- a/include/linux/drm_fourcc.h
> >> +++ b/include/linux/drm_fourcc.h
> >> @@ -447,6 +447,8 @@ extern "C" {
> >> #define DRM_FORMAT_SGRBG10 fourcc_code('B', 'A', '1', '0')
> >> #define DRM_FORMAT_SGBRG10 fourcc_code('G', 'B', '1', '0')
> >> #define DRM_FORMAT_SBGGR10 fourcc_code('B', 'G', '1', '0')
> >> +/* Mixed 10 bit bayer + ir pixel pattern found on Omnivision ov01a1s */
> >> +#define DRM_FORMAT_SIGIG_GBGR_IGIG_GRGB10 fourcc_code('O', 'V', '1', 'S')
> >>
> >> /* 12-bit Bayer formats */
> >> #define DRM_FORMAT_SRGGB12 fourcc_code('R', 'G', '1', '2')
> >> diff --git a/include/linux/media-bus-format.h b/include/linux/media-bus-format.h
> >> index b6acf8c8..e2938f0d 100644
> >> --- a/include/linux/media-bus-format.h
> >> +++ b/include/linux/media-bus-format.h
> >> @@ -122,7 +122,7 @@
> >> #define MEDIA_BUS_FMT_YUV16_1X48 0x202a
> >> #define MEDIA_BUS_FMT_UYYVYY16_0_5X48 0x202b
> >>
> >> -/* Bayer - next is 0x3025 */
> >> +/* Bayer - next is 0x3026 */
> >> #define MEDIA_BUS_FMT_SBGGR8_1X8 0x3001
> >> #define MEDIA_BUS_FMT_SGBRG8_1X8 0x3013
> >> #define MEDIA_BUS_FMT_SGRBG8_1X8 0x3002
> >> @@ -159,6 +159,8 @@
> >> #define MEDIA_BUS_FMT_SGBRG20_1X20 0x3022
> >> #define MEDIA_BUS_FMT_SGRBG20_1X20 0x3023
> >> #define MEDIA_BUS_FMT_SRGGB20_1X20 0x3024
> >> +/* Mixed bayer + ir pixel pattern found on ov01a1s */
> >> +#define MEDIA_BUS_FMT_SIGIG_GBGR_IGIG_GRGB10_1X10 0x3025
> >>
> >> /* JPEG compressed formats - next is 0x4002 */
> >> #define MEDIA_BUS_FMT_JPEG_1X8 0x4001
> >> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> >> index 3829c0b6..1b91ed0e 100644
> >> --- a/include/linux/videodev2.h
> >> +++ b/include/linux/videodev2.h
> >> @@ -706,6 +706,9 @@ struct v4l2_pix_format {
> >> #define V4L2_PIX_FMT_SGBRG16 v4l2_fourcc('G', 'B', '1', '6') /* 16 GBGB.. RGRG.. */
> >> #define V4L2_PIX_FMT_SGRBG16 v4l2_fourcc('G', 'R', '1', '6') /* 16 GRGR.. BGBG.. */
> >> #define V4L2_PIX_FMT_SRGGB16 v4l2_fourcc('R', 'G', '1', '6') /* 16 RGRG.. GBGB.. */
> >> + /* 10bit mixed bayer + ir pixel pattern found on ov01a1s */
> >> +#define V4L2_PIX_FMT_SIGIG_GBGR_IGIG_GRGB10 v4l2_fourcc('O', 'V', '1', 'S') /* unpacked */
> >> +#define V4L2_PIX_FMT_SIGIG_GBGR_IGIG_GRGB10P v4l2_fourcc('O', 'V', '1', 'P') /* packed */
> >>
> >> /* HSV formats */
> >> #define V4L2_PIX_FMT_HSV24 v4l2_fourcc('H', 'S', 'V', '3')
> >>
> >> For this, note:
> >>
> >> 1. This is against libcamera's copy of the relevant linux headers, the paths
> >> to the .h files are different in the kernel
> >>
> >> 2. Since I wrote this I learned that it looks like the intel driver for
> >> the ov01a1s:
> >>
> >> https://github.com/intel/ipu6-drivers/blob/master/drivers/media/i2c/ov01a1s.c
> >>
> >> may have enabled horizontal-flip / mirroring by default, which means that
> >> the order of each of the quads needs to be flipped.
> >>
> >> IMHO we need to resolve how to communicate the color-filters used on
> >> these OV xxxxx"S" models to userspace. When I last discussed this with
> >> Laurent, Laurent suggested using V4L2_PIX_FMT_Y10, combined with
> >> a new field or v4l2-control to query the actual filter.
> >
> > Yes, adding new pixel formats and media bus codes to represent CFA
> > patterns won't scale. We need to switch to using controls to report
> > those. Sakari is already working on a series for that.
>
> Interesting, do you have a link to Sakari's work ?
I'm about to post v5 soonish, I'll cc you.
The current patches are two top-most here (metadata-raw branch)
<URL:https://git.retiisi.eu/?p=~sailus/linux.git;a=commitdiff;h=81aa6e3aad1fdf3b2da4a930a0f4e0520a805edd>.
>
> >> The idea is to separate the depth/packing info from the filter info,
> >> avoiding the possible combinatioral explosion of needing this special
> >> filter with all possible deths. I gave this a quick try but at least on
> >> the libcamera side there is still a lot of code assuming that both
> >> depth and color-filter-type are communicated through a single fmt
> >> integer. Also it seems that in practice there only is this one new
> >> RGB-IR color filter used on the OV xxxxx"S" models so I think we
> >> need just 1 new V4L2_PIX_FMT_ define (*).
> >
> > Changes in libcamera are surely needed. There's work to be done, we'll
> > do the work, and we'll solve this problem. Let's focus the effort in
> > that direction.
>
> Ok, so what does that mean for upstreaming support for omnivision
> OVxxxxS sensors? Clearly advertising MEDIA_BUS_FMT_SGRBG10_1X10 is
> wrong. So I guess that upstreaming this driver needs to wait until
> at least the kernel API side of this is resolved?
>
> Sensors relying on the new CFA control to communicatethe CFA type
> could use a new (e.g.) MEDIA_BUS_FMT_RAW_1X10 or are we going to re-use
> the monochrome (Y only) media bus fmts, so e.g. this sensor would
> advertise MEDIA_BUS_FMT_Y10_1X10 and then the CFA control would provide
> the actual CFA info ?
>
> IMHO re-using the monochrome (Y only) media bus fmts seems best
> to avoid needing to have to make a "RAW" copy of all of them.
That's an interesting idea. I'll consider this for the next version of the
sensor API patchset, currently I don't see issues with this approach. The
advantage would be that the receiver drivers don't need to explicitly
support the common CSI-2 raw formats.
>
> This also matches with what we are already doing for IR only sensors.
> AFAIK MEDIA_BUS_FMT_Y10_1X10 is currently already used for infrared
> sensors, so sticking with that and adding a IR CFA option to
> the CFA control to make clear this really is IR (*) seems to make
> sensor for IR only sensors. At which point extending this to RGB-IR
> sensors seems logical.
--
Kind regards,
Sakari Ailus
