| Message ID | 20190915202318.5825-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thank you for the patch. On Sun, Sep 15, 2019 at 11:23:18PM +0300, Laurent Pinchart wrote: > When enumerating IPAs, the system IPA directory and all the directories > listed in the LIBCAMERA_IPA_MODULE_PATH environment variable are listed > in turn. Failing to list any of those directories results in an error > message being printed for every failure. This is particularly common > when developing libcamera, as IPAs may not have been installed locally. > > To avoid unnecessarily worrying error messages, rework the enumeration > procedure to only print a message when no IPA can be found at all. > Individual missing directories are not considered as an issue anymore. > The message is also downgraded from an error to a warning as the > situation may still be normal. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/ipa_manager.cpp | 24 ++++++++++++++++++------ > 1 file changed, 18 insertions(+), 6 deletions(-) > > diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp > index 0a908eae6261..f3e08375820c 100644 > --- a/src/libcamera/ipa_manager.cpp > +++ b/src/libcamera/ipa_manager.cpp > @@ -94,11 +94,19 @@ LOG_DEFINE_CATEGORY(IPAManager) > > IPAManager::IPAManager() > { > - addDir(IPA_MODULE_DIR); > + unsigned int ipaCount = 0; > + int ret; > + > + ret = addDir(IPA_MODULE_DIR); > + if (ret > 0) > + ipaCount += ret; > const char *modulePaths = utils::secure_getenv("LIBCAMERA_IPA_MODULE_PATH"); > - if (!modulePaths) > + if (!modulePaths) { > + LOG(IPAManager, Warning) > + << "No IPA found in '" IPA_MODULE_DIR "'"; We need to check ipaCount here, since we could have loaded some IPAs from the global dir, and just not have the environment variable specified. > return; > + } > > while (1) { > const char *delim = strchrnul(modulePaths, ':'); > @@ -106,7 +114,9 @@ IPAManager::IPAManager() > > if (count) { > std::string path(modulePaths, count); > - addDir(path.c_str()); > + ret = addDir(path.c_str()); > + if (ret > 0) > + ipaCount += ret; > } > > if (*delim == '\0') > @@ -114,6 +124,11 @@ IPAManager::IPAManager() > > modulePaths += count + 1; > } > + > + if (!ipaCount) > + LOG(IPAManager, Warning) > + << "No IPA found in '" IPA_MODULE_DIR "' and '" > + << modulePaths << "'"; I presume you're fine with modulePaths being printed as-is, as a colon-separated list of directories. > } > > IPAManager::~IPAManager() > @@ -155,9 +170,6 @@ int IPAManager::addDir(const char *libDir) > dir = opendir(libDir); > if (!dir) { > int ret = -errno; > - LOG(IPAManager, Error) > - << "Invalid path " << libDir << " for IPA modules: " > - << strerror(-ret); > return ret; Could this be shortened to return -errno; ? > } With these changes, Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> Paul
Hi Paul, On Tue, Sep 17, 2019 at 02:07:03AM -0400, Paul Elder wrote: > On Sun, Sep 15, 2019 at 11:23:18PM +0300, Laurent Pinchart wrote: > > When enumerating IPAs, the system IPA directory and all the directories > > listed in the LIBCAMERA_IPA_MODULE_PATH environment variable are listed > > in turn. Failing to list any of those directories results in an error > > message being printed for every failure. This is particularly common > > when developing libcamera, as IPAs may not have been installed locally. > > > > To avoid unnecessarily worrying error messages, rework the enumeration > > procedure to only print a message when no IPA can be found at all. > > Individual missing directories are not considered as an issue anymore. > > The message is also downgraded from an error to a warning as the > > situation may still be normal. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/ipa_manager.cpp | 24 ++++++++++++++++++------ > > 1 file changed, 18 insertions(+), 6 deletions(-) > > > > diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp > > index 0a908eae6261..f3e08375820c 100644 > > --- a/src/libcamera/ipa_manager.cpp > > +++ b/src/libcamera/ipa_manager.cpp > > @@ -94,11 +94,19 @@ LOG_DEFINE_CATEGORY(IPAManager) > > > > IPAManager::IPAManager() > > { > > - addDir(IPA_MODULE_DIR); > > + unsigned int ipaCount = 0; > > + int ret; > > + > > + ret = addDir(IPA_MODULE_DIR); > > + if (ret > 0) > > + ipaCount += ret; > > const char *modulePaths = utils::secure_getenv("LIBCAMERA_IPA_MODULE_PATH"); > > - if (!modulePaths) > > + if (!modulePaths) { > > + LOG(IPAManager, Warning) > > + << "No IPA found in '" IPA_MODULE_DIR "'"; > > We need to check ipaCount here, since we could have loaded some IPAs > from the global dir, and just not have the environment variable > specified. Oops, indeed. I'll fix this. > > return; > > + } > > > > while (1) { > > const char *delim = strchrnul(modulePaths, ':'); > > @@ -106,7 +114,9 @@ IPAManager::IPAManager() > > > > if (count) { > > std::string path(modulePaths, count); > > - addDir(path.c_str()); > > + ret = addDir(path.c_str()); > > + if (ret > 0) > > + ipaCount += ret; > > } > > > > if (*delim == '\0') > > @@ -114,6 +124,11 @@ IPAManager::IPAManager() > > > > modulePaths += count + 1; > > } > > + > > + if (!ipaCount) > > + LOG(IPAManager, Warning) > > + << "No IPA found in '" IPA_MODULE_DIR "' and '" > > + << modulePaths << "'"; > > I presume you're fine with modulePaths being printed as-is, as a > colon-separated list of directories. I think it's good enough for now, yes. > > } > > > > IPAManager::~IPAManager() > > @@ -155,9 +170,6 @@ int IPAManager::addDir(const char *libDir) > > dir = opendir(libDir); > > if (!dir) { > > int ret = -errno; > > - LOG(IPAManager, Error) > > - << "Invalid path " << libDir << " for IPA modules: " > > - << strerror(-ret); > > return ret; > > Could this be shortened to > return -errno; ? Unfortunately not, as LOG() could result in errno being modified. That's why we have these constructs through the code where we store -errno in ret. > > } > > With these changes, > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Hi Laurent, On 18/09/2019 14:14, Laurent Pinchart wrote: > Hi Paul, > > On Tue, Sep 17, 2019 at 02:07:03AM -0400, Paul Elder wrote: >> On Sun, Sep 15, 2019 at 11:23:18PM +0300, Laurent Pinchart wrote: >>> When enumerating IPAs, the system IPA directory and all the directories >>> listed in the LIBCAMERA_IPA_MODULE_PATH environment variable are listed >>> in turn. Failing to list any of those directories results in an error >>> message being printed for every failure. This is particularly common >>> when developing libcamera, as IPAs may not have been installed locally. >>> >>> To avoid unnecessarily worrying error messages, rework the enumeration >>> procedure to only print a message when no IPA can be found at all. >>> Individual missing directories are not considered as an issue anymore. >>> The message is also downgraded from an error to a warning as the >>> situation may still be normal. >>> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> src/libcamera/ipa_manager.cpp | 24 ++++++++++++++++++------ >>> 1 file changed, 18 insertions(+), 6 deletions(-) >>> >>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp >>> index 0a908eae6261..f3e08375820c 100644 >>> --- a/src/libcamera/ipa_manager.cpp >>> +++ b/src/libcamera/ipa_manager.cpp >>> @@ -94,11 +94,19 @@ LOG_DEFINE_CATEGORY(IPAManager) >>> >>> IPAManager::IPAManager() >>> { >>> - addDir(IPA_MODULE_DIR); >>> + unsigned int ipaCount = 0; >>> + int ret; >>> + >>> + ret = addDir(IPA_MODULE_DIR); >>> + if (ret > 0) >>> + ipaCount += ret; >>> const char *modulePaths = utils::secure_getenv("LIBCAMERA_IPA_MODULE_PATH"); >>> - if (!modulePaths) >>> + if (!modulePaths) { >>> + LOG(IPAManager, Warning) >>> + << "No IPA found in '" IPA_MODULE_DIR "'"; >> >> We need to check ipaCount here, since we could have loaded some IPAs >> from the global dir, and just not have the environment variable >> specified. > > Oops, indeed. I'll fix this. > >>> return; >>> + } >>> >>> while (1) { >>> const char *delim = strchrnul(modulePaths, ':'); >>> @@ -106,7 +114,9 @@ IPAManager::IPAManager() >>> >>> if (count) { >>> std::string path(modulePaths, count); >>> - addDir(path.c_str()); >>> + ret = addDir(path.c_str()); >>> + if (ret > 0) >>> + ipaCount += ret; >>> } >>> >>> if (*delim == '\0') >>> @@ -114,6 +124,11 @@ IPAManager::IPAManager() >>> >>> modulePaths += count + 1; >>> } >>> + >>> + if (!ipaCount) >>> + LOG(IPAManager, Warning) >>> + << "No IPA found in '" IPA_MODULE_DIR "' and '" >>> + << modulePaths << "'"; >> >> I presume you're fine with modulePaths being printed as-is, as a >> colon-separated list of directories. > > I think it's good enough for now, yes. > >>> } >>> >>> IPAManager::~IPAManager() >>> @@ -155,9 +170,6 @@ int IPAManager::addDir(const char *libDir) >>> dir = opendir(libDir); >>> if (!dir) { >>> int ret = -errno; >>> - LOG(IPAManager, Error) >>> - << "Invalid path " << libDir << " for IPA modules: " >>> - << strerror(-ret); >>> return ret; >> >> Could this be shortened to >> return -errno; ? > > Unfortunately not, as LOG() could result in errno being modified. That's > why we have these constructs through the code where we store -errno in > ret. You mean the LOG() which you have removed right ;-) > >>> } >> >> With these changes, >> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> >
Hi Kieran, On Wed, Sep 18, 2019 at 02:31:58PM +0100, Kieran Bingham wrote: > On 18/09/2019 14:14, Laurent Pinchart wrote: > > On Tue, Sep 17, 2019 at 02:07:03AM -0400, Paul Elder wrote: > >> On Sun, Sep 15, 2019 at 11:23:18PM +0300, Laurent Pinchart wrote: > >>> When enumerating IPAs, the system IPA directory and all the directories > >>> listed in the LIBCAMERA_IPA_MODULE_PATH environment variable are listed > >>> in turn. Failing to list any of those directories results in an error > >>> message being printed for every failure. This is particularly common > >>> when developing libcamera, as IPAs may not have been installed locally. > >>> > >>> To avoid unnecessarily worrying error messages, rework the enumeration > >>> procedure to only print a message when no IPA can be found at all. > >>> Individual missing directories are not considered as an issue anymore. > >>> The message is also downgraded from an error to a warning as the > >>> situation may still be normal. > >>> > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> --- > >>> src/libcamera/ipa_manager.cpp | 24 ++++++++++++++++++------ > >>> 1 file changed, 18 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp > >>> index 0a908eae6261..f3e08375820c 100644 > >>> --- a/src/libcamera/ipa_manager.cpp > >>> +++ b/src/libcamera/ipa_manager.cpp > >>> @@ -94,11 +94,19 @@ LOG_DEFINE_CATEGORY(IPAManager) > >>> > >>> IPAManager::IPAManager() > >>> { > >>> - addDir(IPA_MODULE_DIR); > >>> + unsigned int ipaCount = 0; > >>> + int ret; > >>> + > >>> + ret = addDir(IPA_MODULE_DIR); > >>> + if (ret > 0) > >>> + ipaCount += ret; > >>> const char *modulePaths = utils::secure_getenv("LIBCAMERA_IPA_MODULE_PATH"); > >>> - if (!modulePaths) > >>> + if (!modulePaths) { > >>> + LOG(IPAManager, Warning) > >>> + << "No IPA found in '" IPA_MODULE_DIR "'"; > >> > >> We need to check ipaCount here, since we could have loaded some IPAs > >> from the global dir, and just not have the environment variable > >> specified. > > > > Oops, indeed. I'll fix this. > > > >>> return; > >>> + } > >>> > >>> while (1) { > >>> const char *delim = strchrnul(modulePaths, ':'); > >>> @@ -106,7 +114,9 @@ IPAManager::IPAManager() > >>> > >>> if (count) { > >>> std::string path(modulePaths, count); > >>> - addDir(path.c_str()); > >>> + ret = addDir(path.c_str()); > >>> + if (ret > 0) > >>> + ipaCount += ret; > >>> } > >>> > >>> if (*delim == '\0') > >>> @@ -114,6 +124,11 @@ IPAManager::IPAManager() > >>> > >>> modulePaths += count + 1; > >>> } > >>> + > >>> + if (!ipaCount) > >>> + LOG(IPAManager, Warning) > >>> + << "No IPA found in '" IPA_MODULE_DIR "' and '" > >>> + << modulePaths << "'"; > >> > >> I presume you're fine with modulePaths being printed as-is, as a > >> colon-separated list of directories. > > > > I think it's good enough for now, yes. > > > >>> } > >>> > >>> IPAManager::~IPAManager() > >>> @@ -155,9 +170,6 @@ int IPAManager::addDir(const char *libDir) > >>> dir = opendir(libDir); > >>> if (!dir) { > >>> int ret = -errno; > >>> - LOG(IPAManager, Error) > >>> - << "Invalid path " << libDir << " for IPA modules: " > >>> - << strerror(-ret); > >>> return ret; > >> > >> Could this be shortened to > >> return -errno; ? > > > > Unfortunately not, as LOG() could result in errno being modified. That's > > why we have these constructs through the code where we store -errno in > > ret. > > You mean the LOG() which you have removed right ;-) This is embarassing... :-) I'll fix it when applying if there's no other comment to v2, or in v3 otherwise. > >>> } > >> > >> With these changes, > >> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp index 0a908eae6261..f3e08375820c 100644 --- a/src/libcamera/ipa_manager.cpp +++ b/src/libcamera/ipa_manager.cpp @@ -94,11 +94,19 @@ LOG_DEFINE_CATEGORY(IPAManager) IPAManager::IPAManager() { - addDir(IPA_MODULE_DIR); + unsigned int ipaCount = 0; + int ret; + + ret = addDir(IPA_MODULE_DIR); + if (ret > 0) + ipaCount += ret; const char *modulePaths = utils::secure_getenv("LIBCAMERA_IPA_MODULE_PATH"); - if (!modulePaths) + if (!modulePaths) { + LOG(IPAManager, Warning) + << "No IPA found in '" IPA_MODULE_DIR "'"; return; + } while (1) { const char *delim = strchrnul(modulePaths, ':'); @@ -106,7 +114,9 @@ IPAManager::IPAManager() if (count) { std::string path(modulePaths, count); - addDir(path.c_str()); + ret = addDir(path.c_str()); + if (ret > 0) + ipaCount += ret; } if (*delim == '\0') @@ -114,6 +124,11 @@ IPAManager::IPAManager() modulePaths += count + 1; } + + if (!ipaCount) + LOG(IPAManager, Warning) + << "No IPA found in '" IPA_MODULE_DIR "' and '" + << modulePaths << "'"; } IPAManager::~IPAManager() @@ -155,9 +170,6 @@ int IPAManager::addDir(const char *libDir) dir = opendir(libDir); if (!dir) { int ret = -errno; - LOG(IPAManager, Error) - << "Invalid path " << libDir << " for IPA modules: " - << strerror(-ret); return ret; }
When enumerating IPAs, the system IPA directory and all the directories listed in the LIBCAMERA_IPA_MODULE_PATH environment variable are listed in turn. Failing to list any of those directories results in an error message being printed for every failure. This is particularly common when developing libcamera, as IPAs may not have been installed locally. To avoid unnecessarily worrying error messages, rework the enumeration procedure to only print a message when no IPA can be found at all. Individual missing directories are not considered as an issue anymore. The message is also downgraded from an error to a warning as the situation may still be normal. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/ipa_manager.cpp | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)