| Message ID | 20250625155308.2325438-3-dan.scally@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On Wed, Jun 25, 2025 at 04:53:08PM +0100, Daniel Scally wrote: > Some kernel drivers give their entities names that will differ from > implementation to implementation; for example the drivers for the > Camera Receiver Unit and CSI-2 receiver in the RZ/V2H SoC give their > entities names that include their memory address, in the format > "csi-16000400.csi2". Passing that entity name to > MediaDevice::getEntityByName() is too inflexible given it would only > then work if that specific CSI-2 receiver were the one being used. > > Update MediaDevice::getEntityByName() 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 to find a matching entity. This > allows us to pass a string that will form a regex to match to the > entity instead, for example "csi-[0-9]{8}.csi2". > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > src/libcamera/media_device.cpp | 12 ++++++++---- > src/libcamera/v4l2_subdevice.cpp | 4 ++-- > 2 files changed, 10 insertions(+), 6 deletions(-) > > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp > index 353f34a8..27180ca5 100644 > --- a/src/libcamera/media_device.cpp > +++ b/src/libcamera/media_device.cpp > @@ -9,6 +9,7 @@ > > #include <errno.h> > #include <fcntl.h> > +#include <regex> > #include <stdint.h> > #include <string> > #include <string.h> > @@ -328,14 +329,17 @@ done: > */ > > /** > - * \brief Return the MediaEntity with name \a name > - * \param[in] name The entity name > - * \return The entity with \a name, or nullptr if no such entity is found > + * \brief Return the MediaEntity with name matching the regex \a name "whose name matches" ? > + * \param[in] name Regex to match against the entity name > + * \return The entity with name matching \a name, or nullptr if no such entity > + * is found Entity names are unique, but with regex that's not the case anymore. If multiple entities match the regex, the behaviour of this function will not be predictable. Is that a concern ? It should at least be properly documented. > */ > MediaEntity *MediaDevice::getEntityByName(const std::string &name) const > { > + std::regex name_regex(name); > + > for (MediaEntity *e : entities_) > - if (e->name() == name) > + if (std::regex_search(e->name(), name_regex)) > return e; > > return nullptr; > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > index 33279654..4868785d 100644 > --- a/src/libcamera/v4l2_subdevice.cpp > +++ b/src/libcamera/v4l2_subdevice.cpp > @@ -1748,10 +1748,10 @@ const std::string &V4L2Subdevice::model() > */ > > /** > - * \brief Create a new video subdevice instance from \a entity in media device > + * \brief Create a new video subdevice instance from an entity in media device "\a entity" refers to the entity parameter. > * \a media > * \param[in] media The media device where the entity is registered > - * \param[in] entity The media entity name > + * \param[in] entity A regex that will match media entity name > * > * \return A newly created V4L2Subdevice on success, nullptr otherwise > */
Hi Laurent On 26/06/2025 14:14, Laurent Pinchart wrote: > On Wed, Jun 25, 2025 at 04:53:08PM +0100, Daniel Scally wrote: >> Some kernel drivers give their entities names that will differ from >> implementation to implementation; for example the drivers for the >> Camera Receiver Unit and CSI-2 receiver in the RZ/V2H SoC give their >> entities names that include their memory address, in the format >> "csi-16000400.csi2". Passing that entity name to >> MediaDevice::getEntityByName() is too inflexible given it would only >> then work if that specific CSI-2 receiver were the one being used. >> >> Update MediaDevice::getEntityByName() 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 to find a matching entity. This >> allows us to pass a string that will form a regex to match to the >> entity instead, for example "csi-[0-9]{8}.csi2". >> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >> --- >> src/libcamera/media_device.cpp | 12 ++++++++---- >> src/libcamera/v4l2_subdevice.cpp | 4 ++-- >> 2 files changed, 10 insertions(+), 6 deletions(-) >> >> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp >> index 353f34a8..27180ca5 100644 >> --- a/src/libcamera/media_device.cpp >> +++ b/src/libcamera/media_device.cpp >> @@ -9,6 +9,7 @@ >> >> #include <errno.h> >> #include <fcntl.h> >> +#include <regex> >> #include <stdint.h> >> #include <string> >> #include <string.h> >> @@ -328,14 +329,17 @@ done: >> */ >> >> /** >> - * \brief Return the MediaEntity with name \a name >> - * \param[in] name The entity name >> - * \return The entity with \a name, or nullptr if no such entity is found >> + * \brief Return the MediaEntity with name matching the regex \a name > "whose name matches" ? Sure > >> + * \param[in] name Regex to match against the entity name >> + * \return The entity with name matching \a name, or nullptr if no such entity >> + * is found > Entity names are unique, but with regex that's not the case anymore. If > multiple entities match the regex, the behaviour of this function will > not be predictable. Is that a concern ? It should at least be properly > documented. I guess it might be...perhaps this should count the returned matches and complain heavily if the answer is >1? > >> */ >> MediaEntity *MediaDevice::getEntityByName(const std::string &name) const >> { >> + std::regex name_regex(name); >> + >> for (MediaEntity *e : entities_) >> - if (e->name() == name) >> + if (std::regex_search(e->name(), name_regex)) >> return e; >> >> return nullptr; >> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp >> index 33279654..4868785d 100644 >> --- a/src/libcamera/v4l2_subdevice.cpp >> +++ b/src/libcamera/v4l2_subdevice.cpp >> @@ -1748,10 +1748,10 @@ const std::string &V4L2Subdevice::model() >> */ >> >> /** >> - * \brief Create a new video subdevice instance from \a entity in media device >> + * \brief Create a new video subdevice instance from an entity in media device > "\a entity" refers to the entity parameter. I think I changed that because the horrible regex string wouldn't necessarily be easily read as the entity anymore...but perhaps that's too persnickity > >> * \a media >> * \param[in] media The media device where the entity is registered >> - * \param[in] entity The media entity name >> + * \param[in] entity A regex that will match media entity name >> * >> * \return A newly created V4L2Subdevice on success, nullptr otherwise >> */
On Thu, Jun 26, 2025 at 02:29:02PM +0100, Daniel Scally wrote: > On 26/06/2025 14:14, Laurent Pinchart wrote: > > On Wed, Jun 25, 2025 at 04:53:08PM +0100, Daniel Scally wrote: > >> Some kernel drivers give their entities names that will differ from > >> implementation to implementation; for example the drivers for the > >> Camera Receiver Unit and CSI-2 receiver in the RZ/V2H SoC give their > >> entities names that include their memory address, in the format > >> "csi-16000400.csi2". Passing that entity name to > >> MediaDevice::getEntityByName() is too inflexible given it would only > >> then work if that specific CSI-2 receiver were the one being used. > >> > >> Update MediaDevice::getEntityByName() 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 to find a matching entity. This > >> allows us to pass a string that will form a regex to match to the > >> entity instead, for example "csi-[0-9]{8}.csi2". > >> > >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > >> --- > >> src/libcamera/media_device.cpp | 12 ++++++++---- > >> src/libcamera/v4l2_subdevice.cpp | 4 ++-- > >> 2 files changed, 10 insertions(+), 6 deletions(-) > >> > >> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp > >> index 353f34a8..27180ca5 100644 > >> --- a/src/libcamera/media_device.cpp > >> +++ b/src/libcamera/media_device.cpp > >> @@ -9,6 +9,7 @@ > >> > >> #include <errno.h> > >> #include <fcntl.h> > >> +#include <regex> > >> #include <stdint.h> > >> #include <string> > >> #include <string.h> > >> @@ -328,14 +329,17 @@ done: > >> */ > >> > >> /** > >> - * \brief Return the MediaEntity with name \a name > >> - * \param[in] name The entity name > >> - * \return The entity with \a name, or nullptr if no such entity is found > >> + * \brief Return the MediaEntity with name matching the regex \a name > > > > "whose name matches" ? > > Sure > > >> + * \param[in] name Regex to match against the entity name > >> + * \return The entity with name matching \a name, or nullptr if no such entity > >> + * is found > > > > Entity names are unique, but with regex that's not the case anymore. If > > multiple entities match the regex, the behaviour of this function will > > not be predictable. Is that a concern ? It should at least be properly > > documented. > > I guess it might be...perhaps this should count the returned matches and complain heavily if the > answer is >1? Is that the most appropriate behaviour for the use cases you envision ? What are those use cases actually ? > >> */ > >> MediaEntity *MediaDevice::getEntityByName(const std::string &name) const > >> { > >> + std::regex name_regex(name); > >> + > >> for (MediaEntity *e : entities_) > >> - if (e->name() == name) > >> + if (std::regex_search(e->name(), name_regex)) > >> return e; > >> > >> return nullptr; > >> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > >> index 33279654..4868785d 100644 > >> --- a/src/libcamera/v4l2_subdevice.cpp > >> +++ b/src/libcamera/v4l2_subdevice.cpp > >> @@ -1748,10 +1748,10 @@ const std::string &V4L2Subdevice::model() > >> */ > >> > >> /** > >> - * \brief Create a new video subdevice instance from \a entity in media device > >> + * \brief Create a new video subdevice instance from an entity in media device > > > > "\a entity" refers to the entity parameter. > > I think I changed that because the horrible regex string wouldn't necessarily be easily read as the > entity anymore...but perhaps that's too persnickity > > >> * \a media > >> * \param[in] media The media device where the entity is registered > >> - * \param[in] entity The media entity name > >> + * \param[in] entity A regex that will match media entity name > >> * > >> * \return A newly created V4L2Subdevice on success, nullptr otherwise > >> */
diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp index 353f34a8..27180ca5 100644 --- a/src/libcamera/media_device.cpp +++ b/src/libcamera/media_device.cpp @@ -9,6 +9,7 @@ #include <errno.h> #include <fcntl.h> +#include <regex> #include <stdint.h> #include <string> #include <string.h> @@ -328,14 +329,17 @@ done: */ /** - * \brief Return the MediaEntity with name \a name - * \param[in] name The entity name - * \return The entity with \a name, or nullptr if no such entity is found + * \brief Return the MediaEntity with name matching the regex \a name + * \param[in] name Regex to match against the entity name + * \return The entity with name matching \a name, or nullptr if no such entity + * is found */ MediaEntity *MediaDevice::getEntityByName(const std::string &name) const { + std::regex name_regex(name); + for (MediaEntity *e : entities_) - if (e->name() == name) + if (std::regex_search(e->name(), name_regex)) return e; return nullptr; diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp index 33279654..4868785d 100644 --- a/src/libcamera/v4l2_subdevice.cpp +++ b/src/libcamera/v4l2_subdevice.cpp @@ -1748,10 +1748,10 @@ const std::string &V4L2Subdevice::model() */ /** - * \brief Create a new video subdevice instance from \a entity in media device + * \brief Create a new video subdevice instance from an entity in media device * \a media * \param[in] media The media device where the entity is registered - * \param[in] entity The media entity name + * \param[in] entity A regex that will match media entity name * * \return A newly created V4L2Subdevice on success, nullptr otherwise */
Some kernel drivers give their entities names that will differ from implementation to implementation; for example the drivers for the Camera Receiver Unit and CSI-2 receiver in the RZ/V2H SoC give their entities names that include their memory address, in the format "csi-16000400.csi2". Passing that entity name to MediaDevice::getEntityByName() is too inflexible given it would only then work if that specific CSI-2 receiver were the one being used. Update MediaDevice::getEntityByName() 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 to find a matching entity. This allows us to pass a string that will form a regex to match to the entity instead, for example "csi-[0-9]{8}.csi2". Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> --- src/libcamera/media_device.cpp | 12 ++++++++---- src/libcamera/v4l2_subdevice.cpp | 4 ++-- 2 files changed, 10 insertions(+), 6 deletions(-)