[libcamera-devel] [PATCH 07/10] libcamera: ipu3: cio2: Add function to generate configuration
Jacopo Mondi
jacopo at jmondi.org
Tue Jun 2 14:08:37 CEST 2020
Hi Niklas,
On Tue, Jun 02, 2020 at 03:39:06AM +0200, Niklas Söderlund wrote:
> Collect the code used to generate configurations for the CIO2 block in
> the CIO2Device class. This allows for both simplifying the code while
> extending it's functionality. With this change applications can now
> switch which Bayer format pattern are used instead being more or less
> forced to use SBGGR10.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> src/libcamera/pipeline/ipu3/cio2.cpp | 44 +++++++++++++++++++++
> src/libcamera/pipeline/ipu3/cio2.h | 5 +++
> src/libcamera/pipeline/ipu3/ipu3.cpp | 57 ++++++----------------------
> 3 files changed, 60 insertions(+), 46 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index 113486e3e3d0f2f1..2263d6530ec6b672 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -11,6 +11,13 @@ namespace libcamera {
>
> LOG_DECLARE_CATEGORY(IPU3)
>
> +static const std::map<uint32_t, PixelFormat> sensorMbusToPixel = {
> + { MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) },
> + { MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) },
> + { MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) },
> + { MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) },
> +};
> +
What about an anonymous namespace instead of static ?
> /**
> * \brief Initialize components of the CIO2 device with \a index
> * \param[in] media The CIO2 media device
> @@ -153,6 +160,43 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
> return 0;
> }
>
No documentation ?
src/libcamera/pipeline/ is excluded from the Doxyegen search path, but
I would like to see documentation anyhow, especially about the
(optional) parameters.
> +StreamConfiguration
> +CIO2Device::generateConfiguration(const PixelFormat desiredPixelFormat,
> + const Size desiredSize) const
Is generateConfiguration the correct name ? That's the Camera and
pipeline handler API name, while this is internal to the IPU3
machinery. I don't have much better ideas, so feel free to keep it if
you are not concerned about the name clash.
> +{
> + StreamConfiguration cfg;
> +
> + /* If no desired pixelformat allow all supported.*/
s/all supported/all supported ones/
Also, missing space at the end
> + std::vector<unsigned int> mbusCodes = {
You should use array<> as the size is fixed. Also, const.
> + MEDIA_BUS_FMT_SBGGR10_1X10,
> + MEDIA_BUS_FMT_SGBRG10_1X10,
> + MEDIA_BUS_FMT_SGRBG10_1X10,
> + MEDIA_BUS_FMT_SRGGB10_1X10
> + };
> + if (desiredPixelFormat.isValid()) {
> + for (const auto &iter : sensorMbusToPixel) {
> + if (iter.second == desiredPixelFormat) {
> + mbusCodes = { iter.first };
Ah no, you can't use array<>
However this code doesn't make sure you actually find any matching
format, and if you don't application has requested an unsupported raw
format, so you should fail loudly here imo.
> + break;
> + }
> + }
> + }
> +
> + /* 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. */
Which static information ? You get the current configured format and
use it...
> + V4L2SubdeviceFormat sensorFormat = sensor_->getFormat(mbusCodes, size);
> +
> + cfg.size = sensorFormat.size;
> + cfg.pixelFormat = sensorMbusToPixel.at(sensorFormat.mbus_code);
> + 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 d923038bb4ba356f..2e268a7154b2d241 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.h
> +++ b/src/libcamera/pipeline/ipu3/cio2.h
> @@ -12,6 +12,8 @@
>
> #include <linux/media-bus-format.h>
>
> +#include <libcamera/stream.h>
> +
> #include "libcamera/internal/camera_sensor.h"
> #include "libcamera/internal/media_device.h"
> #include "libcamera/internal/v4l2_subdevice.h"
> @@ -39,6 +41,9 @@ public:
> int init(const MediaDevice *media, unsigned int index);
> int configure(const Size &size, V4L2DeviceFormat *outputFormat);
>
> + StreamConfiguration generateConfiguration(const PixelFormat desiredPixelFormat = {},
> + const Size desiredSize = {}) const;
> +
This could probably be simplified in its implementation by providing
two overloaded methods instead of defaulting parameters and
mix-matching them in the implementation (and default parameters in C++
are a pain to follow, as they are declared as optional in the class
declaration only).
> int allocateBuffers();
> void freeBuffers();
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 2047deac299dbf75..56cc3ca10414f0d2 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -35,13 +35,6 @@ LOG_DEFINE_CATEGORY(IPU3)
>
> class IPU3CameraData;
>
> -static const std::map<uint32_t, PixelFormat> sensorMbusToPixel = {
> - { MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) },
> - { MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) },
> - { MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) },
> - { MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) },
> -};
> -
> class ImgUDevice
> {
> public:
> @@ -146,7 +139,7 @@ public:
>
> Status validate() override;
>
> - const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
> + const StreamConfiguration &sensorFormat() const { return sensorFormat_; };
> const std::vector<const IPU3Stream *> &streams() { return streams_; }
>
> private:
> @@ -164,7 +157,7 @@ private:
> std::shared_ptr<Camera> camera_;
> const IPU3CameraData *data_;
>
> - V4L2SubdeviceFormat sensorFormat_;
> + StreamConfiguration sensorFormat_;
> std::vector<const IPU3Stream *> streams_;
> };
>
> @@ -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())
> @@ -325,32 +317,21 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> status = Adjusted;
> }
>
> - /*
> - * Select the sensor format by collecting the maximum width and height
> - * and picking the closest larger match, as the IPU3 can downscale
> - * only. If no resolution is requested for any stream, or if no sensor
> - * resolution is large enough, pick the largest one.
> - */
> + /* Find largets size and raw format (if any) in the configuration. */
> Size size = {};
> -
> + PixelFormat pixelFormat = {};
> for (const StreamConfiguration &cfg : config_) {
> if (cfg.size.width > size.width)
> size.width = cfg.size.width;
> if (cfg.size.height > size.height)
> size.height = cfg.size.height;
> +
> + if (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED)
> + pixelFormat = cfg.pixelFormat;
> }
>
> - 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. */
> + sensorFormat_ = data_->cio2_.generateConfiguration(pixelFormat, size);
>
> /* Assign streams to each configuration entry. */
> if (updateStreams())
> @@ -363,13 +344,7 @@ 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 = sensorFormat_;
> } else {
> bool scale = stream == &data_->vfStream_;
> adjustStream(config_[i], scale);
> @@ -452,17 +427,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> }
>
> stream = &data->rawStream_;
> -
> - 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();
> break;
> }
>
> --
> 2.26.2
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list