[PATCH v3 09/16] libcamera: ipa: add Soft IPA

[Barnabás Pőcze]

Hi


2024. február 14., szerda 18:01 keltezéssel, Hans de Goede <hdegoede at redhat.com> írta:

> From: Andrey Konovalov <andrey.konovalov at linaro.org>
> 
> Define the Soft IPA main and event interfaces, add the Soft IPA
> implementation.
> 
> The current src/ipa/meson.build assumes the IPA name to match the
> pipeline name. For this reason "-Dipas=simple" is used for the
> Soft IPA module.
> 
> Auto exposure/gain and AWB implementation by Dennis, Toon and Martti.
> 
> Auto exposure/gain targets a Mean Sample Value of 2.5 following
> the MSV calculation algorithm from:
> https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
> 
> 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>
> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> Co-developed-by: Dennis Bonke <admin at dennisbonke.com>
> Signed-off-by: Dennis Bonke <admin at dennisbonke.com>
> Co-developed-by: Marttico <g.martti at gmail.com>
> Signed-off-by: Marttico <g.martti at gmail.com>
> Co-developed-by: Toon Langendam <t.langendam at gmail.com>
> Signed-off-by: Toon Langendam <t.langendam at gmail.com>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
> Changes in v3:
> - Use new SwIspStats::kYHistogramSize constexpr and adjust
>   the auto-exposure/-gain code so that it can deal with
>   that having a different value then 16 (modify the loop
>   to divide the histogram in 5 bins to not have hardcoded
>   values)
> - Rename a few foo_bar symbols to fooBar
> ---
>  Documentation/Doxyfile.in         |   1 +
>  include/libcamera/ipa/meson.build |   1 +
>  include/libcamera/ipa/soft.mojom  |  28 +++
>  meson_options.txt                 |   2 +-
>  src/ipa/simple/data/meson.build   |   9 +
>  src/ipa/simple/data/soft.conf     |   3 +
>  src/ipa/simple/meson.build        |  25 +++
>  src/ipa/simple/soft_simple.cpp    | 308 ++++++++++++++++++++++++++++++
>  8 files changed, 376 insertions(+), 1 deletion(-)
>  create mode 100644 include/libcamera/ipa/soft.mojom
>  create mode 100644 src/ipa/simple/data/meson.build
>  create mode 100644 src/ipa/simple/data/soft.conf
>  create mode 100644 src/ipa/simple/meson.build
>  create mode 100644 src/ipa/simple/soft_simple.cpp
> 
> [...]
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> new file mode 100644
> index 00000000..f7ac02f9
> --- /dev/null
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -0,0 +1,308 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Linaro Ltd
> + *
> + * soft_simple.cpp - Simple Software Image Processing Algorithm module
> + */
> +
> +#include <sys/mman.h>
> +
> +#include <libcamera/base/file.h>
> +#include <libcamera/base/log.h>
> +#include <libcamera/base/shared_fd.h>
> +
> +#include <libcamera/control_ids.h>
> +#include <libcamera/controls.h>
> +
> +#include <libcamera/ipa/ipa_interface.h>
> +#include <libcamera/ipa/ipa_module_info.h>
> +#include <libcamera/ipa/soft_ipa_interface.h>
> +
> +#include "libcamera/internal/camera_sensor.h"
> +#include "libcamera/internal/software_isp/debayer_params.h"
> +#include "libcamera/internal/software_isp/swisp_stats.h"
> +
> +#define EXPOSURE_OPTIMAL_VALUE 2.5
> +#define EXPOSURE_SATISFACTORY_OFFSET 0.2
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(IPASoft)
> +
> +namespace ipa::soft {
> +
> +class IPASoftSimple : public ipa::soft::IPASoftInterface
> +{
> +public:
> +	IPASoftSimple()
> +		: ignore_updates_(0)
> +	{
> +	}
> +
> +	~IPASoftSimple()
> +	{
> +		if (stats_)

if (stats_ != MAP_FAILED)


> +			munmap(stats_, sizeof(SwIspStats));
> +		if (params_)
> +			munmap(params_, sizeof(DebayerParams));
> +	}
> +
> +	int init(const IPASettings &settings,
> +		 const SharedFD &fdStats,
> +		 const SharedFD &fdParams,
> +		 const ControlInfoMap &sensorInfoMap) override;
> +	int configure(const ControlInfoMap &sensorInfoMap) override;
> +
> +	int start() override;
> +	void stop() override;
> +
> +	void processStats(const ControlList &sensorControls) override;
> +
> +private:
> +	void updateExposure(double exposureMSV);
> +
> +	SharedFD fdStats_;
> +	SharedFD fdParams_;
> +	DebayerParams *params_;
> +	SwIspStats *stats_;

The above two should be initialized to MAP_FAILED.


> +	int exposure_min_, exposure_max_;
> +	int again_min_, again_max_;
> +	int again_, exposure_;
> +	int ignore_updates_;
> +};
> +
> +int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,
> +			const SharedFD &fdStats,
> +			const SharedFD &fdParams,
> +			const ControlInfoMap &sensorInfoMap)
> +{
> +	fdStats_ = std::move(fdStats);

std::move(fdStats) has the type `const SharedFD&&`, so `fdStats_ = ...`
will result in a call to the copy assignment operator `operator=(const SharedFD&)`.
So `std::move()` does not really do anything here, and can be removed.
Alternatively, if possible, you can change the argument type to be just `SharedFD`,
and then moving makes sense (preferably in the member initializer list).


> +	if (!fdStats_.isValid()) {
> +		LOG(IPASoft, Error) << "Invalid Statistics handle";
> +		return -ENODEV;
> +	}
> +
> +	fdParams_ = std::move(fdParams);
> +	if (!fdParams_.isValid()) {
> +		LOG(IPASoft, Error) << "Invalid Parameters handle";
> +		return -ENODEV;
> +	}
> +
> +	params_ = static_cast<DebayerParams *>(mmap(nullptr, sizeof(DebayerParams),
> +						    PROT_WRITE, MAP_SHARED,
> +						    fdParams_.get(), 0));
> +	if (!params_) {

mmap returns MAP_FAILED on failure.


> +		LOG(IPASoft, Error) << "Unable to map Parameters";
> +		return -ENODEV;

Maybe using `errno` would be better?


> +	}
> +
> +	stats_ = static_cast<SwIspStats *>(mmap(nullptr, sizeof(SwIspStats),
> +						PROT_READ, MAP_SHARED,
> +						fdStats_.get(), 0));
> +	if (!stats_) {
> +		LOG(IPASoft, Error) << "Unable to map Statistics";
> +		return -ENODEV;
> +	}
> +
> +	if (sensorInfoMap.find(V4L2_CID_EXPOSURE) == sensorInfoMap.end()) {
> +		LOG(IPASoft, Error) << "Don't have exposure control";
> +		return -EINVAL;
> +	}
> +
> +	if (sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN) == sensorInfoMap.end()) {
> +		LOG(IPASoft, Error) << "Don't have gain control";
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> [...]