[3/3] libcamera: software_isp: Allow running IPA and stats on raw

28398 diff mbox series

`simple' pipeline can basically pass raw buffers from V4L2 to the
output.  While the raw data is untouched, we may still want to:

- Gather statistics from it.
- Run IPA on the images (without modifying the images).
- Provide metadata from the IPA.
- Adjust exposure/gain.

This patch implements all of that.

Since this imposes some overhead, a new configuration option
pipelines.simple.enable_raw_ipa is added.  The option is on by default
as at least the metadata and exposure adjustments may be useful.  Using
a configuration option rather than a runtime-adjustable option is not
perfect, but I don't know what better mechanism to use.

The patch attempts to fit into the current structure as close as
possible, with minimum changes.  Future enhancements might include
computing the stats asynchronously or even splitting `simple' pipeline
into separate components (debayering, converter, raw output).

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 Documentation/runtime_configuration.rst       | 10 +++++
 .../internal/software_isp/software_isp.h      |  3 +-
 src/libcamera/pipeline/simple/simple.cpp      | 36 ++++++++++++++---
 src/libcamera/software_isp/software_isp.cpp   | 40 ++++++++++++++++---
 4 files changed, 77 insertions(+), 12 deletions(-)

Quoting Milan Zamazal (2026-10-05 14:16:58)
> `simple' pipeline can basically pass raw buffers from V4L2 to the
> output.  While the raw data is untouched, we may still want to:
> 
> - Gather statistics from it.
> - Run IPA on the images (without modifying the images).
> - Provide metadata from the IPA.
> - Adjust exposure/gain.
> 
> This patch implements all of that.
> 
> Since this imposes some overhead, a new configuration option
> pipelines.simple.enable_raw_ipa is added.  The option is on by default
> as at least the metadata and exposure adjustments may be useful.  Using
> a configuration option rather than a runtime-adjustable option is not
> perfect, but I don't know what better mechanism to use.
> 
> The patch attempts to fit into the current structure as close as
> possible, with minimum changes.  Future enhancements might include
> computing the stats asynchronously or even splitting `simple' pipeline
> into separate components (debayering, converter, raw output).

It really feels like we need some architecture here already, but that
could be quite a bit of work on top. Perhaps we just really need to
consider how the components here should really be operated, and what can
be standalone etc...

I have tested this series and it works though.

Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  Documentation/runtime_configuration.rst       | 10 +++++
>  .../internal/software_isp/software_isp.h      |  3 +-
>  src/libcamera/pipeline/simple/simple.cpp      | 36 ++++++++++++++---
>  src/libcamera/software_isp/software_isp.cpp   | 40 ++++++++++++++++---
>  4 files changed, 77 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst
> index a71b715a1..1d93ad3a8 100644
> --- a/Documentation/runtime_configuration.rst
> +++ b/Documentation/runtime_configuration.rst
> @@ -46,6 +46,7 @@ file structure:
>          supported_devices:
>            - driver: # driver name, e.g. `mxc-isi`
>              software_isp: # true/false
> +        enable_raw_ipa: # true/false
>      software_isp:
>        copy_input_buffer: # true/false
>        measure:
> @@ -81,6 +82,7 @@ Configuration file example
>           supported_devices:
>             - driver: mxc-isi
>               software_isp: true
> +         enable_raw_ipa: true
>       software_isp:
>         copy_input_buffer: false
>         measure:
> @@ -157,6 +159,14 @@ pipelines.simple.supported_devices.driver, pipelines.simple.supported_devices.so
>  
>     Example `software_isp` value: ``true``
>  
> +pipelines.simple.enable_raw_ipa
> +   Whether image processing algorithms are enabled also with raw-only
> +   output. When enabled, statistics is gathered for raw-only output,
> +   image processing algorithms are run on it, metadata is produced and
> +   exposure is adjusted.
> +
> +   Example value: ``true``


I half want to argue that this isn't necessary as if we have a raw
bayer sensor we should likely want stats. But they do take CPU time...


> +
>  software_isp.copy_input_buffer
>     Define whether input buffers should be copied into standard (cached)
>     memory in software ISP. This is done by default to prevent very slow
> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
> index 3e420242b..b4c57dbf5 100644
> --- a/include/libcamera/internal/software_isp/software_isp.h
> +++ b/include/libcamera/internal/software_isp/software_isp.h
> @@ -67,7 +67,8 @@ public:
>         int configure(const StreamConfiguration &inputCfg,
>                       const std::vector<std::reference_wrapper<const StreamConfiguration>> &outputCfgs,
>                       const ipa::softisp::IPAConfigInfo &configInfo,
> -                     ControlInfoMap *ipaControls);
> +                     ControlInfoMap *ipaControls,
> +                     bool debayerEnabled);
>  
>         int exportBuffers(const Stream *stream, unsigned int count,
>                           std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 35c29ceca..dd1d69a5b 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -427,6 +427,7 @@ public:
>         V4L2Subdevice *subdev(const MediaEntity *entity);
>         std::shared_ptr<MediaDevice> converter() { return converter_; }
>         bool swIspEnabled() const { return swIspEnabled_; }
> +       bool rawIpaEnabled() const { return rawIpaEnabled_; }
>  
>  protected:
>         int queueRequestDevice(Camera *camera, Request *request) override;
> @@ -460,6 +461,7 @@ private:
>  
>         std::shared_ptr<MediaDevice> converter_;
>         bool swIspEnabled_;
> +       bool rawIpaEnabled_ = true;
>  };
>  
>  /* -----------------------------------------------------------------------------
> @@ -968,6 +970,11 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
>  
>                 conversionQueue_.pop();
>                 return;
> +       } else if (pipe->rawIpaEnabled()) {
> +               /* \todo Make the processing asynchronous? */
> +               swIsp_->process(request->sequence(), buffer, nullptr);
> +               pipe->completeBuffer(request, buffer);
> +               return;
>         }
>  
>         /* Otherwise simply complete the request. */
> @@ -1591,7 +1598,8 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>                         data->rawStream_ = &data->streams_[i];
>         }
>  
> -       if (outputCfgs.empty())
> +       if (outputCfgs.empty() &&
> +           (data->converter_ || !data->rawStream_ || !rawIpaEnabled_))
>                 return 0;
>  
>         StreamConfiguration inputCfg;
> @@ -1605,7 +1613,12 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>         } else {
>                 ipa::softisp::IPAConfigInfo configInfo;
>                 configInfo.sensorControls = data->sensor_->controls();
> -               return data->swIsp_->configure(inputCfg, outputCfgs, configInfo, &data->controlInfo_);
> +               return data->swIsp_->configure(
> +                       inputCfg,
> +                       outputCfgs,
> +                       configInfo,
> +                       &data->controlInfo_,
> +                       data->useConversion_);
>         }
>  }
>  
> @@ -1698,6 +1711,12 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
>                 if (!data->rawStream_)
>                         for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_)
>                                 video->queueBuffer(buffer.get());
> +       } else if (data->swIsp_) {
> +               ret = data->swIsp_->startIpa();
> +               if (ret < 0) {
> +                       stop(camera);
> +                       return ret;
> +               }
>         }
>  
>         return 0;
> @@ -1720,6 +1739,8 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>                         data->converter_->stop();
>                 else if (data->swIsp_)
>                         data->swIsp_->stop();
> +       } else if (data->swIsp_) {
> +               data->swIsp_->stopIpa();

Maybe this is the architecture part, but now I see two paths with two
different ways to 'stop' the ISP.


>         }
>  
>         video->streamOff();
> @@ -1763,6 +1784,8 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
>                 data->conversionQueue_.push({ request, std::move(buffers) });
>                 if (data->swIsp_)
>                         data->swIsp_->queueRequest(request->sequence(), request->controls());
> +       } else if (data->swIsp_ && rawIpaEnabled_) {
> +               data->swIsp_->queueRequest(request->sequence(), request->controls());

And not a lot of context in this hunk, but curious duplication there.


>         }
>  
>         return 0;
> @@ -1882,9 +1905,8 @@ bool SimplePipelineHandler::matchDevice(std::shared_ptr<MediaDevice> media,
>  
>         swIspEnabled_ = info.swIspEnabled;
>         const GlobalConfiguration &configuration = cameraManager()->_d()->configuration();
> -       for (const ValueNode &entry :
> -            configuration.configuration()["pipelines"]["simple"]["supported_devices"]
> -                    .asList()) {
> +       auto &simpleConfiguration = configuration.configuration()["pipelines"]["simple"];
> +       for (const ValueNode &entry : simpleConfiguration["supported_devices"].asList()) {
>                 auto name = entry["driver"].get<std::string>();
>                 if (name == info.driver) {
>                         swIspEnabled_ = entry["software_isp"].get<bool>().value_or(swIspEnabled_);
> @@ -1903,6 +1925,10 @@ bool SimplePipelineHandler::matchDevice(std::shared_ptr<MediaDevice> media,
>                  */
>                 ASSERT(!converter_);
>                 numStreams = 2;
> +
> +               auto enableRawIpa = simpleConfiguration["enable_raw_ipa"].get<bool>();
> +               if (enableRawIpa)
> +                       rawIpaEnabled_ = enableRawIpa.value();
>         }
>  
>         /* Locate the sensors. */
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index 11e0c49ed..594125f8f 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -25,6 +25,7 @@
>  
>  #include "libcamera/internal/bayer_format.h"
>  #include "libcamera/internal/framebuffer.h"
> +#include "libcamera/internal/mapped_framebuffer.h"
>  #include "libcamera/internal/software_isp/debayer_params.h"
>  
>  #include "debayer_cpu.h"
> @@ -280,19 +281,34 @@ uint32_t SoftwareIsp::preferredInputStride(const PixelFormat &inputFormat, const
>   * \param[in] outputCfgs The output configurations
>   * \param[in] configInfo The IPA configuration data, received from the pipeline handler
>   * \param[out] ipaControls The IPA controls to update
> + * \param[in] debayerEnabled Whether debayering should be performed in addition
> + *   to stats and IPA processing
>   * \return 0 on success, a negative errno on failure
>   */
>  int SoftwareIsp::configure(const StreamConfiguration &inputCfg,
>                            const std::vector<std::reference_wrapper<const StreamConfiguration>> &outputCfgs,
>                            const ipa::softisp::IPAConfigInfo &configInfo,
> -                          ControlInfoMap *ipaControls)
> +                          ControlInfoMap *ipaControls,
> +                          bool debayerEnabled)
>  {
> -       ASSERT(ipa_ && debayer_);

I feel a comment could help here ... the assert is getting a bit
terse, and it's taking me a while to parse it:

	/* Ensure we have a debayer_ when debayerEnabled */

> +       ASSERT(ipa_ && (!debayerEnabled || debayer_));
> +
> +       int ret;
> +       if (!debayerEnabled) {
> +               ret = stats_->configure(inputCfg);
> +               if (ret < 0)
> +                       return ret;
> +               stats_->setWindow(Rectangle(inputCfg.size));
> +       }
>  
> -       int ret = ipa_->configure(configInfo, ipaControls);
> +       ret = ipa_->configure(configInfo, ipaControls);
>         if (ret < 0)
>                 return ret;
>  
> +       if (!debayerEnabled) {
> +               return 0;
> +       }

No { } on single line conditionals usually.


> +
>         ret = debayer_->configure(inputCfg, outputCfgs, ccmEnabled_);
>         if (ret < 0)
>                 return ret;
> @@ -452,13 +468,25 @@ void SoftwareIsp::stopIpa()
>   * \brief Passes the input framebuffer to the ISP worker to process
>   * \param[in] frame The frame number
>   * \param[in] input The input framebuffer
> - * \param[out] output The framebuffer to write the processed frame to
> + * \param[out] output The framebuffer to write the processed frame to; if
> + *   nullptr then debayering is skipped and stats are processed synchronously
>   */
>  void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output)
>  {
>         ipa_->computeParams(frame);
> -       debayer_->invokeMethod(&Debayer::process,
> -                              ConnectionTypeQueued, frame, input, output, debayerParams_);
> +
> +       if (output) {
> +               debayer_->invokeMethod(&Debayer::process,
> +                                      ConnectionTypeQueued, frame, input, output, debayerParams_);
> +       } else {
> +               /* Compute stats, which are otherwise computed in debayering */
> +               if (frame % SwStatsCpu::kStatPerNumFrames) {
> +                       stats_->finishFrame(frame, 0);
> +               } else {
> +                       MappedFrameBuffer inputMapped(input, MappedFrameBuffer::MapFlag::Read);
> +                       stats_->processFrame(frame, 0, inputMapped);
> +               }
> +       }

Without digging through to figure out how we could re-design this - it
feels like we're duplicating code paths for specific use cases which
makes me feel like something needs to be split up.

But I don't know how yet ...

So otherwise:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>  }
>  
>  void SoftwareIsp::saveIspParams([[maybe_unused]] uint32_t frame)
> -- 
> 2.55.0
>

Hi Milan

On Mon, Oct 05, 2026 at 03:16:58PM +0200, Milan Zamazal wrote:
> `simple' pipeline can basically pass raw buffers from V4L2 to the
> output.  While the raw data is untouched, we may still want to:
>
> - Gather statistics from it.
> - Run IPA on the images (without modifying the images).
> - Provide metadata from the IPA.
> - Adjust exposure/gain.
>
> This patch implements all of that.
>
> Since this imposes some overhead, a new configuration option
> pipelines.simple.enable_raw_ipa is added.  The option is on by default
> as at least the metadata and exposure adjustments may be useful.  Using
> a configuration option rather than a runtime-adjustable option is not
> perfect, but I don't know what better mechanism to use.
>
> The patch attempts to fit into the current structure as close as
> possible, with minimum changes.  Future enhancements might include
> computing the stats asynchronously or even splitting `simple' pipeline
> into separate components (debayering, converter, raw output).
>
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  Documentation/runtime_configuration.rst       | 10 +++++
>  .../internal/software_isp/software_isp.h      |  3 +-
>  src/libcamera/pipeline/simple/simple.cpp      | 36 ++++++++++++++---
>  src/libcamera/software_isp/software_isp.cpp   | 40 ++++++++++++++++---
>  4 files changed, 77 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst
> index a71b715a1..1d93ad3a8 100644
> --- a/Documentation/runtime_configuration.rst
> +++ b/Documentation/runtime_configuration.rst
> @@ -46,6 +46,7 @@ file structure:
>          supported_devices:
>            - driver: # driver name, e.g. `mxc-isi`
>              software_isp: # true/false
> +        enable_raw_ipa: # true/false
>      software_isp:
>        copy_input_buffer: # true/false
>        measure:
> @@ -81,6 +82,7 @@ Configuration file example
>           supported_devices:
>             - driver: mxc-isi
>               software_isp: true
> +         enable_raw_ipa: true
>       software_isp:
>         copy_input_buffer: false
>         measure:
> @@ -157,6 +159,14 @@ pipelines.simple.supported_devices.driver, pipelines.simple.supported_devices.so
>
>     Example `software_isp` value: ``true``
>
> +pipelines.simple.enable_raw_ipa
> +   Whether image processing algorithms are enabled also with raw-only
> +   output. When enabled, statistics is gathered for raw-only output,
> +   image processing algorithms are run on it, metadata is produced and
> +   exposure is adjusted.
> +
> +   Example value: ``true``
> +
>  software_isp.copy_input_buffer
>     Define whether input buffers should be copied into standard (cached)
>     memory in software ISP. This is done by default to prevent very slow
> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
> index 3e420242b..b4c57dbf5 100644
> --- a/include/libcamera/internal/software_isp/software_isp.h
> +++ b/include/libcamera/internal/software_isp/software_isp.h
> @@ -67,7 +67,8 @@ public:
>  	int configure(const StreamConfiguration &inputCfg,
>  		      const std::vector<std::reference_wrapper<const StreamConfiguration>> &outputCfgs,
>  		      const ipa::softisp::IPAConfigInfo &configInfo,
> -		      ControlInfoMap *ipaControls);
> +		      ControlInfoMap *ipaControls,
> +		      bool debayerEnabled);

fits on the previous line ?

>
>  	int exportBuffers(const Stream *stream, unsigned int count,
>  			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 35c29ceca..dd1d69a5b 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -427,6 +427,7 @@ public:
>  	V4L2Subdevice *subdev(const MediaEntity *entity);
>  	std::shared_ptr<MediaDevice> converter() { return converter_; }
>  	bool swIspEnabled() const { return swIspEnabled_; }
> +	bool rawIpaEnabled() const { return rawIpaEnabled_; }
>
>  protected:
>  	int queueRequestDevice(Camera *camera, Request *request) override;
> @@ -460,6 +461,7 @@ private:
>
>  	std::shared_ptr<MediaDevice> converter_;
>  	bool swIspEnabled_;
> +	bool rawIpaEnabled_ = true;
>  };
>
>  /* -----------------------------------------------------------------------------
> @@ -968,6 +970,11 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
>
>  		conversionQueue_.pop();
>  		return;
> +	} else if (pipe->rawIpaEnabled()) {

this branch is taken if !data->useConversion which is initialized with
CameraData::needConversion_, which is initialized as

	needConversion_ = config_.size() > 1 + rawCount;

in SimpleCameraConfiguration::validate().

Doesn't it mean data->useConversion is true when we have a Raw stream
?

> +		/* \todo Make the processing asynchronous? */
> +		swIsp_->process(request->sequence(), buffer, nullptr);
> +		pipe->completeBuffer(request, buffer);
> +		return;

There is quite some booking and ad-hoc handling of special cases here
to accommodate the "conversion" and "isRaw" situation, and this is
where a redesign might really help before the code base becomes too
convoluted.

Have you considered, in example, if instrumenting
SoftwareIsp::queueBuffers() with support for Raw would require less
ad-hoc handling than doing it here ?

>  	}
>
>  	/* Otherwise simply complete the request. */
> @@ -1591,7 +1598,8 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  			data->rawStream_ = &data->streams_[i];
>  	}
>
> -	if (outputCfgs.empty())
> +	if (outputCfgs.empty() &&
> +	    (data->converter_ || !data->rawStream_ || !rawIpaEnabled_))
>  		return 0;
>
>  	StreamConfiguration inputCfg;
> @@ -1605,7 +1613,12 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  	} else {
>  		ipa::softisp::IPAConfigInfo configInfo;
>  		configInfo.sensorControls = data->sensor_->controls();
> -		return data->swIsp_->configure(inputCfg, outputCfgs, configInfo, &data->controlInfo_);
> +		return data->swIsp_->configure(
> +			inputCfg,
> +			outputCfgs,
> +			configInfo,
> +			&data->controlInfo_,
> +			data->useConversion_);

Why can't this be

		return data->swIsp_->configure(inputCfg, outputCfgs, configInfo,
                                               &data->controlInfo_, data->useConversion);

>  	}
>  }
>
> @@ -1698,6 +1711,12 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
>  		if (!data->rawStream_)
>  			for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_)
>  				video->queueBuffer(buffer.get());
> +	} else if (data->swIsp_) {
> +		ret = data->swIsp_->startIpa();
> +		if (ret < 0) {
> +			stop(camera);
> +			return ret;
> +		}

The above branch has

		else if (data->swIsp_)
			ret = data->swIsp_->start();

have you considered how hard would it be to unify the data->useConversion_
and isRaw_ situation and delegate to softISP the handling of Raw ?

>  	}
>
>  	return 0;
> @@ -1720,6 +1739,8 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>  			data->converter_->stop();
>  		else if (data->swIsp_)
>  			data->swIsp_->stop();
> +	} else if (data->swIsp_) {
> +		data->swIsp_->stopIpa();
>  	}
>
>  	video->streamOff();
> @@ -1763,6 +1784,8 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
>  		data->conversionQueue_.push({ request, std::move(buffers) });
>  		if (data->swIsp_)
>  			data->swIsp_->queueRequest(request->sequence(), request->controls());
> +	} else if (data->swIsp_ && rawIpaEnabled_) {
> +		data->swIsp_->queueRequest(request->sequence(), request->controls());

Same question as above

>  	}
>
>  	return 0;
> @@ -1882,9 +1905,8 @@ bool SimplePipelineHandler::matchDevice(std::shared_ptr<MediaDevice> media,
>
>  	swIspEnabled_ = info.swIspEnabled;
>  	const GlobalConfiguration &configuration = cameraManager()->_d()->configuration();
> -	for (const ValueNode &entry :
> -	     configuration.configuration()["pipelines"]["simple"]["supported_devices"]
> -		     .asList()) {
> +	auto &simpleConfiguration = configuration.configuration()["pipelines"]["simple"];
> +	for (const ValueNode &entry : simpleConfiguration["supported_devices"].asList()) {
>  		auto name = entry["driver"].get<std::string>();
>  		if (name == info.driver) {
>  			swIspEnabled_ = entry["software_isp"].get<bool>().value_or(swIspEnabled_);
> @@ -1903,6 +1925,10 @@ bool SimplePipelineHandler::matchDevice(std::shared_ptr<MediaDevice> media,
>  		 */
>  		ASSERT(!converter_);
>  		numStreams = 2;
> +
> +		auto enableRawIpa = simpleConfiguration["enable_raw_ipa"].get<bool>();
> +		if (enableRawIpa)
> +			rawIpaEnabled_ = enableRawIpa.value();
>  	}
>
>  	/* Locate the sensors. */
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index 11e0c49ed..594125f8f 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -25,6 +25,7 @@
>
>  #include "libcamera/internal/bayer_format.h"
>  #include "libcamera/internal/framebuffer.h"
> +#include "libcamera/internal/mapped_framebuffer.h"
>  #include "libcamera/internal/software_isp/debayer_params.h"
>
>  #include "debayer_cpu.h"
> @@ -280,19 +281,34 @@ uint32_t SoftwareIsp::preferredInputStride(const PixelFormat &inputFormat, const
>   * \param[in] outputCfgs The output configurations
>   * \param[in] configInfo The IPA configuration data, received from the pipeline handler
>   * \param[out] ipaControls The IPA controls to update
> + * \param[in] debayerEnabled Whether debayering should be performed in addition
> + *   to stats and IPA processing
>   * \return 0 on success, a negative errno on failure
>   */
>  int SoftwareIsp::configure(const StreamConfiguration &inputCfg,
>  			   const std::vector<std::reference_wrapper<const StreamConfiguration>> &outputCfgs,
>  			   const ipa::softisp::IPAConfigInfo &configInfo,
> -			   ControlInfoMap *ipaControls)
> +			   ControlInfoMap *ipaControls,
> +			   bool debayerEnabled)
>  {
> -	ASSERT(ipa_ && debayer_);
> +	ASSERT(ipa_ && (!debayerEnabled || debayer_));
> +
> +	int ret;
> +	if (!debayerEnabled) {
> +		ret = stats_->configure(inputCfg);
> +		if (ret < 0)
> +			return ret;
> +		stats_->setWindow(Rectangle(inputCfg.size));
> +	}
>
> -	int ret = ipa_->configure(configInfo, ipaControls);
> +	ret = ipa_->configure(configInfo, ipaControls);
>  	if (ret < 0)
>  		return ret;
>
> +	if (!debayerEnabled) {
> +		return 0;
> +	}
> +
>  	ret = debayer_->configure(inputCfg, outputCfgs, ccmEnabled_);
>  	if (ret < 0)
>  		return ret;
> @@ -452,13 +468,25 @@ void SoftwareIsp::stopIpa()
>   * \brief Passes the input framebuffer to the ISP worker to process
>   * \param[in] frame The frame number
>   * \param[in] input The input framebuffer
> - * \param[out] output The framebuffer to write the processed frame to
> + * \param[out] output The framebuffer to write the processed frame to; if
> + *   nullptr then debayering is skipped and stats are processed synchronously
>   */
>  void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output)
>  {
>  	ipa_->computeParams(frame);
> -	debayer_->invokeMethod(&Debayer::process,
> -			       ConnectionTypeQueued, frame, input, output, debayerParams_);
> +
> +	if (output) {
> +		debayer_->invokeMethod(&Debayer::process,
> +				       ConnectionTypeQueued, frame, input, output, debayerParams_);
> +	} else {
> +		/* Compute stats, which are otherwise computed in debayering */
> +		if (frame % SwStatsCpu::kStatPerNumFrames) {
> +			stats_->finishFrame(frame, 0);
> +		} else {
> +			MappedFrameBuffer inputMapped(input, MappedFrameBuffer::MapFlag::Read);
> +			stats_->processFrame(frame, 0, inputMapped);
> +		}
> +	}

The fact you already handle the raw case in SoftwareIsp class for some
functions makes me wonder, as said above, if it couldn't be entirely
done here.

Thanks
  j

>  }
>
>  void SoftwareIsp::saveIspParams([[maybe_unused]] uint32_t frame)
> --
> 2.55.0
>

Hi Kieran,

thank you for review.

Kieran Bingham <kieran.bingham@ideasonboard.com> writes:

> Quoting Milan Zamazal (2026-10-05 14:16:58)
>> `simple' pipeline can basically pass raw buffers from V4L2 to the
>> output.  While the raw data is untouched, we may still want to:
>
>> 
>> - Gather statistics from it.
>> - Run IPA on the images (without modifying the images).
>> - Provide metadata from the IPA.
>> - Adjust exposure/gain.
>> 
>> This patch implements all of that.
>> 
>> Since this imposes some overhead, a new configuration option
>> pipelines.simple.enable_raw_ipa is added.  The option is on by default
>> as at least the metadata and exposure adjustments may be useful.  Using
>> a configuration option rather than a runtime-adjustable option is not
>> perfect, but I don't know what better mechanism to use.
>> 
>> The patch attempts to fit into the current structure as close as
>> possible, with minimum changes.  Future enhancements might include
>> computing the stats asynchronously or even splitting `simple' pipeline
>> into separate components (debayering, converter, raw output).
>
> It really feels like we need some architecture here already, but that
> could be quite a bit of work on top. Perhaps we just really need to
> consider how the components here should really be operated, and what can
> be standalone etc...

The series tries to address the immediate needs, which wouldn't be quite
satisfied if it took a year...  One option is to use the series
out-of-tree for the given purposes but I think it's better to avoid it;
out-of-tree patches rot and are generally not easy to access.  I'm not
comfortable with making things even worse with this series but it is not
large and solves an important problem for now.  Also, it would be useful
to introduce some form of testing of software ISP before starting larger
refactorings.

> I have tested this series and it works though.

Thanks for testing.

> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>> 
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>  Documentation/runtime_configuration.rst       | 10 +++++
>>  .../internal/software_isp/software_isp.h      |  3 +-
>>  src/libcamera/pipeline/simple/simple.cpp      | 36 ++++++++++++++---
>>  src/libcamera/software_isp/software_isp.cpp   | 40 ++++++++++++++++---
>>  4 files changed, 77 insertions(+), 12 deletions(-)
>> 
>> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst
>> index a71b715a1..1d93ad3a8 100644
>> --- a/Documentation/runtime_configuration.rst
>> +++ b/Documentation/runtime_configuration.rst
>> @@ -46,6 +46,7 @@ file structure:
>>          supported_devices:
>>            - driver: # driver name, e.g. `mxc-isi`
>>              software_isp: # true/false
>> +        enable_raw_ipa: # true/false
>>      software_isp:
>>        copy_input_buffer: # true/false
>>        measure:
>> @@ -81,6 +82,7 @@ Configuration file example
>>           supported_devices:
>>             - driver: mxc-isi
>>               software_isp: true
>> +         enable_raw_ipa: true
>>       software_isp:
>>         copy_input_buffer: false
>>         measure:
>> @@ -157,6 +159,14 @@ pipelines.simple.supported_devices.driver, pipelines.simple.supported_devices.so
>>  
>>     Example `software_isp` value: ``true``
>>  
>> +pipelines.simple.enable_raw_ipa
>> +   Whether image processing algorithms are enabled also with raw-only
>> +   output. When enabled, statistics is gathered for raw-only output,
>> +   image processing algorithms are run on it, metadata is produced and
>> +   exposure is adjusted.
>> +
>> +   Example value: ``true``
>
>
> I half want to argue that this isn't necessary as if we have a raw
> bayer sensor we should likely want stats. But they do take CPU time...

My idea is that some people may want to get the raw output without
touching the controls and without using any processing resources.  If
there are any, this series would be a regression for them without the
option to disable it.

>
>
>> +
>>  software_isp.copy_input_buffer
>>     Define whether input buffers should be copied into standard (cached)
>>     memory in software ISP. This is done by default to prevent very slow
>> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
>> index 3e420242b..b4c57dbf5 100644
>> --- a/include/libcamera/internal/software_isp/software_isp.h
>> +++ b/include/libcamera/internal/software_isp/software_isp.h
>> @@ -67,7 +67,8 @@ public:
>>         int configure(const StreamConfiguration &inputCfg,
>>                       const std::vector<std::reference_wrapper<const StreamConfiguration>> &outputCfgs,
>>                       const ipa::softisp::IPAConfigInfo &configInfo,
>> -                     ControlInfoMap *ipaControls);
>> +                     ControlInfoMap *ipaControls,
>> +                     bool debayerEnabled);
>>  
>>         int exportBuffers(const Stream *stream, unsigned int count,
>>                           std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index 35c29ceca..dd1d69a5b 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -427,6 +427,7 @@ public:
>>         V4L2Subdevice *subdev(const MediaEntity *entity);
>>         std::shared_ptr<MediaDevice> converter() { return converter_; }
>>         bool swIspEnabled() const { return swIspEnabled_; }
>> +       bool rawIpaEnabled() const { return rawIpaEnabled_; }
>>  
>>  protected:
>>         int queueRequestDevice(Camera *camera, Request *request) override;
>> @@ -460,6 +461,7 @@ private:
>>  
>>         std::shared_ptr<MediaDevice> converter_;
>>         bool swIspEnabled_;
>> +       bool rawIpaEnabled_ = true;
>>  };
>>  
>>  /* -----------------------------------------------------------------------------
>> @@ -968,6 +970,11 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
>>  
>>                 conversionQueue_.pop();
>>                 return;
>> +       } else if (pipe->rawIpaEnabled()) {
>> +               /* \todo Make the processing asynchronous? */
>> +               swIsp_->process(request->sequence(), buffer, nullptr);
>> +               pipe->completeBuffer(request, buffer);
>> +               return;
>>         }
>>  
>>         /* Otherwise simply complete the request. */
>> @@ -1591,7 +1598,8 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>>                         data->rawStream_ = &data->streams_[i];
>>         }
>>  
>> -       if (outputCfgs.empty())
>> +       if (outputCfgs.empty() &&
>> +           (data->converter_ || !data->rawStream_ || !rawIpaEnabled_))
>>                 return 0;
>>  
>>         StreamConfiguration inputCfg;
>> @@ -1605,7 +1613,12 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>>         } else {
>>                 ipa::softisp::IPAConfigInfo configInfo;
>>                 configInfo.sensorControls = data->sensor_->controls();
>> -               return data->swIsp_->configure(inputCfg, outputCfgs, configInfo, &data->controlInfo_);
>> +               return data->swIsp_->configure(
>> +                       inputCfg,
>> +                       outputCfgs,
>> +                       configInfo,
>> +                       &data->controlInfo_,
>> +                       data->useConversion_);
>>         }
>>  }
>>  
>> @@ -1698,6 +1711,12 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
>>                 if (!data->rawStream_)
>>                         for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_)
>>                                 video->queueBuffer(buffer.get());
>> +       } else if (data->swIsp_) {
>> +               ret = data->swIsp_->startIpa();
>> +               if (ret < 0) {
>> +                       stop(camera);
>> +                       return ret;
>> +               }
>>         }
>>  
>>         return 0;
>> @@ -1720,6 +1739,8 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>>                         data->converter_->stop();
>>                 else if (data->swIsp_)
>>                         data->swIsp_->stop();
>> +       } else if (data->swIsp_) {
>> +               data->swIsp_->stopIpa();
>
> Maybe this is the architecture part, but now I see two paths with two
> different ways to 'stop' the ISP.

One thing is to stop the whole software ISP and the other one is to stop
the IPA.  Since there actions both before and after stopIpa() call
inside data->swIsp_->stop(), stopIpa() cannot be detached from it
without making things more complicated.

>
>
>>         }
>>  
>>         video->streamOff();
>> @@ -1763,6 +1784,8 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
>>                 data->conversionQueue_.push({ request, std::move(buffers) });
>>                 if (data->swIsp_)
>>                         data->swIsp_->queueRequest(request->sequence(), request->controls());
>> +       } else if (data->swIsp_ && rawIpaEnabled_) {
>> +               data->swIsp_->queueRequest(request->sequence(), request->controls());
>
> And not a lot of context in this hunk, but curious duplication there.

The block

  if (data->useConversion_) {
  	data->conversionQueue_.push({ request, std::move(buffers) });
  	if (data->swIsp_)
  		data->swIsp_->queueRequest(request->sequence(), request->controls());
  } else if (data->swIsp_ && rawIpaEnabled_) {
  	data->swIsp_->queueRequest(request->sequence(), request->controls());
  }

can be alternatively, probably better, expressed as

  if (data->useConversion_)
  	data->conversionQueue_.push({ request, std::move(buffers) });
  if (data->swIsp_ && (data->useConversion_ || rawIpaEnabled_))
  	data->swIsp_->queueRequest(request->sequence(), request->controls());

>
>
>>         }
>>  
>>         return 0;
>> @@ -1882,9 +1905,8 @@ bool SimplePipelineHandler::matchDevice(std::shared_ptr<MediaDevice> media,
>>  
>>         swIspEnabled_ = info.swIspEnabled;
>>         const GlobalConfiguration &configuration = cameraManager()->_d()->configuration();
>> -       for (const ValueNode &entry :
>> -            configuration.configuration()["pipelines"]["simple"]["supported_devices"]
>> -                    .asList()) {
>> +       auto &simpleConfiguration = configuration.configuration()["pipelines"]["simple"];
>> +       for (const ValueNode &entry : simpleConfiguration["supported_devices"].asList()) {
>>                 auto name = entry["driver"].get<std::string>();
>>                 if (name == info.driver) {
>>                         swIspEnabled_ = entry["software_isp"].get<bool>().value_or(swIspEnabled_);
>> @@ -1903,6 +1925,10 @@ bool SimplePipelineHandler::matchDevice(std::shared_ptr<MediaDevice> media,
>>                  */
>>                 ASSERT(!converter_);
>>                 numStreams = 2;
>> +
>> +               auto enableRawIpa = simpleConfiguration["enable_raw_ipa"].get<bool>();
>> +               if (enableRawIpa)
>> +                       rawIpaEnabled_ = enableRawIpa.value();
>>         }
>>  
>>         /* Locate the sensors. */
>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
>> index 11e0c49ed..594125f8f 100644
>> --- a/src/libcamera/software_isp/software_isp.cpp
>> +++ b/src/libcamera/software_isp/software_isp.cpp
>> @@ -25,6 +25,7 @@
>>  
>>  #include "libcamera/internal/bayer_format.h"
>>  #include "libcamera/internal/framebuffer.h"
>> +#include "libcamera/internal/mapped_framebuffer.h"
>>  #include "libcamera/internal/software_isp/debayer_params.h"
>>  
>>  #include "debayer_cpu.h"
>> @@ -280,19 +281,34 @@ uint32_t SoftwareIsp::preferredInputStride(const PixelFormat &inputFormat, const
>>   * \param[in] outputCfgs The output configurations
>>   * \param[in] configInfo The IPA configuration data, received from the pipeline handler
>>   * \param[out] ipaControls The IPA controls to update
>> + * \param[in] debayerEnabled Whether debayering should be performed in addition
>> + *   to stats and IPA processing
>>   * \return 0 on success, a negative errno on failure
>>   */
>>  int SoftwareIsp::configure(const StreamConfiguration &inputCfg,
>>                            const std::vector<std::reference_wrapper<const StreamConfiguration>> &outputCfgs,
>>                            const ipa::softisp::IPAConfigInfo &configInfo,
>> -                          ControlInfoMap *ipaControls)
>> +                          ControlInfoMap *ipaControls,
>> +                          bool debayerEnabled)
>>  {
>> -       ASSERT(ipa_ && debayer_);
>
> I feel a comment could help here ... the assert is getting a bit
> terse, and it's taking me a while to parse it:
>
> 	/* Ensure we have a debayer_ when debayerEnabled */
>
>> +       ASSERT(ipa_ && (!debayerEnabled || debayer_));

It should be additionally split to two ASSERTs.

>> +
>> +       int ret;
>> +       if (!debayerEnabled) {
>> +               ret = stats_->configure(inputCfg);
>> +               if (ret < 0)
>> +                       return ret;
>> +               stats_->setWindow(Rectangle(inputCfg.size));
>> +       }
>>  
>> -       int ret = ipa_->configure(configInfo, ipaControls);
>> +       ret = ipa_->configure(configInfo, ipaControls);
>>         if (ret < 0)
>>                 return ret;
>>  
>> +       if (!debayerEnabled) {
>> +               return 0;
>> +       }
>
> No { } on single line conditionals usually.

OK.

>
>
>> +
>>         ret = debayer_->configure(inputCfg, outputCfgs, ccmEnabled_);
>>         if (ret < 0)
>>                 return ret;
>> @@ -452,13 +468,25 @@ void SoftwareIsp::stopIpa()
>>   * \brief Passes the input framebuffer to the ISP worker to process
>>   * \param[in] frame The frame number
>>   * \param[in] input The input framebuffer
>> - * \param[out] output The framebuffer to write the processed frame to
>> + * \param[out] output The framebuffer to write the processed frame to; if
>> + *   nullptr then debayering is skipped and stats are processed synchronously
>>   */
>>  void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output)
>>  {
>>         ipa_->computeParams(frame);
>> -       debayer_->invokeMethod(&Debayer::process,
>> -                              ConnectionTypeQueued, frame, input, output, debayerParams_);
>> +
>> +       if (output) {
>> +               debayer_->invokeMethod(&Debayer::process,
>> +                                      ConnectionTypeQueued, frame, input, output, debayerParams_);
>> +       } else {
>> +               /* Compute stats, which are otherwise computed in debayering */
>> +               if (frame % SwStatsCpu::kStatPerNumFrames) {
>> +                       stats_->finishFrame(frame, 0);
>> +               } else {
>> +                       MappedFrameBuffer inputMapped(input, MappedFrameBuffer::MapFlag::Read);
>> +                       stats_->processFrame(frame, 0, inputMapped);
>> +               }
>> +       }
>
> Without digging through to figure out how we could re-design this - it
> feels like we're duplicating code paths for specific use cases which
> makes me feel like something needs to be split up.
>
> But I don't know how yet ...

Neither I.

>
> So otherwise:
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>>  }
>>  
>>  void SoftwareIsp::saveIspParams([[maybe_unused]] uint32_t frame)
>> -- 
>> 2.55.0
>>

Hi Jacopo,

thank you for the reviews.

Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes:

> Hi Milan
>
> On Mon, Oct 05, 2026 at 03:16:58PM +0200, Milan Zamazal wrote:
>> `simple' pipeline can basically pass raw buffers from V4L2 to the
>> output.  While the raw data is untouched, we may still want to:
>>
>> - Gather statistics from it.
>> - Run IPA on the images (without modifying the images).
>> - Provide metadata from the IPA.
>> - Adjust exposure/gain.
>>
>> This patch implements all of that.
>>
>> Since this imposes some overhead, a new configuration option
>> pipelines.simple.enable_raw_ipa is added.  The option is on by default
>> as at least the metadata and exposure adjustments may be useful.  Using
>> a configuration option rather than a runtime-adjustable option is not
>> perfect, but I don't know what better mechanism to use.
>>
>> The patch attempts to fit into the current structure as close as
>> possible, with minimum changes.  Future enhancements might include
>> computing the stats asynchronously or even splitting `simple' pipeline
>> into separate components (debayering, converter, raw output).
>>
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>  Documentation/runtime_configuration.rst       | 10 +++++
>>  .../internal/software_isp/software_isp.h      |  3 +-
>>  src/libcamera/pipeline/simple/simple.cpp      | 36 ++++++++++++++---
>>  src/libcamera/software_isp/software_isp.cpp   | 40 ++++++++++++++++---
>>  4 files changed, 77 insertions(+), 12 deletions(-)
>>
>> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst
>> index a71b715a1..1d93ad3a8 100644
>> --- a/Documentation/runtime_configuration.rst
>> +++ b/Documentation/runtime_configuration.rst
>> @@ -46,6 +46,7 @@ file structure:
>>          supported_devices:
>>            - driver: # driver name, e.g. `mxc-isi`
>>              software_isp: # true/false
>> +        enable_raw_ipa: # true/false
>>      software_isp:
>>        copy_input_buffer: # true/false
>>        measure:
>> @@ -81,6 +82,7 @@ Configuration file example
>>           supported_devices:
>>             - driver: mxc-isi
>>               software_isp: true
>> +         enable_raw_ipa: true
>>       software_isp:
>>         copy_input_buffer: false
>>         measure:
>> @@ -157,6 +159,14 @@ pipelines.simple.supported_devices.driver, pipelines.simple.supported_devices.so
>>
>>     Example `software_isp` value: ``true``
>>
>> +pipelines.simple.enable_raw_ipa
>> +   Whether image processing algorithms are enabled also with raw-only
>> +   output. When enabled, statistics is gathered for raw-only output,
>> +   image processing algorithms are run on it, metadata is produced and
>> +   exposure is adjusted.
>> +
>> +   Example value: ``true``
>> +
>>  software_isp.copy_input_buffer
>>     Define whether input buffers should be copied into standard (cached)
>>     memory in software ISP. This is done by default to prevent very slow
>> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
>> index 3e420242b..b4c57dbf5 100644
>> --- a/include/libcamera/internal/software_isp/software_isp.h
>> +++ b/include/libcamera/internal/software_isp/software_isp.h
>> @@ -67,7 +67,8 @@ public:
>>  	int configure(const StreamConfiguration &inputCfg,
>>  		      const std::vector<std::reference_wrapper<const StreamConfiguration>> &outputCfgs,
>>  		      const ipa::softisp::IPAConfigInfo &configInfo,
>> -		      ControlInfoMap *ipaControls);
>> +		      ControlInfoMap *ipaControls,
>> +		      bool debayerEnabled);
>
> fits on the previous line ?

Yes, but isn't one argument per-line a common pattern in such cases?  (I
personally don't care, just to prevent complaints from other reviewers.)

>
>>
>>  	int exportBuffers(const Stream *stream, unsigned int count,
>>  			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index 35c29ceca..dd1d69a5b 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -427,6 +427,7 @@ public:
>>  	V4L2Subdevice *subdev(const MediaEntity *entity);
>>  	std::shared_ptr<MediaDevice> converter() { return converter_; }
>>  	bool swIspEnabled() const { return swIspEnabled_; }
>> +	bool rawIpaEnabled() const { return rawIpaEnabled_; }
>>
>>  protected:
>>  	int queueRequestDevice(Camera *camera, Request *request) override;
>> @@ -460,6 +461,7 @@ private:
>>
>>  	std::shared_ptr<MediaDevice> converter_;
>>  	bool swIspEnabled_;
>> +	bool rawIpaEnabled_ = true;
>>  };
>>
>>  /* -----------------------------------------------------------------------------
>> @@ -968,6 +970,11 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
>>
>>  		conversionQueue_.pop();
>>  		return;
>> +	} else if (pipe->rawIpaEnabled()) {
>
> this branch is taken if !data->useConversion which is initialized with
> CameraData::needConversion_, which is initialized as
>
> 	needConversion_ = config_.size() > 1 + rawCount;
>
> in SimpleCameraConfiguration::validate().
>
> Doesn't it mean data->useConversion is true when we have a Raw stream
> ?

No.  A trivial example is when there is just a raw stream and no other
stream.

data->useConversion_ is true when:

- We have more than one non-raw stream (the condition above).

- We need to convert formats, which is typically the case with Bayer
  input.

>
>> +		/* \todo Make the processing asynchronous? */
>> +		swIsp_->process(request->sequence(), buffer, nullptr);
>> +		pipe->completeBuffer(request, buffer);
>> +		return;
>
> There is quite some booking and ad-hoc handling of special cases here
> to accommodate the "conversion" and "isRaw" situation, and this is
> where a redesign might really help before the code base becomes too
> convoluted.
>
> Have you considered, in example, if instrumenting
> SoftwareIsp::queueBuffers() with support for Raw would require less
> ad-hoc handling than doing it here ?

It's tempting but I'm not sure it would be easy.  I can try it, as a
patch on top of this one, and let's see.

>
>>  	}
>>
>>  	/* Otherwise simply complete the request. */
>> @@ -1591,7 +1598,8 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>>  			data->rawStream_ = &data->streams_[i];
>>  	}
>>
>> -	if (outputCfgs.empty())
>> +	if (outputCfgs.empty() &&
>> +	    (data->converter_ || !data->rawStream_ || !rawIpaEnabled_))
>>  		return 0;
>>
>>  	StreamConfiguration inputCfg;
>> @@ -1605,7 +1613,12 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>>  	} else {
>>  		ipa::softisp::IPAConfigInfo configInfo;
>>  		configInfo.sensorControls = data->sensor_->controls();
>> -		return data->swIsp_->configure(inputCfg, outputCfgs, configInfo, &data->controlInfo_);
>> +		return data->swIsp_->configure(
>> +			inputCfg,
>> +			outputCfgs,
>> +			configInfo,
>> +			&data->controlInfo_,
>> +			data->useConversion_);
>
> Why can't this be
>
> 		return data->swIsp_->configure(inputCfg, outputCfgs, configInfo,
>                                                &data->controlInfo_, data->useConversion);

I'm not sure how to interpret "necessary" in

  * 2 "Breaking Long Lines" striving to fit code within 80 columns and
    accepting up to 120 columns when necessary

but checkstyle is happy with your suggestion so I am too.

>
>>  	}
>>  }
>>
>> @@ -1698,6 +1711,12 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
>>  		if (!data->rawStream_)
>>  			for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_)
>>  				video->queueBuffer(buffer.get());
>> +	} else if (data->swIsp_) {
>> +		ret = data->swIsp_->startIpa();
>> +		if (ret < 0) {
>> +			stop(camera);
>> +			return ret;
>> +		}
>
> The above branch has
>
> 		else if (data->swIsp_)
> 			ret = data->swIsp_->start();
>
> have you considered how hard would it be to unify the data->useConversion_
> and isRaw_ situation and delegate to softISP the handling of Raw ?

No, but I can try in another patch. 

>
>>  	}
>>
>>  	return 0;
>> @@ -1720,6 +1739,8 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>>  			data->converter_->stop();
>>  		else if (data->swIsp_)
>>  			data->swIsp_->stop();
>> +	} else if (data->swIsp_) {
>> +		data->swIsp_->stopIpa();
>>  	}
>>
>>  	video->streamOff();
>> @@ -1763,6 +1784,8 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
>>  		data->conversionQueue_.push({ request, std::move(buffers) });
>>  		if (data->swIsp_)
>>  			data->swIsp_->queueRequest(request->sequence(), request->controls());
>> +	} else if (data->swIsp_ && rawIpaEnabled_) {
>> +		data->swIsp_->queueRequest(request->sequence(), request->controls());
>
> Same question as above
>

Here the "unification" may be simply changing the code to:

  if (data->useConversion_)
  	data->conversionQueue_.push({ request, std::move(buffers) });
  if (data->swIsp_ && (data->useConversion_ || rawIpaEnabled_))
  	data->swIsp_->queueRequest(request->sequence(), request->controls());

(The same what I suggested in the response to Kieran.)

But yes, even this could be possibly simplified, let's experiment a bit.

>>  	}
>>
>>  	return 0;
>> @@ -1882,9 +1905,8 @@ bool SimplePipelineHandler::matchDevice(std::shared_ptr<MediaDevice> media,
>>
>>  	swIspEnabled_ = info.swIspEnabled;
>>  	const GlobalConfiguration &configuration = cameraManager()->_d()->configuration();
>> -	for (const ValueNode &entry :
>> -	     configuration.configuration()["pipelines"]["simple"]["supported_devices"]
>> -		     .asList()) {
>> +	auto &simpleConfiguration = configuration.configuration()["pipelines"]["simple"];
>> +	for (const ValueNode &entry : simpleConfiguration["supported_devices"].asList()) {
>>  		auto name = entry["driver"].get<std::string>();
>>  		if (name == info.driver) {
>>  			swIspEnabled_ = entry["software_isp"].get<bool>().value_or(swIspEnabled_);
>> @@ -1903,6 +1925,10 @@ bool SimplePipelineHandler::matchDevice(std::shared_ptr<MediaDevice> media,
>>  		 */
>>  		ASSERT(!converter_);
>>  		numStreams = 2;
>> +
>> +		auto enableRawIpa = simpleConfiguration["enable_raw_ipa"].get<bool>();
>> +		if (enableRawIpa)
>> +			rawIpaEnabled_ = enableRawIpa.value();
>>  	}
>>
>>  	/* Locate the sensors. */
>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
>> index 11e0c49ed..594125f8f 100644
>> --- a/src/libcamera/software_isp/software_isp.cpp
>> +++ b/src/libcamera/software_isp/software_isp.cpp
>> @@ -25,6 +25,7 @@
>>
>>  #include "libcamera/internal/bayer_format.h"
>>  #include "libcamera/internal/framebuffer.h"
>> +#include "libcamera/internal/mapped_framebuffer.h"
>>  #include "libcamera/internal/software_isp/debayer_params.h"
>>
>>  #include "debayer_cpu.h"
>> @@ -280,19 +281,34 @@ uint32_t SoftwareIsp::preferredInputStride(const PixelFormat &inputFormat, const
>>   * \param[in] outputCfgs The output configurations
>>   * \param[in] configInfo The IPA configuration data, received from the pipeline handler
>>   * \param[out] ipaControls The IPA controls to update
>> + * \param[in] debayerEnabled Whether debayering should be performed in addition
>> + *   to stats and IPA processing
>>   * \return 0 on success, a negative errno on failure
>>   */
>>  int SoftwareIsp::configure(const StreamConfiguration &inputCfg,
>>  			   const std::vector<std::reference_wrapper<const StreamConfiguration>> &outputCfgs,
>>  			   const ipa::softisp::IPAConfigInfo &configInfo,
>> -			   ControlInfoMap *ipaControls)
>> +			   ControlInfoMap *ipaControls,
>> +			   bool debayerEnabled)
>>  {
>> -	ASSERT(ipa_ && debayer_);
>> +	ASSERT(ipa_ && (!debayerEnabled || debayer_));
>> +
>> +	int ret;
>> +	if (!debayerEnabled) {
>> +		ret = stats_->configure(inputCfg);
>> +		if (ret < 0)
>> +			return ret;
>> +		stats_->setWindow(Rectangle(inputCfg.size));
>> +	}
>>
>> -	int ret = ipa_->configure(configInfo, ipaControls);
>> +	ret = ipa_->configure(configInfo, ipaControls);
>>  	if (ret < 0)
>>  		return ret;
>>
>> +	if (!debayerEnabled) {
>> +		return 0;
>> +	}
>> +
>>  	ret = debayer_->configure(inputCfg, outputCfgs, ccmEnabled_);
>>  	if (ret < 0)
>>  		return ret;
>> @@ -452,13 +468,25 @@ void SoftwareIsp::stopIpa()
>>   * \brief Passes the input framebuffer to the ISP worker to process
>>   * \param[in] frame The frame number
>>   * \param[in] input The input framebuffer
>> - * \param[out] output The framebuffer to write the processed frame to
>> + * \param[out] output The framebuffer to write the processed frame to; if
>> + *   nullptr then debayering is skipped and stats are processed synchronously
>>   */
>>  void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output)
>>  {
>>  	ipa_->computeParams(frame);
>> -	debayer_->invokeMethod(&Debayer::process,
>> -			       ConnectionTypeQueued, frame, input, output, debayerParams_);
>> +
>> +	if (output) {
>> +		debayer_->invokeMethod(&Debayer::process,
>> +				       ConnectionTypeQueued, frame, input, output, debayerParams_);
>> +	} else {
>> +		/* Compute stats, which are otherwise computed in debayering */
>> +		if (frame % SwStatsCpu::kStatPerNumFrames) {
>> +			stats_->finishFrame(frame, 0);
>> +		} else {
>> +			MappedFrameBuffer inputMapped(input, MappedFrameBuffer::MapFlag::Read);
>> +			stats_->processFrame(frame, 0, inputMapped);
>> +		}
>> +	}
>
> The fact you already handle the raw case in SoftwareIsp class for some
> functions makes me wonder, as said above, if it couldn't be entirely
> done here.

Maybe, I'll try.  I don't think it's worth investing into it too much as
we may have bigger dream^Wplans with the simple pipeline but maybe it's
not too difficult.

>
> Thanks
>   j
>
>>  }
>>
>>  void SoftwareIsp::saveIspParams([[maybe_unused]] uint32_t frame)
>> --
>> 2.55.0
>>

Hi Milan

On Mon, Oct 19, 2026 at 08:26:29PM +0200, Milan Zamazal wrote:
> Hi Jacopo,
>
> thank you for the reviews.
>
> Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes:
>
> > Hi Milan
> >
> > On Mon, Oct 05, 2026 at 03:16:58PM +0200, Milan Zamazal wrote:
> >> `simple' pipeline can basically pass raw buffers from V4L2 to the
> >> output.  While the raw data is untouched, we may still want to:
> >>
> >> - Gather statistics from it.
> >> - Run IPA on the images (without modifying the images).
> >> - Provide metadata from the IPA.
> >> - Adjust exposure/gain.
> >>
> >> This patch implements all of that.
> >>
> >> Since this imposes some overhead, a new configuration option
> >> pipelines.simple.enable_raw_ipa is added.  The option is on by default
> >> as at least the metadata and exposure adjustments may be useful.  Using
> >> a configuration option rather than a runtime-adjustable option is not
> >> perfect, but I don't know what better mechanism to use.
> >>
> >> The patch attempts to fit into the current structure as close as
> >> possible, with minimum changes.  Future enhancements might include
> >> computing the stats asynchronously or even splitting `simple' pipeline
> >> into separate components (debayering, converter, raw output).
> >>
> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> >> ---
> >>  Documentation/runtime_configuration.rst       | 10 +++++
> >>  .../internal/software_isp/software_isp.h      |  3 +-
> >>  src/libcamera/pipeline/simple/simple.cpp      | 36 ++++++++++++++---
> >>  src/libcamera/software_isp/software_isp.cpp   | 40 ++++++++++++++++---
> >>  4 files changed, 77 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst
> >> index a71b715a1..1d93ad3a8 100644
> >> --- a/Documentation/runtime_configuration.rst
> >> +++ b/Documentation/runtime_configuration.rst
> >> @@ -46,6 +46,7 @@ file structure:
> >>          supported_devices:
> >>            - driver: # driver name, e.g. `mxc-isi`
> >>              software_isp: # true/false
> >> +        enable_raw_ipa: # true/false
> >>      software_isp:
> >>        copy_input_buffer: # true/false
> >>        measure:
> >> @@ -81,6 +82,7 @@ Configuration file example
> >>           supported_devices:
> >>             - driver: mxc-isi
> >>               software_isp: true
> >> +         enable_raw_ipa: true
> >>       software_isp:
> >>         copy_input_buffer: false
> >>         measure:
> >> @@ -157,6 +159,14 @@ pipelines.simple.supported_devices.driver, pipelines.simple.supported_devices.so
> >>
> >>     Example `software_isp` value: ``true``
> >>
> >> +pipelines.simple.enable_raw_ipa
> >> +   Whether image processing algorithms are enabled also with raw-only
> >> +   output. When enabled, statistics is gathered for raw-only output,
> >> +   image processing algorithms are run on it, metadata is produced and
> >> +   exposure is adjusted.
> >> +
> >> +   Example value: ``true``
> >> +
> >>  software_isp.copy_input_buffer
> >>     Define whether input buffers should be copied into standard (cached)
> >>     memory in software ISP. This is done by default to prevent very slow
> >> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
> >> index 3e420242b..b4c57dbf5 100644
> >> --- a/include/libcamera/internal/software_isp/software_isp.h
> >> +++ b/include/libcamera/internal/software_isp/software_isp.h
> >> @@ -67,7 +67,8 @@ public:
> >>  	int configure(const StreamConfiguration &inputCfg,
> >>  		      const std::vector<std::reference_wrapper<const StreamConfiguration>> &outputCfgs,
> >>  		      const ipa::softisp::IPAConfigInfo &configInfo,
> >> -		      ControlInfoMap *ipaControls);
> >> +		      ControlInfoMap *ipaControls,
> >> +		      bool debayerEnabled);
> >
> > fits on the previous line ?
>
> Yes, but isn't one argument per-line a common pattern in such cases?  (I
> personally don't care, just to prevent complaints from other reviewers.)
>

Why break a line if not necessary ?

> >
> >>
> >>  	int exportBuffers(const Stream *stream, unsigned int count,
> >>  			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> >> index 35c29ceca..dd1d69a5b 100644
> >> --- a/src/libcamera/pipeline/simple/simple.cpp
> >> +++ b/src/libcamera/pipeline/simple/simple.cpp
> >> @@ -427,6 +427,7 @@ public:
> >>  	V4L2Subdevice *subdev(const MediaEntity *entity);
> >>  	std::shared_ptr<MediaDevice> converter() { return converter_; }
> >>  	bool swIspEnabled() const { return swIspEnabled_; }
> >> +	bool rawIpaEnabled() const { return rawIpaEnabled_; }
> >>
> >>  protected:
> >>  	int queueRequestDevice(Camera *camera, Request *request) override;
> >> @@ -460,6 +461,7 @@ private:
> >>
> >>  	std::shared_ptr<MediaDevice> converter_;
> >>  	bool swIspEnabled_;
> >> +	bool rawIpaEnabled_ = true;
> >>  };
> >>
> >>  /* -----------------------------------------------------------------------------
> >> @@ -968,6 +970,11 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
> >>
> >>  		conversionQueue_.pop();
> >>  		return;
> >> +	} else if (pipe->rawIpaEnabled()) {
> >
> > this branch is taken if !data->useConversion which is initialized with
> > CameraData::needConversion_, which is initialized as
> >
> > 	needConversion_ = config_.size() > 1 + rawCount;
> >
> > in SimpleCameraConfiguration::validate().
> >
> > Doesn't it mean data->useConversion is true when we have a Raw stream
> > ?
>
> No.  A trivial example is when there is just a raw stream and no other
> stream.
>
> data->useConversion_ is true when:
>
> - We have more than one non-raw stream (the condition above).
>
> - We need to convert formats, which is typically the case with Bayer
>   input.

I see.

I feel like we could go a long way if the Simple pipeline would handle
the "next" step in a more abstracted way instead of relying on chains
of

if (needConversion_) { if (converter) {} else if (swIsp_) {} else {} }

I know that's a big rework but this series makes the above if/else
based flow even more fragile.

Anyway, I don't have ownership on this part of the code base and you
mentioned big plans, so I nobody else complains, I won't as well.

>
> >
> >> +		/* \todo Make the processing asynchronous? */
> >> +		swIsp_->process(request->sequence(), buffer, nullptr);
> >> +		pipe->completeBuffer(request, buffer);
> >> +		return;
> >
> > There is quite some booking and ad-hoc handling of special cases here
> > to accommodate the "conversion" and "isRaw" situation, and this is
> > where a redesign might really help before the code base becomes too
> > convoluted.
> >
> > Have you considered, in example, if instrumenting
> > SoftwareIsp::queueBuffers() with support for Raw would require less
> > ad-hoc handling than doing it here ?
>
> It's tempting but I'm not sure it would be easy.  I can try it, as a
> patch on top of this one, and let's see.
>

Thanks

> >
> >>  	}
> >>
> >>  	/* Otherwise simply complete the request. */
> >> @@ -1591,7 +1598,8 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
> >>  			data->rawStream_ = &data->streams_[i];
> >>  	}
> >>
> >> -	if (outputCfgs.empty())
> >> +	if (outputCfgs.empty() &&
> >> +	    (data->converter_ || !data->rawStream_ || !rawIpaEnabled_))
> >>  		return 0;
> >>
> >>  	StreamConfiguration inputCfg;
> >> @@ -1605,7 +1613,12 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
> >>  	} else {
> >>  		ipa::softisp::IPAConfigInfo configInfo;
> >>  		configInfo.sensorControls = data->sensor_->controls();
> >> -		return data->swIsp_->configure(inputCfg, outputCfgs, configInfo, &data->controlInfo_);
> >> +		return data->swIsp_->configure(
> >> +			inputCfg,
> >> +			outputCfgs,
> >> +			configInfo,
> >> +			&data->controlInfo_,
> >> +			data->useConversion_);
> >
> > Why can't this be
> >
> > 		return data->swIsp_->configure(inputCfg, outputCfgs, configInfo,
> >                                                &data->controlInfo_, data->useConversion);
>
> I'm not sure how to interpret "necessary" in
>
>   * 2 "Breaking Long Lines" striving to fit code within 80 columns and
>     accepting up to 120 columns when necessary
>
> but checkstyle is happy with your suggestion so I am too.
>

I wonder, was checkstyle happy with

		return data->swIsp_->configure(
			inputCfg,
			outputCfgs,
			configInfo,
			&data->controlInfo_,
			data->useConversion_);
??

> >
> >>  	}
> >>  }
> >>
> >> @@ -1698,6 +1711,12 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
> >>  		if (!data->rawStream_)
> >>  			for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_)
> >>  				video->queueBuffer(buffer.get());
> >> +	} else if (data->swIsp_) {
> >> +		ret = data->swIsp_->startIpa();
> >> +		if (ret < 0) {
> >> +			stop(camera);
> >> +			return ret;
> >> +		}
> >
> > The above branch has
> >
> > 		else if (data->swIsp_)
> > 			ret = data->swIsp_->start();
> >
> > have you considered how hard would it be to unify the data->useConversion_
> > and isRaw_ situation and delegate to softISP the handling of Raw ?
>
> No, but I can try in another patch.
>

Big reworks apart, the problem to me sees that the current code only
deals with data->swIsp_ under the if (needConversion_) {} condition,
while we now want to pass stats to swIsp_ also in case of Raw.

Maybe the needConversion_ flag is not really necessary anymore ?

> >
> >>  	}
> >>
> >>  	return 0;
> >> @@ -1720,6 +1739,8 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
> >>  			data->converter_->stop();
> >>  		else if (data->swIsp_)
> >>  			data->swIsp_->stop();
> >> +	} else if (data->swIsp_) {
> >> +		data->swIsp_->stopIpa();
> >>  	}
> >>
> >>  	video->streamOff();
> >> @@ -1763,6 +1784,8 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
> >>  		data->conversionQueue_.push({ request, std::move(buffers) });
> >>  		if (data->swIsp_)
> >>  			data->swIsp_->queueRequest(request->sequence(), request->controls());
> >> +	} else if (data->swIsp_ && rawIpaEnabled_) {
> >> +		data->swIsp_->queueRequest(request->sequence(), request->controls());
> >
> > Same question as above
> >
>
> Here the "unification" may be simply changing the code to:
>
>   if (data->useConversion_)
>   	data->conversionQueue_.push({ request, std::move(buffers) });
>   if (data->swIsp_ && (data->useConversion_ || rawIpaEnabled_))
>   	data->swIsp_->queueRequest(request->sequence(), request->controls());
>
> (The same what I suggested in the response to Kieran.)
>
> But yes, even this could be possibly simplified, let's experiment a bit.

if you could find a way to express "do I need softIsp_ ?"  condition
as a CameraData function (in exampla  data->useSwIsp()) one could
write

        if (data->useSwIsp()) {
                if (data->needConversion_)
                   	data->conversionQueue_.push({ request, std::move(buffers) });
                data->swIsp_->queueRequest(request->sequence(), request->controls());
        }

It would be sweet. Not sure if it's possible though.

>
> >>  	}
> >>
> >>  	return 0;
> >> @@ -1882,9 +1905,8 @@ bool SimplePipelineHandler::matchDevice(std::shared_ptr<MediaDevice> media,
> >>
> >>  	swIspEnabled_ = info.swIspEnabled;
> >>  	const GlobalConfiguration &configuration = cameraManager()->_d()->configuration();
> >> -	for (const ValueNode &entry :
> >> -	     configuration.configuration()["pipelines"]["simple"]["supported_devices"]
> >> -		     .asList()) {
> >> +	auto &simpleConfiguration = configuration.configuration()["pipelines"]["simple"];
> >> +	for (const ValueNode &entry : simpleConfiguration["supported_devices"].asList()) {
> >>  		auto name = entry["driver"].get<std::string>();
> >>  		if (name == info.driver) {
> >>  			swIspEnabled_ = entry["software_isp"].get<bool>().value_or(swIspEnabled_);
> >> @@ -1903,6 +1925,10 @@ bool SimplePipelineHandler::matchDevice(std::shared_ptr<MediaDevice> media,
> >>  		 */
> >>  		ASSERT(!converter_);
> >>  		numStreams = 2;
> >> +
> >> +		auto enableRawIpa = simpleConfiguration["enable_raw_ipa"].get<bool>();
> >> +		if (enableRawIpa)
> >> +			rawIpaEnabled_ = enableRawIpa.value();
> >>  	}
> >>
> >>  	/* Locate the sensors. */
> >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> >> index 11e0c49ed..594125f8f 100644
> >> --- a/src/libcamera/software_isp/software_isp.cpp
> >> +++ b/src/libcamera/software_isp/software_isp.cpp
> >> @@ -25,6 +25,7 @@
> >>
> >>  #include "libcamera/internal/bayer_format.h"
> >>  #include "libcamera/internal/framebuffer.h"
> >> +#include "libcamera/internal/mapped_framebuffer.h"
> >>  #include "libcamera/internal/software_isp/debayer_params.h"
> >>
> >>  #include "debayer_cpu.h"
> >> @@ -280,19 +281,34 @@ uint32_t SoftwareIsp::preferredInputStride(const PixelFormat &inputFormat, const
> >>   * \param[in] outputCfgs The output configurations
> >>   * \param[in] configInfo The IPA configuration data, received from the pipeline handler
> >>   * \param[out] ipaControls The IPA controls to update
> >> + * \param[in] debayerEnabled Whether debayering should be performed in addition
> >> + *   to stats and IPA processing
> >>   * \return 0 on success, a negative errno on failure
> >>   */
> >>  int SoftwareIsp::configure(const StreamConfiguration &inputCfg,
> >>  			   const std::vector<std::reference_wrapper<const StreamConfiguration>> &outputCfgs,
> >>  			   const ipa::softisp::IPAConfigInfo &configInfo,
> >> -			   ControlInfoMap *ipaControls)
> >> +			   ControlInfoMap *ipaControls,
> >> +			   bool debayerEnabled)
> >>  {
> >> -	ASSERT(ipa_ && debayer_);
> >> +	ASSERT(ipa_ && (!debayerEnabled || debayer_));
> >> +
> >> +	int ret;
> >> +	if (!debayerEnabled) {
> >> +		ret = stats_->configure(inputCfg);
> >> +		if (ret < 0)
> >> +			return ret;
> >> +		stats_->setWindow(Rectangle(inputCfg.size));
> >> +	}
> >>
> >> -	int ret = ipa_->configure(configInfo, ipaControls);
> >> +	ret = ipa_->configure(configInfo, ipaControls);
> >>  	if (ret < 0)
> >>  		return ret;
> >>
> >> +	if (!debayerEnabled) {
> >> +		return 0;
> >> +	}
> >> +
> >>  	ret = debayer_->configure(inputCfg, outputCfgs, ccmEnabled_);
> >>  	if (ret < 0)
> >>  		return ret;
> >> @@ -452,13 +468,25 @@ void SoftwareIsp::stopIpa()
> >>   * \brief Passes the input framebuffer to the ISP worker to process
> >>   * \param[in] frame The frame number
> >>   * \param[in] input The input framebuffer
> >> - * \param[out] output The framebuffer to write the processed frame to
> >> + * \param[out] output The framebuffer to write the processed frame to; if
> >> + *   nullptr then debayering is skipped and stats are processed synchronously
> >>   */
> >>  void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output)
> >>  {
> >>  	ipa_->computeParams(frame);
> >> -	debayer_->invokeMethod(&Debayer::process,
> >> -			       ConnectionTypeQueued, frame, input, output, debayerParams_);
> >> +
> >> +	if (output) {
> >> +		debayer_->invokeMethod(&Debayer::process,
> >> +				       ConnectionTypeQueued, frame, input, output, debayerParams_);
> >> +	} else {
> >> +		/* Compute stats, which are otherwise computed in debayering */
> >> +		if (frame % SwStatsCpu::kStatPerNumFrames) {
> >> +			stats_->finishFrame(frame, 0);
> >> +		} else {
> >> +			MappedFrameBuffer inputMapped(input, MappedFrameBuffer::MapFlag::Read);
> >> +			stats_->processFrame(frame, 0, inputMapped);
> >> +		}
> >> +	}
> >
> > The fact you already handle the raw case in SoftwareIsp class for some
> > functions makes me wonder, as said above, if it couldn't be entirely
> > done here.
>
> Maybe, I'll try.  I don't think it's worth investing into it too much as
> we may have bigger dream^Wplans with the simple pipeline but maybe it's
> not too difficult.
>

Ack, mine are just suggestions. As I'm not aware of the plans with
simple pipeline, I'll trust your judgment on how much time is a
"reasonable" investment on this topic.

Thanks
  j
> >
> > Thanks
> >   j
> >
> >>  }
> >>
> >>  void SoftwareIsp::saveIspParams([[maybe_unused]] uint32_t frame)
> >> --
> >> 2.55.0
> >>
>

Hi Jacopo,

Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes:

> Hi Milan
>
> On Mon, Oct 19, 2026 at 08:26:29PM +0200, Milan Zamazal wrote:
>> Hi Jacopo,
>>
>> thank you for the reviews.
>>
>> Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes:
>>
>> > Hi Milan
>> >
>> > On Mon, Oct 05, 2026 at 03:16:58PM +0200, Milan Zamazal wrote:
>> >> `simple' pipeline can basically pass raw buffers from V4L2 to the
>> >> output.  While the raw data is untouched, we may still want to:
>> >>
>> >> - Gather statistics from it.
>> >> - Run IPA on the images (without modifying the images).
>> >> - Provide metadata from the IPA.
>> >> - Adjust exposure/gain.
>> >>
>> >> This patch implements all of that.
>> >>
>> >> Since this imposes some overhead, a new configuration option
>> >> pipelines.simple.enable_raw_ipa is added.  The option is on by default
>> >> as at least the metadata and exposure adjustments may be useful.  Using
>> >> a configuration option rather than a runtime-adjustable option is not
>> >> perfect, but I don't know what better mechanism to use.
>> >>
>> >> The patch attempts to fit into the current structure as close as
>> >> possible, with minimum changes.  Future enhancements might include
>> >> computing the stats asynchronously or even splitting `simple' pipeline
>> >> into separate components (debayering, converter, raw output).
>> >>
>> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> >> ---
>> >>  Documentation/runtime_configuration.rst       | 10 +++++
>> >>  .../internal/software_isp/software_isp.h      |  3 +-
>> >>  src/libcamera/pipeline/simple/simple.cpp      | 36 ++++++++++++++---
>> >>  src/libcamera/software_isp/software_isp.cpp   | 40 ++++++++++++++++---
>> >>  4 files changed, 77 insertions(+), 12 deletions(-)
>> >>
>> >> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst
>> >> index a71b715a1..1d93ad3a8 100644
>> >> --- a/Documentation/runtime_configuration.rst
>> >> +++ b/Documentation/runtime_configuration.rst
>> >> @@ -46,6 +46,7 @@ file structure:
>> >>          supported_devices:
>> >>            - driver: # driver name, e.g. `mxc-isi`
>> >>              software_isp: # true/false
>> >> +        enable_raw_ipa: # true/false
>> >>      software_isp:
>> >>        copy_input_buffer: # true/false
>> >>        measure:
>> >> @@ -81,6 +82,7 @@ Configuration file example
>> >>           supported_devices:
>> >>             - driver: mxc-isi
>> >>               software_isp: true
>> >> +         enable_raw_ipa: true
>> >>       software_isp:
>> >>         copy_input_buffer: false
>> >>         measure:
>> >> @@ -157,6 +159,14 @@ pipelines.simple.supported_devices.driver, pipelines.simple.supported_devices.so
>> >>
>> >>     Example `software_isp` value: ``true``
>> >>
>> >> +pipelines.simple.enable_raw_ipa
>> >> +   Whether image processing algorithms are enabled also with raw-only
>> >> +   output. When enabled, statistics is gathered for raw-only output,
>> >> +   image processing algorithms are run on it, metadata is produced and
>> >> +   exposure is adjusted.
>> >> +
>> >> +   Example value: ``true``
>> >> +
>> >>  software_isp.copy_input_buffer
>> >>     Define whether input buffers should be copied into standard (cached)
>> >>     memory in software ISP. This is done by default to prevent very slow
>> >> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
>> >> index 3e420242b..b4c57dbf5 100644
>> >> --- a/include/libcamera/internal/software_isp/software_isp.h
>> >> +++ b/include/libcamera/internal/software_isp/software_isp.h
>> >> @@ -67,7 +67,8 @@ public:
>> >>  	int configure(const StreamConfiguration &inputCfg,
>> >>  		      const std::vector<std::reference_wrapper<const StreamConfiguration>> &outputCfgs,
>> >>  		      const ipa::softisp::IPAConfigInfo &configInfo,
>> >> -		      ControlInfoMap *ipaControls);
>> >> +		      ControlInfoMap *ipaControls,
>> >> +		      bool debayerEnabled);
>> >
>> > fits on the previous line ?
>>
>> Yes, but isn't one argument per-line a common pattern in such cases?  (I
>> personally don't care, just to prevent complaints from other reviewers.)
>>
>
> Why break a line if not necessary ?

I can certainly join the lines.

>
>> >
>> >>
>> >>  	int exportBuffers(const Stream *stream, unsigned int count,
>> >>  			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> >> index 35c29ceca..dd1d69a5b 100644
>> >> --- a/src/libcamera/pipeline/simple/simple.cpp
>> >> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> >> @@ -427,6 +427,7 @@ public:
>> >>  	V4L2Subdevice *subdev(const MediaEntity *entity);
>> >>  	std::shared_ptr<MediaDevice> converter() { return converter_; }
>> >>  	bool swIspEnabled() const { return swIspEnabled_; }
>> >> +	bool rawIpaEnabled() const { return rawIpaEnabled_; }
>> >>
>> >>  protected:
>> >>  	int queueRequestDevice(Camera *camera, Request *request) override;
>> >> @@ -460,6 +461,7 @@ private:
>> >>
>> >>  	std::shared_ptr<MediaDevice> converter_;
>> >>  	bool swIspEnabled_;
>> >> +	bool rawIpaEnabled_ = true;
>> >>  };
>> >>
>> >>  /* -----------------------------------------------------------------------------
>> >> @@ -968,6 +970,11 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
>> >>
>> >>  		conversionQueue_.pop();
>> >>  		return;
>> >> +	} else if (pipe->rawIpaEnabled()) {
>> >
>> > this branch is taken if !data->useConversion which is initialized with
>> > CameraData::needConversion_, which is initialized as
>> >
>> > 	needConversion_ = config_.size() > 1 + rawCount;
>> >
>> > in SimpleCameraConfiguration::validate().
>> >
>> > Doesn't it mean data->useConversion is true when we have a Raw stream
>> > ?
>>
>> No.  A trivial example is when there is just a raw stream and no other
>> stream.
>>
>> data->useConversion_ is true when:
>>
>> - We have more than one non-raw stream (the condition above).
>>
>> - We need to convert formats, which is typically the case with Bayer
>>   input.
>
> I see.
>
> I feel like we could go a long way if the Simple pipeline would handle
> the "next" step in a more abstracted way instead of relying on chains
> of
>
> if (needConversion_) { if (converter) {} else if (swIsp_) {} else {} }
>
> I know that's a big rework but this series makes the above if/else
> based flow even more fragile.
>
> Anyway, I don't have ownership on this part of the code base and you
> mentioned big plans, so I nobody else complains, I won't as well.

I think there are intentions to make pipelines more modular generally
and `simple' pipeline already serves several different purposes
(converters, software ISP, raw streams, ...).  Tying raw streams more to
software ISP doesn't seem to fit in the desired direction but if it
helps with keeping `simple' pipeline manageable (it's a well known fact
that it's not easy to understand already) then it's probably OK for now.
I'll think what can be reasonably done.

>
>>
>> >
>> >> +		/* \todo Make the processing asynchronous? */
>> >> +		swIsp_->process(request->sequence(), buffer, nullptr);
>> >> +		pipe->completeBuffer(request, buffer);
>> >> +		return;
>> >
>> > There is quite some booking and ad-hoc handling of special cases here
>> > to accommodate the "conversion" and "isRaw" situation, and this is
>> > where a redesign might really help before the code base becomes too
>> > convoluted.
>> >
>> > Have you considered, in example, if instrumenting
>> > SoftwareIsp::queueBuffers() with support for Raw would require less
>> > ad-hoc handling than doing it here ?
>>
>> It's tempting but I'm not sure it would be easy.  I can try it, as a
>> patch on top of this one, and let's see.
>>
>
> Thanks
>
>> >
>> >>  	}
>> >>
>> >>  	/* Otherwise simply complete the request. */
>> >> @@ -1591,7 +1598,8 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>> >>  			data->rawStream_ = &data->streams_[i];
>> >>  	}
>> >>
>> >> -	if (outputCfgs.empty())
>> >> +	if (outputCfgs.empty() &&
>> >> +	    (data->converter_ || !data->rawStream_ || !rawIpaEnabled_))
>> >>  		return 0;
>> >>
>> >>  	StreamConfiguration inputCfg;
>> >> @@ -1605,7 +1613,12 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>> >>  	} else {
>> >>  		ipa::softisp::IPAConfigInfo configInfo;
>> >>  		configInfo.sensorControls = data->sensor_->controls();
>> >> -		return data->swIsp_->configure(inputCfg, outputCfgs, configInfo, &data->controlInfo_);
>> >> +		return data->swIsp_->configure(
>> >> +			inputCfg,
>> >> +			outputCfgs,
>> >> +			configInfo,
>> >> +			&data->controlInfo_,
>> >> +			data->useConversion_);
>> >
>> > Why can't this be
>> >
>> > 		return data->swIsp_->configure(inputCfg, outputCfgs, configInfo,
>> >                                                &data->controlInfo_, data->useConversion);
>>
>> I'm not sure how to interpret "necessary" in
>>
>>   * 2 "Breaking Long Lines" striving to fit code within 80 columns and
>>     accepting up to 120 columns when necessary
>>
>> but checkstyle is happy with your suggestion so I am too.
>>
>
> I wonder, was checkstyle happy with
>
> 		return data->swIsp_->configure(
> 			inputCfg,
> 			outputCfgs,
> 			configInfo,
> 			&data->controlInfo_,
> 			data->useConversion_);
> ??

Yes.

>
>> >
>> >>  	}
>> >>  }
>> >>
>> >> @@ -1698,6 +1711,12 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
>> >>  		if (!data->rawStream_)
>> >>  			for (std::unique_ptr<FrameBuffer> &buffer : data->conversionBuffers_)
>> >>  				video->queueBuffer(buffer.get());
>> >> +	} else if (data->swIsp_) {
>> >> +		ret = data->swIsp_->startIpa();
>> >> +		if (ret < 0) {
>> >> +			stop(camera);
>> >> +			return ret;
>> >> +		}
>> >
>> > The above branch has
>> >
>> > 		else if (data->swIsp_)
>> > 			ret = data->swIsp_->start();
>> >
>> > have you considered how hard would it be to unify the data->useConversion_
>> > and isRaw_ situation and delegate to softISP the handling of Raw ?
>>
>> No, but I can try in another patch.
>>
>
> Big reworks apart, the problem to me sees that the current code only
> deals with data->swIsp_ under the if (needConversion_) {} condition,
> while we now want to pass stats to swIsp_ also in case of Raw.
>
> Maybe the needConversion_ flag is not really necessary anymore ?

That's my thought too, whether it could be done with less flags.

>
>> >
>> >>  	}
>> >>
>> >>  	return 0;
>> >> @@ -1720,6 +1739,8 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>> >>  			data->converter_->stop();
>> >>  		else if (data->swIsp_)
>> >>  			data->swIsp_->stop();
>> >> +	} else if (data->swIsp_) {
>> >> +		data->swIsp_->stopIpa();
>> >>  	}
>> >>
>> >>  	video->streamOff();
>> >> @@ -1763,6 +1784,8 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
>> >>  		data->conversionQueue_.push({ request, std::move(buffers) });
>> >>  		if (data->swIsp_)
>> >>  			data->swIsp_->queueRequest(request->sequence(), request->controls());
>> >> +	} else if (data->swIsp_ && rawIpaEnabled_) {
>> >> +		data->swIsp_->queueRequest(request->sequence(), request->controls());
>> >
>> > Same question as above
>> >
>>
>> Here the "unification" may be simply changing the code to:
>>
>>   if (data->useConversion_)
>>   	data->conversionQueue_.push({ request, std::move(buffers) });
>>   if (data->swIsp_ && (data->useConversion_ || rawIpaEnabled_))
>>   	data->swIsp_->queueRequest(request->sequence(), request->controls());
>>
>> (The same what I suggested in the response to Kieran.)
>>
>> But yes, even this could be possibly simplified, let's experiment a bit.
>
> if you could find a way to express "do I need softIsp_ ?"  condition
> as a CameraData function (in exampla  data->useSwIsp()) one could
> write
>
>         if (data->useSwIsp()) {
>                 if (data->needConversion_)
>                    	data->conversionQueue_.push({ request, std::move(buffers) });
>                 data->swIsp_->queueRequest(request->sequence(), request->controls());
>         }
>
> It would be sweet. Not sure if it's possible though.

Let's not forget about the case when there is a converter, without
software ISP.

>
>>
>> >>  	}
>> >>
>> >>  	return 0;
>> >> @@ -1882,9 +1905,8 @@ bool SimplePipelineHandler::matchDevice(std::shared_ptr<MediaDevice> media,
>> >>
>> >>  	swIspEnabled_ = info.swIspEnabled;
>> >>  	const GlobalConfiguration &configuration = cameraManager()->_d()->configuration();
>> >> -	for (const ValueNode &entry :
>> >> -	     configuration.configuration()["pipelines"]["simple"]["supported_devices"]
>> >> -		     .asList()) {
>> >> +	auto &simpleConfiguration = configuration.configuration()["pipelines"]["simple"];
>> >> +	for (const ValueNode &entry : simpleConfiguration["supported_devices"].asList()) {
>> >>  		auto name = entry["driver"].get<std::string>();
>> >>  		if (name == info.driver) {
>> >>  			swIspEnabled_ = entry["software_isp"].get<bool>().value_or(swIspEnabled_);
>> >> @@ -1903,6 +1925,10 @@ bool SimplePipelineHandler::matchDevice(std::shared_ptr<MediaDevice> media,
>> >>  		 */
>> >>  		ASSERT(!converter_);
>> >>  		numStreams = 2;
>> >> +
>> >> +		auto enableRawIpa = simpleConfiguration["enable_raw_ipa"].get<bool>();
>> >> +		if (enableRawIpa)
>> >> +			rawIpaEnabled_ = enableRawIpa.value();
>> >>  	}
>> >>
>> >>  	/* Locate the sensors. */
>> >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
>> >> index 11e0c49ed..594125f8f 100644
>> >> --- a/src/libcamera/software_isp/software_isp.cpp
>> >> +++ b/src/libcamera/software_isp/software_isp.cpp
>> >> @@ -25,6 +25,7 @@
>> >>
>> >>  #include "libcamera/internal/bayer_format.h"
>> >>  #include "libcamera/internal/framebuffer.h"
>> >> +#include "libcamera/internal/mapped_framebuffer.h"
>> >>  #include "libcamera/internal/software_isp/debayer_params.h"
>> >>
>> >>  #include "debayer_cpu.h"
>> >> @@ -280,19 +281,34 @@ uint32_t SoftwareIsp::preferredInputStride(const PixelFormat &inputFormat, const
>> >>   * \param[in] outputCfgs The output configurations
>> >>   * \param[in] configInfo The IPA configuration data, received from the pipeline handler
>> >>   * \param[out] ipaControls The IPA controls to update
>> >> + * \param[in] debayerEnabled Whether debayering should be performed in addition
>> >> + *   to stats and IPA processing
>> >>   * \return 0 on success, a negative errno on failure
>> >>   */
>> >>  int SoftwareIsp::configure(const StreamConfiguration &inputCfg,
>> >>  			   const std::vector<std::reference_wrapper<const StreamConfiguration>> &outputCfgs,
>> >>  			   const ipa::softisp::IPAConfigInfo &configInfo,
>> >> -			   ControlInfoMap *ipaControls)
>> >> +			   ControlInfoMap *ipaControls,
>> >> +			   bool debayerEnabled)
>> >>  {
>> >> -	ASSERT(ipa_ && debayer_);
>> >> +	ASSERT(ipa_ && (!debayerEnabled || debayer_));
>> >> +
>> >> +	int ret;
>> >> +	if (!debayerEnabled) {
>> >> +		ret = stats_->configure(inputCfg);
>> >> +		if (ret < 0)
>> >> +			return ret;
>> >> +		stats_->setWindow(Rectangle(inputCfg.size));
>> >> +	}
>> >>
>> >> -	int ret = ipa_->configure(configInfo, ipaControls);
>> >> +	ret = ipa_->configure(configInfo, ipaControls);
>> >>  	if (ret < 0)
>> >>  		return ret;
>> >>
>> >> +	if (!debayerEnabled) {
>> >> +		return 0;
>> >> +	}
>> >> +
>> >>  	ret = debayer_->configure(inputCfg, outputCfgs, ccmEnabled_);
>> >>  	if (ret < 0)
>> >>  		return ret;
>> >> @@ -452,13 +468,25 @@ void SoftwareIsp::stopIpa()
>> >>   * \brief Passes the input framebuffer to the ISP worker to process
>> >>   * \param[in] frame The frame number
>> >>   * \param[in] input The input framebuffer
>> >> - * \param[out] output The framebuffer to write the processed frame to
>> >> + * \param[out] output The framebuffer to write the processed frame to; if
>> >> + *   nullptr then debayering is skipped and stats are processed synchronously
>> >>   */
>> >>  void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output)
>> >>  {
>> >>  	ipa_->computeParams(frame);
>> >> -	debayer_->invokeMethod(&Debayer::process,
>> >> -			       ConnectionTypeQueued, frame, input, output, debayerParams_);
>> >> +
>> >> +	if (output) {
>> >> +		debayer_->invokeMethod(&Debayer::process,
>> >> +				       ConnectionTypeQueued, frame, input, output, debayerParams_);
>> >> +	} else {
>> >> +		/* Compute stats, which are otherwise computed in debayering */
>> >> +		if (frame % SwStatsCpu::kStatPerNumFrames) {
>> >> +			stats_->finishFrame(frame, 0);
>> >> +		} else {
>> >> +			MappedFrameBuffer inputMapped(input, MappedFrameBuffer::MapFlag::Read);
>> >> +			stats_->processFrame(frame, 0, inputMapped);
>> >> +		}
>> >> +	}
>> >
>> > The fact you already handle the raw case in SoftwareIsp class for some
>> > functions makes me wonder, as said above, if it couldn't be entirely
>> > done here.
>>
>> Maybe, I'll try.  I don't think it's worth investing into it too much as
>> we may have bigger dream^Wplans with the simple pipeline but maybe it's
>> not too difficult.
>>
>
> Ack, mine are just suggestions. As I'm not aware of the plans with
> simple pipeline, I'll trust your judgment on how much time is a
> "reasonable" investment on this topic.
>
> Thanks
>   j
>> >
>> > Thanks
>> >   j
>> >
>> >>  }
>> >>
>> >>  void SoftwareIsp::saveIspParams([[maybe_unused]] uint32_t frame)
>> >> --
>> >> 2.55.0
>> >>
>>