[libcamera-devel] [PATCH v2 07/10] libcamera: ipu3: cio2: Add function to generate configuration
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Jun 6 23:38:52 CEST 2020
Hi Niklas,
Thank you for the patch.
On Sat, Jun 06, 2020 at 05:04:33PM +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 its functionality. With this change applications can now
> function with pixelformats other then SBGGR10 which previously was hard
> coded.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> * 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 | 55 ++++++++++++++++++++++++++
> src/libcamera/pipeline/ipu3/cio2.h | 8 +++-
> src/libcamera/pipeline/ipu3/ipu3.cpp | 58 +++++++---------------------
> 3 files changed, 75 insertions(+), 46 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index 1a9f185bc4e161c5..0d961ae8f5a0682b 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -9,6 +9,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"
> @@ -18,6 +20,17 @@ namespace libcamera {
>
> LOG_DECLARE_CATEGORY(IPU3)
>
> +namespace {
> +
> +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) },
> +};
> +
> +} /* namespace */
> +
> CIO2Device::CIO2Device()
> : output_(nullptr), csi2_(nullptr), sensor_(nullptr)
> {
> @@ -172,6 +185,48 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
> return 0;
> }
>
> +StreamConfiguration
> +CIO2Device::generateConfiguration(const PixelFormat desiredPixelFormat,
> + const Size desiredSize) const
I think Jacopo mentioned he would like this function to be documented. I
think it's a good idea in general, even if the documentation is ignored
by Doxygen, as the code may serve as a reference implementation.
> +{
> + StreamConfiguration cfg;
> +
> + /* If no desired pixelformat allow all the supported ones. */
> + std::vector<unsigned int> mbusCodes = {
> + 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 };
> + break;
> + }
> + }
> + }
I still don't really see the point of this. Maybe I'm missing something
obvious, but given that the sensor has a hardcoded Bayer pattern,
what's the point in trying to select a particular desired pattern ? What
could be selected is the number of bits per pixels, but we only support
10bpp for now.
> +
> + /* 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. */
> + 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 = 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 b2c4f89d682d6cfb..b7eb69a4e104f400 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.h
> +++ b/src/libcamera/pipeline/ipu3/cio2.h
> @@ -11,6 +11,9 @@
> #include <queue>
> #include <vector>
>
> +#include <libcamera/geometry.h>
> +#include <libcamera/pixel_format.h>
> +
> namespace libcamera {
>
> class CameraSensor;
> @@ -19,7 +22,7 @@ class MediaDevice;
> class V4L2DeviceFormat;
> class V4L2Subdevice;
> class V4L2VideoDevice;
> -struct Size;
> +struct StreamConfiguration;
>
> class CIO2Device
> {
> @@ -32,6 +35,9 @@ public:
> int init(const MediaDevice *media, unsigned int index);
> int configure(const Size &size, V4L2DeviceFormat *outputFormat);
>
> + StreamConfiguration generateConfiguration(const PixelFormat desiredPixelFormat = {},
If the pixel format isn't a reference, const isn't required.
> + const Size desiredSize = {}) const;
Shouldn't you pass a reference to Size ? You could then keep the forward
declaration of Size.
> +
> int allocateBuffers();
> void freeBuffers();
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 257f5441c9ad7f5d..85d4e64396e77222 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_;
That seems an abuse of StreamConfiguration :-( You should at the very
least rename the variable, both because it's not a format anymore, and
because it's the CIO2 output format, not the sensor format. The
sensorFormat() function should also be renamed accordingly.
> std::vector<const IPU3Stream *> streams_;
> };
>
> @@ -304,7 +297,6 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
>
> CameraConfiguration::Status IPU3CameraConfiguration::validate()
> {
> - const CameraSensor *sensor = data_->cio2_.sensor_;
> Status status = Valid;
>
> if (config_.empty())
> @@ -316,31 +308,23 @@ 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 = {};
No need for explicit initialization of pixelFormat (or size, for that
matter).
> 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);
> + if (!sensorFormat_.pixelFormat.isValid())
> + return Invalid;
>
> /* Assign streams to each configuration entry. */
> assignStreams();
> @@ -352,13 +336,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);
> @@ -442,17 +420,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;
> }
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list