| Message ID | 20260831144807.654907-1-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, thank you for the patch (for whatever reason I can't see it in patchwork). Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > Commit a19f72569558 ("libcamera: simple: Set the number of software ISP streams to 2") > introduced an assertion to ensure that no converter is present when using the > software-isp. However, it was not noticed that the earlier commit 4a5ebcf675a4 > ("libcamera: simple: Enable SoftISP for imx7-csi") enabled software-isp for > "imx7-csi", and since that driver also has an associated converter, and if it > is found, the assertion will abort. > > To resolve the above situation, apply the following rules when deciding > whether to use the software-isp or one of the available converters: > > 1. if the configuration file selects `software_isp: true`, then use that; > 2. if at least one converter is available, use the first available one; > 3. if the configuration file does not disable the software-isp > and the static configuration allows it, use that; > 4. otherwise, continue without either > > Link: https://patchwork.libcamera.org/patch/27955/ > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 41 ++++++++++++++++-------- > 1 file changed, 28 insertions(+), 13 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index ca98e74790..991f542c5d 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -250,7 +250,7 @@ struct SimplePipelineInfo { > * > * The Software ISP can't be used together with the converters. > */ > - bool swIspEnabled; > + bool swIspAllowed; > }; > > namespace { > @@ -459,7 +459,7 @@ private: > std::map<const MediaEntity *, EntityData> entities_; > > std::shared_ptr<MediaDevice> converter_; > - bool swIspEnabled_; > + bool swIspEnabled_ = false; > }; > > /* ----------------------------------------------------------------------------- > @@ -1869,40 +1869,55 @@ bool SimplePipelineHandler::matchDevice(std::shared_ptr<MediaDevice> media, > const SimplePipelineInfo &info, > DeviceEnumerator *enumerator) > { > - unsigned int numStreams = 1; > - > + struct { > + std::shared_ptr<MediaDevice> dev; > + unsigned int streams; > + } converter = {}; > for (const auto &[name, streams] : info.converters) { > DeviceMatch converterMatch(name); > - converter_ = acquireMediaDevice(enumerator, converterMatch); > - if (converter_) { > - numStreams = streams; > + converter.dev = acquireMediaDevice(enumerator, converterMatch); > + if (converter.dev) { > + converter.streams = streams; > break; > } > } > > - swIspEnabled_ = info.swIspEnabled; > + std::optional<bool> swIspEnabled; > const GlobalConfiguration &configuration = cameraManager()->_d()->configuration(); > for (const ValueNode &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_); > + if (name != info.driver) > + continue; > + > + swIspEnabled = entry["software_isp"].get<bool>(); > + if (swIspEnabled) { > LOG(SimplePipeline, Debug) > << "Configuration file overrides software ISP for " > - << info.driver << " to " << swIspEnabled_; > + << info.driver << " to " << *swIspEnabled; > break; > } > } > > - if (swIspEnabled_) { > + const bool useSwIsp = > + (swIspEnabled && *swIspEnabled) || /* forced by configuration */ > + (info.swIspAllowed && /* allowed by static configuration */ > + !converter.dev && /* and there is no converter */ > + !(swIspEnabled && !*swIspEnabled)); /* and not disabled by configuration */ I wonder whether the last condition could be written in a less scary way, e.g. swIspEnabled.value_or(true) or even (not recommended but possible due to the first condition) !swIspEnabled Up to you, either way: Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > + > + unsigned int numStreams = 1; /* Only 1 "raw" stream by default. */ > + if (useSwIsp) { > /* > * When the software ISP is enabled, the simple pipeline handler > * exposes the raw stream, giving a total of two streams. This > * is mutually exclusive with the presence of a converter. > */ > - ASSERT(!converter_); > numStreams = 2; > + swIspEnabled_ = true; > + } else if (converter.dev) { > + converter_ = std::move(converter.dev); > + numStreams = converter.streams; > } > > /* Locate the sensors. */
Hi Barnabás, Thank you for the patch — and for picking this up. I tested it on platform supporting imx7-csi. The capture and lc-compliance run correctly. Tested-by: Alexi Birlinger <alexi.birlinger@nxp.com> Thanks, Alexi On 8/31/26 16:48, Barnabás Pőcze wrote: > Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button > > > Commit a19f72569558 ("libcamera: simple: Set the number of software ISP streams to 2") > introduced an assertion to ensure that no converter is present when using the > software-isp. However, it was not noticed that the earlier commit 4a5ebcf675a4 > ("libcamera: simple: Enable SoftISP for imx7-csi") enabled software-isp for > "imx7-csi", and since that driver also has an associated converter, and if it > is found, the assertion will abort. > > To resolve the above situation, apply the following rules when deciding > whether to use the software-isp or one of the available converters: > > 1. if the configuration file selects `software_isp: true`, then use that; > 2. if at least one converter is available, use the first available one; > 3. if the configuration file does not disable the software-isp > and the static configuration allows it, use that; > 4. otherwise, continue without either > > Link: https://patchwork.libcamera.org/patch/27955/ > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 41 ++++++++++++++++-------- > 1 file changed, 28 insertions(+), 13 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index ca98e74790..991f542c5d 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -250,7 +250,7 @@ struct SimplePipelineInfo { > * > * The Software ISP can't be used together with the converters. > */ > - bool swIspEnabled; > + bool swIspAllowed; > }; > > namespace { > @@ -459,7 +459,7 @@ private: > std::map<const MediaEntity *, EntityData> entities_; > > std::shared_ptr<MediaDevice> converter_; > - bool swIspEnabled_; > + bool swIspEnabled_ = false; > }; > > /* ----------------------------------------------------------------------------- > @@ -1869,40 +1869,55 @@ bool SimplePipelineHandler::matchDevice(std::shared_ptr<MediaDevice> media, > const SimplePipelineInfo &info, > DeviceEnumerator *enumerator) > { > - unsigned int numStreams = 1; > - > + struct { > + std::shared_ptr<MediaDevice> dev; > + unsigned int streams; > + } converter = {}; > for (const auto &[name, streams] : info.converters) { > DeviceMatch converterMatch(name); > - converter_ = acquireMediaDevice(enumerator, converterMatch); > - if (converter_) { > - numStreams = streams; > + converter.dev = acquireMediaDevice(enumerator, converterMatch); > + if (converter.dev) { > + converter.streams = streams; > break; > } > } > > - swIspEnabled_ = info.swIspEnabled; > + std::optional<bool> swIspEnabled; > const GlobalConfiguration &configuration = cameraManager()->_d()->configuration(); > for (const ValueNode &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_); > + if (name != info.driver) > + continue; > + > + swIspEnabled = entry["software_isp"].get<bool>(); > + if (swIspEnabled) { > LOG(SimplePipeline, Debug) > << "Configuration file overrides software ISP for " > - << info.driver << " to " << swIspEnabled_; > + << info.driver << " to " << *swIspEnabled; > break; > } > } > > - if (swIspEnabled_) { > + const bool useSwIsp = > + (swIspEnabled && *swIspEnabled) || /* forced by configuration */ > + (info.swIspAllowed && /* allowed by static configuration */ > + !converter.dev && /* and there is no converter */ > + !(swIspEnabled && !*swIspEnabled)); /* and not disabled by configuration */ > + > + unsigned int numStreams = 1; /* Only 1 "raw" stream by default. */ > + if (useSwIsp) { > /* > * When the software ISP is enabled, the simple pipeline handler > * exposes the raw stream, giving a total of two streams. This > * is mutually exclusive with the presence of a converter. > */ > - ASSERT(!converter_); > numStreams = 2; > + swIspEnabled_ = true; > + } else if (converter.dev) { > + converter_ = std::move(converter.dev); > + numStreams = converter.streams; > } > > /* Locate the sensors. */ > -- > 2.55.0 >
2026. 08. 31. 21:05 keltezéssel, Milan Zamazal írta: > Hi Barnabás, > > thank you for the patch (for whatever reason I can't see it in > patchwork). > > Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > >> Commit a19f72569558 ("libcamera: simple: Set the number of software ISP streams to 2") >> introduced an assertion to ensure that no converter is present when using the >> software-isp. However, it was not noticed that the earlier commit 4a5ebcf675a4 >> ("libcamera: simple: Enable SoftISP for imx7-csi") enabled software-isp for >> "imx7-csi", and since that driver also has an associated converter, and if it >> is found, the assertion will abort. >> >> To resolve the above situation, apply the following rules when deciding >> whether to use the software-isp or one of the available converters: >> >> 1. if the configuration file selects `software_isp: true`, then use that; >> 2. if at least one converter is available, use the first available one; >> 3. if the configuration file does not disable the software-isp >> and the static configuration allows it, use that; >> 4. otherwise, continue without either >> >> Link: https://patchwork.libcamera.org/patch/27955/ >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/libcamera/pipeline/simple/simple.cpp | 41 ++++++++++++++++-------- >> 1 file changed, 28 insertions(+), 13 deletions(-) >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index ca98e74790..991f542c5d 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -250,7 +250,7 @@ struct SimplePipelineInfo { >> * >> * The Software ISP can't be used together with the converters. >> */ >> - bool swIspEnabled; >> + bool swIspAllowed; >> }; >> >> namespace { >> @@ -459,7 +459,7 @@ private: >> std::map<const MediaEntity *, EntityData> entities_; >> >> std::shared_ptr<MediaDevice> converter_; >> - bool swIspEnabled_; >> + bool swIspEnabled_ = false; >> }; >> >> /* ----------------------------------------------------------------------------- >> @@ -1869,40 +1869,55 @@ bool SimplePipelineHandler::matchDevice(std::shared_ptr<MediaDevice> media, >> const SimplePipelineInfo &info, >> DeviceEnumerator *enumerator) >> { >> - unsigned int numStreams = 1; >> - >> + struct { >> + std::shared_ptr<MediaDevice> dev; >> + unsigned int streams; >> + } converter = {}; >> for (const auto &[name, streams] : info.converters) { >> DeviceMatch converterMatch(name); >> - converter_ = acquireMediaDevice(enumerator, converterMatch); >> - if (converter_) { >> - numStreams = streams; >> + converter.dev = acquireMediaDevice(enumerator, converterMatch); >> + if (converter.dev) { >> + converter.streams = streams; >> break; >> } >> } >> >> - swIspEnabled_ = info.swIspEnabled; >> + std::optional<bool> swIspEnabled; >> const GlobalConfiguration &configuration = cameraManager()->_d()->configuration(); >> for (const ValueNode &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_); >> + if (name != info.driver) >> + continue; >> + >> + swIspEnabled = entry["software_isp"].get<bool>(); >> + if (swIspEnabled) { >> LOG(SimplePipeline, Debug) >> << "Configuration file overrides software ISP for " >> - << info.driver << " to " << swIspEnabled_; >> + << info.driver << " to " << *swIspEnabled; >> break; >> } >> } >> >> - if (swIspEnabled_) { >> + const bool useSwIsp = >> + (swIspEnabled && *swIspEnabled) || /* forced by configuration */ >> + (info.swIspAllowed && /* allowed by static configuration */ >> + !converter.dev && /* and there is no converter */ >> + !(swIspEnabled && !*swIspEnabled)); /* and not disabled by configuration */ > > I wonder whether the last condition could be written in a less scary > way, e.g. > > swIspEnabled.value_or(true) `swIspEnabled.value_or(false)` could be used for the first, and `swIspEnabled.value_or(true)` for the last. Maybe it's better, I'm not sure yet. > > or even (not recommended but possible due to the first condition) > > !swIspEnabled Yeah I discarded this option because it seems harder to interpret. > > Up to you, either way: > > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > >> + >> + unsigned int numStreams = 1; /* Only 1 "raw" stream by default. */ >> + if (useSwIsp) { >> /* >> * When the software ISP is enabled, the simple pipeline handler >> * exposes the raw stream, giving a total of two streams. This >> * is mutually exclusive with the presence of a converter. >> */ >> - ASSERT(!converter_); >> numStreams = 2; >> + swIspEnabled_ = true; >> + } else if (converter.dev) { >> + converter_ = std::move(converter.dev); >> + numStreams = converter.streams; >> } >> >> /* Locate the sensors. */ >
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index ca98e74790..991f542c5d 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -250,7 +250,7 @@ struct SimplePipelineInfo { * * The Software ISP can't be used together with the converters. */ - bool swIspEnabled; + bool swIspAllowed; }; namespace { @@ -459,7 +459,7 @@ private: std::map<const MediaEntity *, EntityData> entities_; std::shared_ptr<MediaDevice> converter_; - bool swIspEnabled_; + bool swIspEnabled_ = false; }; /* ----------------------------------------------------------------------------- @@ -1869,40 +1869,55 @@ bool SimplePipelineHandler::matchDevice(std::shared_ptr<MediaDevice> media, const SimplePipelineInfo &info, DeviceEnumerator *enumerator) { - unsigned int numStreams = 1; - + struct { + std::shared_ptr<MediaDevice> dev; + unsigned int streams; + } converter = {}; for (const auto &[name, streams] : info.converters) { DeviceMatch converterMatch(name); - converter_ = acquireMediaDevice(enumerator, converterMatch); - if (converter_) { - numStreams = streams; + converter.dev = acquireMediaDevice(enumerator, converterMatch); + if (converter.dev) { + converter.streams = streams; break; } } - swIspEnabled_ = info.swIspEnabled; + std::optional<bool> swIspEnabled; const GlobalConfiguration &configuration = cameraManager()->_d()->configuration(); for (const ValueNode &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_); + if (name != info.driver) + continue; + + swIspEnabled = entry["software_isp"].get<bool>(); + if (swIspEnabled) { LOG(SimplePipeline, Debug) << "Configuration file overrides software ISP for " - << info.driver << " to " << swIspEnabled_; + << info.driver << " to " << *swIspEnabled; break; } } - if (swIspEnabled_) { + const bool useSwIsp = + (swIspEnabled && *swIspEnabled) || /* forced by configuration */ + (info.swIspAllowed && /* allowed by static configuration */ + !converter.dev && /* and there is no converter */ + !(swIspEnabled && !*swIspEnabled)); /* and not disabled by configuration */ + + unsigned int numStreams = 1; /* Only 1 "raw" stream by default. */ + if (useSwIsp) { /* * When the software ISP is enabled, the simple pipeline handler * exposes the raw stream, giving a total of two streams. This * is mutually exclusive with the presence of a converter. */ - ASSERT(!converter_); numStreams = 2; + swIspEnabled_ = true; + } else if (converter.dev) { + converter_ = std::move(converter.dev); + numStreams = converter.streams; } /* Locate the sensors. */
Commit a19f72569558 ("libcamera: simple: Set the number of software ISP streams to 2") introduced an assertion to ensure that no converter is present when using the software-isp. However, it was not noticed that the earlier commit 4a5ebcf675a4 ("libcamera: simple: Enable SoftISP for imx7-csi") enabled software-isp for "imx7-csi", and since that driver also has an associated converter, and if it is found, the assertion will abort. To resolve the above situation, apply the following rules when deciding whether to use the software-isp or one of the available converters: 1. if the configuration file selects `software_isp: true`, then use that; 2. if at least one converter is available, use the first available one; 3. if the configuration file does not disable the software-isp and the static configuration allows it, use that; 4. otherwise, continue without either Link: https://patchwork.libcamera.org/patch/27955/ Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/libcamera/pipeline/simple/simple.cpp | 41 ++++++++++++++++-------- 1 file changed, 28 insertions(+), 13 deletions(-)