[{"id":165,"web_url":"https://patchwork.libcamera.org/comment/165/","msgid":"<3050321.m0LTbSzqVd@avalon>","date":"2019-01-02T01:06:05","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: MediaDevice: Create\n\tentities with major and minor numbers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wednesday, 2 January 2019 02:49:02 EET Laurent Pinchart wrote:\n> From: Jacopo Mondi \n> \n> Extend the MediaEntity object with device node major and minor numbers,\n> and retrieve them from the media graph using interfaces. They will be\n> used by the DeviceEnumerator to retrieve the devnode path.\n> \n> Signed-off-by: Jacopo Mondi \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/include/media_device.h | 2 +\n> src/libcamera/include/media_object.h | 9 +++-\n> src/libcamera/media_device.cpp | 65 +++++++++++++++++++++++++++-\n> src/libcamera/media_object.cpp | 46 ++++++++++++++++++--\n> 4 files changed, 116 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/libcamera/include/media_device.h\n> b/src/libcamera/include/media_device.h index 3fcdb4b4d5f8..8d491a87867c\n> 100644\n> --- a/src/libcamera/include/media_device.h\n> +++ b/src/libcamera/include/media_device.h\n> @@ -54,6 +54,8 @@ private:\n> \n> \tstd::vector entities_;\n> \n> +\tstruct media_v2_interface *findInterface(const struct media_v2_topology\n> &topology, +\t\t\t\t\t\t unsigned int entityId);\n> \tbool populateEntities(const struct media_v2_topology &topology);\n> \tbool populatePads(const struct media_v2_topology &topology);\n> \tbool populateLinks(const struct media_v2_topology &topology);\n> diff --git a/src/libcamera/include/media_object.h\n> b/src/libcamera/include/media_object.h index 65b55085a3b0..950a33286690\n> 100644\n> --- a/src/libcamera/include/media_object.h\n> +++ b/src/libcamera/include/media_object.h\n> @@ -80,21 +80,28 @@ class MediaEntity : public MediaObject\n> {\n> public:\n> \tconst std::string &name() const { return name_; }\n> +\tunsigned int major() const { return major_; }\n> +\tunsigned int minor() const { return minor_; }\n> \n> \tconst std::vector &pads() const { return pads_; }\n> \n> \tconst MediaPad *getPadByIndex(unsigned int index) const;\n> \tconst MediaPad *getPadById(unsigned int id) const;\n> \n> +\tint setDeviceNode(const std::string &devnode);\n> +\n> private:\n> \tfriend class MediaDevice;\n> \n> -\tMediaEntity(const struct media_v2_entity *entity);\n> +\tMediaEntity(const struct media_v2_entity *entity,\n> +\t\t unsigned int major = 0, unsigned int minor = 0);\n> \tMediaEntity(const MediaEntity &) = delete;\n> \t~MediaEntity();\n> \n> \tstd::string name_;\n> \tstd::string devnode_;\n> +\tunsigned int major_;\n> +\tunsigned int minor_;\n> \n> \tstd::vector pads_;\n> \n> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> index 605e504be124..70b3fff3f492 100644\n> --- a/src/libcamera/media_device.cpp\n> +++ b/src/libcamera/media_device.cpp\n> @@ -208,6 +208,7 @@ int MediaDevice::populate()\n> \tstruct media_v2_entity *ents = nullptr;\n> \tstruct media_v2_link *links = nullptr;\n> \tstruct media_v2_pad *pads = nullptr;\n> +\tstruct media_v2_interface *interfaces = nullptr;\n> \t__u64 version = -1;\n> \tint ret;\n> \n> @@ -221,6 +222,7 @@ int MediaDevice::populate()\n> \t\ttopology.ptr_entities = reinterpret_cast<__u64>(ents);\n> \t\ttopology.ptr_links = reinterpret_cast<__u64>(links);\n> \t\ttopology.ptr_pads = reinterpret_cast<__u64>(pads);\n> +\t\ttopology.ptr_interfaces = reinterpret_cast<__u64>(interfaces);\n> \n> \t\tret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);\n> \t\tif (ret < 0) {\n> @@ -236,10 +238,12 @@ int MediaDevice::populate()\n> \t\tdelete[] links;\n> \t\tdelete[] ents;\n> \t\tdelete[] pads;\n> +\t\tdelete[] interfaces;\n> \n> \t\tents = new media_v2_entity[topology.num_entities];\n> \t\tlinks = new media_v2_link[topology.num_links];\n> \t\tpads = new media_v2_pad[topology.num_pads];\n> +\t\tinterfaces = new media_v2_interface[topology.num_interfaces];\n> \n> \t\tversion = topology.topology_version;\n> \t}\n> @@ -253,6 +257,7 @@ int MediaDevice::populate()\n> \tdelete[] links;\n> \tdelete[] ents;\n> \tdelete[] pads;\n> +\tdelete[] interfaces;\n> \n> \tif (!valid_) {\n> \t\tclear();\n> @@ -367,6 +372,45 @@ void MediaDevice::clear()\n> * \\brief Global list of media entities in the media graph\n> */\n> \n> +/**\n> + * \\brief Find the interface associated with an entity\n> + * \\param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY\n> + * \\param entityId The entity id\n> + * \\return A pointer to the interface if found, or nullptr otherwise\n> + */\n> +struct media_v2_interface *MediaDevice::findInterface(const struct\n> media_v2_topology &topology, +\t\t\t unsigned int entityId)\n> +{\n> +\tstruct media_v2_link *links = reinterpret_cast\n> +\t\t\t\t\t\t(topology.ptr_links);\n> +\tunsigned int ifaceId = -1;\n> +\n> +\tfor (unsigned int i = 0; i < topology.num_links; ++i) {\n> +\t\t/* Search for the interface to entity link. */\n> +\t\tif (links[i].sink_id != entityId)\n> +\t\t\tcontinue;\n> +\n> +\t\tif ((links[i].flags & MEDIA_LNK_FL_LINK_TYPE) !=\n> +\t\t MEDIA_LNK_FL_INTERFACE_LINK)\n> +\t\t\tcontinue;\n> +\n> +\t\tifaceId = links[i].source_id;\n> +\t}\n> +\n> +\tif (ifaceId == static_cast(-1))\n> +\t\treturn nullptr;\n> +\n> +\tstruct media_v2_interface *ifaces = reinterpret_cast media_v2_interface *> +\t\t\t\t\t\t(topology.ptr_interfaces);\n> +\n> +\tfor (unsigned int i = 0; i < topology.num_interfaces; ++i) {\n> +\t\tif (ifaces[i].id == ifaceId)\n> +\t\t\treturn &ifaces[i];\n> +\t}\n> +\n> +\treturn nullptr;\n> +}\n> +\n> /*\n> * For each entity in the media graph create a MediaEntity and store a\n> * reference in the media device objects map and entities list.\n> @@ -377,7 +421,26 @@ bool MediaDevice::populateEntities(const struct\n> media_v2_topology &topology) (topology.ptr_entities);\n> \n> \tfor (unsigned int i = 0; i < topology.num_entities; ++i) {\n> -\t\tMediaEntity *entity = new MediaEntity(&mediaEntities[i]);\n> +\t\t/*\n> +\t\t * Find the interface linked to this entity to get the device\n> +\t\t * node major and minor numbers.\n> +\t\t */\n> +\t\tstruct media_v2_interface *iface =\n> +\t\t\tfindInterface(topology, mediaEntities[i].id);\n> +\t\tif (!iface) {\n> +\t\t\tLOG(Error) << \"Failed to find interface link for \"\n> +\t\t\t\t << \"entity with id: \" << mediaEntities[i].id;\n> +\t\t\treturn false;\n> +\t\t}\n\nThe above five lines should be dropped. So much for testing patches after \nsending them out :-S\n\n> +\n> +\t\tMediaEntity *entity;\n> +\t\tif (iface)\n> +\t\t\tentity = new MediaEntity(&mediaEntities[i],\n> +\t\t\t\t\t\t iface->devnode.major,\n> +\t\t\t\t\t\t iface->devnode.minor);\n> +\t\telse\n> +\t\t\tentity = new MediaEntity(&mediaEntities[i]);\n> +\n> \t\tif (!addObject(entity)) {\n> \t\t\tdelete entity;\n> \t\t\treturn false;\n> diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp\n> index b64dcc3c8fb4..69e5cc74264d 100644\n> --- a/src/libcamera/media_object.cpp\n> +++ b/src/libcamera/media_object.cpp\n> @@ -6,9 +6,8 @@\n> */\n> \n> #include \n> -#include \n> #include \n> -#include \n> +#include \n> \n> #include \n> #include \n> @@ -212,6 +211,20 @@ void MediaPad::addLink(MediaLink *link)\n> * \\return The entity name\n> */\n> \n> +/**\n> + * \\fn MediaEntity::major()\n> + * \\brief Retrieve the major number of the interface associated with the\n> entity + * \\return The interface major number, or 0 if the entity isn't\n> associated with + * an interface\n> + */\n> +\n> +/**\n> + * \\fn MediaEntity::minor()\n> + * \\brief Retrieve the minor number of the interface associated with the\n> entity + * \\return The interface minor number, or 0 if the entity isn't\n> associated with + * an interface\n> + */\n> +\n> /**\n> * \\fn MediaEntity::pads()\n> * \\brief Retrieve all pads of the entity\n> @@ -238,6 +251,7 @@ const MediaPad *MediaEntity::getPadByIndex(unsigned int\n> index) const * \\param id The pad id\n> * \\return The pad identified by \\a id, or nullptr if no such pad exist\n> */\n> +\n> const MediaPad *MediaEntity::getPadById(unsigned int id) const\n> {\n> \tfor (MediaPad *p : pads_) {\n> @@ -248,12 +262,36 @@ const MediaPad *MediaEntity::getPadById(unsigned int\n> id) const return nullptr;\n> }\n> \n> +/**\n> + * \\brief Set the path to the device node for the associated interface\n> + * \\param devnode The interface device node path associated with this\n> entity + * \\return 0 on success, or a negative error code if the device\n> node can't be + * accessed\n> + */\n> +int MediaEntity::setDeviceNode(const std::string &devnode)\n> +{\n> +\t/* Make sure the device node can be accessed. */\n> +\tint ret = ::access(devnode.c_str(), R_OK | W_OK);\n> +\tif (ret < 0) {\n> +\t\tret = -errno;\n> +\t\tLOG(Error) << \"Device node \" << devnode << \" can't be accessed: \"\n> +\t\t\t << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> /**\n> * \\brief Construct a MediaEntity\n> * \\param entity The media entity kernel data\n> + * \\param major The major number of the entity associated interface\n> + * \\param minor The minor number of the entity associated interface\n> */\n> -MediaEntity::MediaEntity(const struct media_v2_entity *entity)\n> -\t: MediaObject(entity->id), name_(entity->name)\n> +MediaEntity::MediaEntity(const struct media_v2_entity *entity,\n> +\t\t\t unsigned int major, unsigned int minor)\n> +\t: MediaObject(entity->id), name_(entity->name),\n> +\t major_(major), minor_(minor)\n> {\n> }","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C0B6760B30\n\tfor ;\n\tWed, 2 Jan 2019 02:05:06 +0100 (CET)","from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 44ADE505\n\tfor ;\n\tWed, 2 Jan 2019 02:05:05 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546391105;\n\tbh=mdnSqUYRzY2j2esOZCeX//nilXLgjin07tq9L0tevG4=;\n\th=From:To:Subject:Date:In-Reply-To:References:From;\n\tb=efjpf/+2ALXiz6TUIy6QaZogOOSkWXyQwcjrv4EgknZ5zgiTIvKaUI7vD0zxvkTzU\n\t2p0jaHcNiiGsSY+tC/Arl26ea4Ms5q8jjPhfgks6p0S1PJ1sMc6TMKaB7+FjxF/ZzV\n\t5AK371VbbC6AMZ64k3tSdmmpPMvwFkLLtnuVrLg4=","From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Date":"Wed, 02 Jan 2019 03:06:05 +0200","Message-ID":"<3050321.m0LTbSzqVd@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20190102004903.24190-2-laurent.pinchart@ideasonboard.com>","References":"<20190102004903.24190-1-laurent.pinchart@ideasonboard.com>\n\t<20190102004903.24190-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: MediaDevice: Create\n\tentities with major and minor numbers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 02 Jan 2019 01:05:07 -0000"}},{"id":175,"web_url":"https://patchwork.libcamera.org/comment/175/","msgid":"<20190102103823.ylo2v6a43ms3p7bw@uno.localdomain>","date":"2019-01-02T10:38:23","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: MediaDevice: Create\n\tentities with major and minor numbers","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Wed, Jan 02, 2019 at 02:49:02AM +0200, Laurent Pinchart wrote:\n> From: Jacopo Mondi \n>\n> Extend the MediaEntity object with device node major and minor numbers,\n> and retrieve them from the media graph using interfaces. They will be\n> used by the DeviceEnumerator to retrieve the devnode path.\n>\n> Signed-off-by: Jacopo Mondi \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/include/media_device.h | 2 +\n> src/libcamera/include/media_object.h | 9 +++-\n> src/libcamera/media_device.cpp | 65 +++++++++++++++++++++++++++-\n> src/libcamera/media_object.cpp | 46 ++++++++++++++++++--\n> 4 files changed, 116 insertions(+), 6 deletions(-)\n>\n> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h\n> index 3fcdb4b4d5f8..8d491a87867c 100644\n> --- a/src/libcamera/include/media_device.h\n> +++ b/src/libcamera/include/media_device.h\n> @@ -54,6 +54,8 @@ private:\n>\n> \tstd::vector entities_;\n>\n> +\tstruct media_v2_interface *findInterface(const struct media_v2_topology &topology,\n> +\t\t\t\t\t\t unsigned int entityId);\n\nTo respect the 80-col, I dropped 'struct' from media_v2_ variables in\nthe 'populate*()' functions.\n\n> \tbool populateEntities(const struct media_v2_topology &topology);\n> \tbool populatePads(const struct media_v2_topology &topology);\n> \tbool populateLinks(const struct media_v2_topology &topology);\n> diff --git a/src/libcamera/include/media_object.h b/src/libcamera/include/media_object.h\n> index 65b55085a3b0..950a33286690 100644\n> --- a/src/libcamera/include/media_object.h\n> +++ b/src/libcamera/include/media_object.h\n> @@ -80,21 +80,28 @@ class MediaEntity : public MediaObject\n> {\n> public:\n> \tconst std::string &name() const { return name_; }\n> +\tunsigned int major() const { return major_; }\n> +\tunsigned int minor() const { return minor_; }\n>\n> \tconst std::vector &pads() const { return pads_; }\n>\n> \tconst MediaPad *getPadByIndex(unsigned int index) const;\n> \tconst MediaPad *getPadById(unsigned int id) const;\n>\n> +\tint setDeviceNode(const std::string &devnode);\n> +\n\nI'm fine adding it here, but it is not currently used.\n\n> private:\n> \tfriend class MediaDevice;\n>\n> -\tMediaEntity(const struct media_v2_entity *entity);\n> +\tMediaEntity(const struct media_v2_entity *entity,\n> +\t\t unsigned int major = 0, unsigned int minor = 0);\n> \tMediaEntity(const MediaEntity &) = delete;\n> \t~MediaEntity();\n>\n> \tstd::string name_;\n> \tstd::string devnode_;\n> +\tunsigned int major_;\n> +\tunsigned int minor_;\n>\n> \tstd::vector pads_;\n>\n> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> index 605e504be124..70b3fff3f492 100644\n> --- a/src/libcamera/media_device.cpp\n> +++ b/src/libcamera/media_device.cpp\n> @@ -208,6 +208,7 @@ int MediaDevice::populate()\n> \tstruct media_v2_entity *ents = nullptr;\n> \tstruct media_v2_link *links = nullptr;\n> \tstruct media_v2_pad *pads = nullptr;\n> +\tstruct media_v2_interface *interfaces = nullptr;\n\nThat's my OCD, but could you move this line 3 lines up? This gives\nlines a nicer ordering\n\n> \t__u64 version = -1;\n> \tint ret;\n>\n> @@ -221,6 +222,7 @@ int MediaDevice::populate()\n> \t\ttopology.ptr_entities = reinterpret_cast<__u64>(ents);\n> \t\ttopology.ptr_links = reinterpret_cast<__u64>(links);\n> \t\ttopology.ptr_pads = reinterpret_cast<__u64>(pads);\n> +\t\ttopology.ptr_interfaces = reinterpret_cast<__u64>(interfaces);\n\nSame here\n\n>\n> \t\tret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);\n> \t\tif (ret < 0) {\n> @@ -236,10 +238,12 @@ int MediaDevice::populate()\n> \t\tdelete[] links;\n> \t\tdelete[] ents;\n> \t\tdelete[] pads;\n> +\t\tdelete[] interfaces;\n\nSame here\n\n>\n> \t\tents = new media_v2_entity[topology.num_entities];\n> \t\tlinks = new media_v2_link[topology.num_links];\n> \t\tpads = new media_v2_pad[topology.num_pads];\n> +\t\tinterfaces = new media_v2_interface[topology.num_interfaces];\n\nAnd here.\n\n>\n> \t\tversion = topology.topology_version;\n> \t}\n> @@ -253,6 +257,7 @@ int MediaDevice::populate()\n> \tdelete[] links;\n> \tdelete[] ents;\n> \tdelete[] pads;\n> +\tdelete[] interfaces;\n\nAnd here.\n\nThe patch will look weird, but the resulting code will be nicer.\n\n>\n> \tif (!valid_) {\n> \t\tclear();\n> @@ -367,6 +372,45 @@ void MediaDevice::clear()\n> * \\brief Global list of media entities in the media graph\n> */\n>\n> +/**\n> + * \\brief Find the interface associated with an entity\n> + * \\param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY\n> + * \\param entityId The entity id\n> + * \\return A pointer to the interface if found, or nullptr otherwise\n> + */\n\nThis is a private function, we can document it, I agree, but leave\ndoxygen commands out.\n\n> +struct media_v2_interface *MediaDevice::findInterface(const struct media_v2_topology &topology,\n> +\t\t\t unsigned int entityId)\n> +{\n> +\tstruct media_v2_link *links = reinterpret_cast\n> +\t\t\t\t\t\t(topology.ptr_links);\n\n\tmedia_v2_link *links = reinterpret_cast\n (topology.ptr_links);\n\nTo make it the same as in other populate*() functions\n\n> +\tunsigned int ifaceId = -1;\n\nunsigned int initialized with -1 ?\n\n> +\n> +\tfor (unsigned int i = 0; i < topology.num_links; ++i) {\n> +\t\t/* Search for the interface to entity link. */\n> +\t\tif (links[i].sink_id != entityId)\n> +\t\t\tcontinue;\n> +\n> +\t\tif ((links[i].flags & MEDIA_LNK_FL_LINK_TYPE) !=\n> +\t\t MEDIA_LNK_FL_INTERFACE_LINK)\n> +\t\t\tcontinue;\n> +\n> +\t\tifaceId = links[i].source_id;\n\nbreak?\n\n> +\t}\n> +\n> +\tif (ifaceId == static_cast(-1))\n> +\t\treturn nullptr;\n> +\n> +\tstruct media_v2_interface *ifaces = reinterpret_cast\n> +\t\t\t\t\t\t(topology.ptr_interfaces);\n\ndrop struct here as well?\n\n> +\n> +\tfor (unsigned int i = 0; i < topology.num_interfaces; ++i) {\n> + if (ifaces[i].id == ifaceId)\n> +\t\t\treturn &ifaces[i];\n> +\t}\n> +\n> +\treturn nullptr;\n> +}\n> +\n> /*\n> * For each entity in the media graph create a MediaEntity and store a\n> * reference in the media device objects map and entities list.\n> @@ -377,7 +421,26 @@ bool MediaDevice::populateEntities(const struct media_v2_topology &topology)\n> \t\t\t\t\t (topology.ptr_entities);\n>\n> \tfor (unsigned int i = 0; i < topology.num_entities; ++i) {\n> -\t\tMediaEntity *entity = new MediaEntity(&mediaEntities[i]);\n> +\t\t/*\n> +\t\t * Find the interface linked to this entity to get the device\n> +\t\t * node major and minor numbers.\n> +\t\t */\n> +\t\tstruct media_v2_interface *iface =\n> +\t\t\tfindInterface(topology, mediaEntities[i].id);\n> +\t\tif (!iface) {\n> +\t\t\tLOG(Error) << \"Failed to find interface link for \"\n> +\t\t\t\t << \"entity with id: \" << mediaEntities[i].id;\n> +\t\t\treturn false;\n> +\t\t}\n\nAs you pointed out already, it is fine to have entities without an\ninterface associated.\n\n> +\n> +\t\tMediaEntity *entity;\n> +\t\tif (iface)\n> +\t\t\tentity = new MediaEntity(&mediaEntities[i],\n> +\t\t\t\t\t\t iface->devnode.major,\n> +\t\t\t\t\t\t iface->devnode.minor);\n> +\t\telse\n> +\t\t\tentity = new MediaEntity(&mediaEntities[i]);\n> diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp\n> index b64dcc3c8fb4..69e5cc74264d 100644\n> --- a/src/libcamera/media_object.cpp\n> +++ b/src/libcamera/media_object.cpp\n> @@ -6,9 +6,8 @@\n> */\n>\n> #include \n> -#include \n> #include \n> -#include \n> +#include \n>\n> #include \n> #include \n> @@ -212,6 +211,20 @@ void MediaPad::addLink(MediaLink *link)\n> * \\return The entity name\n> */\n>\n> +/**\n> + * \\fn MediaEntity::major()\n> + * \\brief Retrieve the major number of the interface associated with the entity\n> + * \\return The interface major number, or 0 if the entity isn't associated with\n> + * an interface\n> + */\n> +\n> +/**\n> + * \\fn MediaEntity::minor()\n> + * \\brief Retrieve the minor number of the interface associated with the entity\n> + * \\return The interface minor number, or 0 if the entity isn't associated with\n> + * an interface\n> + */\n> +\n> /**\n> * \\fn MediaEntity::pads()\n> * \\brief Retrieve all pads of the entity\n> @@ -238,6 +251,7 @@ const MediaPad *MediaEntity::getPadByIndex(unsigned int index) const\n> * \\param id The pad id\n> * \\return The pad identified by \\a id, or nullptr if no such pad exist\n> */\n> +\n\nIs this needed?\n\n> const MediaPad *MediaEntity::getPadById(unsigned int id) const\n> {\n> \tfor (MediaPad *p : pads_) {\n> @@ -248,12 +262,36 @@ const MediaPad *MediaEntity::getPadById(unsigned int id) const\n> \treturn nullptr;\n> }\n>\n> +/**\n> + * \\brief Set the path to the device node for the associated interface\n> + * \\param devnode The interface device node path associated with this entity\n> + * \\return 0 on success, or a negative error code if the device node can't be\n> + * accessed\n> + */\n> +int MediaEntity::setDeviceNode(const std::string &devnode)\n> +{\n> +\t/* Make sure the device node can be accessed. */\n> +\tint ret = ::access(devnode.c_str(), R_OK | W_OK);\n> +\tif (ret < 0) {\n> +\t\tret = -errno;\n> +\t\tLOG(Error) << \"Device node \" << devnode << \" can't be accessed: \"\n> +\t\t\t << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> /**\n> * \\brief Construct a MediaEntity\n> * \\param entity The media entity kernel data\n> + * \\param major The major number of the entity associated interface\n> + * \\param minor The minor number of the entity associated interface\n\nShould we say they're defaulted to 0 ?\n\nThanks\n j\n\n\n> */\n> -MediaEntity::MediaEntity(const struct media_v2_entity *entity)\n> -\t: MediaObject(entity->id), name_(entity->name)\n> +MediaEntity::MediaEntity(const struct media_v2_entity *entity,\n> +\t\t\t unsigned int major, unsigned int minor)\n> +\t: MediaObject(entity->id), name_(entity->name),\n> +\t major_(major), minor_(minor)\n> {\n> }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0BD5A60B30\n\tfor ;\n\tWed, 2 Jan 2019 11:38:20 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 8A78720004;\n\tWed, 2 Jan 2019 10:38:19 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 2 Jan 2019 11:38:23 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190102103823.ylo2v6a43ms3p7bw@uno.localdomain>","References":"<20190102004903.24190-1-laurent.pinchart@ideasonboard.com>\n\t<20190102004903.24190-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"yeo4vgbrxsp47nkb\"","Content-Disposition":"inline","In-Reply-To":"<20190102004903.24190-2-laurent.pinchart@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: MediaDevice: Create\n\tentities with major and minor numbers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 02 Jan 2019 10:38:20 -0000"}},{"id":176,"web_url":"https://patchwork.libcamera.org/comment/176/","msgid":"<2238593.UCP04szAmJ@avalon>","date":"2019-01-02T10:55:00","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: MediaDevice: Create\n\tentities with major and minor numbers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wednesday, 2 January 2019 12:38:23 EET Jacopo Mondi wrote:\n> On Wed, Jan 02, 2019 at 02:49:02AM +0200, Laurent Pinchart wrote:\n> > From: Jacopo Mondi \n> > \n> > Extend the MediaEntity object with device node major and minor numbers,\n> > and retrieve them from the media graph using interfaces. They will be\n> > used by the DeviceEnumerator to retrieve the devnode path.\n> > \n> > Signed-off-by: Jacopo Mondi \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > \n> > src/libcamera/include/media_device.h | 2 +\n> > src/libcamera/include/media_object.h | 9 +++-\n> > src/libcamera/media_device.cpp | 65 +++++++++++++++++++++++++++-\n> > src/libcamera/media_object.cpp | 46 ++++++++++++++++++--\n> > 4 files changed, 116 insertions(+), 6 deletions(-)\n> > \n> > diff --git a/src/libcamera/include/media_device.h\n> > b/src/libcamera/include/media_device.h index 3fcdb4b4d5f8..8d491a87867c\n> > 100644\n> > --- a/src/libcamera/include/media_device.h\n> > +++ b/src/libcamera/include/media_device.h\n> > \n> > @@ -54,6 +54,8 @@ private:\n> > \tstd::vector entities_;\n> > \n> > +\tstruct media_v2_interface *findInterface(const struct media_v2_topology\n> > &topology,\n> > +\t\t\t\t\t\t unsigned int entityId);\n> \n> To respect the 80-col, I dropped 'struct' from media_v2_ variables in\n> the 'populate*()' functions.\n\nC++ allows us to do that, but I've kept it to emphasize that we're dealing \nwith kernel structures. We're free to pick one option or another, but it \nshould be applied globally.\n\n> > \tbool populateEntities(const struct media_v2_topology &topology);\n> > \tbool populatePads(const struct media_v2_topology &topology);\n> > \tbool populateLinks(const struct media_v2_topology &topology);\n> > diff --git a/src/libcamera/include/media_object.h\n> > b/src/libcamera/include/media_object.h index 65b55085a3b0..950a33286690\n> > 100644\n> > --- a/src/libcamera/include/media_object.h\n> > +++ b/src/libcamera/include/media_object.h\n> > @@ -80,21 +80,28 @@ class MediaEntity : public MediaObject\n> > {\n> > public:\n> > \tconst std::string &name() const { return name_; }\n> > +\tunsigned int major() const { return major_; }\n> > +\tunsigned int minor() const { return minor_; }\n> > \n> > \tconst std::vector &pads() const { return pads_; }\n> > \t\n> > \tconst MediaPad *getPadByIndex(unsigned int index) const;\n> > \tconst MediaPad *getPadById(unsigned int id) const;\n> > \n> > +\tint setDeviceNode(const std::string &devnode);\n> > +\n> \n> I'm fine adding it here, but it is not currently used.\n\nI think it was added here in your original patch :-)\n\n> > private:\n> > \tfriend class MediaDevice;\n> > \n> > -\tMediaEntity(const struct media_v2_entity *entity);\n> > +\tMediaEntity(const struct media_v2_entity *entity,\n> > +\t\t unsigned int major = 0, unsigned int minor = 0);\n> > \tMediaEntity(const MediaEntity &) = delete;\n> > \t~MediaEntity();\n> > \t\n> > \tstd::string name_;\n> > \tstd::string devnode_;\n> > +\tunsigned int major_;\n> > +\tunsigned int minor_;\n> > \n> > \tstd::vector pads_;\n> > \n> > diff --git a/src/libcamera/media_device.cpp\n> > b/src/libcamera/media_device.cpp index 605e504be124..70b3fff3f492 100644\n> > --- a/src/libcamera/media_device.cpp\n> > +++ b/src/libcamera/media_device.cpp\n> > @@ -208,6 +208,7 @@ int MediaDevice::populate()\n> > \tstruct media_v2_entity *ents = nullptr;\n> > \tstruct media_v2_link *links = nullptr;\n> > \tstruct media_v2_pad *pads = nullptr;\n> > +\tstruct media_v2_interface *interfaces = nullptr;\n> \n> That's my OCD, but could you move this line 3 lines up? This gives\n> lines a nicer ordering\n\nI wanted to keep the same order as in the media_v2_topology structure, but I \nnow realize that interfaces come just after entities (and links after pads). \nHow about reordering them in that order in a follow-up patch ?\n\n[snip]\n\n> > @@ -367,6 +372,45 @@ void MediaDevice::clear()\n> > * \\brief Global list of media entities in the media graph\n> > */\n> > \n> > +/**\n> > + * \\brief Find the interface associated with an entity\n> > + * \\param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY\n> > + * \\param entityId The entity id\n> > + * \\return A pointer to the interface if found, or nullptr otherwise\n> > + */\n> \n> This is a private function, we can document it, I agree, but leave\n> doxygen commands out.\n\nWhy so ? I think we should standardize on a single syntax for documentation.\n\n> > +struct media_v2_interface *MediaDevice::findInterface(const struct\n> > media_v2_topology &topology, +\t\t\t unsigned int entityId)\n> > +{\n> > +\tstruct media_v2_link *links = reinterpret_cast\n> > +\t\t\t\t\t\t(topology.ptr_links);\n> \n> \tmedia_v2_link *links = reinterpret_cast\n> (topology.ptr_links);\n> \n> To make it the same as in other populate*() functions\n\nLet's discuss struct media_v2_link vs. media_v2_link globally as mentioned \nabove, and update the code accordingly.\n\n> > +\tunsigned int ifaceId = -1;\n> \n> unsigned int initialized with -1 ?\n\nThe compiler doesn't warn :-)\n\n> > +\n> > +\tfor (unsigned int i = 0; i < topology.num_links; ++i) {\n> > +\t\t/* Search for the interface to entity link. */\n> > +\t\tif (links[i].sink_id != entityId)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tif ((links[i].flags & MEDIA_LNK_FL_LINK_TYPE) !=\n> > +\t\t MEDIA_LNK_FL_INTERFACE_LINK)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tifaceId = links[i].source_id;\n> \n> break?\n\nIndeed. Fixed.\n\n> > +\t}\n> > +\n> > +\tif (ifaceId == static_cast(-1))\n> > +\t\treturn nullptr;\n> > +\n> > +\tstruct media_v2_interface *ifaces = reinterpret_cast > media_v2_interface *>\n> > +\t\t\t\t\t\t(topology.ptr_interfaces);\n> \n> drop struct here as well?\n> \n> > +\n> > +\tfor (unsigned int i = 0; i < topology.num_interfaces; ++i) {\n> > + if (ifaces[i].id == ifaceId)\n> > +\t\t\treturn &ifaces[i];\n> > +\t}\n> > +\n> > +\treturn nullptr;\n> > +}\n> > +\n> > /*\n> > * For each entity in the media graph create a MediaEntity and store a\n> > * reference in the media device objects map and entities list.\n> > @@ -377,7 +421,26 @@ bool MediaDevice::populateEntities(const struct\n> > media_v2_topology &topology)> \n> > \t\t\t\t\t (topology.ptr_entities);\n> > \t\n> > \tfor (unsigned int i = 0; i < topology.num_entities; ++i) {\n> > \n> > -\t\tMediaEntity *entity = new MediaEntity(&mediaEntities[i]);\n> > +\t\t/*\n> > +\t\t * Find the interface linked to this entity to get the device\n> > +\t\t * node major and minor numbers.\n> > +\t\t */\n> > +\t\tstruct media_v2_interface *iface =\n> > +\t\t\tfindInterface(topology, mediaEntities[i].id);\n> > +\t\tif (!iface) {\n> > +\t\t\tLOG(Error) << \"Failed to find interface link for \"\n> > +\t\t\t\t << \"entity with id: \" << mediaEntities[i].id;\n> > +\t\t\treturn false;\n> > +\t\t}\n> \n> As you pointed out already, it is fine to have entities without an\n> interface associated.\n\nYes, I've dropped this check.\n\n> > +\n> > +\t\tMediaEntity *entity;\n> > +\t\tif (iface)\n> > +\t\t\tentity = new MediaEntity(&mediaEntities[i],\n> > +\t\t\t\t\t\t iface->devnode.major,\n> > +\t\t\t\t\t\t iface->devnode.minor);\n> > +\t\telse\n> > +\t\t\tentity = new MediaEntity(&mediaEntities[i]);\n> > diff --git a/src/libcamera/media_object.cpp\n> > b/src/libcamera/media_object.cpp index b64dcc3c8fb4..69e5cc74264d 100644\n> > --- a/src/libcamera/media_object.cpp\n> > +++ b/src/libcamera/media_object.cpp\n\n[snip]\n\n> > @@ -238,6 +251,7 @@ const MediaPad *MediaEntity::getPadByIndex(unsigned\n> > int index) const\n> > * \\param id The pad id\n> > * \\return The pad identified by \\a id, or nullptr if no such pad exist\n> > */\n> > +\n> \n> Is this needed?\n\nNot at all, dropped.\n\n> > const MediaPad *MediaEntity::getPadById(unsigned int id) const\n> > {\n> > \tfor (MediaPad *p : pads_) {\n> > @@ -248,12 +262,36 @@ const MediaPad *MediaEntity::getPadById(unsigned int\n> > id) const\n> > \treturn nullptr;\n> > }\n> > \n> > +/**\n> > + * \\brief Set the path to the device node for the associated interface\n> > + * \\param devnode The interface device node path associated with this\n> > entity + * \\return 0 on success, or a negative error code if the device\n> > node can't be + * accessed\n> > + */\n> > +int MediaEntity::setDeviceNode(const std::string &devnode)\n> > +{\n> > +\t/* Make sure the device node can be accessed. */\n> > +\tint ret = ::access(devnode.c_str(), R_OK | W_OK);\n> > +\tif (ret < 0) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(Error) << \"Device node \" << devnode << \" can't be accessed: \"\n> > +\t\t\t << strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > /**\n> > * \\brief Construct a MediaEntity\n> > * \\param entity The media entity kernel data\n> > + * \\param major The major number of the entity associated interface\n> > + * \\param minor The minor number of the entity associated interface\n> \n> Should we say they're defaulted to 0 ?\n\nDoxygen shows the default value in the function prototype already, I don't \nthink we need to duplicate that information.\n\n> > */\n> > -MediaEntity::MediaEntity(const struct media_v2_entity *entity)\n> > -\t: MediaObject(entity->id), name_(entity->name)\n> > +MediaEntity::MediaEntity(const struct media_v2_entity *entity,\n> > +\t\t\t unsigned int major, unsigned int minor)\n> > +\t: MediaObject(entity->id), name_(entity->name),\n> > +\t major_(major), minor_(minor)\n> > \n> > {\n> > }","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B84C960B30\n\tfor ;\n\tWed, 2 Jan 2019 11:54:00 +0100 (CET)","from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 26729505;\n\tWed, 2 Jan 2019 11:54:00 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546426440;\n\tbh=atcWJ9hVLSUwRB8e0B6T+YfUF8A8p5Pt/+oPZCUqu9w=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=A4Rn9FD5QE4Ud9hOzyQLai7NAc1e81rbnkACCcozLDBc7kWVM6MPyE/uosZEXIwGH\n\tcoHOimtvhPQJLtqD11bgnNRRIFoT3qVeiYwaeh5qIz+KZmT0F6mSt5bkXvZo7jiGDZ\n\t97a6Fl5YxZf7A+T0BBOhlfJItd6qjAaU7+135VaA=","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Wed, 02 Jan 2019 12:55:00 +0200","Message-ID":"<2238593.UCP04szAmJ@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20190102103823.ylo2v6a43ms3p7bw@uno.localdomain>","References":"<20190102004903.24190-1-laurent.pinchart@ideasonboard.com>\n\t<20190102004903.24190-2-laurent.pinchart@ideasonboard.com>\n\t<20190102103823.ylo2v6a43ms3p7bw@uno.localdomain>","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: MediaDevice: Create\n\tentities with major and minor numbers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 02 Jan 2019 10:54:00 -0000"}},{"id":179,"web_url":"https://patchwork.libcamera.org/comment/179/","msgid":"<20190102111340.amrfg5telk6htrg4@uno.localdomain>","date":"2019-01-02T11:13:40","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: MediaDevice: Create\n\tentities with major and minor numbers","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Wed, Jan 02, 2019 at 12:55:00PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Wednesday, 2 January 2019 12:38:23 EET Jacopo Mondi wrote:\n> > On Wed, Jan 02, 2019 at 02:49:02AM +0200, Laurent Pinchart wrote:\n> > > From: Jacopo Mondi \n> > >\n> > > Extend the MediaEntity object with device node major and minor numbers,\n> > > and retrieve them from the media graph using interfaces. They will be\n> > > used by the DeviceEnumerator to retrieve the devnode path.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > >\n> > > src/libcamera/include/media_device.h | 2 +\n> > > src/libcamera/include/media_object.h | 9 +++-\n> > > src/libcamera/media_device.cpp | 65 +++++++++++++++++++++++++++-\n> > > src/libcamera/media_object.cpp | 46 ++++++++++++++++++--\n> > > 4 files changed, 116 insertions(+), 6 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/include/media_device.h\n> > > b/src/libcamera/include/media_device.h index 3fcdb4b4d5f8..8d491a87867c\n> > > 100644\n> > > --- a/src/libcamera/include/media_device.h\n> > > +++ b/src/libcamera/include/media_device.h\n> > >\n> > > @@ -54,6 +54,8 @@ private:\n> > > \tstd::vector entities_;\n> > >\n> > > +\tstruct media_v2_interface *findInterface(const struct media_v2_topology\n> > > &topology,\n> > > +\t\t\t\t\t\t unsigned int entityId);\n> >\n> > To respect the 80-col, I dropped 'struct' from media_v2_ variables in\n> > the 'populate*()' functions.\n>\n> C++ allows us to do that, but I've kept it to emphasize that we're dealing\n> with kernel structures. We're free to pick one option or another, but it\n> should be applied globally.\n\nI'm not a fan of omitting 'struct' neither, but in that case the code\nwas more readable and fit in 80 columns.\n\nLet's define a rule globally. I would tend to keep struct\nPlease not the populate*() functions will have to be re-adjusted, I\ncould do that, send a patch, and then you can rebase on top, or you\ncould do it here.\n\n>\n> > > \tbool populateEntities(const struct media_v2_topology &topology);\n> > > \tbool populatePads(const struct media_v2_topology &topology);\n> > > \tbool populateLinks(const struct media_v2_topology &topology);\n> > > diff --git a/src/libcamera/include/media_object.h\n> > > b/src/libcamera/include/media_object.h index 65b55085a3b0..950a33286690\n> > > 100644\n> > > --- a/src/libcamera/include/media_object.h\n> > > +++ b/src/libcamera/include/media_object.h\n> > > @@ -80,21 +80,28 @@ class MediaEntity : public MediaObject\n> > > {\n> > > public:\n> > > \tconst std::string &name() const { return name_; }\n> > > +\tunsigned int major() const { return major_; }\n> > > +\tunsigned int minor() const { return minor_; }\n> > >\n> > > \tconst std::vector &pads() const { return pads_; }\n> > >\n> > > \tconst MediaPad *getPadByIndex(unsigned int index) const;\n> > > \tconst MediaPad *getPadById(unsigned int id) const;\n> > >\n> > > +\tint setDeviceNode(const std::string &devnode);\n> > > +\n> >\n> > I'm fine adding it here, but it is not currently used.\n>\n> I think it was added here in your original patch :-)\n>\n\nTold you: \"I'm fine adding it here\"\n:)\n\n\n> > > private:\n> > > \tfriend class MediaDevice;\n> > >\n> > > -\tMediaEntity(const struct media_v2_entity *entity);\n> > > +\tMediaEntity(const struct media_v2_entity *entity,\n> > > +\t\t unsigned int major = 0, unsigned int minor = 0);\n> > > \tMediaEntity(const MediaEntity &) = delete;\n> > > \t~MediaEntity();\n> > >\n> > > \tstd::string name_;\n> > > \tstd::string devnode_;\n> > > +\tunsigned int major_;\n> > > +\tunsigned int minor_;\n> > >\n> > > \tstd::vector pads_;\n> > >\n> > > diff --git a/src/libcamera/media_device.cpp\n> > > b/src/libcamera/media_device.cpp index 605e504be124..70b3fff3f492 100644\n> > > --- a/src/libcamera/media_device.cpp\n> > > +++ b/src/libcamera/media_device.cpp\n> > > @@ -208,6 +208,7 @@ int MediaDevice::populate()\n> > > \tstruct media_v2_entity *ents = nullptr;\n> > > \tstruct media_v2_link *links = nullptr;\n> > > \tstruct media_v2_pad *pads = nullptr;\n> > > +\tstruct media_v2_interface *interfaces = nullptr;\n> >\n> > That's my OCD, but could you move this line 3 lines up? This gives\n> > lines a nicer ordering\n>\n> I wanted to keep the same order as in the media_v2_topology structure, but I\n> now realize that interfaces come just after entities (and links after pads).\n> How about reordering them in that order in a follow-up patch ?\n\nHow about reordering them in reverse-xmas-tree order in a follow-up\npatch instead :)\n\nUltimate bikeshedding here... I'm fine with both actually\n\n>\n> [snip]\n>\n> > > @@ -367,6 +372,45 @@ void MediaDevice::clear()\n> > > * \\brief Global list of media entities in the media graph\n> > > */\n> > >\n> > > +/**\n> > > + * \\brief Find the interface associated with an entity\n> > > + * \\param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY\n> > > + * \\param entityId The entity id\n> > > + * \\return A pointer to the interface if found, or nullptr otherwise\n> > > + */\n> >\n> > This is a private function, we can document it, I agree, but leave\n> > doxygen commands out.\n>\n> Why so ? I think we should standardize on a single syntax for documentation.\n>\n\nOk for consistency. Be aware Doxygen does not parse them though.\n\n> > > +struct media_v2_interface *MediaDevice::findInterface(const struct\n> > > media_v2_topology &topology, +\t\t\t unsigned int entityId)\n> > > +{\n> > > +\tstruct media_v2_link *links = reinterpret_cast\n> > > +\t\t\t\t\t\t(topology.ptr_links);\n> >\n> > \tmedia_v2_link *links = reinterpret_cast\n> > (topology.ptr_links);\n> >\n> > To make it the same as in other populate*() functions\n>\n> Let's discuss struct media_v2_link vs. media_v2_link globally as mentioned\n> above, and update the code accordingly.\n>\n> > > +\tunsigned int ifaceId = -1;\n> >\n> > unsigned int initialized with -1 ?\n>\n> The compiler doesn't warn :-)\n>\n\nSo for the purpose of using this variable to check for a match, this\nshould read s/-1/UINT_MAX/ ?\n\n> > > +\n> > > +\tfor (unsigned int i = 0; i < topology.num_links; ++i) {\n> > > +\t\t/* Search for the interface to entity link. */\n> > > +\t\tif (links[i].sink_id != entityId)\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tif ((links[i].flags & MEDIA_LNK_FL_LINK_TYPE) !=\n> > > +\t\t MEDIA_LNK_FL_INTERFACE_LINK)\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tifaceId = links[i].source_id;\n> >\n> > break?\n>\n> Indeed. Fixed.\n>\n> > > +\t}\n> > > +\n> > > +\tif (ifaceId == static_cast(-1))\n> > > +\t\treturn nullptr;\n\nI wonder what is the value of making it unsigned, and having to go\nthrough a static cast to use -1 as UINT_MAX.\n\nI mean, I now if you declare it as signed int, it has then to be\ncasted to unsigned when comparing it with ids in kernel structure, but\nwhat about checking here for\n\n if (i == topology.num_links)\n\ninstead, and keep using unsigned for ifaceId (dropping the assignament and\ncomparison to -1) ?\n\n> > > +\n> > > +\tstruct media_v2_interface *ifaces = reinterpret_cast > > media_v2_interface *>\n> > > +\t\t\t\t\t\t(topology.ptr_interfaces);\n> >\n> > drop struct here as well?\n> >\n> > > +\n> > > +\tfor (unsigned int i = 0; i < topology.num_interfaces; ++i) {\n> > > + if (ifaces[i].id == ifaceId)\n> > > +\t\t\treturn &ifaces[i];\n> > > +\t}\n> > > +\n> > > +\treturn nullptr;\n> > > +}\n> > > +\n> > > /*\n> > > * For each entity in the media graph create a MediaEntity and store a\n> > > * reference in the media device objects map and entities list.\n> > > @@ -377,7 +421,26 @@ bool MediaDevice::populateEntities(const struct\n> > > media_v2_topology &topology)>\n> > > \t\t\t\t\t (topology.ptr_entities);\n> > >\n> > > \tfor (unsigned int i = 0; i < topology.num_entities; ++i) {\n> > >\n> > > -\t\tMediaEntity *entity = new MediaEntity(&mediaEntities[i]);\n> > > +\t\t/*\n> > > +\t\t * Find the interface linked to this entity to get the device\n> > > +\t\t * node major and minor numbers.\n> > > +\t\t */\n> > > +\t\tstruct media_v2_interface *iface =\n> > > +\t\t\tfindInterface(topology, mediaEntities[i].id);\n> > > +\t\tif (!iface) {\n> > > +\t\t\tLOG(Error) << \"Failed to find interface link for \"\n> > > +\t\t\t\t << \"entity with id: \" << mediaEntities[i].id;\n> > > +\t\t\treturn false;\n> > > +\t\t}\n> >\n> > As you pointed out already, it is fine to have entities without an\n> > interface associated.\n>\n> Yes, I've dropped this check.\n>\n> > > +\n> > > +\t\tMediaEntity *entity;\n> > > +\t\tif (iface)\n> > > +\t\t\tentity = new MediaEntity(&mediaEntities[i],\n> > > +\t\t\t\t\t\t iface->devnode.major,\n> > > +\t\t\t\t\t\t iface->devnode.minor);\n> > > +\t\telse\n> > > +\t\t\tentity = new MediaEntity(&mediaEntities[i]);\n> > > diff --git a/src/libcamera/media_object.cpp\n> > > b/src/libcamera/media_object.cpp index b64dcc3c8fb4..69e5cc74264d 100644\n> > > --- a/src/libcamera/media_object.cpp\n> > > +++ b/src/libcamera/media_object.cpp\n>\n> [snip]\n>\n> > > @@ -238,6 +251,7 @@ const MediaPad *MediaEntity::getPadByIndex(unsigned\n> > > int index) const\n> > > * \\param id The pad id\n> > > * \\return The pad identified by \\a id, or nullptr if no such pad exist\n> > > */\n> > > +\n> >\n> > Is this needed?\n>\n> Not at all, dropped.\n>\n> > > const MediaPad *MediaEntity::getPadById(unsigned int id) const\n> > > {\n> > > \tfor (MediaPad *p : pads_) {\n> > > @@ -248,12 +262,36 @@ const MediaPad *MediaEntity::getPadById(unsigned int\n> > > id) const\n> > > \treturn nullptr;\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Set the path to the device node for the associated interface\n> > > + * \\param devnode The interface device node path associated with this\n> > > entity + * \\return 0 on success, or a negative error code if the device\n> > > node can't be + * accessed\n> > > + */\n> > > +int MediaEntity::setDeviceNode(const std::string &devnode)\n> > > +{\n> > > +\t/* Make sure the device node can be accessed. */\n> > > +\tint ret = ::access(devnode.c_str(), R_OK | W_OK);\n> > > +\tif (ret < 0) {\n> > > +\t\tret = -errno;\n> > > +\t\tLOG(Error) << \"Device node \" << devnode << \" can't be accessed: \"\n> > > +\t\t\t << strerror(-ret);\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Construct a MediaEntity\n> > > * \\param entity The media entity kernel data\n> > > + * \\param major The major number of the entity associated interface\n> > > + * \\param minor The minor number of the entity associated interface\n> >\n> > Should we say they're defaulted to 0 ?\n>\n> Doxygen shows the default value in the function prototype already, I don't\n> think we need to duplicate that information.\n>\nAh fine, I didn't know.\n\nFine with me, with the few bit addressed (as you like the most, that's\nminor stuff) please push this.\n\nThanks\n j\n\n> > > */\n> > > -MediaEntity::MediaEntity(const struct media_v2_entity *entity)\n> > > -\t: MediaObject(entity->id), name_(entity->name)\n> > > +MediaEntity::MediaEntity(const struct media_v2_entity *entity,\n> > > +\t\t\t unsigned int major, unsigned int minor)\n> > > +\t: MediaObject(entity->id), name_(entity->name),\n> > > +\t major_(major), minor_(minor)\n> > >\n> > > {\n> > > }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n>\n>","headers":{"Return-Path":"","Received":["from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B91C460B0C\n\tfor ;\n\tWed, 2 Jan 2019 12:13:37 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 2122FE0011;\n\tWed, 2 Jan 2019 11:13:35 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 2 Jan 2019 12:13:40 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190102111340.amrfg5telk6htrg4@uno.localdomain>","References":"<20190102004903.24190-1-laurent.pinchart@ideasonboard.com>\n\t<20190102004903.24190-2-laurent.pinchart@ideasonboard.com>\n\t<20190102103823.ylo2v6a43ms3p7bw@uno.localdomain>\n\t<2238593.UCP04szAmJ@avalon>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"g3rhge7rl5zp6vc2\"","Content-Disposition":"inline","In-Reply-To":"<2238593.UCP04szAmJ@avalon>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: MediaDevice: Create\n\tentities with major and minor numbers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 02 Jan 2019 11:13:38 -0000"}},{"id":180,"web_url":"https://patchwork.libcamera.org/comment/180/","msgid":"<26666434.3MmAZ76pvh@avalon>","date":"2019-01-02T11:29:02","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: MediaDevice: Create\n\tentities with major and minor numbers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wednesday, 2 January 2019 13:13:40 EET Jacopo Mondi wrote:\n> On Wed, Jan 02, 2019 at 12:55:00PM +0200, Laurent Pinchart wrote:\n> > On Wednesday, 2 January 2019 12:38:23 EET Jacopo Mondi wrote:\n> > > On Wed, Jan 02, 2019 at 02:49:02AM +0200, Laurent Pinchart wrote:\n> > > > From: Jacopo Mondi \n> > > > \n> > > > Extend the MediaEntity object with device node major and minor\n> > > > numbers, and retrieve them from the media graph using interfaces. They\n> > > > will be used by the DeviceEnumerator to retrieve the devnode path.\n> > > > \n> > > > Signed-off-by: Jacopo Mondi \n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > \n> > > > src/libcamera/include/media_device.h | 2 +\n> > > > src/libcamera/include/media_object.h | 9 +++-\n> > > > src/libcamera/media_device.cpp | 65 ++++++++++++++++++++++++++-\n> > > > src/libcamera/media_object.cpp | 46 ++++++++++++++++++--\n> > > > 4 files changed, 116 insertions(+), 6 deletions(-)\n> > > > \n> > > > diff --git a/src/libcamera/include/media_device.h\n> > > > b/src/libcamera/include/media_device.h index\n> > > > 3fcdb4b4d5f8..8d491a87867c\n> > > > 100644\n> > > > --- a/src/libcamera/include/media_device.h\n> > > > +++ b/src/libcamera/include/media_device.h\n> > > > \n> > > > @@ -54,6 +54,8 @@ private:\n> > > > \tstd::vector entities_;\n> > > > \n> > > > +\tstruct media_v2_interface *findInterface(const struct\n> > > > media_v2_topology\n> > > > &topology,\n> > > > +\t\t\t\t\t\t unsigned int entityId);\n> > > \n> > > To respect the 80-col, I dropped 'struct' from media_v2_ variables in\n> > > the 'populate*()' functions.\n> > \n> > C++ allows us to do that, but I've kept it to emphasize that we're dealing\n> > with kernel structures. We're free to pick one option or another, but it\n> > should be applied globally.\n> \n> I'm not a fan of omitting 'struct' neither, but in that case the code\n> was more readable and fit in 80 columns.\n> \n> Let's define a rule globally. I would tend to keep struct\n> Please not the populate*() functions will have to be re-adjusted, I\n> could do that, send a patch, and then you can rebase on top, or you\n> could do it here.\n\nKeeping the struct keyword has my preference too. You can submit a patch on \ntop of the master branch, or I can do it if you prefer.\n\n> > > > \tbool populateEntities(const struct media_v2_topology &topology);\n> > > > \tbool populatePads(const struct media_v2_topology &topology);\n> > > > \tbool populateLinks(const struct media_v2_topology &topology);\n\n[snip]\n\n> > > > diff --git a/src/libcamera/media_device.cpp\n> > > > b/src/libcamera/media_device.cpp index 605e504be124..70b3fff3f492\n> > > > 100644\n> > > > --- a/src/libcamera/media_device.cpp\n> > > > +++ b/src/libcamera/media_device.cpp\n> > > > @@ -208,6 +208,7 @@ int MediaDevice::populate()\n> > > > \n> > > > \tstruct media_v2_entity *ents = nullptr;\n> > > > \tstruct media_v2_link *links = nullptr;\n> > > > \tstruct media_v2_pad *pads = nullptr;\n> > > > \n> > > > +\tstruct media_v2_interface *interfaces = nullptr;\n> > > \n> > > That's my OCD, but could you move this line 3 lines up? This gives\n> > > lines a nicer ordering\n> > \n> > I wanted to keep the same order as in the media_v2_topology structure, but\n> > I now realize that interfaces come just after entities (and links after\n> > pads). How about reordering them in that order in a follow-up patch ?\n> \n> How about reordering them in reverse-xmas-tree order in a follow-up\n> patch instead :)\n\nFollow-up patch it should be as I've pushed this patch already :-)\n\n> Ultimate bikeshedding here... I'm fine with both actually\n> \n> > [snip]\n> > \n> > > > @@ -367,6 +372,45 @@ void MediaDevice::clear()\n> > > > * \\brief Global list of media entities in the media graph\n> > > > */\n> > > > \n> > > > +/**\n> > > > + * \\brief Find the interface associated with an entity\n> > > > + * \\param topology The media topology as returned by\n> > > > MEDIA_IOC_G_TOPOLOGY\n> > > > + * \\param entityId The entity id\n> > > > + * \\return A pointer to the interface if found, or nullptr otherwise\n> > > > + */\n> > > \n> > > This is a private function, we can document it, I agree, but leave\n> > > doxygen commands out.\n> > \n> > Why so ? I think we should standardize on a single syntax for\n> > documentation.\n> \n> Ok for consistency. Be aware Doxygen does not parse them though.\n\nI know. That's a bit of a shame.\n\n> > > > +struct media_v2_interface *MediaDevice::findInterface(const struct\n> > > > media_v2_topology &topology,\n> > > > +\t\t\t unsigned int entityId)\n> > > > +{\n> > > > +\tstruct media_v2_link *links = reinterpret_cast > > > *>\n> > > > +\t\t\t\t\t\t(topology.ptr_links);\n> > > \t\n> > > \tmedia_v2_link *links = reinterpret_cast\n> > > \t\n> > > (topology.ptr_links);\n> > > \n> > > To make it the same as in other populate*() functions\n> > \n> > Let's discuss struct media_v2_link vs. media_v2_link globally as mentioned\n> > above, and update the code accordingly.\n> > \n> > > > +\tunsigned int ifaceId = -1;\n> > > \n> > > unsigned int initialized with -1 ?\n> > \n> > The compiler doesn't warn :-)\n> \n> So for the purpose of using this variable to check for a match, this\n> should read s/-1/UINT_MAX/ ?\n> \n> > > > +\n> > > > +\tfor (unsigned int i = 0; i < topology.num_links; ++i) {\n> > > > +\t\t/* Search for the interface to entity link. */\n> > > > +\t\tif (links[i].sink_id != entityId)\n> > > > +\t\t\tcontinue;\n> > > > +\n> > > > +\t\tif ((links[i].flags & MEDIA_LNK_FL_LINK_TYPE) !=\n> > > > +\t\t MEDIA_LNK_FL_INTERFACE_LINK)\n> > > > +\t\t\tcontinue;\n> > > > +\n> > > > +\t\tifaceId = links[i].source_id;\n> > > \n> > > break?\n> > \n> > Indeed. Fixed.\n> > \n> > > > +\t}\n> > > > +\n> > > > +\tif (ifaceId == static_cast(-1))\n> > > > +\t\treturn nullptr;\n> \n> I wonder what is the value of making it unsigned, and having to go\n> through a static cast to use -1 as UINT_MAX.\n> \n> I mean, I now if you declare it as signed int, it has then to be\n> casted to unsigned when comparing it with ids in kernel structure, but\n> what about checking here for\n> \n> if (i == topology.num_links)\n> \n> instead, and keep using unsigned for ifaceId (dropping the assignament and\n> comparison to -1) ?\n\nIt's a good point. Feel free to submit a follow-up patch if you think this \nshould be fixed.\n\n> > > > +\n> > > > +\tstruct media_v2_interface *ifaces = reinterpret_cast > > > media_v2_interface *>\n> > > > +\t\t\t\t\t\t(topology.ptr_interfaces);\n> > > \n> > > drop struct here as well?\n> > > \n> > > > +\n> > > > +\tfor (unsigned int i = 0; i < topology.num_interfaces; ++i) {\n> > > > + if (ifaces[i].id == ifaceId)\n> > > > +\t\t\treturn &ifaces[i];\n> > > > +\t}\n> > > > +\n> > > > +\treturn nullptr;\n> > > > +}\n\n[snip]","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2919A60B2F\n\tfor ;\n\tWed, 2 Jan 2019 12:28:03 +0100 (CET)","from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 49AE2505;\n\tWed, 2 Jan 2019 12:28:02 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546428482;\n\tbh=19YEVPx7di+CVtVx55UTAaZ2TtXEnfoISR/+qq18YvA=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=OEUWmuBiCRoNLgKpGajse89SVQm3AGmvtbQ8nIrHZuKhtwq/WiUxY+E2Q8WYVdeA/\n\t+auvHJaUjN9QcP8KDYJma91lR50i1e13r6YEfuT5gBKHRZTj/bNPnA382m/IfDEDiL\n\tJ1XCpLJ+qxVYvDBT+sF1aU8apbgNYsLmipot/dIc=","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Wed, 02 Jan 2019 13:29:02 +0200","Message-ID":"<26666434.3MmAZ76pvh@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20190102111340.amrfg5telk6htrg4@uno.localdomain>","References":"<20190102004903.24190-1-laurent.pinchart@ideasonboard.com>\n\t<2238593.UCP04szAmJ@avalon>\n\t<20190102111340.amrfg5telk6htrg4@uno.localdomain>","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: MediaDevice: Create\n\tentities with major and minor numbers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 02 Jan 2019 11:28:03 -0000"}}]