[libcamera-devel] [PATCH 07/10] libcamera: ipu3: cio2: Add function to generate configuration
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sat Jun 6 15:55:50 CEST 2020
Hello,
On 2020-06-06 01:06:34 +0300, Laurent Pinchart wrote:
> 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 ?
It is, what I tried to expressed is that other pixelformats then SBGGR10
is now possible to select from the application, will update.
>
> > > 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.
It can't be const as it's (possibly) modified bellow.
> >
> > > + 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.
If no match is found the mbusCodes is not modified and all 4 possible
mbus codes are considered when probing the sensor for a format. No need
to warn IMHO.
> >
> > > + 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...
Not true, CameraSensor::getFormat() returns the best match for the mbus
code and size from the static/cached information it enumerated in
CameraSensor::init().
> >
> > > + V4L2SubdeviceFormat sensorFormat = sensor_->getFormat(mbusCodes, size);
>
> If the requested format isn't supported by the sensor, this function
> will return a default constructed V4L2SubdeviceFormat...
Good point, will fix.
>
> > > +
> > > + 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.
As discussed on IRC I disagree wit this. I think the sensor belongs
inside the CIO2Device as it's part of the CIO2 media graph. Handling it
outside the CIO2Device will IMHO only create a spaghetti mess similar to
the one we already have where all objects in the IPU3 pipeline access
member variables all over the place.
I will for v2 remove the proxy helpers to access the sensor and replace
it with a CIO2Device::sensor() in hops this will suite Jacopos and your
concerns, lets see how it plays out :-)
>
> I would by the way also split the ImgUDevice class to a separate file.
I plan to do so on top of this series, as mentioned in the cover letter.
>
> > > +
> > > /**
> > > * \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).
I slightly prefer default arguments over multiple implementations, but
it's not a strong feeling. But if we are to drop default arguments lets
do so as part of a series that address the issue everywhere.
> >
> > > 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
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list