[libcamera-devel] [PATCH 3/4] libcamera: media_object: Add functions to entities

[Jacopo Mondi]

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 at 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
>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190115/54f0139d/attachment.sig>