| Message ID | 20250625155308.2325438-2-dan.scally@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Dan, Thank you for the patch. On Wed, Jun 25, 2025 at 04:53:07PM +0100, Daniel Scally wrote: > Some entities in a media graph have names that might differ from > implementation to implementation; for example the Camera Receiver > Unit and CSI-2 receiver on the KakiP board have entities with names > that include their address, in the form "csi-16000400.csi2". Passing > that entity name to DeviceMatch is too inflexible given it would only > work if that specific CSI-2 receiver were the one being used. > > Update DeviceMatch.add() such that the string passed to it is used to > create a std::basic_regex, and use std::regex_search() instead of a > direct string comparison in DeviceMatch::match(). This allows us to > pass a string that will form a regex expression to match to the entity > instead, for example "csi-[0-9]{8}.csi2". So you'll only support SoCs whose CSI-2 receiver are mapped to an address that contains no alphabetic digits ? :-) > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > .../libcamera/internal/device_enumerator.h | 3 ++- > src/libcamera/device_enumerator.cpp | 27 ++++++++++--------- > 2 files changed, 16 insertions(+), 14 deletions(-) > > diff --git a/include/libcamera/internal/device_enumerator.h b/include/libcamera/internal/device_enumerator.h > index db3532a9..fce7f0ea 100644 > --- a/include/libcamera/internal/device_enumerator.h > +++ b/include/libcamera/internal/device_enumerator.h > @@ -8,6 +8,7 @@ > #pragma once > > #include <memory> > +#include <regex> See commit b01918978c82 ("libcamera: v4l2_subdevice: Work around false positive warning"). > #include <string> > #include <vector> > > @@ -28,7 +29,7 @@ public: > > private: > std::string driver_; > - std::vector<std::string> entities_; > + std::vector<std::regex> entities_; > }; > > class DeviceEnumerator > diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp > index ae17862f..e22d2822 100644 > --- a/src/libcamera/device_enumerator.cpp > +++ b/src/libcamera/device_enumerator.cpp > @@ -52,15 +52,15 @@ LOG_DEFINE_CATEGORY(DeviceEnumerator) > * device enumerator to find matching media devices. > * > * A DeviceMatch is created with a specific Linux device driver in mind, > - * therefore the name of the driver is a required property. One or more Entity > - * names can be added as match criteria. > + * therefore the name of the driver is a required property. Regex expressions to > + * match one or more Entity names can be added as match criteria. > * > - * Pipeline handlers are recommended to add entities to DeviceMatch as > - * appropriate to ensure that the media device they need can be uniquely > - * identified. This is useful when the corresponding kernel driver can produce > - * different graphs, for instance as a result of different driver versions or > - * hardware configurations, and not all those graphs are suitable for a pipeline > - * handler. > + * Pipeline handlers are recommended to add regex expressions for entities to > + * DeviceMatch as appropriate to ensure that the media device they need can be > + * uniquely identified. This is useful when the corresponding kernel driver can > + * produce different graphs, for instance as a result of different driver > + * versions or hardware configurations, and not all those graphs are suitable > + * for a pipeline handler. > */ > > /** > @@ -73,12 +73,13 @@ DeviceMatch::DeviceMatch(const std::string &driver) > } > > /** > - * \brief Add a media entity name to the search pattern > - * \param[in] entity The name of the entity in the media graph > + * \brief Add a media entity regex string to the search pattern > + * \param[in] entity A regex string to match the entity in the media graph > */ > void DeviceMatch::add(const std::string &entity) > { > - entities_.push_back(entity); > + std::regex entity_regex(entity); camelCase. Same below. > + entities_.push_back(entity_regex); Construct in-place with emplace_back, don't use a local variable. > } > > /** > @@ -96,11 +97,11 @@ bool DeviceMatch::match(const MediaDevice *device) const > if (driver_ != device->driver()) > return false; > > - for (const std::string &name : entities_) { > + for (const std::regex &name_regex : entities_) { > bool found = false; > > for (const MediaEntity *entity : device->entities()) { > - if (name == entity->name()) { > + if (std::regex_search(entity->name(), name_regex)) { > if (!entity->deviceNode().empty()) { > found = true; > break;
On Thu, Jun 26, 2025 at 04:10:31PM +0300, Laurent Pinchart wrote: > Hi Dan, > > Thank you for the patch. > > On Wed, Jun 25, 2025 at 04:53:07PM +0100, Daniel Scally wrote: > > Some entities in a media graph have names that might differ from > > implementation to implementation; for example the Camera Receiver > > Unit and CSI-2 receiver on the KakiP board have entities with names > > that include their address, in the form "csi-16000400.csi2". Passing > > that entity name to DeviceMatch is too inflexible given it would only > > work if that specific CSI-2 receiver were the one being used. > > > > Update DeviceMatch.add() such that the string passed to it is used to > > create a std::basic_regex, and use std::regex_search() instead of a > > direct string comparison in DeviceMatch::match(). This allows us to > > pass a string that will form a regex expression to match to the entity > > instead, for example "csi-[0-9]{8}.csi2". > > So you'll only support SoCs whose CSI-2 receiver are mapped to an > address that contains no alphabetic digits ? :-) Also note that the '.' in the regex can now match any character. Do we need to update all users of DeviceMatch to escape '.' ? > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > > --- > > .../libcamera/internal/device_enumerator.h | 3 ++- > > src/libcamera/device_enumerator.cpp | 27 ++++++++++--------- > > 2 files changed, 16 insertions(+), 14 deletions(-) > > > > diff --git a/include/libcamera/internal/device_enumerator.h b/include/libcamera/internal/device_enumerator.h > > index db3532a9..fce7f0ea 100644 > > --- a/include/libcamera/internal/device_enumerator.h > > +++ b/include/libcamera/internal/device_enumerator.h > > @@ -8,6 +8,7 @@ > > #pragma once > > > > #include <memory> > > +#include <regex> > > See commit b01918978c82 ("libcamera: v4l2_subdevice: Work around false > positive warning"). > > > #include <string> > > #include <vector> > > > > @@ -28,7 +29,7 @@ public: > > > > private: > > std::string driver_; > > - std::vector<std::string> entities_; > > + std::vector<std::regex> entities_; > > }; > > > > class DeviceEnumerator > > diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp > > index ae17862f..e22d2822 100644 > > --- a/src/libcamera/device_enumerator.cpp > > +++ b/src/libcamera/device_enumerator.cpp > > @@ -52,15 +52,15 @@ LOG_DEFINE_CATEGORY(DeviceEnumerator) > > * device enumerator to find matching media devices. > > * > > * A DeviceMatch is created with a specific Linux device driver in mind, > > - * therefore the name of the driver is a required property. One or more Entity > > - * names can be added as match criteria. > > + * therefore the name of the driver is a required property. Regex expressions to > > + * match one or more Entity names can be added as match criteria. > > * > > - * Pipeline handlers are recommended to add entities to DeviceMatch as > > - * appropriate to ensure that the media device they need can be uniquely > > - * identified. This is useful when the corresponding kernel driver can produce > > - * different graphs, for instance as a result of different driver versions or > > - * hardware configurations, and not all those graphs are suitable for a pipeline > > - * handler. > > + * Pipeline handlers are recommended to add regex expressions for entities to > > + * DeviceMatch as appropriate to ensure that the media device they need can be > > + * uniquely identified. This is useful when the corresponding kernel driver can > > + * produce different graphs, for instance as a result of different driver > > + * versions or hardware configurations, and not all those graphs are suitable > > + * for a pipeline handler. > > */ > > > > /** > > @@ -73,12 +73,13 @@ DeviceMatch::DeviceMatch(const std::string &driver) > > } > > > > /** > > - * \brief Add a media entity name to the search pattern > > - * \param[in] entity The name of the entity in the media graph > > + * \brief Add a media entity regex string to the search pattern > > + * \param[in] entity A regex string to match the entity in the media graph > > */ > > void DeviceMatch::add(const std::string &entity) > > { > > - entities_.push_back(entity); > > + std::regex entity_regex(entity); > > camelCase. Same below. > > > + entities_.push_back(entity_regex); > > Construct in-place with emplace_back, don't use a local variable. > > > } > > > > /** > > @@ -96,11 +97,11 @@ bool DeviceMatch::match(const MediaDevice *device) const > > if (driver_ != device->driver()) > > return false; > > > > - for (const std::string &name : entities_) { > > + for (const std::regex &name_regex : entities_) { > > bool found = false; > > > > for (const MediaEntity *entity : device->entities()) { > > - if (name == entity->name()) { > > + if (std::regex_search(entity->name(), name_regex)) { > > if (!entity->deviceNode().empty()) { > > found = true; > > break; > > -- > Regards, > > Laurent Pinchart
Hi Laurent On 26/06/2025 14:10, Laurent Pinchart wrote: > Hi Dan, > > Thank you for the patch. > > On Wed, Jun 25, 2025 at 04:53:07PM +0100, Daniel Scally wrote: >> Some entities in a media graph have names that might differ from >> implementation to implementation; for example the Camera Receiver >> Unit and CSI-2 receiver on the KakiP board have entities with names >> that include their address, in the form "csi-16000400.csi2". Passing >> that entity name to DeviceMatch is too inflexible given it would only >> work if that specific CSI-2 receiver were the one being used. >> >> Update DeviceMatch.add() such that the string passed to it is used to >> create a std::basic_regex, and use std::regex_search() instead of a >> direct string comparison in DeviceMatch::match(). This allows us to >> pass a string that will form a regex expression to match to the entity >> instead, for example "csi-[0-9]{8}.csi2". > So you'll only support SoCs whose CSI-2 receiver are mapped to an > address that contains no alphabetic digits ? :-) Oh yeah...erm, "csi-[0-9a-fA-F]{8}.csi2" then? > >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >> --- >> .../libcamera/internal/device_enumerator.h | 3 ++- >> src/libcamera/device_enumerator.cpp | 27 ++++++++++--------- >> 2 files changed, 16 insertions(+), 14 deletions(-) >> >> diff --git a/include/libcamera/internal/device_enumerator.h b/include/libcamera/internal/device_enumerator.h >> index db3532a9..fce7f0ea 100644 >> --- a/include/libcamera/internal/device_enumerator.h >> +++ b/include/libcamera/internal/device_enumerator.h >> @@ -8,6 +8,7 @@ >> #pragma once >> >> #include <memory> >> +#include <regex> > See commit b01918978c82 ("libcamera: v4l2_subdevice: Work around false > positive warning"). Ooh. OK...I guess it's time for a wrapper then...do you mean in your commit message comment that we ought to create a libcamera::regex class that wraps the std class, with that #pragma guarding the include in the source file? > >> #include <string> >> #include <vector> >> >> @@ -28,7 +29,7 @@ public: >> >> private: >> std::string driver_; >> - std::vector<std::string> entities_; >> + std::vector<std::regex> entities_; >> }; >> >> class DeviceEnumerator >> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp >> index ae17862f..e22d2822 100644 >> --- a/src/libcamera/device_enumerator.cpp >> +++ b/src/libcamera/device_enumerator.cpp >> @@ -52,15 +52,15 @@ LOG_DEFINE_CATEGORY(DeviceEnumerator) >> * device enumerator to find matching media devices. >> * >> * A DeviceMatch is created with a specific Linux device driver in mind, >> - * therefore the name of the driver is a required property. One or more Entity >> - * names can be added as match criteria. >> + * therefore the name of the driver is a required property. Regex expressions to >> + * match one or more Entity names can be added as match criteria. >> * >> - * Pipeline handlers are recommended to add entities to DeviceMatch as >> - * appropriate to ensure that the media device they need can be uniquely >> - * identified. This is useful when the corresponding kernel driver can produce >> - * different graphs, for instance as a result of different driver versions or >> - * hardware configurations, and not all those graphs are suitable for a pipeline >> - * handler. >> + * Pipeline handlers are recommended to add regex expressions for entities to >> + * DeviceMatch as appropriate to ensure that the media device they need can be >> + * uniquely identified. This is useful when the corresponding kernel driver can >> + * produce different graphs, for instance as a result of different driver >> + * versions or hardware configurations, and not all those graphs are suitable >> + * for a pipeline handler. >> */ >> >> /** >> @@ -73,12 +73,13 @@ DeviceMatch::DeviceMatch(const std::string &driver) >> } >> >> /** >> - * \brief Add a media entity name to the search pattern >> - * \param[in] entity The name of the entity in the media graph >> + * \brief Add a media entity regex string to the search pattern >> + * \param[in] entity A regex string to match the entity in the media graph >> */ >> void DeviceMatch::add(const std::string &entity) >> { >> - entities_.push_back(entity); >> + std::regex entity_regex(entity); > camelCase. Same below. > >> + entities_.push_back(entity_regex); > Construct in-place with emplace_back, don't use a local variable. > >> } >> >> /** >> @@ -96,11 +97,11 @@ bool DeviceMatch::match(const MediaDevice *device) const >> if (driver_ != device->driver()) >> return false; >> >> - for (const std::string &name : entities_) { >> + for (const std::regex &name_regex : entities_) { >> bool found = false; >> >> for (const MediaEntity *entity : device->entities()) { >> - if (name == entity->name()) { >> + if (std::regex_search(entity->name(), name_regex)) { >> if (!entity->deviceNode().empty()) { >> found = true; >> break;
On Thu, Jun 26, 2025 at 02:26:29PM +0100, Daniel Scally wrote: > On 26/06/2025 14:10, Laurent Pinchart wrote: > > On Wed, Jun 25, 2025 at 04:53:07PM +0100, Daniel Scally wrote: > >> Some entities in a media graph have names that might differ from > >> implementation to implementation; for example the Camera Receiver > >> Unit and CSI-2 receiver on the KakiP board have entities with names > >> that include their address, in the form "csi-16000400.csi2". Passing > >> that entity name to DeviceMatch is too inflexible given it would only > >> work if that specific CSI-2 receiver were the one being used. > >> > >> Update DeviceMatch.add() such that the string passed to it is used to > >> create a std::basic_regex, and use std::regex_search() instead of a > >> direct string comparison in DeviceMatch::match(). This allows us to > >> pass a string that will form a regex expression to match to the entity > >> instead, for example "csi-[0-9]{8}.csi2". > > > > So you'll only support SoCs whose CSI-2 receiver are mapped to an > > address that contains no alphabetic digits ? :-) > > Oh yeah...erm, "csi-[0-9a-fA-F]{8}.csi2" then? I would have hoped the kernel would only use lowercase hex values. > >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > >> --- > >> .../libcamera/internal/device_enumerator.h | 3 ++- > >> src/libcamera/device_enumerator.cpp | 27 ++++++++++--------- > >> 2 files changed, 16 insertions(+), 14 deletions(-) > >> > >> diff --git a/include/libcamera/internal/device_enumerator.h b/include/libcamera/internal/device_enumerator.h > >> index db3532a9..fce7f0ea 100644 > >> --- a/include/libcamera/internal/device_enumerator.h > >> +++ b/include/libcamera/internal/device_enumerator.h > >> @@ -8,6 +8,7 @@ > >> #pragma once > >> > >> #include <memory> > >> +#include <regex> > > > > See commit b01918978c82 ("libcamera: v4l2_subdevice: Work around false > > positive warning"). > > Ooh. OK...I guess it's time for a wrapper then...do you mean in your commit message comment that we > ought to create a libcamera::regex class that wraps the std class, with that #pragma guarding the > include in the source file? I think a libcamera/base/regex.h file that wraps regex is enough, no need for a class. > >> #include <string> > >> #include <vector> > >> > >> @@ -28,7 +29,7 @@ public: > >> > >> private: > >> std::string driver_; > >> - std::vector<std::string> entities_; > >> + std::vector<std::regex> entities_; > >> }; > >> > >> class DeviceEnumerator > >> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp > >> index ae17862f..e22d2822 100644 > >> --- a/src/libcamera/device_enumerator.cpp > >> +++ b/src/libcamera/device_enumerator.cpp > >> @@ -52,15 +52,15 @@ LOG_DEFINE_CATEGORY(DeviceEnumerator) > >> * device enumerator to find matching media devices. > >> * > >> * A DeviceMatch is created with a specific Linux device driver in mind, > >> - * therefore the name of the driver is a required property. One or more Entity > >> - * names can be added as match criteria. > >> + * therefore the name of the driver is a required property. Regex expressions to > >> + * match one or more Entity names can be added as match criteria. > >> * > >> - * Pipeline handlers are recommended to add entities to DeviceMatch as > >> - * appropriate to ensure that the media device they need can be uniquely > >> - * identified. This is useful when the corresponding kernel driver can produce > >> - * different graphs, for instance as a result of different driver versions or > >> - * hardware configurations, and not all those graphs are suitable for a pipeline > >> - * handler. > >> + * Pipeline handlers are recommended to add regex expressions for entities to > >> + * DeviceMatch as appropriate to ensure that the media device they need can be > >> + * uniquely identified. This is useful when the corresponding kernel driver can > >> + * produce different graphs, for instance as a result of different driver > >> + * versions or hardware configurations, and not all those graphs are suitable > >> + * for a pipeline handler. > >> */ > >> > >> /** > >> @@ -73,12 +73,13 @@ DeviceMatch::DeviceMatch(const std::string &driver) > >> } > >> > >> /** > >> - * \brief Add a media entity name to the search pattern > >> - * \param[in] entity The name of the entity in the media graph > >> + * \brief Add a media entity regex string to the search pattern > >> + * \param[in] entity A regex string to match the entity in the media graph > >> */ > >> void DeviceMatch::add(const std::string &entity) > >> { > >> - entities_.push_back(entity); > >> + std::regex entity_regex(entity); > > > > camelCase. Same below. > > > >> + entities_.push_back(entity_regex); > > > > Construct in-place with emplace_back, don't use a local variable. > > > >> } > >> > >> /** > >> @@ -96,11 +97,11 @@ bool DeviceMatch::match(const MediaDevice *device) const > >> if (driver_ != device->driver()) > >> return false; > >> > >> - for (const std::string &name : entities_) { > >> + for (const std::regex &name_regex : entities_) { > >> bool found = false; > >> > >> for (const MediaEntity *entity : device->entities()) { > >> - if (name == entity->name()) { > >> + if (std::regex_search(entity->name(), name_regex)) { > >> if (!entity->deviceNode().empty()) { > >> found = true; > >> break;
diff --git a/include/libcamera/internal/device_enumerator.h b/include/libcamera/internal/device_enumerator.h index db3532a9..fce7f0ea 100644 --- a/include/libcamera/internal/device_enumerator.h +++ b/include/libcamera/internal/device_enumerator.h @@ -8,6 +8,7 @@ #pragma once #include <memory> +#include <regex> #include <string> #include <vector> @@ -28,7 +29,7 @@ public: private: std::string driver_; - std::vector<std::string> entities_; + std::vector<std::regex> entities_; }; class DeviceEnumerator diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp index ae17862f..e22d2822 100644 --- a/src/libcamera/device_enumerator.cpp +++ b/src/libcamera/device_enumerator.cpp @@ -52,15 +52,15 @@ LOG_DEFINE_CATEGORY(DeviceEnumerator) * device enumerator to find matching media devices. * * A DeviceMatch is created with a specific Linux device driver in mind, - * therefore the name of the driver is a required property. One or more Entity - * names can be added as match criteria. + * therefore the name of the driver is a required property. Regex expressions to + * match one or more Entity names can be added as match criteria. * - * Pipeline handlers are recommended to add entities to DeviceMatch as - * appropriate to ensure that the media device they need can be uniquely - * identified. This is useful when the corresponding kernel driver can produce - * different graphs, for instance as a result of different driver versions or - * hardware configurations, and not all those graphs are suitable for a pipeline - * handler. + * Pipeline handlers are recommended to add regex expressions for entities to + * DeviceMatch as appropriate to ensure that the media device they need can be + * uniquely identified. This is useful when the corresponding kernel driver can + * produce different graphs, for instance as a result of different driver + * versions or hardware configurations, and not all those graphs are suitable + * for a pipeline handler. */ /** @@ -73,12 +73,13 @@ DeviceMatch::DeviceMatch(const std::string &driver) } /** - * \brief Add a media entity name to the search pattern - * \param[in] entity The name of the entity in the media graph + * \brief Add a media entity regex string to the search pattern + * \param[in] entity A regex string to match the entity in the media graph */ void DeviceMatch::add(const std::string &entity) { - entities_.push_back(entity); + std::regex entity_regex(entity); + entities_.push_back(entity_regex); } /** @@ -96,11 +97,11 @@ bool DeviceMatch::match(const MediaDevice *device) const if (driver_ != device->driver()) return false; - for (const std::string &name : entities_) { + for (const std::regex &name_regex : entities_) { bool found = false; for (const MediaEntity *entity : device->entities()) { - if (name == entity->name()) { + if (std::regex_search(entity->name(), name_regex)) { if (!entity->deviceNode().empty()) { found = true; break;
Some entities in a media graph have names that might differ from implementation to implementation; for example the Camera Receiver Unit and CSI-2 receiver on the KakiP board have entities with names that include their address, in the form "csi-16000400.csi2". Passing that entity name to DeviceMatch is too inflexible given it would only work if that specific CSI-2 receiver were the one being used. Update DeviceMatch.add() such that the string passed to it is used to create a std::basic_regex, and use std::regex_search() instead of a direct string comparison in DeviceMatch::match(). This allows us to pass a string that will form a regex expression to match to the entity instead, for example "csi-[0-9]{8}.csi2". Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> --- .../libcamera/internal/device_enumerator.h | 3 ++- src/libcamera/device_enumerator.cpp | 27 ++++++++++--------- 2 files changed, 16 insertions(+), 14 deletions(-)