[PATCH v6 10/18] libcamera: introduce SoftwareIsp
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Mar 27 18:27:07 CET 2024
Hi Milan and Andrey,
Thank you for the patch.
On Tue, Mar 19, 2024 at 01:35:57PM +0100, Milan Zamazal wrote:
> From: Andrey Konovalov <andrey.konovalov at linaro.org>
>
> Doxygen documentation by Dennis Bonke.
>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue at linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel at ucw.cz>
> Reviewed-by: Pavel Machek <pavel at ucw.cz>
> Co-developed-by: Dennis Bonke <admin at dennisbonke.com>
> Signed-off-by: Dennis Bonke <admin at dennisbonke.com>
> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
> .../internal/software_isp/meson.build | 1 +
> .../internal/software_isp/software_isp.h | 98 +++++
> src/libcamera/software_isp/meson.build | 1 +
> src/libcamera/software_isp/software_isp.cpp | 349 ++++++++++++++++++
> 4 files changed, 449 insertions(+)
> create mode 100644 include/libcamera/internal/software_isp/software_isp.h
> create mode 100644 src/libcamera/software_isp/software_isp.cpp
>
> diff --git a/include/libcamera/internal/software_isp/meson.build b/include/libcamera/internal/software_isp/meson.build
> index a620e16d..508ddddc 100644
> --- a/include/libcamera/internal/software_isp/meson.build
> +++ b/include/libcamera/internal/software_isp/meson.build
> @@ -2,5 +2,6 @@
>
> libcamera_internal_headers += files([
> 'debayer_params.h',
> + 'software_isp.h',
> 'swisp_stats.h',
> ])
> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
> new file mode 100644
> index 00000000..8d25e979
> --- /dev/null
> +++ b/include/libcamera/internal/software_isp/software_isp.h
> @@ -0,0 +1,98 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Linaro Ltd
> + *
> + * software_isp.h - Simple software ISP implementation
> + */
> +
> +#pragma once
> +
> +#include <functional>
> +#include <initializer_list>
> +#include <map>
> +#include <memory>
> +#include <string>
> +#include <tuple>
> +#include <vector>
> +
> +#include <libcamera/base/class.h>
> +#include <libcamera/base/log.h>
> +#include <libcamera/base/signal.h>
> +#include <libcamera/base/thread.h>
> +
> +#include <libcamera/geometry.h>
> +#include <libcamera/pixel_format.h>
> +
> +#include <libcamera/ipa/soft_ipa_interface.h>
> +#include <libcamera/ipa/soft_ipa_proxy.h>
> +
> +#include "libcamera/internal/dma_heaps.h"
> +#include "libcamera/internal/pipeline_handler.h"
> +#include "libcamera/internal/shared_mem_object.h"
> +#include "libcamera/internal/software_isp/debayer_params.h"
> +
> +namespace libcamera {
> +
> +class DebayerCpu;
> +class FrameBuffer;
> +class PixelFormat;
> +struct StreamConfiguration;
> +
> +LOG_DECLARE_CATEGORY(SoftwareIsp)
> +
> +class SoftwareIsp
> +{
> +public:
> + SoftwareIsp(PipelineHandler *pipe, const ControlInfoMap &sensorControls);
> + ~SoftwareIsp();
> +
> + int loadConfiguration([[maybe_unused]] const std::string &filename) { return 0; }
> +
> + bool isValid() const;
> +
> + std::vector<PixelFormat> formats(PixelFormat input);
> +
> + SizeRange sizes(PixelFormat inputFormat, const Size &inputSize);
> +
> + std::tuple<unsigned int, unsigned int>
> + strideAndFrameSize(const PixelFormat &outputFormat, const Size &size);
> +
> + int configure(const StreamConfiguration &inputCfg,
> + const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,
> + const ControlInfoMap &sensorControls);
> +
> + int exportBuffers(unsigned int output, unsigned int count,
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> +
> + void processStats(const ControlList &sensorControls);
> +
> + int start();
> + void stop();
> +
> + int queueBuffers(FrameBuffer *input,
> + const std::map<unsigned int, FrameBuffer *> &outputs);
> +
> + void process(FrameBuffer *input, FrameBuffer *output);
> +
> + Signal<FrameBuffer *> inputBufferReady;
> + Signal<FrameBuffer *> outputBufferReady;
> + Signal<int> ispStatsReady;
> + Signal<const ControlList &> setSensorControls;
> +
> +private:
> + void saveIspParams(int dummy);
> + void setSensorCtrls(const ControlList &sensorControls);
> + void statsReady(int dummy);
> + void inputReady(FrameBuffer *input);
> + void outputReady(FrameBuffer *output);
> +
> + std::unique_ptr<DebayerCpu> debayer_;
> + Thread ispWorkerThread_;
> + SharedMemObject<DebayerParams> sharedParams_;
> + DebayerParams debayerParams_;
> + DmaHeap dmaHeap_;
> +
> + std::unique_ptr<ipa::soft::IPAProxySoft> ipa_;
> +};
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/software_isp/meson.build b/src/libcamera/software_isp/meson.build
> index 71b46539..e9266e54 100644
> --- a/src/libcamera/software_isp/meson.build
> +++ b/src/libcamera/software_isp/meson.build
> @@ -10,5 +10,6 @@ endif
> libcamera_sources += files([
> 'debayer.cpp',
> 'debayer_cpu.cpp',
> + 'software_isp.cpp',
> 'swstats_cpu.cpp',
> ])
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> new file mode 100644
> index 00000000..388b4496
> --- /dev/null
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -0,0 +1,349 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Linaro Ltd
> + *
> + * software_isp.cpp - Simple software ISP implementation
> + */
> +
> +#include "libcamera/internal/software_isp/software_isp.h"
> +
> +#include <sys/mman.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include <libcamera/formats.h>
> +#include <libcamera/stream.h>
> +
> +#include "libcamera/internal/bayer_format.h"
> +#include "libcamera/internal/framebuffer.h"
> +#include "libcamera/internal/ipa_manager.h"
> +#include "libcamera/internal/mapped_framebuffer.h"
> +
> +#include "debayer_cpu.h"
> +
> +/**
> + * \file software_isp.cpp
> + * \brief Simple software ISP implementation
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(SoftwareIsp)
> +
> +/**
> + * \class SoftwareIsp
> + * \brief Class for the Software ISP
> + */
> +
> +/**
> + * \var SoftwareIsp::inputBufferReady
> + * \brief A signal emitted when the input frame buffer completes
> + */
> +
> +/**
> + * \var SoftwareIsp::outputBufferReady
> + * \brief A signal emitted when the output frame buffer completes
> + */
> +
> +/**
> + * \var SoftwareIsp::ispStatsReady
> + * \brief A signal emitted when the statistics for IPA are ready
> + *
> + * The int parameter isn't actually used.
Drop it :-)
> + */
> +
> +/**
> + * \var SoftwareIsp::setSensorControls
To make components reusable, signals should be named based on the event
they report, not based on what the connected slot is expected to do.
> + * \brief A signal emitted when the values to write to the sensor controls are ready
> + */
> +
> +/**
> + * \brief Constructs SoftwareIsp object
> + * \param[in] pipe The pipeline handler in use
> + * \param[in] sensorControls ControlInfoMap describing the controls supported by the sensor
> + */
> +SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const ControlInfoMap &sensorControls)
> + : debayer_(nullptr),
Not needed, the default unique_ptr<> constructor will handle that.
> + debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10, DebayerParams::kGain10, 0.5f },
> + dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)
> +{
> + if (!dmaHeap_.isValid()) {
> + LOG(SoftwareIsp, Error) << "Failed to create DmaHeap object";
> + return;
> + }
> +
> + sharedParams_ = SharedMemObject<DebayerParams>("softIsp_params");
> + if (!sharedParams_) {
> + LOG(SoftwareIsp, Error) << "Failed to create shared memory for parameters";
> + return;
> + }
> +
> + auto stats = std::make_unique<SwStatsCpu>();
> + if (!stats->isValid()) {
> + LOG(SoftwareIsp, Error) << "Failed to create SwStatsCpu object";
> + return;
> + }
> + stats->statsReady.connect(this, &SoftwareIsp::statsReady);
> +
> + debayer_ = std::make_unique<DebayerCpu>(std::move(stats));
> + debayer_->inputBufferReady.connect(this, &SoftwareIsp::inputReady);
> + debayer_->outputBufferReady.connect(this, &SoftwareIsp::outputReady);
> +
> + ipa_ = IPAManager::createIPA<ipa::soft::IPAProxySoft>(pipe, 0, 0);
> + if (!ipa_) {
> + LOG(SoftwareIsp, Error)
> + << "Creating IPA for software ISP failed";
> + debayer_.reset();
Is this needed, or can we let the destructor handle it ?
> + return;
> + }
> +
> + int ret = ipa_->init(IPASettings{ "No cfg file", "No sensor model" },
The change from "No sensor model" to using the CameraSensor class, as
done in patch 18/18, should be squashed in this patch.
> + debayer_->getStatsFD(),
> + sharedParams_.fd(),
> + sensorControls);
> + if (ret) {
> + LOG(SoftwareIsp, Error) << "IPA init failed";
> + debayer_.reset();
> + return;
> + }
> +
> + ipa_->setIspParams.connect(this, &SoftwareIsp::saveIspParams);
> + ipa_->setSensorControls.connect(this, &SoftwareIsp::setSensorCtrls);
> +
> + debayer_->moveToThread(&ispWorkerThread_);
> +}
> +
> +SoftwareIsp::~SoftwareIsp()
> +{
> + /* make sure to destroy the DebayerCpu before the ispWorkerThread_ is gone */
> + debayer_.reset();
> +}
> +
> +/**
> + * \fn int SoftwareIsp::loadConfiguration([[maybe_unused]] const std::string &filename)
> + * \brief Load a configuration from a file
> + * \param[in] filename The file to load the configuration data from
> + *
> + * Currently is a stub doing nothing and always returning "success".
> + *
> + * \return 0 on success
> + */
> +
> +/**
> + * \brief Process the statistics gathered
> + * \param[in] sensorControls The sensor controls
> + *
> + * Requests the IPA to calculate new parameters for ISP and new control
> + * values for the sensor.
> + */
> +void SoftwareIsp::processStats(const ControlList &sensorControls)
> +{
> + ASSERT(ipa_);
> + ipa_->processStats(sensorControls);
> +}
> +
> +/**
> + * \brief Check the validity of Software Isp object
> + * \return True if Software Isp is valid, false otherwise
> + */
> +bool SoftwareIsp::isValid() const
> +{
> + return !!debayer_;
> +}
> +
> +/**
> + * \brief Get the output formats supported for the given input format
> + * \param[in] inputFormat The input format
> + * \return All the supported output formats or an empty vector if there are none
> + */
> +std::vector<PixelFormat> SoftwareIsp::formats(PixelFormat inputFormat)
> +{
> + ASSERT(debayer_ != nullptr);
You can write
ASSERT(debayer_);
Same below.
> +
> + return debayer_->formats(inputFormat);
> +}
> +
> +/**
> + * \brief Get the supported output sizes for the given input format and size
> + * \param[in] inputFormat The input format
> + * \param[in] inputSize The input frame size
> + * \return The valid size range or an empty range if there are none
> + */
> +SizeRange SoftwareIsp::sizes(PixelFormat inputFormat, const Size &inputSize)
> +{
> + ASSERT(debayer_ != nullptr);
> +
> + return debayer_->sizes(inputFormat, inputSize);
> +}
> +
> +/**
> + * Get the output stride and the frame size in bytes for the given output format and size
> + * \param[in] outputFormat The output format
> + * \param[in] size The output size (width and height in pixels)
> + * \return A tuple of the stride and the frame size in bytes, or a tuple of 0,0
> + * if there is no valid output config
> + */
> +std::tuple<unsigned int, unsigned int>
> +SoftwareIsp::strideAndFrameSize(const PixelFormat &outputFormat, const Size &size)
> +{
> + ASSERT(debayer_ != nullptr);
> +
> + return debayer_->strideAndFrameSize(outputFormat, size);
> +}
> +
> +/**
> + * \brief Configure the SoftwareIsp object according to the passed in parameters
> + * \param[in] inputCfg The input configuration
> + * \param[in] outputCfgs The output configurations
> + * \param[in] sensorControls ControlInfoMap of the controls supported by the sensor
> + * \return 0 on success, a negative errno on failure
> + */
> +int SoftwareIsp::configure(const StreamConfiguration &inputCfg,
> + const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,
> + const ControlInfoMap &sensorControls)
> +{
> + ASSERT(ipa_ != nullptr && debayer_ != nullptr);
Is there a specific reason to check ipa_ here ?
> +
> + int ret = ipa_->configure(sensorControls);
> + if (ret < 0)
> + return ret;
> +
> + return debayer_->configure(inputCfg, outputCfgs);
> +}
> +
> +/**
> + * \brief Export the buffers from the Software ISP
> + * \param[in] output Output stream index exporting the buffers
> + * \param[in] count Number of buffers to allocate
> + * \param[out] buffers Vector to store the allocated buffers
> + * \return The number of allocated buffers on success or a negative error code
> + * otherwise
> + */
> +int SoftwareIsp::exportBuffers(unsigned int output, unsigned int count,
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> +{
> + ASSERT(debayer_ != nullptr);
> +
> + /* single output for now */
> + if (output >= 1)
> + return -EINVAL;
> +
> + for (unsigned int i = 0; i < count; i++) {
> + const std::string name = "frame-" + std::to_string(i);
> + const size_t frameSize = debayer_->frameSize();
> +
> + FrameBuffer::Plane outPlane;
> + outPlane.fd = SharedFD(dmaHeap_.alloc(name.c_str(), frameSize));
> + if (!outPlane.fd.isValid()) {
> + LOG(SoftwareIsp, Error)
> + << "failed to allocate a dma_buf";
> + return -ENOMEM;
> + }
> + outPlane.offset = 0;
> + outPlane.length = frameSize;
> +
> + std::vector<FrameBuffer::Plane> planes{ outPlane };
> + buffers->emplace_back(std::make_unique<FrameBuffer>(std::move(planes)));
> + }
> +
> + return count;
> +}
> +
> +/**
> + * \brief Queue buffers to Software ISP
> + * \param[in] input The input framebuffer
> + * \param[in] outputs The container holding the output stream indexes and
> + * their respective frame buffer outputs
> + * \return 0 on success, a negative errno on failure
> + */
> +int SoftwareIsp::queueBuffers(FrameBuffer *input,
> + const std::map<unsigned int, FrameBuffer *> &outputs)
> +{
> + unsigned int mask = 0;
> +
> + /*
> + * Validate the outputs as a sanity check: at least one output is
> + * required, all outputs must reference a valid stream and no two
> + * outputs can reference the same stream.
> + */
> + if (outputs.empty())
> + return -EINVAL;
> +
> + for (auto [index, buffer] : outputs) {
> + if (!buffer)
> + return -EINVAL;
> + if (index >= 1) /* only single stream atm */
> + return -EINVAL;
> + if (mask & (1 << index))
> + return -EINVAL;
> +
> + mask |= 1 << index;
> + }
> +
> + process(input, outputs.at(0));
> +
> + return 0;
> +}
> +
> +/**
> + * \brief Starts the Software ISP streaming operation
> + * \return 0 on success, any other value indicates an error
> + */
> +int SoftwareIsp::start()
> +{
> + int ret = ipa_->start();
> + if (ret)
> + return ret;
> +
> + ispWorkerThread_.start();
> + return 0;
> +}
> +
> +/**
> + * \brief Stops the Software ISP streaming operation
> + */
> +void SoftwareIsp::stop()
> +{
> + ispWorkerThread_.exit();
> + ispWorkerThread_.wait();
> +
> + ipa_->stop();
> +}
> +
> +/**
> + * \brief Passes the input framebuffer to the ISP worker to process
> + * \param[in] input The input framebuffer
> + * \param[out] output The framebuffer to write the processed frame to
> + */
> +void SoftwareIsp::process(FrameBuffer *input, FrameBuffer *output)
> +{
> + debayer_->invokeMethod(&DebayerCpu::process,
> + ConnectionTypeQueued, input, output, debayerParams_);
> +}
> +
> +void SoftwareIsp::saveIspParams([[maybe_unused]] int dummy)
> +{
> + debayerParams_ = *sharedParams_;
> +}
> +
> +void SoftwareIsp::setSensorCtrls(const ControlList &sensorControls)
> +{
> + setSensorControls.emit(sensorControls);
> +}
> +
> +void SoftwareIsp::statsReady(int dummy)
> +{
> + ispStatsReady.emit(dummy);
> +}
> +
> +void SoftwareIsp::inputReady(FrameBuffer *input)
> +{
> + inputBufferReady.emit(input);
> +}
> +
> +void SoftwareIsp::outputReady(FrameBuffer *output)
> +{
> + outputBufferReady.emit(output);
> +}
> +
> +} /* namespace libcamera */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list