| Message ID | 20190115140749.8297-4-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On 15/01/2019 14:07, Jacopo Mondi wrote: > Media entities convey information about their main function in the > 'function' field of 'struct media_v2_entity'. > > Store the main function in the MediaEntity function_ class member and provide > a getter function for that. > > While at there update comments and remove a stale TODO entry. > I jumped to the next patch first to see how this was used. It seems reasonable to me: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/include/media_object.h | 2 ++ > src/libcamera/media_object.cpp | 18 +++++++++++++----- > 2 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/src/libcamera/include/media_object.h b/src/libcamera/include/media_object.h > index 7fc4441..a10f7e1 100644 > --- a/src/libcamera/include/media_object.h > +++ b/src/libcamera/include/media_object.h > @@ -84,6 +84,7 @@ class MediaEntity : public MediaObject > { > public: > const std::string &name() const { return name_; } > + unsigned int function() const { return function_; } > unsigned int deviceMajor() const { return major_; } > unsigned int deviceMinor() const { return minor_; } > > @@ -103,6 +104,7 @@ private: > ~MediaEntity(); > > std::string name_; > + unsigned int function_; > std::string devnode_; > unsigned int major_; > unsigned int minor_; > diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp > index cb3af85..c47fb53 100644 > --- a/src/libcamera/media_object.cpp > +++ b/src/libcamera/media_object.cpp > @@ -243,10 +243,8 @@ void MediaPad::addLink(MediaLink *link) > * API in the media_v2_entity structure. They reference the pads() they contain. > * > * In addition to its graph id, every media graph entity is identified by a > - * name() unique in the media device context. > - * > - * \todo Add support for associating a devnode to the entity when integrating > - * with DeviceEnumerator. > + * name() unique in the media device context, a function() and its associated > + * devnode, if any. > */ > > /** > @@ -255,6 +253,16 @@ void MediaPad::addLink(MediaLink *link) > * \return The entity name > */ > > +/** > + * \fn MediaEntity::function() > + * \brief Retrieve the entity main function > + * > + * Media entity functions are expressed using the MEDIA_ENT_F_* macros > + * defined by the Media Controller API. > + * > + * \return The entity function > + */ > + > /** > * \fn MediaEntity::deviceMajor() > * \brief Retrieve the major number of the interface associated with the entity > @@ -336,7 +344,7 @@ MediaEntity::MediaEntity(MediaDevice *dev, > const struct media_v2_entity *entity, > unsigned int major, unsigned int minor) > : MediaObject(dev, entity->id), name_(entity->name), > - major_(major), minor_(minor) > + function_(entity->function), major_(major), minor_(minor) > { > } > >
Hi Jacopo, Thank you for the patch. On Tuesday, 15 January 2019 16:07:48 EET Jacopo Mondi wrote: > Media entities convey information about their main function in the > 'function' field of 'struct media_v2_entity'. > > Store the main function in the MediaEntity function_ class member and > provide a getter function for that. > > While at there update comments and remove a stale TODO entry. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/include/media_object.h | 2 ++ > src/libcamera/media_object.cpp | 18 +++++++++++++----- > 2 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/src/libcamera/include/media_object.h > b/src/libcamera/include/media_object.h index 7fc4441..a10f7e1 100644 > --- a/src/libcamera/include/media_object.h > +++ b/src/libcamera/include/media_object.h > @@ -84,6 +84,7 @@ class MediaEntity : public MediaObject > { > public: > const std::string &name() const { return name_; } > + unsigned int function() const { return function_; } > unsigned int deviceMajor() const { return major_; } > unsigned int deviceMinor() const { return minor_; } > > @@ -103,6 +104,7 @@ private: > ~MediaEntity(); > > std::string name_; > + unsigned int function_; > std::string devnode_; > unsigned int major_; > unsigned int minor_; > diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp > index cb3af85..c47fb53 100644 > --- a/src/libcamera/media_object.cpp > +++ b/src/libcamera/media_object.cpp > @@ -243,10 +243,8 @@ void MediaPad::addLink(MediaLink *link) > * API in the media_v2_entity structure. They reference the pads() they > contain. * > * In addition to its graph id, every media graph entity is identified by a > - * name() unique in the media device context. > - * > - * \todo Add support for associating a devnode to the entity when > integrating - * with DeviceEnumerator. > + * name() unique in the media device context, a function() and its > associated + * devnode, if any. The entity isn't "identified" by its function or devnode. How about * In addition to their graph id, media graph entities are identified by a * name() unique in the media device context. Their implement a function(), * and may expose a devnode(). By the way the devnode isn't even set in the existing code, neither it is accessed :-) Is that an oversight ? > */ > > /** > @@ -255,6 +253,16 @@ void MediaPad::addLink(MediaLink *link) > * \return The entity name > */ > > +/** > + * \fn MediaEntity::function() > + * \brief Retrieve the entity main function s/entity/entity's/ ? Apart from that, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + * > + * Media entity functions are expressed using the MEDIA_ENT_F_* macros > + * defined by the Media Controller API. > + * > + * \return The entity function > + */ > + > /** > * \fn MediaEntity::deviceMajor() > * \brief Retrieve the major number of the interface associated with the > entity @@ -336,7 +344,7 @@ MediaEntity::MediaEntity(MediaDevice *dev, > const struct media_v2_entity *entity, > unsigned int major, unsigned int minor) > : MediaObject(dev, entity->id), name_(entity->name), > - major_(major), minor_(minor) > + function_(entity->function), major_(major), minor_(minor) > { > }
Hi Laurent, On Tue, Jan 15, 2019 at 05:57:27PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Tuesday, 15 January 2019 16:07:48 EET Jacopo Mondi wrote: > > Media entities convey information about their main function in the > > 'function' field of 'struct media_v2_entity'. > > > > Store the main function in the MediaEntity function_ class member and > > provide a getter function for that. > > > > While at there update comments and remove a stale TODO entry. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/include/media_object.h | 2 ++ > > src/libcamera/media_object.cpp | 18 +++++++++++++----- > > 2 files changed, 15 insertions(+), 5 deletions(-) > > > > diff --git a/src/libcamera/include/media_object.h > > b/src/libcamera/include/media_object.h index 7fc4441..a10f7e1 100644 > > --- a/src/libcamera/include/media_object.h > > +++ b/src/libcamera/include/media_object.h > > @@ -84,6 +84,7 @@ class MediaEntity : public MediaObject > > { > > public: > > const std::string &name() const { return name_; } > > + unsigned int function() const { return function_; } > > unsigned int deviceMajor() const { return major_; } > > unsigned int deviceMinor() const { return minor_; } > > > > @@ -103,6 +104,7 @@ private: > > ~MediaEntity(); > > > > std::string name_; > > + unsigned int function_; > > std::string devnode_; > > unsigned int major_; > > unsigned int minor_; > > diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp > > index cb3af85..c47fb53 100644 > > --- a/src/libcamera/media_object.cpp > > +++ b/src/libcamera/media_object.cpp > > @@ -243,10 +243,8 @@ void MediaPad::addLink(MediaLink *link) > > * API in the media_v2_entity structure. They reference the pads() they > > contain. * > > * In addition to its graph id, every media graph entity is identified by a > > - * name() unique in the media device context. > > - * > > - * \todo Add support for associating a devnode to the entity when > > integrating - * with DeviceEnumerator. > > + * name() unique in the media device context, a function() and its > > associated + * devnode, if any. > > The entity isn't "identified" by its function or devnode. How about I tried summarize here what 'identity' informations a media entity transports. If that's confusing I'll remove it. > > * In addition to their graph id, media graph entities are identified by a > * name() unique in the media device context. Their implement a function(), They > * and may expose a devnode(). > > By the way the devnode isn't even set in the existing code, neither it is > accessed :-) Is that an oversight ? I didn't get this... The devnode *might* be set.. I think that's captured in your comment here above, so I'll include that. > > > */ > > > > /** > > @@ -255,6 +253,16 @@ void MediaPad::addLink(MediaLink *link) > > * \return The entity name > > */ > > > > +/** > > + * \fn MediaEntity::function() > > + * \brief Retrieve the entity main function > > s/entity/entity's/ ? > > Apart from that, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Thanks j > > + * > > + * Media entity functions are expressed using the MEDIA_ENT_F_* macros > > + * defined by the Media Controller API. > > + * > > + * \return The entity function > > + */ > > + > > /** > > * \fn MediaEntity::deviceMajor() > > * \brief Retrieve the major number of the interface associated with the > > entity @@ -336,7 +344,7 @@ MediaEntity::MediaEntity(MediaDevice *dev, > > const struct media_v2_entity *entity, > > unsigned int major, unsigned int minor) > > : MediaObject(dev, entity->id), name_(entity->name), > > - major_(major), minor_(minor) > > + function_(entity->function), major_(major), minor_(minor) > > { > > } > > -- > Regards, > > Laurent Pinchart > > >
Hi Jacopo, On Tuesday, 15 January 2019 18:06:58 EET Jacopo Mondi wrote: > On Tue, Jan 15, 2019 at 05:57:27PM +0200, Laurent Pinchart wrote: > > On Tuesday, 15 January 2019 16:07:48 EET Jacopo Mondi wrote: > >> Media entities convey information about their main function in the > >> 'function' field of 'struct media_v2_entity'. > >> > >> Store the main function in the MediaEntity function_ class member and > >> provide a getter function for that. > >> > >> While at there update comments and remove a stale TODO entry. > >> > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >> --- > >> > >> src/libcamera/include/media_object.h | 2 ++ > >> src/libcamera/media_object.cpp | 18 +++++++++++++----- > >> 2 files changed, 15 insertions(+), 5 deletions(-) [snip] > >> diff --git a/src/libcamera/media_object.cpp > >> b/src/libcamera/media_object.cpp index cb3af85..c47fb53 100644 > >> --- a/src/libcamera/media_object.cpp > >> +++ b/src/libcamera/media_object.cpp > >> @@ -243,10 +243,8 @@ void MediaPad::addLink(MediaLink *link) > >> * API in the media_v2_entity structure. They reference the pads() they > >> contain. > >> * > >> * In addition to its graph id, every media graph entity is identified > >> by a > >> - * name() unique in the media device context. > >> - * > >> - * \todo Add support for associating a devnode to the entity when > >> integrating - * with DeviceEnumerator. > >> + * name() unique in the media device context, a function() and its > >> associated + * devnode, if any. > > > > The entity isn't "identified" by its function or devnode. How about > > I tried summarize here what 'identity' informations a media entity > transports. If that's confusing I'll remove it. I'm just nitpicking on the wording :-) > > * In addition to their graph id, media graph entities are identified by a > > * name() unique in the media device context. Their implement a > > function(), > > They Oops. > > * and may expose a devnode(). > > > > By the way the devnode isn't even set in the existing code, neither it is > > accessed :-) Is that an oversight ? > > I didn't get this... The devnode *might* be set.. I think that's > captured in your comment here above, so I'll include that. Unless I'm mistaken, setDeviceNode() doesn't assign devnode_, and devnode_ is never used. > >> */ [snip]
Hi Laurent, On Tue, Jan 15, 2019 at 06:35:11PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Tuesday, 15 January 2019 18:06:58 EET Jacopo Mondi wrote: > > On Tue, Jan 15, 2019 at 05:57:27PM +0200, Laurent Pinchart wrote: > > > On Tuesday, 15 January 2019 16:07:48 EET Jacopo Mondi wrote: > > >> Media entities convey information about their main function in the > > >> 'function' field of 'struct media_v2_entity'. > > >> > > >> Store the main function in the MediaEntity function_ class member and > > >> provide a getter function for that. > > >> > > >> While at there update comments and remove a stale TODO entry. > > >> > > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > >> --- > > >> > > >> src/libcamera/include/media_object.h | 2 ++ > > >> src/libcamera/media_object.cpp | 18 +++++++++++++----- > > >> 2 files changed, 15 insertions(+), 5 deletions(-) > > [snip] > > > >> diff --git a/src/libcamera/media_object.cpp > > >> b/src/libcamera/media_object.cpp index cb3af85..c47fb53 100644 > > >> --- a/src/libcamera/media_object.cpp > > >> +++ b/src/libcamera/media_object.cpp > > >> @@ -243,10 +243,8 @@ void MediaPad::addLink(MediaLink *link) > > >> * API in the media_v2_entity structure. They reference the pads() they > > >> contain. > > >> * > > >> * In addition to its graph id, every media graph entity is identified > > >> by a > > >> - * name() unique in the media device context. > > >> - * > > >> - * \todo Add support for associating a devnode to the entity when > > >> integrating - * with DeviceEnumerator. > > >> + * name() unique in the media device context, a function() and its > > >> associated + * devnode, if any. > > > > > > The entity isn't "identified" by its function or devnode. How about > > > > I tried summarize here what 'identity' informations a media entity > > transports. If that's confusing I'll remove it. > > I'm just nitpicking on the wording :-) > > > > * In addition to their graph id, media graph entities are identified by a > > > * name() unique in the media device context. Their implement a > > > function(), > > > > They > > Oops. > > > > * and may expose a devnode(). > > > > > > By the way the devnode isn't even set in the existing code, neither it is > > > accessed :-) Is that an oversight ? > > > > I didn't get this... The devnode *might* be set.. I think that's > > captured in your comment here above, so I'll include that. > > Unless I'm mistaken, setDeviceNode() doesn't assign devnode_, and devnode_ is > never used. int MediaEntity::setDeviceNode(const std::string &devnode) { /* Make sure the device node can be accessed. */ int ret = ::access(devnode.c_str(), R_OK | W_OK); if (ret < 0) { ret = -errno; LOG(Error) << "Device node " << devnode << " can't be accessed: " << strerror(-ret); return ret; } return 0; } This is a "bug" (better, this should assign devnode_). I have no idea why it is not there.... I'll send a patch soon, as the entity devnode will be required when we'll start creating V4L2Devices from MediaEntities. > > > >> */ > > [snip] > > -- > Regards, > > Laurent Pinchart > > >
diff --git a/src/libcamera/include/media_object.h b/src/libcamera/include/media_object.h index 7fc4441..a10f7e1 100644 --- a/src/libcamera/include/media_object.h +++ b/src/libcamera/include/media_object.h @@ -84,6 +84,7 @@ class MediaEntity : public MediaObject { public: const std::string &name() const { return name_; } + unsigned int function() const { return function_; } unsigned int deviceMajor() const { return major_; } unsigned int deviceMinor() const { return minor_; } @@ -103,6 +104,7 @@ private: ~MediaEntity(); std::string name_; + unsigned int function_; std::string devnode_; unsigned int major_; unsigned int minor_; diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp index cb3af85..c47fb53 100644 --- a/src/libcamera/media_object.cpp +++ b/src/libcamera/media_object.cpp @@ -243,10 +243,8 @@ void MediaPad::addLink(MediaLink *link) * API in the media_v2_entity structure. They reference the pads() they contain. * * In addition to its graph id, every media graph entity is identified by a - * name() unique in the media device context. - * - * \todo Add support for associating a devnode to the entity when integrating - * with DeviceEnumerator. + * name() unique in the media device context, a function() and its associated + * devnode, if any. */ /** @@ -255,6 +253,16 @@ void MediaPad::addLink(MediaLink *link) * \return The entity name */ +/** + * \fn MediaEntity::function() + * \brief Retrieve the entity main function + * + * Media entity functions are expressed using the MEDIA_ENT_F_* macros + * defined by the Media Controller API. + * + * \return The entity function + */ + /** * \fn MediaEntity::deviceMajor() * \brief Retrieve the major number of the interface associated with the entity @@ -336,7 +344,7 @@ MediaEntity::MediaEntity(MediaDevice *dev, const struct media_v2_entity *entity, unsigned int major, unsigned int minor) : MediaObject(dev, entity->id), name_(entity->name), - major_(major), minor_(minor) + function_(entity->function), major_(major), minor_(minor) { }
Media entities convey information about their main function in the 'function' field of 'struct media_v2_entity'. Store the main function in the MediaEntity function_ class member and provide a getter function for that. While at there update comments and remove a stale TODO entry. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/include/media_object.h | 2 ++ src/libcamera/media_object.cpp | 18 +++++++++++++----- 2 files changed, 15 insertions(+), 5 deletions(-)