[libcamera-devel] [PATCH 1/9] libcamera: bayer_format: Add toMbusCode method
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Wed Jan 11 16:47:19 CET 2023
Hi
On Wed, Jan 11, 2023 at 01:45:13PM +0000, Dave Stevenson wrote:
> Hi
>
> On Tue, 10 Jan 2023 at 14:47, David Plowman
> <david.plowman at raspberrypi.com> wrote:
> >
> > Hi Jacopo
> >
> > On Tue, 10 Jan 2023 at 12:38, Jacopo Mondi <jacopo at jmondi.org> wrote:
> > >
> > > Hi David,
> > >
> > > On Tue, Jan 10, 2023 at 11:59:19AM +0000, David Plowman wrote:
> > > > Hi again
> > > >
> > > > I was thinking about this a bit more. One thing I wanted to clarify
> > > > was what media bus formats really mean. For example, if I have
> > > > MEDIA_BUS_FMT_SBGGR10_1X10, presumably that means Bayer BGGR with CSI2
> > > > packing. Does that sound right?
> > >
> > > Not per my understanding
> > >
> > > The "CSI-2 packaged" Bayer format are described by the 'P' variant of
> > > the canonical raw Bayer formats packaged in 5 bytes according to the
> > > CSI-2 specifications.
> > >
> > > In example section "11.4.4 RAW10" specifies
> > > The transmission of 10-bit Raw data is accomplished by packing the
> > > 10-bit pixel data to look like 8-bit data format.
> > >
> > > Which means the first 4 bytes will contain the 8 MSB and the last
> > > byte contains the 2 LSB of each pixel.
> > >
> > > V4L2 defines pixel formats for CSI-2 packaged bayer formats, but not
> > > media bus codes as far as I can tell:
> > > https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/pixfmt-srggb10p.html
> > >
> > > This surprises me as I would be expecting that being the formats
> > > defined by the CSI-2 specs that's actually what it is sent on the wire
> > > but as far as I can tell sensors I've seen do not send RAW10 that way?
> > >
> > > Am I confused ?
> >
> > Well, I'm pretty baffled by the whole thing too.
> >
> > We're definitely using MEDIA_BUS_FMT_SBGGR10_1X10 to mean CSI-2 packed
> > 10-bit BGGR, and looking at all the media bus formats, I can't see
> > there's anything else. So it must be true. ??
>
I interpret this as a confirmation that 10 bits RAW bayer is sent as
+------+------+------+------+------+
| MSB0 | MSB1 | MSB2 | MSB3 | LSBs |
+------+------+------+------+------+
by sensors. That's what the CSI-2 specification say, so I shouldn't be
surprised (with a little thinking, it makes sense to arrange data in
8-bits samples for CSI-2 as, at least on D-PHY, it's a multiple (or a
divider) of the number of data lanes...)
If that's the case, yes, I guess MEDIA_BUS_FMT_SBGGR10_1X10 implies
the CSI-2 packaging as you originally suggested,
> It is all rather ugly when it comes to CSI2 as it doesn't define the
> bus format in any sensible shape or form.
>
> The 1X10, 2X8, 1X16 stuff is all related to a parallel bus taking a
> bus width and number of transfers to pass the pixel value. To
> accurately represent CSI2 it would therefore be representing the
> number of data lanes as well as the format, and that is not
> information that userspace should be worrying about.
>
> Fundamentally for CSI-2, there is no padding in the datastream,
> therefore 1X.. is the closest representation. Any unpacking into a 16
> bit representation (ie BGGR10 vs BGGR10P) is done in the receiver.
> RPi doesn't have a parallel camera interface so we really don't care,
> but libcamera doesn't have that constraint.
>
That "constraint" or that "privilege" ? :)
> Even on a parallel bus MEDIA_BUS_FMT's don't fully work, as the
> physical bus isn't necessarily a 1:1 mapping.
> Less so with Bayer data, but there's an ongoing discussion for DPI
> displays where you might have a display that wants RGB666. The panel
> could legitimately advertise MEDIA_BUS_FMT_RGB666_1X18, but the wiring
> to the host could be packed onto a 24 bit bus so wants
> MEDIA_BUS_FMT_RGB888_1X24 (with 6 wires not connected), or
> MEDIA_BUS_FMT_RGB666_1X24_CPADHI, or a number of others. Two different
> formats needed on opposite ends of the same link :-(
>
> Yes with any 2X.. formats it matters, but for any 1X.. format, it
> really doesn't.
>
> > >
> > >
> > > >
> > > > Under those circumstances it seems reasonable to me that the
> > > > mbusCodeToBayer table (bayer_format.cpp) should specify Packing::CSI2P
> > > > for most of the formats there, but with new values (e.g.
> > > > Packing::PADHI_BE) for the ones now under discussion.
> > > >
> > > > I don't think such a change would bother Raspberry Pi (it might well
> > > > "just work", or if not require only very minor tweaks), but someone
> > > > else would have to comment on ipu3 or rkisp1.
> > > >
> > >
> > > Please note that 2X8 formats are for parallel busses only, and the
> > > PAD.. only apply to them. I wonder if we do care about them for real..
> >
> > That's a fair point, though I do feel there's something wrong when we
> > can't get back the same "kind" of mbus format as we put in. If it's
> > not too difficult to sort out, we probably should. ??
>
> The only source for MEDIA_BUS_FMT_SBGGR10_2X8_PAD* is the Sharp
> RJ54N1CB0C, and it also supports MEDIA_BUS_FMT_SBGGR10_1X10.
> That means you haven't a hope of writing generic code to be able to
> reverse the conversion without defining it as packing or similar.
>
> drivers/gpu/ipu-v3/ipu-csi.c and
> drivers/media/platform/intel/pxa_camera have support for receiving
> these formats.
>
> Are i.MX5, i.MX6Q or Intel PXA27x Quick Capture Interface supported by
> libcamera? If not, then drop the mappings, and worry about it when
> someone tries to interface a RJ54N1CB0C with libcamera.
>
These are all fair points and I agree David's patch could be made to
work if we guarantee the mbusCodeToBayer table is a 1:1 mapping. One
BayerFormat for one media bus code and you can get from one to another
without issues.
imx6q is supported by the simple pipeline handler, but I guess the
decision is more about how to support parallel RAW sensor that produce
MEDIA_BUS_FMT_...10_2X8_PAD.. formats than the platform.
As David suggested it could be done by adding a dedicated Packing::
and transform all the existing Packing::None in Packing::CSI2 in
mbusCodeToBayer[].
But there's one point that worries me: we have two V4L2_PIX_FMT_ for
each raw format, as bayer data might be stored into memory:
- as they're received from the CSI-2 bus (the P format)
- re-pacakged by the CSI-2 receiver in 10 bits samples (the non-P format)
Looking at BayerFormat::fromV4L2PixelFormat() I wonder what happens if:
(code from your validate() implementation)
fourcc = V4L2_PIX_FMT_SGRBG10P;
BayerFormat bayer = BayerFormat::fromV4L2PixelFormat(fourcc);
this returns:
{ MEDIA_BUS_FMT_SBGGR8_1X8, { BayerFormat::BGGR, 8, BayerFormat::Packing::CSI2 } },
fourcc = bayer.toV4L2PixelFormat();
this fails as there's no entry with Packing:CSI2 in the
mbusCodeToBayer[] table
If we switch all the entries to use Packing::CSI2 we would have the
same problem if the video device reports the re-packaged pix format:
fourcc = V4L2_PIX_FMT_SGRBG10;
BayerFormat bayer = BayerFormat::fromV4L2PixelFormat(fourcc);
this returns:
{ MEDIA_BUS_FMT_SBGGR8_1X8, { BayerFormat::BGGR, 8, BayerFormat::Packing::None } },
fourcc = bayer.toV4L2PixelFormat();
this fails as there's no entry with Packing:None in the
mbusCodeToBayer[] table
Does you pipeline handler ever select the P format variants from the
unicam video device ?
> > >
> > > > Dave also mentions that all the non-BGGR versions are missing (it
> > > > seems a bit disappointing that they weren't added all at the same
> > > > time), but for the moment I suggest we ignore that.
> > > >
> > > > I also note there's a potential problem with the "ALAW8" and "DPCM8"
> > > > formats. I don't know if folks think those can be handled by more
> > > > "packing" formats, or whether we'd rather have a new "modifiers"
> > > > field?
> > >
> > > ALAW and DPCM8 could be described as "compressions" more than
> > > packaging ? Reading
> > > https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/pixfmt-srggb10alaw8.html
> > > it seems to imply a 10-bit pixel gets compressed to an 8-bit sample,
> > > so that impacts the buffer and line sizes ?
> >
> > I think these would actually work if we treated them as "packing" too.
> > "compression" is just a fancier form of packing, kind of. Or maybe we
> > should rename the "packing" field to "modifier", that might feel less
> > strange.
>
> I'll agree that ALAW and DPCM would be packings as they affect how the
> data has to be interpreted. They also map through to V4L2 formats.
>
> Whether anyone other than vimc actually still uses them is a totally
> different question.
> I see et8ek8 (wow, blast from the past) has a mode using DPCM 10 to 8,
> and CCS supports it, but most users these days care about image
> quality more than bandwidth.
>
> I don't see reference to ALAW in the CSI-2 spec, so I wonder where
> that one came from.
No idea but those concerning to me that the 4 _2X8_PAD variants.
>
> Dave
>
> > >
> > > >
> > > > Thoughts?
> > >
> > > Yes, and all of them rather confused :)
> >
> > Ditto.
> >
> > I would be happy to make up a PR to attempt to improve some of this,
> > but we'd have to find someone else to worry about non-Pi platforms.
> >
> > David
> >
> > >
> > > >
> > > > David
> > > >
> > > > On Fri, 6 Jan 2023 at 17:28, Dave Stevenson
> > > > <dave.stevenson at raspberrypi.com> wrote:
> > > > >
> > > > > Hi David and Jacopo
> > > > >
> > > > > On Fri, 6 Jan 2023 at 16:21, David Plowman via libcamera-devel
> > > > > <libcamera-devel at lists.libcamera.org> wrote:
> > > > > >
> > > > > > Hi Jacopo
> > > > > >
> > > > > > On Wed, 4 Jan 2023 at 13:12, Jacopo Mondi <jacopo at jmondi.org> wrote:
> > > > > > >
> > > > > > > Hi David
> > > > > > > sorry to revive this, but I'm carrying this series in my tree and I
> > > > > > > just found a bug in this patches
> > > > > > >
> > > > > > >
> > > > > > > On Thu, Nov 24, 2022 at 01:12:12PM +0100, Jacopo Mondi wrote:
> > > > > > > > From: David Plowman <david.plowman at raspberrypi.com>
> > > > > > > >
> > > > > > > > This makes it easier to perform transformations on Bayer type mbus
> > > > > > > > codes by converting them to a BayerFormat, doing the transform, and
> > > > > > > > then converting them back again.
> > > > > > > >
> > > > > > > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > > > > > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > > > > > ---
> > > > > > > > include/libcamera/internal/bayer_format.h | 1 +
> > > > > > > > src/libcamera/bayer_format.cpp | 11 +++++++++++
> > > > > > > > 2 files changed, 12 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h
> > > > > > > > index 78ba3969913d..3601dccb7228 100644
> > > > > > > > --- a/include/libcamera/internal/bayer_format.h
> > > > > > > > +++ b/include/libcamera/internal/bayer_format.h
> > > > > > > > @@ -47,6 +47,7 @@ public:
> > > > > > > > }
> > > > > > > >
> > > > > > > > static const BayerFormat &fromMbusCode(unsigned int mbusCode);
> > > > > > > > + unsigned int toMbusCode() const;
> > > > > > > > bool isValid() const { return bitDepth != 0; }
> > > > > > > >
> > > > > > > > std::string toString() const;
> > > > > > > > diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp
> > > > > > > > index f27cc1662d25..fdbc4af1dcc0 100644
> > > > > > > > --- a/src/libcamera/bayer_format.cpp
> > > > > > > > +++ b/src/libcamera/bayer_format.cpp
> > > > > > > > @@ -226,6 +226,17 @@ const BayerFormat &BayerFormat::fromMbusCode(unsigned int mbusCode)
> > > > > > > > return it->second;
> > > > > > > > }
> > > > > > > >
> > > > > > > > +/**
> > > > > > > > + * \brief Retrieve the media bus code corresponding this this BayerFormat
> > > > > > > > + * \return The corresponding media bus code, or zero if none was found
> > > > > > > > + */
> > > > > > > > +unsigned int BayerFormat::toMbusCode() const
> > > > > > > > +{
> > > > > > > > + auto it = std::find_if(mbusCodeToBayer.begin(), mbusCodeToBayer.end(),
> > > > > > > > + [this](const auto &i) { return i.second == *this; });
> > > > > > > > + return it != mbusCodeToBayer.end() ? it->first : 0;
> > > > > > >
> > > > > > > I'm working with a sensor that produces MEDIA_BUS_FMT_SBGGR10_1X10
> > > > > > > It doesn't register flips, so the bayer pattern should not be changed
> > > > > > >
> > > > > > > The code you added in the next patch for the CameraSensor class goes
> > > > > > > as
> > > > > > >
> > > > > > > for (const auto &format : formats) {
> > > > > > > unsigned int mbusCode = format.first;
> > > > > > > BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);
> > > > > > >
> > > > > > > if (bayerFormat.isValid())
> > > > > > > mbusCode = bayerFormat.transform(transform).toMbusCode();
> > > > > > >
> > > > > > > if (mbusCode)
> > > > > > > formats_[mbusCode] = std::move(format.second);
> > > > > > > }
> > > > > > >
> > > > > > >
> > > > > > > The first
> > > > > > >
> > > > > > > BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);
> > > > > > >
> > > > > > > returns { BayerFormat::BGGR, 10, BayerFormat::Packing::None }
> > > > > > >
> > > > > > > we go throgh a transform() which does nothing as it receives Indentity
> > > > > > > and then call toMbusCode() on it
> > > > > > >
> > > > > > > The above implementation of toMbusCode() walks the mbusCodeToBayer map
> > > > > > > looking for a matching bayer. Look at this table section
> > > > > > >
> > > > > > > { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> > > > > > > { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> > > > > > > { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> > > > > > > { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> > > > > > > { MEDIA_BUS_FMT_SBGGR10_1X10, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> > > > > > >
> > > > > > > So here we have that all the MEDIA_BUS_FMT_SBGGR10_2X8_PAD.. and
> > > > > > > MEDIA_BUS_FMT_SBGGR10_1X10 codes match on the same
> > > > > > > { BayerFormat::BGGR, 10, BayerFormat::Packing::None } and I suddenly
> > > > > > > get my original 0x3007 code transformed to 0x3003!
> > > > > > >
> > > > > > > I'm afraid the fact mbus_code-to-BayerFormat relationship is not
> > > > > > > unique is a blocker for this patch
> > > > > >
> > > > > > Oh dear! Thanks for discovering this, I had no idea that these formats existed.
> > > > >
> > > > > Why do those formats exist, and why only the BGGR ordering versions?
> > > > > A partial implementation of something, and presumably on a parallel
> > > > > interface as formats such as YUYV are mandated to use the 1X16
> > > > > versions (not 2X8) on CSI.
> > > > >
> > > > > Are they actually relevant, or legacy cruft?
> > > > >
> > > > > Dave
> > > > >
> > > > > > I wonder if the correct solution is to have 4 extra packing formats,
> > > > > > alongside the existing CSI2 and IPU3 ones. What do you think?
> > > > > >
> > > > > > David
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > /**
> > > > > > > > * \fn BayerFormat::isValid()
> > > > > > > > * \brief Return whether a BayerFormat is valid
> > > > > > > > --
> > > > > > > > 2.38.1
> > > > > > > >
More information about the libcamera-devel
mailing list