[libcamera-devel] [PATCH v3 07/10] libcamera: ipu3: cio2: Add function to generate configuration
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jun 25 03:25:43 CEST 2020
Hi Jacopo,
On Wed, Jun 24, 2020 at 12:22:01PM +0200, Jacopo Mondi wrote:
> On Tue, Jun 23, 2020 at 04:09:27AM +0200, Niklas Söderlund wrote:
> > Collect the code used to generate configurations for the CIO2 block in
> > the CIO2Device class. This allows simplifying the code and allow further
> > changes to only happen at one code location.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > * Changes since v2
> > - Remove unneeded code to pick sensor FourCC.
> > - Remove desiredPixelFormat from generateConfiguration() as it's not
> > needed.
> > - Rename sensorFormat_ cio2Configuration_
> > - Consolidate all format information in a single table.
> >
> > * Changes since v1
> > - Use anonymous namespace instead of static for sensorMbusToPixel map.
> > - Handle case where the requested mbus code is not supported by the sensor.
> > - Update commit message.
> > ---
> > src/libcamera/pipeline/ipu3/cio2.cpp | 54 +++++++++++++++++++++++----
> > src/libcamera/pipeline/ipu3/cio2.h | 3 ++
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 56 +++++++---------------------
> > 3 files changed, 63 insertions(+), 50 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> > index f23128d412e6b1a5..d6bab896706dd60e 100644
> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> > @@ -9,6 +9,9 @@
> >
> > #include <linux/media-bus-format.h>
> >
> > +#include <libcamera/formats.h>
> > +#include <libcamera/stream.h>
> > +
> > #include "libcamera/internal/camera_sensor.h"
> > #include "libcamera/internal/media_device.h"
> > #include "libcamera/internal/v4l2_subdevice.h"
> > @@ -20,11 +23,16 @@ LOG_DECLARE_CATEGORY(IPU3)
> >
> > namespace {
> >
> > -static const std::map<uint32_t, V4L2PixelFormat> mbusCodesToInfo = {
> > - { MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) },
> > - { MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) },
> > - { MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) },
> > - { MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) },
> > +struct MbusInfo {
> > + PixelFormat pixelFormat;
> > + V4L2PixelFormat fourcc;
> > +};
> > +
> > +static const std::map<uint32_t, MbusInfo> mbusCodesToInfo = {
> > + { MEDIA_BUS_FMT_SBGGR10_1X10, { formats::SBGGR10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) } },
> > + { MEDIA_BUS_FMT_SGBRG10_1X10, { formats::SGBRG10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) } },
> > + { MEDIA_BUS_FMT_SGRBG10_1X10, { formats::SGRBG10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) } },
> > + { MEDIA_BUS_FMT_SRGGB10_1X10, { formats::SRGGB10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) } },
> > };
> >
> > } /* namespace */
> > @@ -92,7 +100,7 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
> > std::set<unsigned int> cio2Codes;
> > std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
> > std::inserter(cio2Codes, cio2Codes.begin()),
> > - [](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; });
> > + [](const std::map<uint32_t, MbusInfo>::value_type &pair){ return pair.first; });
> > const std::set<unsigned int> &sensorCodes = sensor_->mbusCodes();
> > if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),
> > cio2Codes.begin(), cio2Codes.end())) {
> > @@ -139,7 +147,7 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
> > std::vector<unsigned int> mbusCodes;
> > std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
> > std::back_inserter(mbusCodes),
> > - [](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; });
> > + [](const std::map<uint32_t, MbusInfo>::value_type &pair){ return pair.first; });
> >
> > sensorFormat = sensor_->getFormat(mbusCodes, size);
> > ret = sensor_->setFormat(&sensorFormat);
> > @@ -154,7 +162,7 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
> > if (itInfo == mbusCodesToInfo.end())
> > return -EINVAL;
> >
> > - outputFormat->fourcc = itInfo->second;
> > + outputFormat->fourcc = itInfo->second.fourcc;
> > outputFormat->size = sensorFormat.size;
> > outputFormat->planesCount = 1;
> >
> > @@ -167,6 +175,36 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
> > return 0;
> > }
> >
> > +StreamConfiguration
> > +CIO2Device::generateConfiguration(const Size &desiredSize) const
> > +{
> > + StreamConfiguration cfg;
> > +
> > + /* If no desired size use the sensor resolution. */
> > + Size size = sensor_->resolution();
> > + if (desiredSize.width && desiredSize.height)
> > + size = desiredSize;
> > +
> > + /* Query the sensor static information for closest match. */
> > + std::vector<unsigned int> mbusCodes;
> > + std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
> > + std::back_inserter(mbusCodes),
> > + [](const std::map<uint32_t, MbusInfo>::value_type &pair){ return pair.first; });
>
> Oh, one more :(
>
> What about a CIO2Device::mbusCodes() helper for this ? With C++ copy
> elision you would not get any performance penality
See "[PATCH] libcamera: utils: Add map_keys() function" :-) We however
have to decide if the function should return a vector or a set.
> > +
> > + V4L2SubdeviceFormat sensorFormat = sensor_->getFormat(mbusCodes, size);
> > +
>
> stray empty line
>
> The rest is good. I think we're abusing StreamConfiguration, as it is
> supposed to report StreamFormats, but it's optional, and we use it
I wonder what's wrong, we tend to agree a lot lately :-)
> internally to transport plaing "formats" between components. You could
> even use an ImageFormat if my series ever gets any feedback, but I
> won't block this one because of this. But really, we need to fix this
> as it's getting confusing enough :)
>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> > + if (!sensorFormat.mbus_code) {
> > + LOG(IPU3, Error) << "Sensor does not support mbus code";
> > + return {};
> > + }
> > +
> > + cfg.size = sensorFormat.size;
> > + cfg.pixelFormat = mbusCodesToInfo.at(sensorFormat.mbus_code).pixelFormat;
> > + cfg.bufferCount = CIO2_BUFFER_COUNT;
> > +
> > + return cfg;
> > +}
> > +
> > /**
> > * \brief Allocate frame buffers for the CIO2 output
> > *
> > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> > index b2c4f89d682d6cfb..6276573f2b585b25 100644
> > --- a/src/libcamera/pipeline/ipu3/cio2.h
> > +++ b/src/libcamera/pipeline/ipu3/cio2.h
> > @@ -20,6 +20,7 @@ class V4L2DeviceFormat;
> > class V4L2Subdevice;
> > class V4L2VideoDevice;
> > struct Size;
> > +struct StreamConfiguration;
> >
> > class CIO2Device
> > {
> > @@ -32,6 +33,8 @@ public:
> > int init(const MediaDevice *media, unsigned int index);
> > int configure(const Size &size, V4L2DeviceFormat *outputFormat);
> >
> > + StreamConfiguration generateConfiguration(const Size &desiredSize) const;
> > +
> > int allocateBuffers();
> > void freeBuffers();
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 6e5eb5609a3c2388..c0e727e592f46883 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -36,13 +36,6 @@ LOG_DEFINE_CATEGORY(IPU3)
> >
> > class IPU3CameraData;
> >
> > -static const std::map<uint32_t, PixelFormat> sensorMbusToPixel = {
> > - { MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10_IPU3 },
> > - { MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10_IPU3 },
> > - { MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10_IPU3 },
> > - { MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10_IPU3 },
> > -};
> > -
> > class ImgUDevice
> > {
> > public:
> > @@ -147,7 +140,7 @@ public:
> >
> > Status validate() override;
> >
> > - const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
> > + const StreamConfiguration &cio2Format() const { return cio2Configuration_; };
> > const std::vector<const IPU3Stream *> &streams() { return streams_; }
> >
> > private:
> > @@ -165,7 +158,7 @@ private:
> > std::shared_ptr<Camera> camera_;
> > const IPU3CameraData *data_;
> >
> > - V4L2SubdeviceFormat sensorFormat_;
> > + StreamConfiguration cio2Configuration_;
> > std::vector<const IPU3Stream *> streams_;
> > };
> >
> > @@ -252,7 +245,7 @@ void IPU3CameraConfiguration::assignStreams()
> >
> > if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> > stream = &data_->rawStream_;
> > - else if (cfg.size == sensorFormat_.size)
> > + else if (cfg.size == cio2Configuration_.size)
> > stream = &data_->outStream_;
> > else
> > stream = &data_->vfStream_;
> > @@ -277,8 +270,8 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
> > */
> > if (!cfg.size.width || !cfg.size.height) {
> > cfg.size.width = 1280;
> > - cfg.size.height = 1280 * sensorFormat_.size.height
> > - / sensorFormat_.size.width;
> > + cfg.size.height = 1280 * cio2Configuration_.size.height
> > + / cio2Configuration_.size.width;
> > }
> >
> > /*
> > @@ -297,7 +290,7 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
> > * \todo: Properly support cropping when the ImgU driver
> > * interface will be cleaned up.
> > */
> > - cfg.size = sensorFormat_.size;
> > + cfg.size = cio2Configuration_.size;
> > }
> >
> > /*
> > @@ -313,7 +306,6 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
> >
> > CameraConfiguration::Status IPU3CameraConfiguration::validate()
> > {
> > - const CameraSensor *sensor = data_->cio2_.sensor_;
> > Status status = Valid;
> >
> > if (config_.empty())
> > @@ -340,16 +332,10 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> > size.height = cfg.size.height;
> > }
> >
> > - if (!size.width || !size.height)
> > - size = sensor->resolution();
> > -
> > - sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,
> > - MEDIA_BUS_FMT_SGBRG10_1X10,
> > - MEDIA_BUS_FMT_SGRBG10_1X10,
> > - MEDIA_BUS_FMT_SRGGB10_1X10 },
> > - size);
> > - if (!sensorFormat_.size.width || !sensorFormat_.size.height)
> > - sensorFormat_.size = sensor->resolution();
> > + /* Generate raw configuration from CIO2. */
> > + cio2Configuration_ = data_->cio2_.generateConfiguration(size);
> > + if (!cio2Configuration_.pixelFormat.isValid())
> > + return Invalid;
> >
> > /* Assign streams to each configuration entry. */
> > assignStreams();
> > @@ -361,20 +347,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> > const IPU3Stream *stream = streams_[i];
> >
> > if (stream->raw_) {
> > - const auto &itFormat =
> > - sensorMbusToPixel.find(sensorFormat_.mbus_code);
> > - if (itFormat == sensorMbusToPixel.end())
> > - return Invalid;
> > -
> > - cfg.pixelFormat = itFormat->second;
> > - cfg.size = sensorFormat_.size;
> > + cfg = cio2Configuration_;
> > } else {
> > bool scale = stream == &data_->vfStream_;
> > adjustStream(config_[i], scale);
> > + cfg.bufferCount = IPU3_BUFFER_COUNT;
> > }
> >
> > - cfg.bufferCount = IPU3_BUFFER_COUNT;
> > -
> > if (cfg.pixelFormat != oldCfg.pixelFormat ||
> > cfg.size != oldCfg.size) {
> > LOG(IPU3, Debug)
> > @@ -454,14 +433,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> >
> > cfg.size = data->cio2_.sensor_->resolution();
> >
> > - V4L2SubdeviceFormat sensorFormat =
> > - data->cio2_.sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,
> > - MEDIA_BUS_FMT_SGBRG10_1X10,
> > - MEDIA_BUS_FMT_SGRBG10_1X10,
> > - MEDIA_BUS_FMT_SRGGB10_1X10 },
> > - cfg.size);
> > - cfg.pixelFormat =
> > - sensorMbusToPixel.at(sensorFormat.mbus_code);
> > + cfg = data->cio2_.generateConfiguration(cfg.size);
> > break;
> > }
> >
> > @@ -575,7 +547,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > * Pass the requested stream size to the CIO2 unit and get back the
> > * adjusted format to be propagated to the ImgU output devices.
> > */
> > - const Size &sensorSize = config->sensorFormat().size;
> > + const Size &sensorSize = config->cio2Format().size;
> > V4L2DeviceFormat cio2Format = {};
> > ret = cio2->configure(sensorSize, &cio2Format);
> > if (ret)
> > --
> > 2.27.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list