| Message ID | 20200602013909.3170593-8-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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; > > } > >
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@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
Hi Niklas, On Sat, Jun 06, 2020 at 03:55:50PM +0200, Niklas Söderlund wrote: > On 2020-06-06 01:06:34 +0300, Laurent Pinchart wrote: > > 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@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. There are pros and cons in both cases, but the fact that the sensor is part of the CIO2 media graph isn't a decise argument in my opinion. The media graph is a kernel abstraction, and we should design a userspace abstraction that makes the userspace implementation as clean and easy to understand as possible. It may match the media graph partition, or it may not. > 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 think that's a good step forward, I'll check if I find it clean enough :-) > > 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; > > > > } > > > >
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@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
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) }, +}; + /** * \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; } +StreamConfiguration +CIO2Device::generateConfiguration(const PixelFormat desiredPixelFormat, + const Size desiredSize) const +{ + StreamConfiguration cfg; + + /* If no desired pixelformat allow all supported.*/ + 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; + } + } + } + + /* 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); + + 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; + 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; }
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@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(-)