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

[Andrei Konovalov]

Hi Barnabás,

On 14.02.2024 21:15, Barnabás Pőcze wrote:
> 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).

Good catch

>> +	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.

Right. I'll fix this and the related parts in the next version of the patch set.

>> +		LOG(IPASoft, Error) << "Unable to map Parameters";
>> +		return -ENODEV;
> 
> Maybe using `errno` would be better?

Yes, sounds reasonable.

Thanks,
Andrei

>> +	}
>> +
>> +	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;
>> +}
>> [...]