diff mbox series

[1/6] media: uapi: Add MEDIA_BUS_FMT_SGRGB_IGIG_GBGR_IGIG media bus formats

Message ID 20210427120701.21809-2-m.felsch@pengutronix.de (mailing list archive)
State New, archived
Headers show
Series Add new bayer ir formats | expand

Commit Message

Marco Felsch April 27, 2021, 12:06 p.m. UTC
Add special 8/12bit bayer media bus format for the OnSemi AR0237IR
camera sensor [1]. OnSemi calls this format RGB-IR, the pixel array
with the interleaved IR pixels looks like:

        |  G |  R |  G |  B | ...
        +----+----+----+----+---
        | IR |  G | IR |  G | ...
        +----+----+----+----+---
        |  G |  B |  G |  R | ...
        +----+----+----+----+---
        | IR |  G | IR |  G | ...
        +----+----+----+----+---
        | .. | .. | .. | .. | ..

[1] https://www.framos.com/media/pdf/96/ac/8f/AR0237CS-D-PDF-framos.pdf

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 .../media/v4l/subdev-formats.rst              | 42 +++++++++++++++++++
 include/uapi/linux/media-bus-format.h         |  4 +-
 2 files changed, 45 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart April 29, 2021, 1:51 a.m. UTC | #1
Hi Marco,

Thank you for the patch.

On Tue, Apr 27, 2021 at 02:06:56PM +0200, Marco Felsch wrote:
> Add special 8/12bit bayer media bus format for the OnSemi AR0237IR
> camera sensor [1]. OnSemi calls this format RGB-IR, the pixel array
> with the interleaved IR pixels looks like:
> 
>         |  G |  R |  G |  B | ...
>         +----+----+----+----+---
>         | IR |  G | IR |  G | ...
>         +----+----+----+----+---
>         |  G |  B |  G |  R | ...
>         +----+----+----+----+---
>         | IR |  G | IR |  G | ...
>         +----+----+----+----+---
>         | .. | .. | .. | .. | ..
> 
> [1] https://www.framos.com/media/pdf/96/ac/8f/AR0237CS-D-PDF-framos.pdf

I think we're reaching a limit of the media bus codes model here, due to
a historical mistake. The four possible Bayer patterns, times the
different number of bits per pixel, creates a lot of media bus codes,
and drivers for CSI-2 receivers and IP cores further down the pipeline
have to support them all. This is already painful, and if we had a
non-Bayer pattern such as this one, we'll open the door to an explosion
of the number of media bus codes (imagine all the different possible
arrangements of this pattern, for instance if you enable horizontal
and/or vertical flipping on the sensor). All drivers would need to be
updated to support these new bus codes, and this really kills the
current model.

The historical mistake was to tie the Bayer pattern with the media bus
code. We should really have specified raw 8/10/12/14/16 media bus codes,
and conveyed the pattern separately. Most IP cores in the pipeline don't
need to care about the pattern at all, and those who do (that's mostly
ISPs) could be programmed explicitly by userspace as long as we have an
API to retrieve the pattern from the sensor. I believe it's time to bite
the bullet and go in that direction. I'm sorry for this case of yak
shaving, but it really wouldn't be manageable anymore :-(

> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  .../media/v4l/subdev-formats.rst              | 42 +++++++++++++++++++
>  include/uapi/linux/media-bus-format.h         |  4 +-
>  2 files changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> index bd68588b2683..d774ccd57c1b 100644
> --- a/Documentation/userspace-api/media/v4l/subdev-formats.rst
> +++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> @@ -2252,6 +2252,27 @@ organization is given as an example for the first pixel only.
>        - g\ :sub:`2`
>        - g\ :sub:`1`
>        - g\ :sub:`0`
> +    * .. _MEDIA_BUS_FMT_SGRGB_IGIG_GBGR_IGIG8_1X8:
> +
> +      - MEDIA_BUS_FMT_SGRGB_IGIG_GBGR_IGIG8_1X8
> +      - 0x3021
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      - g\ :sub:`7`
> +      - g\ :sub:`6`
> +      - g\ :sub:`5`
> +      - g\ :sub:`4`
> +      - g\ :sub:`3`
> +      - g\ :sub:`2`
> +      - g\ :sub:`1`
> +      - g\ :sub:`0`
>      * .. _MEDIA-BUS-FMT-SRGGB8-1X8:
>  
>        - MEDIA_BUS_FMT_SRGGB8_1X8
> @@ -2748,6 +2769,27 @@ organization is given as an example for the first pixel only.
>        - g\ :sub:`2`
>        - g\ :sub:`1`
>        - g\ :sub:`0`
> +    * .. _MEDIA_BUS_FMT_SGRGB_IGIG_GBGR_IGIG12_1X12:
> +
> +      - MEDIA_BUS_FMT_SGRGB_IGIG_GBGR_IGIG12_1X12
> +      - 0x3022
> +      -
> +      -
> +      -
> +      -
> +      -
> +      - g\ :sub:`11`
> +      - g\ :sub:`10`
> +      - g\ :sub:`9`
> +      - g\ :sub:`8`
> +      - g\ :sub:`7`
> +      - g\ :sub:`6`
> +      - g\ :sub:`5`
> +      - g\ :sub:`4`
> +      - g\ :sub:`3`
> +      - g\ :sub:`2`
> +      - g\ :sub:`1`
> +      - g\ :sub:`0`
>      * .. _MEDIA-BUS-FMT-SRGGB12-1X12:
>  
>        - MEDIA_BUS_FMT_SRGGB12_1X12
> diff --git a/include/uapi/linux/media-bus-format.h b/include/uapi/linux/media-bus-format.h
> index 0dfc11ee243a..cdd995e44926 100644
> --- a/include/uapi/linux/media-bus-format.h
> +++ b/include/uapi/linux/media-bus-format.h
> @@ -112,10 +112,11 @@
>  #define MEDIA_BUS_FMT_YUV16_1X48		0x202a
>  #define MEDIA_BUS_FMT_UYYVYY16_0_5X48		0x202b
>  
> -/* Bayer - next is	0x3021 */
> +/* Bayer - next is	0x3023 */
>  #define MEDIA_BUS_FMT_SBGGR8_1X8		0x3001
>  #define MEDIA_BUS_FMT_SGBRG8_1X8		0x3013
>  #define MEDIA_BUS_FMT_SGRBG8_1X8		0x3002
> +#define MEDIA_BUS_FMT_SGRGB_IGIG_GBGR_IGIG8_1X8	0x3021
>  #define MEDIA_BUS_FMT_SRGGB8_1X8		0x3014
>  #define MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8		0x3015
>  #define MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8		0x3016
> @@ -136,6 +137,7 @@
>  #define MEDIA_BUS_FMT_SBGGR12_1X12		0x3008
>  #define MEDIA_BUS_FMT_SGBRG12_1X12		0x3010
>  #define MEDIA_BUS_FMT_SGRBG12_1X12		0x3011
> +#define MEDIA_BUS_FMT_SGRGB_IGIG_GBGR_IGIG12_1X12	0x3022
>  #define MEDIA_BUS_FMT_SRGGB12_1X12		0x3012
>  #define MEDIA_BUS_FMT_SBGGR14_1X14		0x3019
>  #define MEDIA_BUS_FMT_SGBRG14_1X14		0x301a
Marco Felsch April 29, 2021, 7:49 a.m. UTC | #2
Hi Laurent,

On 21-04-29 04:51, Laurent Pinchart wrote:
> Hi Marco,
> 
> Thank you for the patch.
> 
> On Tue, Apr 27, 2021 at 02:06:56PM +0200, Marco Felsch wrote:
> > Add special 8/12bit bayer media bus format for the OnSemi AR0237IR
> > camera sensor [1]. OnSemi calls this format RGB-IR, the pixel array
> > with the interleaved IR pixels looks like:
> > 
> >         |  G |  R |  G |  B | ...
> >         +----+----+----+----+---
> >         | IR |  G | IR |  G | ...
> >         +----+----+----+----+---
> >         |  G |  B |  G |  R | ...
> >         +----+----+----+----+---
> >         | IR |  G | IR |  G | ...
> >         +----+----+----+----+---
> >         | .. | .. | .. | .. | ..
> > 
> > [1] https://www.framos.com/media/pdf/96/ac/8f/AR0237CS-D-PDF-framos.pdf
> 
> I think we're reaching a limit of the media bus codes model here, due to
> a historical mistake. The four possible Bayer patterns, times the
> different number of bits per pixel, creates a lot of media bus codes,
> and drivers for CSI-2 receivers and IP cores further down the pipeline
> have to support them all.

That's correct but it is not bayer related. Currently it is what it is,
if a new code is added it must be propagated through all the involved
subdevs. On the other hand I wouldn't say that it is better to support
new codes per default for all drivers. Since this would add a lot of
untested code paths.

> This is already painful, and if we had a
> non-Bayer pattern such as this one,

That's not exactly true since it is a bayer pattern but instead of using
4x4 it uses 8x8 and it as some special pixels.

> we'll open the door to an explosion
> of the number of media bus codes (imagine all the different possible
> arrangements of this pattern, for instance if you enable horizontal
> and/or vertical flipping on the sensor). All drivers would need to be
> updated to support these new bus codes, and this really kills the
> current model.

Yep, I know what you mean but as I said above I think that adding it
explicite is the better abbroach since it involves somone who add _and_
test the new code on the particular platform.

> The historical mistake was to tie the Bayer pattern with the media bus
> code. We should really have specified raw 8/10/12/14/16 media bus codes,
> and conveyed the pattern separately. Most IP cores in the pipeline don't
> need to care about the pattern at all, and those who do (that's mostly
> ISPs) could be programmed explicitly by userspace as long as we have an
> API to retrieve the pattern from the sensor. I believe it's time to bite
> the bullet and go in that direction. I'm sorry for this case of yak
> shaving, but it really wouldn't be manageable anymore :-(

I got all your points and would agree but this is not a bayer only
related problem. You will have this problem with all new other formats
as well. I'm with you, most IP cores don't care but I wouldn't
guarantee that.

Regards,
  Marco
Mauro Carvalho Chehab April 29, 2021, 8:44 a.m. UTC | #3
Em Thu, 29 Apr 2021 09:49:03 +0200
Marco Felsch <m.felsch@pengutronix.de> escreveu:

> Hi Laurent,
> 
> On 21-04-29 04:51, Laurent Pinchart wrote:
> > Hi Marco,
> > 
> > Thank you for the patch.
> > 
> > On Tue, Apr 27, 2021 at 02:06:56PM +0200, Marco Felsch wrote:  
> > > Add special 8/12bit bayer media bus format for the OnSemi AR0237IR
> > > camera sensor [1]. OnSemi calls this format RGB-IR, the pixel array
> > > with the interleaved IR pixels looks like:
> > > 
> > >         |  G |  R |  G |  B | ...
> > >         +----+----+----+----+---
> > >         | IR |  G | IR |  G | ...
> > >         +----+----+----+----+---
> > >         |  G |  B |  G |  R | ...
> > >         +----+----+----+----+---
> > >         | IR |  G | IR |  G | ...
> > >         +----+----+----+----+---
> > >         | .. | .. | .. | .. | ..
> > > 
> > > [1] https://www.framos.com/media/pdf/96/ac/8f/AR0237CS-D-PDF-framos.pdf  
> > 
> > I think we're reaching a limit of the media bus codes model here, due to
> > a historical mistake. The four possible Bayer patterns, times the
> > different number of bits per pixel, creates a lot of media bus codes,
> > and drivers for CSI-2 receivers and IP cores further down the pipeline
> > have to support them all.  
> 
> That's correct but it is not bayer related.

Err... there are two separate things here:

1) for the uAPI part, we're not even close to the limit of a 4-bytes
   fourcc;

2) the kAPI is currently sharing the same fourcc from the uAPI,
   because it is a lot simpler than doing something different.

Yet, nothing prevents that the kAPI could be improved in order to
better describe each format, provided that:

1. there will be a 1:1 mapping with the uAPI.
   In other words, it would be possible to write a function that
   would convert a "struct foo" from/to a 32-bits fourcc;

2. such new kAPI should be optional, as usually only drivers
   for hardware with ISP (plus the UVC driver) would be flexible
   enough to allow random formats. For the rest, the bridges are
   typically limited to support only a few formats, so it doesn't
   make sense to modify existing drivers to use such new kAPI.

Ok, someone has to come up with a proposal about what a "struct foo"
would contain. On a real quick brainstorm, something like this could
be a start:

enum v4l2_pixformat_type {
	VIDEO_PIXFORMAT_RGB,
	VIDEO_PIXFORMAT_YUV,
	VIDEO_PIXFORMAT_COMPRESSED,
	VIDEO_PIXFORMAT_BAYER_RGB,
	VIDEO_PIXFORMAT_BAYER_RGB_IR,
};

struct v4l2_pixformat_desc {
	enum v4l2_pixformat_type	pixfmt_type;
	bool				is_packed;
	int				bits_per_component;

	union {
		enum v4l2_pixformat_rgb_order		rgb_order;
		enum v4l2_pixformat_yuv_order		yuv_order;
		enum v4l2_pixformat_bayer_rgb_order	bayer_rgb_order;
		enum v4l2_pixformat_bayer_rgb_ir_order	bayer_rgb_ir_order;
		enum v4l2_pixformat_compress_type	compress_type;
	};
	...
};

Btw, I remember someone suggested a model similar to that in the past,
shared with DRM. Well, I don't think it would be easy to share
an internal subsystem-specific kAPI like that with other subsystems,
as this is the kind of thing that people will change from time to time,
and coordinating something like that can be painful, but we can try
to fork the model applied to DRM on media. For instance, I doubt that
Bayer RGB + IR would make any sense for DRM drivers.

Thanks,
Mauro
Laurent Pinchart April 29, 2021, 4:53 p.m. UTC | #4
Hi Mauro,

On Thu, Apr 29, 2021 at 10:44:41AM +0200, Mauro Carvalho Chehab wrote:
> Em Thu, 29 Apr 2021 09:49:03 +0200 Marco Felsch escreveu:
> > On 21-04-29 04:51, Laurent Pinchart wrote:
> > > On Tue, Apr 27, 2021 at 02:06:56PM +0200, Marco Felsch wrote:  
> > > > Add special 8/12bit bayer media bus format for the OnSemi AR0237IR
> > > > camera sensor [1]. OnSemi calls this format RGB-IR, the pixel array
> > > > with the interleaved IR pixels looks like:
> > > > 
> > > >         |  G |  R |  G |  B | ...
> > > >         +----+----+----+----+---
> > > >         | IR |  G | IR |  G | ...
> > > >         +----+----+----+----+---
> > > >         |  G |  B |  G |  R | ...
> > > >         +----+----+----+----+---
> > > >         | IR |  G | IR |  G | ...
> > > >         +----+----+----+----+---
> > > >         | .. | .. | .. | .. | ..
> > > > 
> > > > [1] https://www.framos.com/media/pdf/96/ac/8f/AR0237CS-D-PDF-framos.pdf  
> > > 
> > > I think we're reaching a limit of the media bus codes model here, due to
> > > a historical mistake. The four possible Bayer patterns, times the
> > > different number of bits per pixel, creates a lot of media bus codes,
> > > and drivers for CSI-2 receivers and IP cores further down the pipeline
> > > have to support them all.  
> > 
> > That's correct but it is not bayer related.
> 
> Err... there are two separate things here:
> 
> 1) for the uAPI part, we're not even close to the limit of a 4-bytes
>    fourcc;
> 
> 2) the kAPI is currently sharing the same fourcc from the uAPI,
>    because it is a lot simpler than doing something different.

Please note that we're talking about media bus codes here, not pixel
formats. Both are part of the UAPI though, and pixel formats suffer from
a similar issue, but I'd like to focus on the media bus codes first.

> Yet, nothing prevents that the kAPI could be improved in order to
> better describe each format, provided that:
> 
> 1. there will be a 1:1 mapping with the uAPI.
>    In other words, it would be possible to write a function that
>    would convert a "struct foo" from/to a 32-bits fourcc;
> 
> 2. such new kAPI should be optional, as usually only drivers
>    for hardware with ISP (plus the UVC driver) would be flexible
>    enough to allow random formats. For the rest, the bridges are
>    typically limited to support only a few formats, so it doesn't
>    make sense to modify existing drivers to use such new kAPI.
> 
> Ok, someone has to come up with a proposal about what a "struct foo"
> would contain. On a real quick brainstorm, something like this could
> be a start:
> 
> enum v4l2_pixformat_type {
> 	VIDEO_PIXFORMAT_RGB,
> 	VIDEO_PIXFORMAT_YUV,
> 	VIDEO_PIXFORMAT_COMPRESSED,
> 	VIDEO_PIXFORMAT_BAYER_RGB,
> 	VIDEO_PIXFORMAT_BAYER_RGB_IR,
> };
> 
> struct v4l2_pixformat_desc {
> 	enum v4l2_pixformat_type	pixfmt_type;
> 	bool				is_packed;
> 	int				bits_per_component;
> 
> 	union {
> 		enum v4l2_pixformat_rgb_order		rgb_order;
> 		enum v4l2_pixformat_yuv_order		yuv_order;
> 		enum v4l2_pixformat_bayer_rgb_order	bayer_rgb_order;
> 		enum v4l2_pixformat_bayer_rgb_ir_order	bayer_rgb_ir_order;
> 		enum v4l2_pixformat_compress_type	compress_type;
> 	};
> 	...
> };
> 
> Btw, I remember someone suggested a model similar to that in the past,
> shared with DRM. Well, I don't think it would be easy to share
> an internal subsystem-specific kAPI like that with other subsystems,
> as this is the kind of thing that people will change from time to time,
> and coordinating something like that can be painful, but we can try
> to fork the model applied to DRM on media. For instance, I doubt that
> Bayer RGB + IR would make any sense for DRM drivers.
Laurent Pinchart April 29, 2021, 10:14 p.m. UTC | #5
Hi Marco,

On Thu, Apr 29, 2021 at 09:49:03AM +0200, Marco Felsch wrote:
> On 21-04-29 04:51, Laurent Pinchart wrote:
> > On Tue, Apr 27, 2021 at 02:06:56PM +0200, Marco Felsch wrote:
> > > Add special 8/12bit bayer media bus format for the OnSemi AR0237IR
> > > camera sensor [1]. OnSemi calls this format RGB-IR, the pixel array
> > > with the interleaved IR pixels looks like:
> > > 
> > >         |  G |  R |  G |  B | ...
> > >         +----+----+----+----+---
> > >         | IR |  G | IR |  G | ...
> > >         +----+----+----+----+---
> > >         |  G |  B |  G |  R | ...
> > >         +----+----+----+----+---
> > >         | IR |  G | IR |  G | ...
> > >         +----+----+----+----+---
> > >         | .. | .. | .. | .. | ..
> > > 
> > > [1] https://www.framos.com/media/pdf/96/ac/8f/AR0237CS-D-PDF-framos.pdf
> > 
> > I think we're reaching a limit of the media bus codes model here, due to
> > a historical mistake. The four possible Bayer patterns, times the
> > different number of bits per pixel, creates a lot of media bus codes,
> > and drivers for CSI-2 receivers and IP cores further down the pipeline
> > have to support them all.
> 
> That's correct but it is not bayer related. Currently it is what it is,
> if a new code is added it must be propagated through all the involved
> subdevs. On the other hand I wouldn't say that it is better to support
> new codes per default for all drivers. Since this would add a lot of
> untested code paths.

It's not an issue limited to Bayer patterns, but they make the issue
worse given the number of possible combinations (think about adding DPCM
and ALAW compression on top of that...).

> > This is already painful, and if we had a
> > non-Bayer pattern such as this one,
> 
> That's not exactly true since it is a bayer pattern but instead of using
> 4x4 it uses 8x8 and it as some special pixels.
> 
> > we'll open the door to an explosion
> > of the number of media bus codes (imagine all the different possible
> > arrangements of this pattern, for instance if you enable horizontal
> > and/or vertical flipping on the sensor). All drivers would need to be
> > updated to support these new bus codes, and this really kills the
> > current model.
> 
> Yep, I know what you mean but as I said above I think that adding it
> explicite is the better abbroach since it involves somone who add _and_
> test the new code on the particular platform.
> 
> > The historical mistake was to tie the Bayer pattern with the media bus
> > code. We should really have specified raw 8/10/12/14/16 media bus codes,
> > and conveyed the pattern separately. Most IP cores in the pipeline don't
> > need to care about the pattern at all, and those who do (that's mostly
> > ISPs) could be programmed explicitly by userspace as long as we have an
> > API to retrieve the pattern from the sensor. I believe it's time to bite
> > the bullet and go in that direction. I'm sorry for this case of yak
> > shaving, but it really wouldn't be manageable anymore :-(
> 
> I got all your points and would agree but this is not a bayer only
> related problem. You will have this problem with all new other formats
> as well. I'm with you, most IP cores don't care but I wouldn't
> guarantee that.

Sorry, but adding new media bus formats like this one will just not
scale. We have two options, either fixing the issue, or considering that
V4L2 is a barely alive API with no future, and merging this without
caring anymore.
Marco Felsch April 30, 2021, 6:51 a.m. UTC | #6
Hi Laurent,

On 21-04-30 01:14, Laurent Pinchart wrote:
> Hi Marco,
> 
> On Thu, Apr 29, 2021 at 09:49:03AM +0200, Marco Felsch wrote:
> > On 21-04-29 04:51, Laurent Pinchart wrote:
> > > On Tue, Apr 27, 2021 at 02:06:56PM +0200, Marco Felsch wrote:
> > > > Add special 8/12bit bayer media bus format for the OnSemi AR0237IR
> > > > camera sensor [1]. OnSemi calls this format RGB-IR, the pixel array
> > > > with the interleaved IR pixels looks like:
> > > > 
> > > >         |  G |  R |  G |  B | ...
> > > >         +----+----+----+----+---
> > > >         | IR |  G | IR |  G | ...
> > > >         +----+----+----+----+---
> > > >         |  G |  B |  G |  R | ...
> > > >         +----+----+----+----+---
> > > >         | IR |  G | IR |  G | ...
> > > >         +----+----+----+----+---
> > > >         | .. | .. | .. | .. | ..
> > > > 
> > > > [1] https://www.framos.com/media/pdf/96/ac/8f/AR0237CS-D-PDF-framos.pdf
> > > 
> > > I think we're reaching a limit of the media bus codes model here, due to
> > > a historical mistake. The four possible Bayer patterns, times the
> > > different number of bits per pixel, creates a lot of media bus codes,
> > > and drivers for CSI-2 receivers and IP cores further down the pipeline
> > > have to support them all.
> > 
> > That's correct but it is not bayer related. Currently it is what it is,
> > if a new code is added it must be propagated through all the involved
> > subdevs. On the other hand I wouldn't say that it is better to support
> > new codes per default for all drivers. Since this would add a lot of
> > untested code paths.
> 
> It's not an issue limited to Bayer patterns, but they make the issue
> worse given the number of possible combinations (think about adding DPCM
> and ALAW compression on top of that...).

You're right and again this will apply to all mbus formats...

> > > This is already painful, and if we had a
> > > non-Bayer pattern such as this one,
> > 
> > That's not exactly true since it is a bayer pattern but instead of using
> > 4x4 it uses 8x8 and it as some special pixels.
> > 
> > > we'll open the door to an explosion
> > > of the number of media bus codes (imagine all the different possible
> > > arrangements of this pattern, for instance if you enable horizontal
> > > and/or vertical flipping on the sensor). All drivers would need to be
> > > updated to support these new bus codes, and this really kills the
> > > current model.
> > 
> > Yep, I know what you mean but as I said above I think that adding it
> > explicite is the better abbroach since it involves somone who add _and_
> > test the new code on the particular platform.
> > 
> > > The historical mistake was to tie the Bayer pattern with the media bus
> > > code. We should really have specified raw 8/10/12/14/16 media bus codes,
> > > and conveyed the pattern separately. Most IP cores in the pipeline don't
> > > need to care about the pattern at all, and those who do (that's mostly
> > > ISPs) could be programmed explicitly by userspace as long as we have an
> > > API to retrieve the pattern from the sensor. I believe it's time to bite
> > > the bullet and go in that direction. I'm sorry for this case of yak
> > > shaving, but it really wouldn't be manageable anymore :-(
> > 
> > I got all your points and would agree but this is not a bayer only
> > related problem. You will have this problem with all new other formats
> > as well. I'm with you, most IP cores don't care but I wouldn't
> > guarantee that.
> 
> Sorry, but adding new media bus formats like this one will just not
> scale. We have two options, either fixing the issue, or considering that
> V4L2 is a barely alive API with no future, and merging this without
> caring anymore.

Hm.. you're right that it doesn't scale, as I said I'm absolute on your
side. So let us consider a new approach @Mauro, @Hans, @Sailus what do
you think about?

BTW: IMHO videobuf2 interface isn't that good as well, since you are
blaming ;)

Regards,
  Marco

> 
> -- 
> Regards,
> 
> Laurent Pinchart
>
Laurent Pinchart April 30, 2021, 12:18 p.m. UTC | #7
Hi Marco,

On Fri, Apr 30, 2021 at 08:51:34AM +0200, Marco Felsch wrote:
> On 21-04-30 01:14, Laurent Pinchart wrote:
> > On Thu, Apr 29, 2021 at 09:49:03AM +0200, Marco Felsch wrote:
> > > On 21-04-29 04:51, Laurent Pinchart wrote:
> > > > On Tue, Apr 27, 2021 at 02:06:56PM +0200, Marco Felsch wrote:
> > > > > Add special 8/12bit bayer media bus format for the OnSemi AR0237IR
> > > > > camera sensor [1]. OnSemi calls this format RGB-IR, the pixel array
> > > > > with the interleaved IR pixels looks like:
> > > > > 
> > > > >         |  G |  R |  G |  B | ...
> > > > >         +----+----+----+----+---
> > > > >         | IR |  G | IR |  G | ...
> > > > >         +----+----+----+----+---
> > > > >         |  G |  B |  G |  R | ...
> > > > >         +----+----+----+----+---
> > > > >         | IR |  G | IR |  G | ...
> > > > >         +----+----+----+----+---
> > > > >         | .. | .. | .. | .. | ..
> > > > > 
> > > > > [1] https://www.framos.com/media/pdf/96/ac/8f/AR0237CS-D-PDF-framos.pdf
> > > > 
> > > > I think we're reaching a limit of the media bus codes model here, due to
> > > > a historical mistake. The four possible Bayer patterns, times the
> > > > different number of bits per pixel, creates a lot of media bus codes,
> > > > and drivers for CSI-2 receivers and IP cores further down the pipeline
> > > > have to support them all.
> > > 
> > > That's correct but it is not bayer related. Currently it is what it is,
> > > if a new code is added it must be propagated through all the involved
> > > subdevs. On the other hand I wouldn't say that it is better to support
> > > new codes per default for all drivers. Since this would add a lot of
> > > untested code paths.
> > 
> > It's not an issue limited to Bayer patterns, but they make the issue
> > worse given the number of possible combinations (think about adding DPCM
> > and ALAW compression on top of that...).
> 
> You're right and again this will apply to all mbus formats...
> 
> > > > This is already painful, and if we had a
> > > > non-Bayer pattern such as this one,
> > > 
> > > That's not exactly true since it is a bayer pattern but instead of using
> > > 4x4 it uses 8x8 and it as some special pixels.
> > > 
> > > > we'll open the door to an explosion
> > > > of the number of media bus codes (imagine all the different possible
> > > > arrangements of this pattern, for instance if you enable horizontal
> > > > and/or vertical flipping on the sensor). All drivers would need to be
> > > > updated to support these new bus codes, and this really kills the
> > > > current model.
> > > 
> > > Yep, I know what you mean but as I said above I think that adding it
> > > explicite is the better abbroach since it involves somone who add _and_
> > > test the new code on the particular platform.
> > > 
> > > > The historical mistake was to tie the Bayer pattern with the media bus
> > > > code. We should really have specified raw 8/10/12/14/16 media bus codes,
> > > > and conveyed the pattern separately. Most IP cores in the pipeline don't
> > > > need to care about the pattern at all, and those who do (that's mostly
> > > > ISPs) could be programmed explicitly by userspace as long as we have an
> > > > API to retrieve the pattern from the sensor. I believe it's time to bite
> > > > the bullet and go in that direction. I'm sorry for this case of yak
> > > > shaving, but it really wouldn't be manageable anymore :-(
> > > 
> > > I got all your points and would agree but this is not a bayer only
> > > related problem. You will have this problem with all new other formats
> > > as well. I'm with you, most IP cores don't care but I wouldn't
> > > guarantee that.
> > 
> > Sorry, but adding new media bus formats like this one will just not
> > scale. We have two options, either fixing the issue, or considering that
> > V4L2 is a barely alive API with no future, and merging this without
> > caring anymore.
> 
> Hm.. you're right that it doesn't scale, as I said I'm absolute on your
> side. So let us consider a new approach @Mauro, @Hans, @Sailus what do
> you think about?

Starting brainstorming, how about new media bus codes for
raw{8,10,12,14,16}, and a read-only CFA pattern control to retrieve the
pattern from the sensor subdev ? We could use the same control to set
the pattern on subdevs that require it, which would mostly be ISPs. As
ISPs are configured using parameter buffers these days, it may be better
to pass the pattern in the parameter buffer instead though.

This shouldn't be too hard to implement, but the devil is of course in
the details, and we should consider how to handle the pattern control
when flipping and/or cropping is configured on the sensor.

> BTW: IMHO videobuf2 interface isn't that good as well, since you are
> blaming ;)

Have you looked at videobuf1 ? ;-) Jokes aside, there's certainly room
for improvement, but it hasn't struck me as a particularly bad part of
the framework. Is there anything in particular you think is painful ?
Mauro Carvalho Chehab April 30, 2021, 12:44 p.m. UTC | #8
Em Thu, 29 Apr 2021 19:53:33 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Thu, Apr 29, 2021 at 10:44:41AM +0200, Mauro Carvalho Chehab wrote:
> > Em Thu, 29 Apr 2021 09:49:03 +0200 Marco Felsch escreveu:  
> > > On 21-04-29 04:51, Laurent Pinchart wrote:  
> > > > On Tue, Apr 27, 2021 at 02:06:56PM +0200, Marco Felsch wrote:    
> > > > > Add special 8/12bit bayer media bus format for the OnSemi AR0237IR
> > > > > camera sensor [1]. OnSemi calls this format RGB-IR, the pixel array
> > > > > with the interleaved IR pixels looks like:
> > > > > 
> > > > >         |  G |  R |  G |  B | ...
> > > > >         +----+----+----+----+---
> > > > >         | IR |  G | IR |  G | ...
> > > > >         +----+----+----+----+---
> > > > >         |  G |  B |  G |  R | ...
> > > > >         +----+----+----+----+---
> > > > >         | IR |  G | IR |  G | ...
> > > > >         +----+----+----+----+---
> > > > >         | .. | .. | .. | .. | ..
> > > > > 
> > > > > [1] https://www.framos.com/media/pdf/96/ac/8f/AR0237CS-D-PDF-framos.pdf    
> > > > 
> > > > I think we're reaching a limit of the media bus codes model here, due to
> > > > a historical mistake. The four possible Bayer patterns, times the
> > > > different number of bits per pixel, creates a lot of media bus codes,
> > > > and drivers for CSI-2 receivers and IP cores further down the pipeline
> > > > have to support them all.    
> > > 
> > > That's correct but it is not bayer related.  
> > 
> > Err... there are two separate things here:
> > 
> > 1) for the uAPI part, we're not even close to the limit of a 4-bytes
> >    fourcc;
> > 
> > 2) the kAPI is currently sharing the same fourcc from the uAPI,
> >    because it is a lot simpler than doing something different.  
> 
> Please note that we're talking about media bus codes here, not pixel
> formats. Both are part of the UAPI though, and pixel formats suffer from
> a similar issue, but I'd like to focus on the media bus codes first.

Yes, I'm aware of that, but the same principle used by fourcc pixel
formats can also be applied to media bus codes and vice versa[1].

[1] IMO, a kAPI change like that should consider the big picture, 
    and allow using the same process for both, even if we start
    implementing it for media bus (where it makes more sense).

On both cases, we're talking about a 32-bit code (either encoded
as fourcc or via MEDIA_BUS_FMT_* codespace).

Both can be 1:1 mapped to some structure similar to:

enum v4l2_pixformat_type {
	VIDEO_PIXFORMAT_RGB,
	VIDEO_PIXFORMAT_YUV,
	VIDEO_PIXFORMAT_COMPRESSED,
	VIDEO_PIXFORMAT_BAYER_RGB,
	VIDEO_PIXFORMAT_BAYER_RGB_IR,
};
 
struct v4l2_pixformat_desc {
	enum v4l2_pixformat_type	pixfmt_type;
	bool				is_packed;
 	int				bits_per_component;
 
	union {
		enum v4l2_pixformat_rgb_order		rgb_order;
		enum v4l2_pixformat_yuv_order		yuv_order;
		enum v4l2_pixformat_bayer_rgb_order	bayer_rgb_order;
		enum v4l2_pixformat_bayer_rgb_ir_order	bayer_rgb_ir_order;
		enum v4l2_pixformat_compress_type	compress_type;
	};
	...
};

And new drivers can use such struct, instead of handling the
fourcc/mbus code directly.

Also, this can be gradually implemented, in order to avoid the
need of touching at the existing drivers.

Thanks,
Mauro
Mauro Carvalho Chehab April 30, 2021, 12:51 p.m. UTC | #9
Em Fri, 30 Apr 2021 15:18:52 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Marco,
> 
> On Fri, Apr 30, 2021 at 08:51:34AM +0200, Marco Felsch wrote:
> > On 21-04-30 01:14, Laurent Pinchart wrote:  
> > > On Thu, Apr 29, 2021 at 09:49:03AM +0200, Marco Felsch wrote:  
> > > > On 21-04-29 04:51, Laurent Pinchart wrote:  
> > > > > On Tue, Apr 27, 2021 at 02:06:56PM +0200, Marco Felsch wrote:  
> > > > > > Add special 8/12bit bayer media bus format for the OnSemi AR0237IR
> > > > > > camera sensor [1]. OnSemi calls this format RGB-IR, the pixel array
> > > > > > with the interleaved IR pixels looks like:
> > > > > > 
> > > > > >         |  G |  R |  G |  B | ...
> > > > > >         +----+----+----+----+---
> > > > > >         | IR |  G | IR |  G | ...
> > > > > >         +----+----+----+----+---
> > > > > >         |  G |  B |  G |  R | ...
> > > > > >         +----+----+----+----+---
> > > > > >         | IR |  G | IR |  G | ...
> > > > > >         +----+----+----+----+---
> > > > > >         | .. | .. | .. | .. | ..
> > > > > > 
> > > > > > [1] https://www.framos.com/media/pdf/96/ac/8f/AR0237CS-D-PDF-framos.pdf  
> > > > > 
> > > > > I think we're reaching a limit of the media bus codes model here, due to
> > > > > a historical mistake. The four possible Bayer patterns, times the
> > > > > different number of bits per pixel, creates a lot of media bus codes,
> > > > > and drivers for CSI-2 receivers and IP cores further down the pipeline
> > > > > have to support them all.  
> > > > 
> > > > That's correct but it is not bayer related. Currently it is what it is,
> > > > if a new code is added it must be propagated through all the involved
> > > > subdevs. On the other hand I wouldn't say that it is better to support
> > > > new codes per default for all drivers. Since this would add a lot of
> > > > untested code paths.  
> > > 
> > > It's not an issue limited to Bayer patterns, but they make the issue
> > > worse given the number of possible combinations (think about adding DPCM
> > > and ALAW compression on top of that...).  
> > 
> > You're right and again this will apply to all mbus formats...
> >   
> > > > > This is already painful, and if we had a
> > > > > non-Bayer pattern such as this one,  
> > > > 
> > > > That's not exactly true since it is a bayer pattern but instead of using
> > > > 4x4 it uses 8x8 and it as some special pixels.
> > > >   
> > > > > we'll open the door to an explosion
> > > > > of the number of media bus codes (imagine all the different possible
> > > > > arrangements of this pattern, for instance if you enable horizontal
> > > > > and/or vertical flipping on the sensor). All drivers would need to be
> > > > > updated to support these new bus codes, and this really kills the
> > > > > current model.  
> > > > 
> > > > Yep, I know what you mean but as I said above I think that adding it
> > > > explicite is the better abbroach since it involves somone who add _and_
> > > > test the new code on the particular platform.
> > > >   
> > > > > The historical mistake was to tie the Bayer pattern with the media bus
> > > > > code. We should really have specified raw 8/10/12/14/16 media bus codes,
> > > > > and conveyed the pattern separately. Most IP cores in the pipeline don't
> > > > > need to care about the pattern at all, and those who do (that's mostly
> > > > > ISPs) could be programmed explicitly by userspace as long as we have an
> > > > > API to retrieve the pattern from the sensor. I believe it's time to bite
> > > > > the bullet and go in that direction. I'm sorry for this case of yak
> > > > > shaving, but it really wouldn't be manageable anymore :-(  
> > > > 
> > > > I got all your points and would agree but this is not a bayer only
> > > > related problem. You will have this problem with all new other formats
> > > > as well. I'm with you, most IP cores don't care but I wouldn't
> > > > guarantee that.  
> > > 
> > > Sorry, but adding new media bus formats like this one will just not
> > > scale. We have two options, either fixing the issue, or considering that
> > > V4L2 is a barely alive API with no future, and merging this without
> > > caring anymore.  
> > 
> > Hm.. you're right that it doesn't scale, as I said I'm absolute on your
> > side. So let us consider a new approach @Mauro, @Hans, @Sailus what do
> > you think about?  
> 
> Starting brainstorming, how about new media bus codes for
> raw{8,10,12,14,16}, 

By "raw", are you meaning vendor-specific formats? If so, that sounds 
a bad idea. Different vendor-specific formats should use different
media bus codes (and fourccs) as otherwise there won't be an easy way
to distinguish them and to describe the raw formats at the media specs.

Thanks,
Mauro
Laurent Pinchart April 30, 2021, 12:58 p.m. UTC | #10
On Fri, Apr 30, 2021 at 02:51:46PM +0200, Mauro Carvalho Chehab wrote:
> Em Fri, 30 Apr 2021 15:18:52 +0300
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> 
> > Hi Marco,
> > 
> > On Fri, Apr 30, 2021 at 08:51:34AM +0200, Marco Felsch wrote:
> > > On 21-04-30 01:14, Laurent Pinchart wrote:  
> > > > On Thu, Apr 29, 2021 at 09:49:03AM +0200, Marco Felsch wrote:  
> > > > > On 21-04-29 04:51, Laurent Pinchart wrote:  
> > > > > > On Tue, Apr 27, 2021 at 02:06:56PM +0200, Marco Felsch wrote:  
> > > > > > > Add special 8/12bit bayer media bus format for the OnSemi AR0237IR
> > > > > > > camera sensor [1]. OnSemi calls this format RGB-IR, the pixel array
> > > > > > > with the interleaved IR pixels looks like:
> > > > > > > 
> > > > > > >         |  G |  R |  G |  B | ...
> > > > > > >         +----+----+----+----+---
> > > > > > >         | IR |  G | IR |  G | ...
> > > > > > >         +----+----+----+----+---
> > > > > > >         |  G |  B |  G |  R | ...
> > > > > > >         +----+----+----+----+---
> > > > > > >         | IR |  G | IR |  G | ...
> > > > > > >         +----+----+----+----+---
> > > > > > >         | .. | .. | .. | .. | ..
> > > > > > > 
> > > > > > > [1] https://www.framos.com/media/pdf/96/ac/8f/AR0237CS-D-PDF-framos.pdf  
> > > > > > 
> > > > > > I think we're reaching a limit of the media bus codes model here, due to
> > > > > > a historical mistake. The four possible Bayer patterns, times the
> > > > > > different number of bits per pixel, creates a lot of media bus codes,
> > > > > > and drivers for CSI-2 receivers and IP cores further down the pipeline
> > > > > > have to support them all.  
> > > > > 
> > > > > That's correct but it is not bayer related. Currently it is what it is,
> > > > > if a new code is added it must be propagated through all the involved
> > > > > subdevs. On the other hand I wouldn't say that it is better to support
> > > > > new codes per default for all drivers. Since this would add a lot of
> > > > > untested code paths.  
> > > > 
> > > > It's not an issue limited to Bayer patterns, but they make the issue
> > > > worse given the number of possible combinations (think about adding DPCM
> > > > and ALAW compression on top of that...).  
> > > 
> > > You're right and again this will apply to all mbus formats...
> > >   
> > > > > > This is already painful, and if we had a
> > > > > > non-Bayer pattern such as this one,  
> > > > > 
> > > > > That's not exactly true since it is a bayer pattern but instead of using
> > > > > 4x4 it uses 8x8 and it as some special pixels.
> > > > >   
> > > > > > we'll open the door to an explosion
> > > > > > of the number of media bus codes (imagine all the different possible
> > > > > > arrangements of this pattern, for instance if you enable horizontal
> > > > > > and/or vertical flipping on the sensor). All drivers would need to be
> > > > > > updated to support these new bus codes, and this really kills the
> > > > > > current model.  
> > > > > 
> > > > > Yep, I know what you mean but as I said above I think that adding it
> > > > > explicite is the better abbroach since it involves somone who add _and_
> > > > > test the new code on the particular platform.
> > > > >   
> > > > > > The historical mistake was to tie the Bayer pattern with the media bus
> > > > > > code. We should really have specified raw 8/10/12/14/16 media bus codes,
> > > > > > and conveyed the pattern separately. Most IP cores in the pipeline don't
> > > > > > need to care about the pattern at all, and those who do (that's mostly
> > > > > > ISPs) could be programmed explicitly by userspace as long as we have an
> > > > > > API to retrieve the pattern from the sensor. I believe it's time to bite
> > > > > > the bullet and go in that direction. I'm sorry for this case of yak
> > > > > > shaving, but it really wouldn't be manageable anymore :-(  
> > > > > 
> > > > > I got all your points and would agree but this is not a bayer only
> > > > > related problem. You will have this problem with all new other formats
> > > > > as well. I'm with you, most IP cores don't care but I wouldn't
> > > > > guarantee that.  
> > > > 
> > > > Sorry, but adding new media bus formats like this one will just not
> > > > scale. We have two options, either fixing the issue, or considering that
> > > > V4L2 is a barely alive API with no future, and merging this without
> > > > caring anymore.  
> > > 
> > > Hm.. you're right that it doesn't scale, as I said I'm absolute on your
> > > side. So let us consider a new approach @Mauro, @Hans, @Sailus what do
> > > you think about?  
> > 
> > Starting brainstorming, how about new media bus codes for
> > raw{8,10,12,14,16}, 
> 
> By "raw", are you meaning vendor-specific formats? If so, that sounds 
> a bad idea. Different vendor-specific formats should use different
> media bus codes (and fourccs) as otherwise there won't be an easy way
> to distinguish them and to describe the raw formats at the media specs.

I mean what the CSI-2 spec means. The exact interpretation of the format
will be a combination of the media bus code and the CFA pattern control.
The whole point of this discussion is to not have different media bus
codes for all possible combinations of formats, as that clearly doesn't
scale.
Marco Felsch Feb. 8, 2023, 7:44 p.m. UTC | #11
Hi all,

sorry for the long absence on this topic. Only a few years later I'm
back on this topic :)

On 21-04-30, Laurent Pinchart wrote:
> On Fri, Apr 30, 2021 at 02:51:46PM +0200, Mauro Carvalho Chehab wrote:
> > Em Fri, 30 Apr 2021 15:18:52 +0300
> > Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> > 
> > > Hi Marco,
> > > 
> > > On Fri, Apr 30, 2021 at 08:51:34AM +0200, Marco Felsch wrote:
> > > > On 21-04-30 01:14, Laurent Pinchart wrote:  
> > > > > On Thu, Apr 29, 2021 at 09:49:03AM +0200, Marco Felsch wrote:  
> > > > > > On 21-04-29 04:51, Laurent Pinchart wrote:  
> > > > > > > On Tue, Apr 27, 2021 at 02:06:56PM +0200, Marco Felsch wrote:  
> > > > > > > > Add special 8/12bit bayer media bus format for the OnSemi AR0237IR
> > > > > > > > camera sensor [1]. OnSemi calls this format RGB-IR, the pixel array
> > > > > > > > with the interleaved IR pixels looks like:
> > > > > > > > 
> > > > > > > >         |  G |  R |  G |  B | ...
> > > > > > > >         +----+----+----+----+---
> > > > > > > >         | IR |  G | IR |  G | ...
> > > > > > > >         +----+----+----+----+---
> > > > > > > >         |  G |  B |  G |  R | ...
> > > > > > > >         +----+----+----+----+---
> > > > > > > >         | IR |  G | IR |  G | ...
> > > > > > > >         +----+----+----+----+---
> > > > > > > >         | .. | .. | .. | .. | ..
> > > > > > > > 
> > > > > > > > [1] https://www.framos.com/media/pdf/96/ac/8f/AR0237CS-D-PDF-framos.pdf  
> > > > > > > 
> > > > > > > I think we're reaching a limit of the media bus codes model here, due to
> > > > > > > a historical mistake. The four possible Bayer patterns, times the
> > > > > > > different number of bits per pixel, creates a lot of media bus codes,
> > > > > > > and drivers for CSI-2 receivers and IP cores further down the pipeline
> > > > > > > have to support them all.  
> > > > > > 
> > > > > > That's correct but it is not bayer related. Currently it is what it is,
> > > > > > if a new code is added it must be propagated through all the involved
> > > > > > subdevs. On the other hand I wouldn't say that it is better to support
> > > > > > new codes per default for all drivers. Since this would add a lot of
> > > > > > untested code paths.  
> > > > > 
> > > > > It's not an issue limited to Bayer patterns, but they make the issue
> > > > > worse given the number of possible combinations (think about adding DPCM
> > > > > and ALAW compression on top of that...).  
> > > > 
> > > > You're right and again this will apply to all mbus formats...
> > > >   
> > > > > > > This is already painful, and if we had a
> > > > > > > non-Bayer pattern such as this one,  
> > > > > > 
> > > > > > That's not exactly true since it is a bayer pattern but instead of using
> > > > > > 4x4 it uses 8x8 and it as some special pixels.
> > > > > >   
> > > > > > > we'll open the door to an explosion
> > > > > > > of the number of media bus codes (imagine all the different possible
> > > > > > > arrangements of this pattern, for instance if you enable horizontal
> > > > > > > and/or vertical flipping on the sensor). All drivers would need to be
> > > > > > > updated to support these new bus codes, and this really kills the
> > > > > > > current model.  
> > > > > > 
> > > > > > Yep, I know what you mean but as I said above I think that adding it
> > > > > > explicite is the better abbroach since it involves somone who add _and_
> > > > > > test the new code on the particular platform.
> > > > > >   
> > > > > > > The historical mistake was to tie the Bayer pattern with the media bus
> > > > > > > code. We should really have specified raw 8/10/12/14/16 media bus codes,
> > > > > > > and conveyed the pattern separately. Most IP cores in the pipeline don't
> > > > > > > need to care about the pattern at all, and those who do (that's mostly
> > > > > > > ISPs) could be programmed explicitly by userspace as long as we have an
> > > > > > > API to retrieve the pattern from the sensor. I believe it's time to bite
> > > > > > > the bullet and go in that direction. I'm sorry for this case of yak
> > > > > > > shaving, but it really wouldn't be manageable anymore :-(  
> > > > > > 
> > > > > > I got all your points and would agree but this is not a bayer only
> > > > > > related problem. You will have this problem with all new other formats
> > > > > > as well. I'm with you, most IP cores don't care but I wouldn't
> > > > > > guarantee that.  
> > > > > 
> > > > > Sorry, but adding new media bus formats like this one will just not
> > > > > scale. We have two options, either fixing the issue, or considering that
> > > > > V4L2 is a barely alive API with no future, and merging this without
> > > > > caring anymore.  
> > > > 
> > > > Hm.. you're right that it doesn't scale, as I said I'm absolute on your
> > > > side. So let us consider a new approach @Mauro, @Hans, @Sailus what do
> > > > you think about?  
> > > 
> > > Starting brainstorming, how about new media bus codes for
> > > raw{8,10,12,14,16}, 
> > 
> > By "raw", are you meaning vendor-specific formats? If so, that sounds 
> > a bad idea. Different vendor-specific formats should use different
> > media bus codes (and fourccs) as otherwise there won't be an easy way
> > to distinguish them and to describe the raw formats at the media specs.
> 
> I mean what the CSI-2 spec means. The exact interpretation of the format
> will be a combination of the media bus code and the CFA pattern control.
> The whole point of this discussion is to not have different media bus
> codes for all possible combinations of formats, as that clearly doesn't
> scale.

You mean that we should propagate the value as raw{size} through the
entire pipeline, if I got this correctly. How the picture should be
interpreted is up to the user-space by calling a new read-only CFA
v4l2-control. This way we don't need to patch each subdev driver and
take the user-space into account of interpreting the data. For the CFA
control we could use a global-unique list so the control returns an
enum.

Regards,
  Marco
Laurent Pinchart Feb. 9, 2023, 9:49 a.m. UTC | #12
Hi Marco,

On Wed, Feb 08, 2023 at 08:44:25PM +0100, Marco Felsch wrote:
> Hi all,
> 
> sorry for the long absence on this topic. Only a few years later I'm
> back on this topic :)

No worries, I rarely complain when I receive less e-mails :-)

> On 21-04-30, Laurent Pinchart wrote:
> > On Fri, Apr 30, 2021 at 02:51:46PM +0200, Mauro Carvalho Chehab wrote:
> > > Em Fri, 30 Apr 2021 15:18:52 +0300 Laurent Pinchart escreveu:
> > > > On Fri, Apr 30, 2021 at 08:51:34AM +0200, Marco Felsch wrote:
> > > > > On 21-04-30 01:14, Laurent Pinchart wrote:  
> > > > > > On Thu, Apr 29, 2021 at 09:49:03AM +0200, Marco Felsch wrote:  
> > > > > > > On 21-04-29 04:51, Laurent Pinchart wrote:  
> > > > > > > > On Tue, Apr 27, 2021 at 02:06:56PM +0200, Marco Felsch wrote:  
> > > > > > > > > Add special 8/12bit bayer media bus format for the OnSemi AR0237IR
> > > > > > > > > camera sensor [1]. OnSemi calls this format RGB-IR, the pixel array
> > > > > > > > > with the interleaved IR pixels looks like:
> > > > > > > > > 
> > > > > > > > >         |  G |  R |  G |  B | ...
> > > > > > > > >         +----+----+----+----+---
> > > > > > > > >         | IR |  G | IR |  G | ...
> > > > > > > > >         +----+----+----+----+---
> > > > > > > > >         |  G |  B |  G |  R | ...
> > > > > > > > >         +----+----+----+----+---
> > > > > > > > >         | IR |  G | IR |  G | ...
> > > > > > > > >         +----+----+----+----+---
> > > > > > > > >         | .. | .. | .. | .. | ..
> > > > > > > > > 
> > > > > > > > > [1] https://www.framos.com/media/pdf/96/ac/8f/AR0237CS-D-PDF-framos.pdf  
> > > > > > > > 
> > > > > > > > I think we're reaching a limit of the media bus codes model here, due to
> > > > > > > > a historical mistake. The four possible Bayer patterns, times the
> > > > > > > > different number of bits per pixel, creates a lot of media bus codes,
> > > > > > > > and drivers for CSI-2 receivers and IP cores further down the pipeline
> > > > > > > > have to support them all.  
> > > > > > > 
> > > > > > > That's correct but it is not bayer related. Currently it is what it is,
> > > > > > > if a new code is added it must be propagated through all the involved
> > > > > > > subdevs. On the other hand I wouldn't say that it is better to support
> > > > > > > new codes per default for all drivers. Since this would add a lot of
> > > > > > > untested code paths.  
> > > > > > 
> > > > > > It's not an issue limited to Bayer patterns, but they make the issue
> > > > > > worse given the number of possible combinations (think about adding DPCM
> > > > > > and ALAW compression on top of that...).  
> > > > > 
> > > > > You're right and again this will apply to all mbus formats...
> > > > >   
> > > > > > > > This is already painful, and if we had a
> > > > > > > > non-Bayer pattern such as this one,  
> > > > > > > 
> > > > > > > That's not exactly true since it is a bayer pattern but instead of using
> > > > > > > 4x4 it uses 8x8 and it as some special pixels.
> > > > > > >   
> > > > > > > > we'll open the door to an explosion
> > > > > > > > of the number of media bus codes (imagine all the different possible
> > > > > > > > arrangements of this pattern, for instance if you enable horizontal
> > > > > > > > and/or vertical flipping on the sensor). All drivers would need to be
> > > > > > > > updated to support these new bus codes, and this really kills the
> > > > > > > > current model.  
> > > > > > > 
> > > > > > > Yep, I know what you mean but as I said above I think that adding it
> > > > > > > explicite is the better abbroach since it involves somone who add _and_
> > > > > > > test the new code on the particular platform.
> > > > > > >   
> > > > > > > > The historical mistake was to tie the Bayer pattern with the media bus
> > > > > > > > code. We should really have specified raw 8/10/12/14/16 media bus codes,
> > > > > > > > and conveyed the pattern separately. Most IP cores in the pipeline don't
> > > > > > > > need to care about the pattern at all, and those who do (that's mostly
> > > > > > > > ISPs) could be programmed explicitly by userspace as long as we have an
> > > > > > > > API to retrieve the pattern from the sensor. I believe it's time to bite
> > > > > > > > the bullet and go in that direction. I'm sorry for this case of yak
> > > > > > > > shaving, but it really wouldn't be manageable anymore :-(  
> > > > > > > 
> > > > > > > I got all your points and would agree but this is not a bayer only
> > > > > > > related problem. You will have this problem with all new other formats
> > > > > > > as well. I'm with you, most IP cores don't care but I wouldn't
> > > > > > > guarantee that.  
> > > > > > 
> > > > > > Sorry, but adding new media bus formats like this one will just not
> > > > > > scale. We have two options, either fixing the issue, or considering that
> > > > > > V4L2 is a barely alive API with no future, and merging this without
> > > > > > caring anymore.  
> > > > > 
> > > > > Hm.. you're right that it doesn't scale, as I said I'm absolute on your
> > > > > side. So let us consider a new approach @Mauro, @Hans, @Sailus what do
> > > > > you think about?  
> > > > 
> > > > Starting brainstorming, how about new media bus codes for
> > > > raw{8,10,12,14,16}, 
> > > 
> > > By "raw", are you meaning vendor-specific formats? If so, that sounds 
> > > a bad idea. Different vendor-specific formats should use different
> > > media bus codes (and fourccs) as otherwise there won't be an easy way
> > > to distinguish them and to describe the raw formats at the media specs.
> > 
> > I mean what the CSI-2 spec means. The exact interpretation of the format
> > will be a combination of the media bus code and the CFA pattern control.
> > The whole point of this discussion is to not have different media bus
> > codes for all possible combinations of formats, as that clearly doesn't
> > scale.
> 
> You mean that we should propagate the value as raw{size} through the
> entire pipeline, if I got this correctly. How the picture should be
> interpreted is up to the user-space by calling a new read-only CFA
> v4l2-control. This way we don't need to patch each subdev driver and
> take the user-space into account of interpreting the data. For the CFA
> control we could use a global-unique list so the control returns an
> enum.

That's the idea, yes. The CFA pattern control would be read-only on the
sensor side (but its value may change if h/v flip is implemented,
depending on the sensor, or possibly if we allow moving the crop
rectangle by one pixel instead of two), and would be writable on any
subdev that needs the information (typically the subdev that implements
CFA interpolation).
diff mbox series

Patch

diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst
index bd68588b2683..d774ccd57c1b 100644
--- a/Documentation/userspace-api/media/v4l/subdev-formats.rst
+++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst
@@ -2252,6 +2252,27 @@  organization is given as an example for the first pixel only.
       - g\ :sub:`2`
       - g\ :sub:`1`
       - g\ :sub:`0`
+    * .. _MEDIA_BUS_FMT_SGRGB_IGIG_GBGR_IGIG8_1X8:
+
+      - MEDIA_BUS_FMT_SGRGB_IGIG_GBGR_IGIG8_1X8
+      - 0x3021
+      -
+      -
+      -
+      -
+      -
+      -
+      -
+      -
+      -
+      - g\ :sub:`7`
+      - g\ :sub:`6`
+      - g\ :sub:`5`
+      - g\ :sub:`4`
+      - g\ :sub:`3`
+      - g\ :sub:`2`
+      - g\ :sub:`1`
+      - g\ :sub:`0`
     * .. _MEDIA-BUS-FMT-SRGGB8-1X8:
 
       - MEDIA_BUS_FMT_SRGGB8_1X8
@@ -2748,6 +2769,27 @@  organization is given as an example for the first pixel only.
       - g\ :sub:`2`
       - g\ :sub:`1`
       - g\ :sub:`0`
+    * .. _MEDIA_BUS_FMT_SGRGB_IGIG_GBGR_IGIG12_1X12:
+
+      - MEDIA_BUS_FMT_SGRGB_IGIG_GBGR_IGIG12_1X12
+      - 0x3022
+      -
+      -
+      -
+      -
+      -
+      - g\ :sub:`11`
+      - g\ :sub:`10`
+      - g\ :sub:`9`
+      - g\ :sub:`8`
+      - g\ :sub:`7`
+      - g\ :sub:`6`
+      - g\ :sub:`5`
+      - g\ :sub:`4`
+      - g\ :sub:`3`
+      - g\ :sub:`2`
+      - g\ :sub:`1`
+      - g\ :sub:`0`
     * .. _MEDIA-BUS-FMT-SRGGB12-1X12:
 
       - MEDIA_BUS_FMT_SRGGB12_1X12
diff --git a/include/uapi/linux/media-bus-format.h b/include/uapi/linux/media-bus-format.h
index 0dfc11ee243a..cdd995e44926 100644
--- a/include/uapi/linux/media-bus-format.h
+++ b/include/uapi/linux/media-bus-format.h
@@ -112,10 +112,11 @@ 
 #define MEDIA_BUS_FMT_YUV16_1X48		0x202a
 #define MEDIA_BUS_FMT_UYYVYY16_0_5X48		0x202b
 
-/* Bayer - next is	0x3021 */
+/* Bayer - next is	0x3023 */
 #define MEDIA_BUS_FMT_SBGGR8_1X8		0x3001
 #define MEDIA_BUS_FMT_SGBRG8_1X8		0x3013
 #define MEDIA_BUS_FMT_SGRBG8_1X8		0x3002
+#define MEDIA_BUS_FMT_SGRGB_IGIG_GBGR_IGIG8_1X8	0x3021
 #define MEDIA_BUS_FMT_SRGGB8_1X8		0x3014
 #define MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8		0x3015
 #define MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8		0x3016
@@ -136,6 +137,7 @@ 
 #define MEDIA_BUS_FMT_SBGGR12_1X12		0x3008
 #define MEDIA_BUS_FMT_SGBRG12_1X12		0x3010
 #define MEDIA_BUS_FMT_SGRBG12_1X12		0x3011
+#define MEDIA_BUS_FMT_SGRGB_IGIG_GBGR_IGIG12_1X12	0x3022
 #define MEDIA_BUS_FMT_SRGGB12_1X12		0x3012
 #define MEDIA_BUS_FMT_SBGGR14_1X14		0x3019
 #define MEDIA_BUS_FMT_SGBRG14_1X14		0x301a