[libcamera-devel] [PATCH 07/10] libcamera: ipu3: cio2: Add function to generate configuration
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Jun 6 00:06:34 CEST 2020
Hi Niklas,
Thank you for the patch.
On Tue, Jun 02, 2020 at 02:08:37PM +0200, Jacopo Mondi wrote:
> 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
s/it's/its/
> > switch which Bayer format pattern are used instead being more or less
> > forced to use SBGGR10.
Isn't the Bayer pattern fixed by the sensor ?
> > 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 ?
That's what we tend to use, yes.
> > /**
> > * \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/
"all *the* 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);
If the requested format isn't supported by the sensor, this function
will return a default constructed V4L2SubdeviceFormat...
> > +
> > + cfg.size = sensorFormat.size;
> > + cfg.pixelFormat = sensorMbusToPixel.at(sensorFormat.mbus_code);
... and this will throw an exception.
> > + cfg.bufferCount = CIO2_BUFFER_COUNT;
> > +
> > + return cfg;
> > +}
I don't think this belongs to the CIO2Device class, as it has nothing to
do with the CIO2. As Jacopo commented separately, the CIO2Device class
should focus on the CIO2. The sensor should be handled by the top-level
pipeline handler.
I would by the way also split the ImgUDevice class to a separate file.
> > +
> > /**
> > * \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;
> > }
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list