[v18,09/12] libcamera: software_isp: Make input buffer copying configurable
diff mbox series

Message ID 20250912142915.53949-11-mzamazal@redhat.com
State Accepted
Headers show
Series
  • Add global configuration file
Related show

Commit Message

Milan Zamazal Sept. 12, 2025, 2:29 p.m. UTC
On some platforms, working directly on the input buffer is very slow due
to disabled caching.  This is why we copy the input buffer into standard
(cached) memory.  This is an unnecessary overhead on platforms with
cached buffers.

Let's make input buffer copying configurable.  The default is still
copying, as its overhead is much lower than contingent operations on
non-cached memory.  Ideally, we should improve this in future to set the
default to non-copying if we can be sure under observable circumstances
that we are working with cached buffers.

Completes software ISP TODO #6.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 Documentation/runtime_configuration.rst     | 13 +++++++++++++
 src/libcamera/software_isp/TODO             | 11 -----------
 src/libcamera/software_isp/debayer_cpu.cpp  |  7 +++++--
 src/libcamera/software_isp/debayer_cpu.h    |  3 ++-
 src/libcamera/software_isp/software_isp.cpp |  3 ++-
 5 files changed, 22 insertions(+), 15 deletions(-)

Comments

Laurent Pinchart Sept. 21, 2025, 2:49 a.m. UTC | #1
Hi Milan,

Thank you for the patch.

On Fri, Sep 12, 2025 at 04:29:11PM +0200, Milan Zamazal wrote:
> On some platforms, working directly on the input buffer is very slow due
> to disabled caching.  This is why we copy the input buffer into standard
> (cached) memory.  This is an unnecessary overhead on platforms with
> cached buffers.
> 
> Let's make input buffer copying configurable.  The default is still
> copying, as its overhead is much lower than contingent operations on
> non-cached memory.  Ideally, we should improve this in future to set the
> default to non-copying if we can be sure under observable circumstances
> that we are working with cached buffers.

Yes, that should be automatic in the future. I suppose it replaces TODO
#6 :-)

> Completes software ISP TODO #6.
> 
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  Documentation/runtime_configuration.rst     | 13 +++++++++++++
>  src/libcamera/software_isp/TODO             | 11 -----------
>  src/libcamera/software_isp/debayer_cpu.cpp  |  7 +++++--
>  src/libcamera/software_isp/debayer_cpu.h    |  3 ++-
>  src/libcamera/software_isp/software_isp.cpp |  3 ++-
>  5 files changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst
> index 8792232d7..6610e8bec 100644
> --- a/Documentation/runtime_configuration.rst
> +++ b/Documentation/runtime_configuration.rst
> @@ -55,6 +55,8 @@ file structure:
>          supported_devices:
>          - driver: # driver name, e.g. `mxc-isi`
>            software_isp: # true/false
> +    software_isp:
> +      copy_input_buffer: # true/false

It may be useful to set this option on a per-camera basis, but I would
prefer working on automating the default instead. In any case, that's
for later.

>  
>  Configuration file example
>  --------------------------
> @@ -88,6 +90,8 @@ Configuration file example
>           supported_devices:
>           - driver: mxc-isi
>             software_isp: true
> +     software_isp:
> +       copy_input_buffer: false
>  
>  List of variables and configuration options
>  -------------------------------------------
> @@ -154,6 +158,15 @@ pipelines.simple.supported_devices.driver, pipelines.simple.supported_devices.so
>  
>     Example `software_isp` 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
> +   processing on platforms with non-cached buffers. It can be set to
> +   false on platforms with cached buffers to avoid an unnecessary
> +   overhead.
> +
> +   Example value: ``false``
> +
>  Further details
>  ---------------
>  
> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO
> index a50db668e..2c919f442 100644
> --- a/src/libcamera/software_isp/TODO
> +++ b/src/libcamera/software_isp/TODO
> @@ -71,17 +71,6 @@ per-frame buffers like we do for hardware ISPs.
>  
>  ---
>  
> -6. Input buffer copying configuration
> -
> -> DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
> -> 	: stats_(std::move(stats)), gammaCorrection_(1.0)
> -> {
> -> 	enableInputMemcpy_ = true;
> -
> -Set this appropriately and/or make it configurable.
> -
> ----
> -
>  7. Performance measurement configuration
>  
>  > void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index 66f6038c1..cec6cc6be 100644
> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> @@ -24,6 +24,7 @@
>  #include "libcamera/internal/bayer_format.h"
>  #include "libcamera/internal/dma_buf_allocator.h"
>  #include "libcamera/internal/framebuffer.h"
> +#include "libcamera/internal/global_configuration.h"
>  #include "libcamera/internal/mapped_framebuffer.h"
>  
>  namespace libcamera {
> @@ -38,8 +39,9 @@ namespace libcamera {
>  /**
>   * \brief Constructs a DebayerCpu object
>   * \param[in] stats Pointer to the stats object to use
> + * \param[in] configuration The global configuration
>   */
> -DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
> +DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats, const GlobalConfiguration &configuration)
>  	: stats_(std::move(stats))
>  {
>  	/*
> @@ -50,7 +52,8 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
>  	 * always set it to true as the safer choice but this should be changed in
>  	 * future.
>  	 */

I'll add a

	/*
	 * \todo Make memcpy automatic based on runtime detection of platform
	 * capabilities.
	 */

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> -	enableInputMemcpy_ = true;
> +	enableInputMemcpy_ =
> +		configuration.option<bool>({ "software_isp", "copy_input_buffer" }).value_or(true);
>  
>  	/* Initialize color lookup tables */
>  	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
> index 926195e98..2f35aa18b 100644
> --- a/src/libcamera/software_isp/debayer_cpu.h
> +++ b/src/libcamera/software_isp/debayer_cpu.h
> @@ -18,6 +18,7 @@
>  #include <libcamera/base/object.h>
>  
>  #include "libcamera/internal/bayer_format.h"
> +#include "libcamera/internal/global_configuration.h"
>  
>  #include "debayer.h"
>  #include "swstats_cpu.h"
> @@ -27,7 +28,7 @@ namespace libcamera {
>  class DebayerCpu : public Debayer, public Object
>  {
>  public:
> -	DebayerCpu(std::unique_ptr<SwStatsCpu> stats);
> +	DebayerCpu(std::unique_ptr<SwStatsCpu> stats, const GlobalConfiguration &configuration);
>  	~DebayerCpu();
>  
>  	int configure(const StreamConfiguration &inputCfg,
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index 28e2a360e..b7651b7d2 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -114,7 +114,8 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,
>  	}
>  	stats->statsReady.connect(this, &SoftwareIsp::statsReady);
>  
> -	debayer_ = std::make_unique<DebayerCpu>(std::move(stats));
> +	const GlobalConfiguration &configuration = pipe->cameraManager()->_d()->configuration();
> +	debayer_ = std::make_unique<DebayerCpu>(std::move(stats), configuration);
>  	debayer_->inputBufferReady.connect(this, &SoftwareIsp::inputReady);
>  	debayer_->outputBufferReady.connect(this, &SoftwareIsp::outputReady);
>

Patch
diff mbox series

diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst
index 8792232d7..6610e8bec 100644
--- a/Documentation/runtime_configuration.rst
+++ b/Documentation/runtime_configuration.rst
@@ -55,6 +55,8 @@  file structure:
         supported_devices:
         - driver: # driver name, e.g. `mxc-isi`
           software_isp: # true/false
+    software_isp:
+      copy_input_buffer: # true/false
 
 Configuration file example
 --------------------------
@@ -88,6 +90,8 @@  Configuration file example
          supported_devices:
          - driver: mxc-isi
            software_isp: true
+     software_isp:
+       copy_input_buffer: false
 
 List of variables and configuration options
 -------------------------------------------
@@ -154,6 +158,15 @@  pipelines.simple.supported_devices.driver, pipelines.simple.supported_devices.so
 
    Example `software_isp` 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
+   processing on platforms with non-cached buffers. It can be set to
+   false on platforms with cached buffers to avoid an unnecessary
+   overhead.
+
+   Example value: ``false``
+
 Further details
 ---------------
 
diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO
index a50db668e..2c919f442 100644
--- a/src/libcamera/software_isp/TODO
+++ b/src/libcamera/software_isp/TODO
@@ -71,17 +71,6 @@  per-frame buffers like we do for hardware ISPs.
 
 ---
 
-6. Input buffer copying configuration
-
-> DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
-> 	: stats_(std::move(stats)), gammaCorrection_(1.0)
-> {
-> 	enableInputMemcpy_ = true;
-
-Set this appropriately and/or make it configurable.
-
----
-
 7. Performance measurement configuration
 
 > void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)
diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
index 66f6038c1..cec6cc6be 100644
--- a/src/libcamera/software_isp/debayer_cpu.cpp
+++ b/src/libcamera/software_isp/debayer_cpu.cpp
@@ -24,6 +24,7 @@ 
 #include "libcamera/internal/bayer_format.h"
 #include "libcamera/internal/dma_buf_allocator.h"
 #include "libcamera/internal/framebuffer.h"
+#include "libcamera/internal/global_configuration.h"
 #include "libcamera/internal/mapped_framebuffer.h"
 
 namespace libcamera {
@@ -38,8 +39,9 @@  namespace libcamera {
 /**
  * \brief Constructs a DebayerCpu object
  * \param[in] stats Pointer to the stats object to use
+ * \param[in] configuration The global configuration
  */
-DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
+DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats, const GlobalConfiguration &configuration)
 	: stats_(std::move(stats))
 {
 	/*
@@ -50,7 +52,8 @@  DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
 	 * always set it to true as the safer choice but this should be changed in
 	 * future.
 	 */
-	enableInputMemcpy_ = true;
+	enableInputMemcpy_ =
+		configuration.option<bool>({ "software_isp", "copy_input_buffer" }).value_or(true);
 
 	/* Initialize color lookup tables */
 	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
index 926195e98..2f35aa18b 100644
--- a/src/libcamera/software_isp/debayer_cpu.h
+++ b/src/libcamera/software_isp/debayer_cpu.h
@@ -18,6 +18,7 @@ 
 #include <libcamera/base/object.h>
 
 #include "libcamera/internal/bayer_format.h"
+#include "libcamera/internal/global_configuration.h"
 
 #include "debayer.h"
 #include "swstats_cpu.h"
@@ -27,7 +28,7 @@  namespace libcamera {
 class DebayerCpu : public Debayer, public Object
 {
 public:
-	DebayerCpu(std::unique_ptr<SwStatsCpu> stats);
+	DebayerCpu(std::unique_ptr<SwStatsCpu> stats, const GlobalConfiguration &configuration);
 	~DebayerCpu();
 
 	int configure(const StreamConfiguration &inputCfg,
diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
index 28e2a360e..b7651b7d2 100644
--- a/src/libcamera/software_isp/software_isp.cpp
+++ b/src/libcamera/software_isp/software_isp.cpp
@@ -114,7 +114,8 @@  SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,
 	}
 	stats->statsReady.connect(this, &SoftwareIsp::statsReady);
 
-	debayer_ = std::make_unique<DebayerCpu>(std::move(stats));
+	const GlobalConfiguration &configuration = pipe->cameraManager()->_d()->configuration();
+	debayer_ = std::make_unique<DebayerCpu>(std::move(stats), configuration);
 	debayer_->inputBufferReady.connect(this, &SoftwareIsp::inputReady);
 	debayer_->outputBufferReady.connect(this, &SoftwareIsp::outputReady);