[libcamera-devel,1/9] libcamera: bayer_format: Add toMbusCode method

17887 diff mbox series

From: David Plowman <david.plowman@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@raspberrypi.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/internal/bayer_format.h |  1 +
 src/libcamera/bayer_format.cpp            | 11 +++++++++++
 2 files changed, 12 insertions(+)

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@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@raspberrypi.com>
> Reviewed-by: Jacopo Mondi <jacopo@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





> +}
> +
>  /**
>   * \fn BayerFormat::isValid()
>   * \brief Return whether a BayerFormat is valid
> --
> 2.38.1
>

Hi Jacopo

On Wed, 4 Jan 2023 at 13:12, Jacopo Mondi <jacopo@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@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@raspberrypi.com>
> > Reviewed-by: Jacopo Mondi <jacopo@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.

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
> >

Hi David and Jacopo

On Fri, 6 Jan 2023 at 16:21, David Plowman via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> Hi Jacopo
>
> On Wed, 4 Jan 2023 at 13:12, Jacopo Mondi <jacopo@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@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@raspberrypi.com>
> > > Reviewed-by: Jacopo Mondi <jacopo@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
> > >

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?

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.

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?

Thoughts?

David

On Fri, 6 Jan 2023 at 17:28, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi David and Jacopo
>
> On Fri, 6 Jan 2023 at 16:21, David Plowman via libcamera-devel
> <libcamera-devel@lists.libcamera.org> wrote:
> >
> > Hi Jacopo
> >
> > On Wed, 4 Jan 2023 at 13:12, Jacopo Mondi <jacopo@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@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@raspberrypi.com>
> > > > Reviewed-by: Jacopo Mondi <jacopo@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
> > > >

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 ?


>
> 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..

> 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 ?

>
> Thoughts?

Yes, and all of them rather confused :)

>
> David
>
> On Fri, 6 Jan 2023 at 17:28, Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> >
> > Hi David and Jacopo
> >
> > On Fri, 6 Jan 2023 at 16:21, David Plowman via libcamera-devel
> > <libcamera-devel@lists.libcamera.org> wrote:
> > >
> > > Hi Jacopo
> > >
> > > On Wed, 4 Jan 2023 at 13:12, Jacopo Mondi <jacopo@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@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@raspberrypi.com>
> > > > > Reviewed-by: Jacopo Mondi <jacopo@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
> > > > >

Hi Jacopo

On Tue, 10 Jan 2023 at 12:38, Jacopo Mondi <jacopo@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. ??

>
>
> >
> > 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. ??

>
> > 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.

>
> >
> > 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@raspberrypi.com> wrote:
> > >
> > > Hi David and Jacopo
> > >
> > > On Fri, 6 Jan 2023 at 16:21, David Plowman via libcamera-devel
> > > <libcamera-devel@lists.libcamera.org> wrote:
> > > >
> > > > Hi Jacopo
> > > >
> > > > On Wed, 4 Jan 2023 at 13:12, Jacopo Mondi <jacopo@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@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@raspberrypi.com>
> > > > > > Reviewed-by: Jacopo Mondi <jacopo@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
> > > > > >

Hi

On Tue, 10 Jan 2023 at 14:47, David Plowman
<david.plowman@raspberrypi.com> wrote:
>
> Hi Jacopo
>
> On Tue, 10 Jan 2023 at 12:38, Jacopo Mondi <jacopo@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@raspberrypi.com> wrote:
> > > >
> > > > Hi David and Jacopo
> > > >
> > > > On Fri, 6 Jan 2023 at 16:21, David Plowman via libcamera-devel
> > > > <libcamera-devel@lists.libcamera.org> wrote:
> > > > >
> > > > > Hi Jacopo
> > > > >
> > > > > On Wed, 4 Jan 2023 at 13:12, Jacopo Mondi <jacopo@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@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@raspberrypi.com>
> > > > > > > Reviewed-by: Jacopo Mondi <jacopo@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
> > > > > > >

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@raspberrypi.com> wrote:
> >
> > Hi Jacopo
> >
> > On Tue, 10 Jan 2023 at 12:38, Jacopo Mondi <jacopo@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@raspberrypi.com> wrote:
> > > > >
> > > > > Hi David and Jacopo
> > > > >
> > > > > On Fri, 6 Jan 2023 at 16:21, David Plowman via libcamera-devel
> > > > > <libcamera-devel@lists.libcamera.org> wrote:
> > > > > >
> > > > > > Hi Jacopo
> > > > > >
> > > > > > On Wed, 4 Jan 2023 at 13:12, Jacopo Mondi <jacopo@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@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@raspberrypi.com>
> > > > > > > > Reviewed-by: Jacopo Mondi <jacopo@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
> > > > > > > >