| Message ID | 20251023105651.78395-1-isaac.scott@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi 2025. 10. 23. 12:56 keltezéssel, Isaac Scott írta: > It is possible that the base tuning file of a camera sensor may not be > applciable to all camera modules including that camera sensor. This is > especially applicable in the case of lenses, which can cause lens > shading corrections to be unapplicable in the base tuning file, or CCMs > configured for the base sensor being incorrect for a given module. > > Read the module identifier string from the > V4L2_CID_CAMERA_MODULE_IDENTIFIER control, and use it as the primary > tuning file to search for when initialising the IPA. > > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> > --- > include/libcamera/internal/camera_sensor.h | 2 ++ > include/libcamera/internal/v4l2_subdevice.h | 2 ++ > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 11 ++++-- > src/libcamera/sensor/camera_sensor.cpp | 10 ++++++ > src/libcamera/sensor/camera_sensor_legacy.cpp | 5 +++ > src/libcamera/sensor/camera_sensor_raw.cpp | 4 +++ > src/libcamera/v4l2_subdevice.cpp | 34 +++++++++++++++++++ > 7 files changed, 65 insertions(+), 3 deletions(-) > > [...] > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > index 31a2ac72a..bc4004e0e 100644 > --- a/src/libcamera/v4l2_subdevice.cpp > +++ b/src/libcamera/v4l2_subdevice.cpp > @@ -7,9 +7,11 @@ > > #include "libcamera/internal/v4l2_subdevice.h" > > +#include <cstdint> > #include <fcntl.h> > #include <sstream> > #include <string.h> > +#include <string_view> > #include <sys/ioctl.h> > #include <unistd.h> > > @@ -1743,6 +1745,38 @@ const std::string &V4L2Subdevice::model() > return model_; > } > > +/** > + * \brief Retrieve the module identifier of the device > + * > + * Where a module uses a sensor within a specific camera module, it may require a > + * specific tuning file for that particular module. This information is exposed > + * through a V4L2 control. > + * > + * \return The module identifier of the device > + */ > +const std::optional<std::string> V4L2Subdevice::module() I am a bit confused here. This function never returns an empty optional as far as I can see. I think I mentioned it earlier, but I think something like an `std::optional<std::optional<std::string>>` would be preferable: if the outer optional has a value, return it if not: initialize the inner optional to empty if no V4L2_CID_CAMERA_MODULE_IDENTIFIER, return if V4L2_CID_CAMERA_MODULE_IDENTIFIER retrieval fails, return set the inner optional to the returned string return the inner optional or something similar. In any case I think there are still two questions: * does this value need to be cached here? (CameraSensor* classes already do that) * should this be moved into the CameraSensor (base) class maybe? (since this feels very sensor specific, but maybe I'm missing something) Regards, Barnabás Pőcze > +{ > + if (module_.has_value()) > + return module_; > + > + /* Check if the control exists for a camera module identifier */ > + if (!controlInfo(V4L2_CID_CAMERA_MODULE_IDENTIFIER)) > + return ""; > + > + /* > + * Get the camera module identifier string of the module, if available > + */ > + auto sensorInfo = getControls(Span<const uint32_t>({ V4L2_CID_CAMERA_MODULE_IDENTIFIER })); > + if (sensorInfo.empty()) > + return ""; > + > + module_.emplace(sensorInfo.begin()->second.get<std::string_view>()); > + LOG(V4L2, Debug) > + << "Camera module identifier: " << module_.value(); > + > + return module_; > +} > + > /** > * \fn V4L2Subdevice::caps() > * \brief Retrieve the subdevice V4L2 capabilities > -- > 2.43.0 >
Hi 2025. 10. 23. 12:56 keltezéssel, Isaac Scott írta: > At present, it is only possible to attempt to find a tuning file at two > paths. These are often the sensor model name, and 'uncalibrated.yaml'. > > If the camera sensor has a CAMERA_MODULE_IDENTIFIER control, we should > first check for a camera module-specific tuning file, with the camera > sensor model name as its fallback tuning file path, ideally followed by > 'uncalibrated.yaml', which is not possible if the configurationFile() > function only takes two potential paths. > > To handle this problem effectively, adapt the configurationFile() > function for all pipelines and unit tests to take an initializer_list of > potential paths. > > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> > --- > include/libcamera/internal/ipa_proxy.h | 7 ++- > src/libcamera/ipa_proxy.cpp | 48 +++++++++++++------- > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- > src/libcamera/pipeline/mali-c55/mali-c55.cpp | 4 +- > src/libcamera/pipeline/vimc/vimc.cpp | 2 +- > src/libcamera/software_isp/software_isp.cpp | 2 +- > test/ipa/ipa_interface_test.cpp | 2 +- > 7 files changed, 40 insertions(+), 27 deletions(-) > > [...] > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp > index a3ccfa603..8c150eae6 100644 > --- a/src/libcamera/ipa_proxy.cpp > +++ b/src/libcamera/ipa_proxy.cpp > @@ -167,33 +167,47 @@ IPAProxy::~IPAProxy() > * \return The full path to the IPA configuration file, or an empty string if > * no configuration file can be found > */ > -std::string IPAProxy::configurationFile(const std::string &name, > - const std::string &fallbackName) const > +std::string IPAProxy::configurationFile(std::initializer_list<std::string_view> names) const > { > /* > * The IPA module name can be used as-is to build directory names as it > * has been validated when loading the module. > */ > const std::string ipaName = ipam_->info().name; > - std::string confPath = ipaConfigurationFile(ipaName, name, configPaths_); > - if (!confPath.empty()) { > - LOG(IPAProxy, Info) << "Using tuning file " << confPath; > - return confPath; > + > + /* > + * Start with any user override through the module-specific environment > + * variable. Use the name of the IPA module up to the first '/' to > + * construct the variable name. > + */ > + std::string ipaEnvName = ipaName.substr(0, ipaName.find('/')); > + std::transform(ipaEnvName.begin(), ipaEnvName.end(), ipaEnvName.begin(), > + [](unsigned char c) { return std::toupper(c); }); > + ipaEnvName = "LIBCAMERA_" + ipaEnvName + "_TUNING_FILE"; > + > + char const *configFromEnv = utils::secure_getenv(ipaEnvName.c_str()); > + if (configFromEnv && *configFromEnv != '\0') { > + LOG(IPAProxy, Info) << "Using tuning file " << configFromEnv; > + return { std::string(configFromEnv) }; > } > > - if (fallbackName.empty()) { > - LOG(IPAProxy, Error) > - << "Configuration file '" << name > - << "' not found for IPA module '" << ipaName << "'"; > - return std::string(); > + > + for (auto name : names) { > + std::string confPath = ipaConfigurationFile(ipaName, std::string(name), configPaths_); The environment variable handling was moved here, so I think it can be removed from `ipaConfigurationFile()`. > + if (!confPath.empty()) { > + LOG(IPAProxy, Info) << "Using tuning file " << confPath; > + return confPath; > + } > + > + if (&name == names.end()) { This will never be true. I think the below message should be moved to after the loop. Regards, Barnabás Pőcze > + LOG(IPAProxy, Error) > + << "Configuration file '" << name > + << "' not found for IPA module '" << ipaName << "'"; > + return std::string(); > + } > } > > - confPath = ipaConfigurationFile(ipaName, fallbackName, configPaths_); > - LOG(IPAProxy, Warning) > - << "Configuration file '" << name > - << "' not found for IPA module '" << ipaName > - << "', falling back to '" << confPath << "'"; > - return confPath; > + return std::string(); > } > > /** > [...]