| Message ID | 20250912142915.53949-11-mzamazal@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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); >
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);