[libcamera-devel] [PATCH 07/10] libcamera: ipu3: cio2: Add function to generate configuration
Jacopo Mondi
jacopo at jmondi.org
Mon Jun 8 10:30:30 CEST 2020
Hi Niklas,
On Sat, Jun 06, 2020 at 03:55:50PM +0200, Niklas Söderlund wrote:
> 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.
>
Actually, to me, if the user requires something, and it gets silently
defaulted to something else, that's an issue which is worth signaling
out. Not sure how often it happens here, but to me if the user is
given somehing different from what it has requested, it should be
signaled. I would say the function should fail in that case, but maybe
it's too harsh ?
> > >
> > > > + 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().
Ok, got confused from the 'static' in the comment.
>
> > >
> > > > + 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.
I wonder if this is a good API in first place.
Wouldn't returning an integer and accepting an input
V4L2SubdeviceFormat * make failues in V4L2Subdevice::getFormat() harder
to overlook ?
>
> >
> > > > +
> > > > + 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 understand the reasoning about the fact they belong to same media
graph. I really disliked accessors, as they created an abstraction
which imho hides where those information come from. Accessing the
sensor * and retreiveing information from there, I'm not opposed.
> >
> > 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.
>
I'm not against default parameters in general, but if their useage
makes the function flow like
if (param1 is ok) {
if (param2 is ok) {{
} else {
}
} else {
}
It means to me these are actually two logically different functions
mashed up together. This case is not that bad as you get away with
just 2 ifs, but to me it's harder to follow than two separate
functions (one of which might just be a wrapper that defauls the mbus
codes if not provided). Up to you.
Thanks
j
> > >
> > > > 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