[libcamera-devel] [PATCH 1/9] libcamera: bayer_format: Add toMbusCode method
Dave Stevenson
dave.stevenson at raspberrypi.com
Wed Jan 11 14:45:13 CET 2023
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. ??
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.
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.
> >
> > > 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.
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