[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:23:45 CEST 2020
Hi Niklas,
Thank you for the patch.
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.
s/ / /
> 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) } },
> };
Would it make sense to keep this as-is, and use PixelFormatInfo::info()
to look up additional information (the V4L2 pixel format in this case,
but potentially more in the future) ? Otherwise we end up duplicating
format information in multiple locations.
>
> } /* 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;
This would become
const PixelFormatInfo &info = PixelFormatInfo::info(itInfo->second);
outputFormat->fourcc = info.v4l2Format;
> 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;
I've just sent a patch titled "PATCH] libcamera: geometry: Add isNull()
function to Size class". You could write this code
Size size = !desiredSize.isNull() ? desiredSize : sensor_->resolution();
Or you could pass the desiredSize parameter by value instead of
reference, rename it to size, and just write
/* If no desired size use the sensor resolution. */
if (size.isNull())
size = sensor_->resolution();
I think that would be the simplest to read option.
> +
> + /* 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; });
I've submitted "[PATCH] libcamera: utils: Add map_keys() function", only
to then realize that here you're storing mbus codes in a vector, not a
set. I have a bit of a feeling that the usage of different types is a
sign that the API isn't consistent. And modifying
CameraSensor::getFormat() to take a set would likely require more
replacement of vectors with sets, which I'm not sure is a good idea (I
haven't checked how far it would spread).
In any case, if we decide to keep vectors, patch "[PATCH] libcamera:
utils: Add map_keys() function" should be changed to return a vector, I
think it would still be useful.
> +
> + V4L2SubdeviceFormat sensorFormat = sensor_->getFormat(mbusCodes, size);
> +
> + 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_;
This may still be an abuse of StreamConfiguration, but I think we can
live with it for now until further refactoring.
> 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)
With the above issues addressed,
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Overall I think this is a nice cleanup, even if there's room for
improvement (mainly addressing the possible abuse of
StreamConfiguration, and the std::vector vs. std::set issue, the latter
not being specific to this patch).
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list