| Message ID | 20251015012251.17508-35-bryan.odonoghue@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Bryan, Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > If GPUISP support is available make it so an environment variable can > switch it on. > > Given we don't have full feature parity with CPUISP just yet on pixel > format output, we should default to CPUISP mode giving the user the option > to switch on GPUISP by setting LIBCAMERA_SOFTISP_MODE=gpu > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > src/libcamera/software_isp/software_isp.cpp | 19 ++++++++++++++++++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > index 1f984a52..869f7320 100644 > --- a/src/libcamera/software_isp/software_isp.cpp > +++ b/src/libcamera/software_isp/software_isp.cpp > @@ -15,6 +15,7 @@ > > #include <libcamera/base/log.h> > #include <libcamera/base/thread.h> > +#include <libcamera/base/utils.h> > > #include <libcamera/controls.h> > #include <libcamera/formats.h> > @@ -25,6 +26,9 @@ > #include "libcamera/internal/software_isp/debayer_params.h" > > #include "debayer_cpu.h" > +#if HAVE_DEBAYER_EGL > +#include "debayer_egl.h" > +#endif > > /** > * \file software_isp.cpp > @@ -116,7 +120,20 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, > } > stats->statsReady.connect(this, &SoftwareIsp::statsReady); > > - debayer_ = std::make_unique<DebayerCpu>(std::move(stats), configuration); > +#if HAVE_DEBAYER_EGL > + const char *softISPMode = utils::secure_getenv("LIBCAMERA_SOFTISP_MODE"); s/softISPMode/softIspMode/ I think. It would be nice to have a configuration file option for this. We already have `configuration' here, so I think something like std::optional<std::string> softIspMode = configuration.envOption("LIBCAMERA_SOFTISP_MODE", { "software_isp", "mode" }); if (softIspMode) { if (softIspMode != "gpu" && softIspMode != "cpu") { LOG(SoftwareIsp, Error) << "LIBCAMERA_SOFISP_MODE " << softIspMode.value() << " invalid. " << "must be \"cpu\" or \"gpu\""; return; } } #if HAVE_DEBAYER_EGL if (!softIspMode || softIspMode != "gpu") debayer_ = std::make_unique<DebayerEGL>(std::move(stats), configuration); #endif should do the job (not tested). In any case, the environment variable/option should be documented in Documentation/runtime_configuration.rst. > + > + if (softISPMode && !strcmp(softISPMode, "gpu")) > + debayer_ = std::make_unique<DebayerEGL>(std::move(stats), configuration); > +#endif > + if (!debayer_) > + debayer_ = std::make_unique<DebayerCpu>(std::move(stats), configuration); > + > + if (!debayer_) { > + LOG(SoftwareIsp, Error) << "Failed to create Debayer object"; > + return; > + } > + > debayer_->inputBufferReady.connect(this, &SoftwareIsp::inputReady); > debayer_->outputBufferReady.connect(this, &SoftwareIsp::outputReady);
Milan Zamazal <mzamazal@redhat.com> writes: > Hi Bryan, > > Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > >> If GPUISP support is available make it so an environment variable can >> switch it on. >> >> Given we don't have full feature parity with CPUISP just yet on pixel >> format output, we should default to CPUISP mode giving the user the option >> to switch on GPUISP by setting LIBCAMERA_SOFTISP_MODE=gpu >> >> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> --- >> src/libcamera/software_isp/software_isp.cpp | 19 ++++++++++++++++++- >> 1 file changed, 18 insertions(+), 1 deletion(-) >> >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp >> index 1f984a52..869f7320 100644 >> --- a/src/libcamera/software_isp/software_isp.cpp >> +++ b/src/libcamera/software_isp/software_isp.cpp >> @@ -15,6 +15,7 @@ >> >> #include <libcamera/base/log.h> >> #include <libcamera/base/thread.h> >> +#include <libcamera/base/utils.h> >> >> #include <libcamera/controls.h> >> #include <libcamera/formats.h> >> @@ -25,6 +26,9 @@ >> #include "libcamera/internal/software_isp/debayer_params.h" >> >> #include "debayer_cpu.h" >> +#if HAVE_DEBAYER_EGL >> +#include "debayer_egl.h" >> +#endif >> >> /** >> * \file software_isp.cpp >> @@ -116,7 +120,20 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, >> } >> stats->statsReady.connect(this, &SoftwareIsp::statsReady); >> >> - debayer_ = std::make_unique<DebayerCpu>(std::move(stats), configuration); >> +#if HAVE_DEBAYER_EGL >> + const char *softISPMode = utils::secure_getenv("LIBCAMERA_SOFTISP_MODE"); > > s/softISPMode/softIspMode/ I think. > > It would be nice to have a configuration file option for this. We > already have `configuration' here, so I think something like > > std::optional<std::string> softIspMode = configuration.envOption("LIBCAMERA_SOFTISP_MODE", { "software_isp", "mode" }); > if (softIspMode) { > if (softIspMode != "gpu" && softIspMode != "cpu") { > LOG(SoftwareIsp, Error) << "LIBCAMERA_SOFISP_MODE " << softIspMode.value() << " invalid. " > << "must be \"cpu\" or \"gpu\""; Either s/must/Must/, or s/invalid./invalid,/. > return; > } > } > > #if HAVE_DEBAYER_EGL > if (!softIspMode || softIspMode != "gpu") > debayer_ = std::make_unique<DebayerEGL>(std::move(stats), configuration); > #endif > > should do the job (not tested). Tested now, works for me. > In any case, the environment variable/option should be documented in > Documentation/runtime_configuration.rst. > >> + >> + if (softISPMode && !strcmp(softISPMode, "gpu")) >> + debayer_ = std::make_unique<DebayerEGL>(std::move(stats), configuration); >> +#endif >> + if (!debayer_) >> + debayer_ = std::make_unique<DebayerCpu>(std::move(stats), configuration); >> + >> + if (!debayer_) { >> + LOG(SoftwareIsp, Error) << "Failed to create Debayer object"; >> + return; >> + } >> + >> debayer_->inputBufferReady.connect(this, &SoftwareIsp::inputReady); >> debayer_->outputBufferReady.connect(this, &SoftwareIsp::outputReady);
diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp index 1f984a52..869f7320 100644 --- a/src/libcamera/software_isp/software_isp.cpp +++ b/src/libcamera/software_isp/software_isp.cpp @@ -15,6 +15,7 @@ #include <libcamera/base/log.h> #include <libcamera/base/thread.h> +#include <libcamera/base/utils.h> #include <libcamera/controls.h> #include <libcamera/formats.h> @@ -25,6 +26,9 @@ #include "libcamera/internal/software_isp/debayer_params.h" #include "debayer_cpu.h" +#if HAVE_DEBAYER_EGL +#include "debayer_egl.h" +#endif /** * \file software_isp.cpp @@ -116,7 +120,20 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, } stats->statsReady.connect(this, &SoftwareIsp::statsReady); - debayer_ = std::make_unique<DebayerCpu>(std::move(stats), configuration); +#if HAVE_DEBAYER_EGL + const char *softISPMode = utils::secure_getenv("LIBCAMERA_SOFTISP_MODE"); + + if (softISPMode && !strcmp(softISPMode, "gpu")) + debayer_ = std::make_unique<DebayerEGL>(std::move(stats), configuration); +#endif + if (!debayer_) + debayer_ = std::make_unique<DebayerCpu>(std::move(stats), configuration); + + if (!debayer_) { + LOG(SoftwareIsp, Error) << "Failed to create Debayer object"; + return; + } + debayer_->inputBufferReady.connect(this, &SoftwareIsp::inputReady); debayer_->outputBufferReady.connect(this, &SoftwareIsp::outputReady);
If GPUISP support is available make it so an environment variable can switch it on. Given we don't have full feature parity with CPUISP just yet on pixel format output, we should default to CPUISP mode giving the user the option to switch on GPUISP by setting LIBCAMERA_SOFTISP_MODE=gpu Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- src/libcamera/software_isp/software_isp.cpp | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)