[libcamera-devel] [PATCH v3 03/13] ipa: rkisp1: Support raw capture in IPA operations

[Jacopo Mondi]

Hi Laurent

On Mon, Oct 24, 2022 at 03:03:46AM +0300, Laurent Pinchart via libcamera-devel wrote:
> The RkISP1 can capture raw frames by bypassing the ISP. In that mode,
> the ISP will not produce statistics nor consume parameters. Most
> algorithms will thus be unable to run, with one exception: the AGC will
> still be able to configure the sensor exposure time and analog gain in
> manual mode.
>
> To prepare for this, add the ability to disable algorithms for the
> duration of the capture session based on the camera configuration.
> Individual algorithms report whether they support raw formats at
> construction time, and the IPA module disables algorithms in configure()
> based on the stream configurations.
>
> Disabled algorithms are skipped during the capture session in the
> processStatsBuffer() operation. As the ISP doesn't produce statistics,
> don't try to access the stats buffer. There is no need for similar logic
> in fillParamsBuffer() as that operation won't be called for raw capture.
>
> All algorithms report not supporting raw capture by default. Raw support
> in AGC will be added separately.
>
> The feature is implemented in the RkISP1 module without any support from
> libipa at this point to avoid designing a generic API based on a single
> user. This may be changed in the future.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

While reading the patch I had the same thought I later found out Paul
has already expressed.

On making the algorithms self-disabling, I guess it would actually be
nicer if they would handle it in configure() by returning immediately,
just after having set their 'disabled' flag. However I understand it's
a more invasive change, and all that encapsulation is not really
necessary. Alternatively, the base class could do the check, but again
it's not worth the complication most probably..

A few minor nits below

> ---
>  src/ipa/rkisp1/algorithms/algorithm.h | 12 +++++++-
>  src/ipa/rkisp1/ipa_context.cpp        |  5 ++++
>  src/ipa/rkisp1/ipa_context.h          |  2 ++
>  src/ipa/rkisp1/rkisp1.cpp             | 42 +++++++++++++++++++++++----
>  4 files changed, 54 insertions(+), 7 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h
> index c3212cff76fe..9454c9a1fc06 100644
> --- a/src/ipa/rkisp1/algorithms/algorithm.h
> +++ b/src/ipa/rkisp1/algorithms/algorithm.h
> @@ -15,7 +15,17 @@ namespace libcamera {
>
>  namespace ipa::rkisp1 {
>
> -using Algorithm = libcamera::ipa::Algorithm<Module>;
> +class Algorithm : public libcamera::ipa::Algorithm<Module>
> +{
> +public:
> +	Algorithm()
> +		: disabled_(false), supportsRaw_(false)
> +	{
> +	}
> +
> +	bool disabled_;
> +	bool supportsRaw_;
> +};
>
>  } /* namespace ipa::rkisp1 */
>
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index 7a987497bd03..9bbf368432fa 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -89,6 +89,11 @@ namespace libcamera::ipa::rkisp1 {
>   * \brief Sensor output resolution
>   */
>
> +/**
> + * \var IPASessionConfiguration::raw
> + * \brief Indicates if the camera is configured to capture raw frames
> + */
> +
>  /**
>   * \struct IPAActiveState
>   * \brief Active state for algorithms
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index bb60ab9eab72..3e47ac663c58 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -48,6 +48,8 @@ struct IPASessionConfiguration {
>  	struct {
>  		rkisp1_cif_isp_version revision;
>  	} hw;
> +
> +	bool raw;
>  };
>
>  struct IPAActiveState {
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index fcb9dacccc3c..538e42cb6f7f 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -24,6 +24,7 @@
>  #include <libcamera/ipa/rkisp1_ipa_interface.h>
>  #include <libcamera/request.h>
>
> +#include "libcamera/internal/formats.h"
>  #include "libcamera/internal/mapped_framebuffer.h"
>  #include "libcamera/internal/yaml_parser.h"
>
> @@ -207,7 +208,7 @@ void IPARkISP1::stop()
>   * before accessing them.
>   */
>  int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> -			 [[maybe_unused]] const std::map<uint32_t, IPAStream> &streamConfig,
> +			 const std::map<uint32_t, IPAStream> &streamConfig,
>  			 const std::map<uint32_t, ControlInfoMap> &entityControls)
>  {
>  	if (entityControls.empty())
> @@ -264,7 +265,21 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
>  	context_.configuration.sensor.minAnalogueGain = camHelper_->gain(minGain);
>  	context_.configuration.sensor.maxAnalogueGain = camHelper_->gain(maxGain);
>
> -	for (auto const &algo : algorithms()) {
> +	context_.configuration.raw = std::any_of(streamConfig.begin(), streamConfig.end(),
> +		[](auto &cfg) -> bool {
> +			PixelFormat pixelFormat{ cfg.second.pixelFormat };

I guess there are no differences between using value contruction or
aggregate construction when the argument is an int, I found () more
natural..

> +			const PixelFormatInfo &format = PixelFormatInfo::info(pixelFormat);
> +			return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
> +		});
> +
> +	for (auto const &a : algorithms()) {
> +		Algorithm *algo = static_cast<Algorithm *>(a.get());
> +
> +		/* Disable algorithms that don't support raw formats. */
> +		algo->disabled_ = context_.configuration.raw && !algo->supportsRaw_;
> +		if (algo->disabled_)
> +			continue;
> +
>  		int ret = algo->configure(context_, info);
>  		if (ret)
>  			return ret;
> @@ -307,8 +322,12 @@ void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls)
>  {
>  	IPAFrameContext &frameContext = context_.frameContexts.alloc(frame);
>
> -	for (auto const &algo : algorithms())
> +	for (auto const &a : algorithms()) {
> +		Algorithm *algo = static_cast<Algorithm *>(a.get());
> +		if (algo->disabled_)
> +			continue;
>  		algo->queueRequest(context_, frame, frameContext, controls);
> +	}
>  }
>
>  void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)
> @@ -333,9 +352,16 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
>  {
>  	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>
> -	const rkisp1_stat_buffer *stats =
> -		reinterpret_cast<rkisp1_stat_buffer *>(
> +	/*
> +	 * In raw capture mode, the ISP is bypassed and no statistics buffer is
> +	 * provided.
> +	 */
> +	const rkisp1_stat_buffer *stats;
> +	if (!context_.configuration.raw)
> +		stats = reinterpret_cast<rkisp1_stat_buffer *>(
>  			mappedBuffers_.at(bufferId).planes()[0].data());
> +	else
> +		stats = nullptr;
>

Or
	const rkisp1_stat_buffer *stats = nullptr;
	if (!context_.configuration.raw)
		stats = reinterpret_cast<rkisp1_stat_buffer *>(
  			mappedBuffers_.at(bufferId).planes()[0].data());

All minors and mostly personal preferences

Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>


>  	frameContext.sensor.exposure =
>  		sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> @@ -344,8 +370,12 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
>
>  	ControlList metadata(controls::controls);
>
> -	for (auto const &algo : algorithms())
> +	for (auto const &a : algorithms()) {
> +		Algorithm *algo = static_cast<Algorithm *>(a.get());
> +		if (algo->disabled_)
> +			continue;
>  		algo->process(context_, frame, frameContext, stats, metadata);
> +	}
>
>  	setControls(frame);
>
> --
> Regards,
>
> Laurent Pinchart
>