| Message ID | 20250729073201.5369-8-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Milan, Thanks for the patch. Quoting Milan Zamazal (2025-07-29 16:31:55) > This patch allows enabling or disabling software ISP in runtime in > addition to compile time. This can be useful for software ISP testing > on various platforms as well as for overriding the defaults in case the > defaults don't work well (e.g. hardware ISP may or may not work on > i.MX8MP depending on the kernel and libcamera patches present in the > given system). > > The configuration is specified as follows: > > configuration: > pipelines: > simple: > supported_devices: > - driver: DRIVER-NAME > software_isp: BOOLEAN > - ... > > For example: > > configuration: > pipelines: > simple: > supported_devices: > - driver: mxc-isi > software_isp: true Ok, so you'd explicitly list the platforms that you want to use the simple pipeline handler with. > > The overall configuration of enabling or disabling software ISP may get > dropped in future but this patch is still useful in the meantime. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> The commit title should s/config:/pipeline: simple/ since this touches simple pipeline code and not global_configuration code. Also maybe s/in runtime/via config file/ ? > --- > src/libcamera/pipeline/simple/simple.cpp | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 4323472e1..70c41aed0 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -30,11 +30,13 @@ > #include <libcamera/stream.h> > > #include "libcamera/internal/camera.h" > +#include "libcamera/internal/camera_manager.h" > #include "libcamera/internal/camera_sensor.h" > #include "libcamera/internal/camera_sensor_properties.h" > #include "libcamera/internal/converter.h" > #include "libcamera/internal/delayed_controls.h" > #include "libcamera/internal/device_enumerator.h" > +#include "libcamera/internal/global_configuration.h" > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/pipeline_handler.h" > #include "libcamera/internal/software_isp/software_isp.h" > @@ -1679,6 +1681,17 @@ bool SimplePipelineHandler::matchDevice(MediaDevice *media, > } > > swIspEnabled_ = info.swIspEnabled; > + const GlobalConfiguration &configuration = cameraManager()->_d()->configuration(); > + for (GlobalConfiguration::Configuration entry : > + configuration.configuration()["pipelines"]["simple"]["supported_devices"] > + .asList()) { I was wondering why you didn't use option() but right YamlObject returns empty lists safetly. That's cool. > + auto name = entry["driver"].get<std::string>(); > + if (name == info.driver) { > + swIspEnabled_ = entry["software_isp"].get<bool>().value_or(swIspEnabled_); > + LOG(SimplePipeline, Debug) << "Overriding software ISP to " << swIspEnabled_; I think it would be nice to also print the platform/driver as well as that the override is coming from config file. Otherwise, looks good! Thanks, Paul > + break; > + } > + } > > /* Locate the sensors. */ > std::vector<MediaEntity *> sensors = locateSensors(media); > -- > 2.50.1 >
Hi Paul, thank you for review. Paul Elder <paul.elder@ideasonboard.com> writes: > Hi Milan, > > Thanks for the patch. > > Quoting Milan Zamazal (2025-07-29 16:31:55) >> This patch allows enabling or disabling software ISP in runtime in >> addition to compile time. This can be useful for software ISP testing >> on various platforms as well as for overriding the defaults in case the >> defaults don't work well (e.g. hardware ISP may or may not work on >> i.MX8MP depending on the kernel and libcamera patches present in the >> given system). >> >> The configuration is specified as follows: >> >> configuration: >> pipelines: >> simple: >> supported_devices: >> - driver: DRIVER-NAME >> software_isp: BOOLEAN >> - ... >> >> For example: >> >> configuration: >> pipelines: >> simple: >> supported_devices: >> - driver: mxc-isi >> software_isp: true > > Ok, so you'd explicitly list the platforms that you want to use the simple > pipeline handler with. Or not to use, the primary intended purpose is to allow overriding the hard-wired defaults. >> >> The overall configuration of enabling or disabling software ISP may get >> dropped in future but this patch is still useful in the meantime. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > > The commit title should s/config:/pipeline: simple/ since this touches simple > pipeline code and not global_configuration code. OK. > Also maybe s/in runtime/via config file/ ? OK. >> --- >> src/libcamera/pipeline/simple/simple.cpp | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index 4323472e1..70c41aed0 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -30,11 +30,13 @@ >> #include <libcamera/stream.h> >> >> #include "libcamera/internal/camera.h" >> +#include "libcamera/internal/camera_manager.h" >> #include "libcamera/internal/camera_sensor.h" >> #include "libcamera/internal/camera_sensor_properties.h" >> #include "libcamera/internal/converter.h" >> #include "libcamera/internal/delayed_controls.h" >> #include "libcamera/internal/device_enumerator.h" >> +#include "libcamera/internal/global_configuration.h" >> #include "libcamera/internal/media_device.h" >> #include "libcamera/internal/pipeline_handler.h" >> #include "libcamera/internal/software_isp/software_isp.h" >> @@ -1679,6 +1681,17 @@ bool SimplePipelineHandler::matchDevice(MediaDevice *media, >> } >> >> swIspEnabled_ = info.swIspEnabled; >> + const GlobalConfiguration &configuration = cameraManager()->_d()->configuration(); >> + for (GlobalConfiguration::Configuration entry : >> + configuration.configuration()["pipelines"]["simple"]["supported_devices"] >> + .asList()) { > > I was wondering why you didn't use option() but right YamlObject returns empty > lists safetly. That's cool. > >> + auto name = entry["driver"].get<std::string>(); >> + if (name == info.driver) { >> + swIspEnabled_ = entry["software_isp"].get<bool>().value_or(swIspEnabled_); >> + LOG(SimplePipeline, Debug) << "Overriding software ISP to " << swIspEnabled_; > > I think it would be nice to also print the platform/driver as well as that the > override is coming from config file. Good idea, I'll add that. > > Otherwise, looks good! > > Thanks, > > Paul > >> + break; >> + } >> + } >> >> /* Locate the sensors. */ >> std::vector<MediaEntity *> sensors = locateSensors(media); >> -- >> 2.50.1 >>
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 4323472e1..70c41aed0 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -30,11 +30,13 @@ #include <libcamera/stream.h> #include "libcamera/internal/camera.h" +#include "libcamera/internal/camera_manager.h" #include "libcamera/internal/camera_sensor.h" #include "libcamera/internal/camera_sensor_properties.h" #include "libcamera/internal/converter.h" #include "libcamera/internal/delayed_controls.h" #include "libcamera/internal/device_enumerator.h" +#include "libcamera/internal/global_configuration.h" #include "libcamera/internal/media_device.h" #include "libcamera/internal/pipeline_handler.h" #include "libcamera/internal/software_isp/software_isp.h" @@ -1679,6 +1681,17 @@ bool SimplePipelineHandler::matchDevice(MediaDevice *media, } swIspEnabled_ = info.swIspEnabled; + const GlobalConfiguration &configuration = cameraManager()->_d()->configuration(); + for (GlobalConfiguration::Configuration entry : + configuration.configuration()["pipelines"]["simple"]["supported_devices"] + .asList()) { + auto name = entry["driver"].get<std::string>(); + if (name == info.driver) { + swIspEnabled_ = entry["software_isp"].get<bool>().value_or(swIspEnabled_); + LOG(SimplePipeline, Debug) << "Overriding software ISP to " << swIspEnabled_; + break; + } + } /* Locate the sensors. */ std::vector<MediaEntity *> sensors = locateSensors(media);
This patch allows enabling or disabling software ISP in runtime in addition to compile time. This can be useful for software ISP testing on various platforms as well as for overriding the defaults in case the defaults don't work well (e.g. hardware ISP may or may not work on i.MX8MP depending on the kernel and libcamera patches present in the given system). The configuration is specified as follows: configuration: pipelines: simple: supported_devices: - driver: DRIVER-NAME software_isp: BOOLEAN - ... For example: configuration: pipelines: simple: supported_devices: - driver: mxc-isi software_isp: true The overall configuration of enabling or disabling software ISP may get dropped in future but this patch is still useful in the meantime. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/libcamera/pipeline/simple/simple.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+)