[libcamera-devel,v3] libcamera: media_device: Fallback to legacy ioctls on older kernels
diff mbox series

Message ID 20190125230942.17521-1-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • [libcamera-devel,v3] libcamera: media_device: Fallback to legacy ioctls on older kernels
Related show

Commit Message

Laurent Pinchart Jan. 25, 2019, 11:09 p.m. UTC 
Prior to kernel v4.19, the MEDIA_IOC_G_TOPOLOGY ioctl didn't expose
entity flags. Fallback to calling MEDIA_IOC_ENUM_ENTITIES for each
entity to retrieve the flags in that case.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
Changes since v1:

- Move MEDIA_V2_ENTITY_HAS_FLAGS() test to populateEntities()
- Rename fixupEntity() to fixupEntityFlag()
---
 src/libcamera/include/media_device.h |  3 ++
 src/libcamera/media_device.cpp       | 41 ++++++++++++++++++++++++++--
 2 files changed, 41 insertions(+), 3 deletions(-)

Comments

Kieran Bingham Jan. 26, 2019, 7:46 a.m. UTC | #1 
Hi Laurent,

Thank you for looking into this issue,

On 25/01/2019 23:09, Laurent Pinchart wrote:
> Prior to kernel v4.19, the MEDIA_IOC_G_TOPOLOGY ioctl didn't expose
> entity flags. Fallback to calling MEDIA_IOC_ENUM_ENTITIES for each
> entity to retrieve the flags in that case.
> 

This does indeed fix my issue.

Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Is a 'fixes' tag appropriate here? Perhaps not, because it's not so much
a bug in the original code, as a missing feature.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Please push this as soon as you're ready.

Thanks.


> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
> Changes since v1:
> 
> - Move MEDIA_V2_ENTITY_HAS_FLAGS() test to populateEntities()
> - Rename fixupEntity() to fixupEntityFlag()
> ---
>  src/libcamera/include/media_device.h |  3 ++
>  src/libcamera/media_device.cpp       | 41 ++++++++++++++++++++++++++--
>  2 files changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h
> index 27a2b46a4392..9f038093b2b2 100644
> --- a/src/libcamera/include/media_device.h
> +++ b/src/libcamera/include/media_device.h
> @@ -56,6 +56,8 @@ private:
>  	std::string driver_;
>  	std::string deviceNode_;
>  	std::string model_;
> +	unsigned int version_;
> +
>  	int fd_;
>  	bool valid_;
>  	bool acquired_;
> @@ -72,6 +74,7 @@ private:
>  	bool populateEntities(const struct media_v2_topology &topology);
>  	bool populatePads(const struct media_v2_topology &topology);
>  	bool populateLinks(const struct media_v2_topology &topology);
> +	void fixupEntityFlags(struct media_v2_entity *entity);
>  
>  	friend int MediaLink::setEnabled(bool enable);
>  	int setupLink(const MediaLink *link, unsigned int flags);
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index be81bd8c4c23..a4995b996601 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -167,6 +167,7 @@ int MediaDevice::open()
>  
>  	driver_ = info.driver;
>  	model_ = info.model;
> +	version_ = info.media_version;
>  
>  	return 0;
>  }
> @@ -553,20 +554,29 @@ bool MediaDevice::populateEntities(const struct media_v2_topology &topology)
>  						(topology.ptr_entities);
>  
>  	for (unsigned int i = 0; i < topology.num_entities; ++i) {
> +		struct media_v2_entity *ent = &mediaEntities[i];
> +
> +		/*
> +		 * The media_v2_entity structure was missing the flag field before
> +		 * v4.19.
> +		 */
> +		if (!MEDIA_V2_ENTITY_HAS_FLAGS(version_))
> +			fixupEntityFlags(ent);
> +
>  		/*
>  		 * Find the interface linked to this entity to get the device
>  		 * node major and minor numbers.
>  		 */
>  		struct media_v2_interface *iface =
> -			findInterface(topology, mediaEntities[i].id);
> +			findInterface(topology, ent->id);
>  
>  		MediaEntity *entity;
>  		if (iface)
> -			entity = new MediaEntity(this, &mediaEntities[i],
> +			entity = new MediaEntity(this, ent,
>  						 iface->devnode.major,
>  						 iface->devnode.minor);
>  		else
> -			entity = new MediaEntity(this, &mediaEntities[i]);
> +			entity = new MediaEntity(this, ent);
>  
>  		if (!addObject(entity)) {
>  			delete entity;
> @@ -657,6 +667,31 @@ bool MediaDevice::populateLinks(const struct media_v2_topology &topology)
>  	return true;
>  }
>  
> +/**
> + * \brief Fixup entity flags using legacy API

(Very) minor nit :

s/using legacy/using the legacy/

> + * \param[in] entity The entity
> + *
> + * This function is used as a fallback to query entity flags using the legacy
> + * MEDIA_IOC_ENUM_ENTITIES ioctl when running on a kernel version that doesn't
> + * provide them through the MEDIA_IOC_G_TOPOLOGY ioctl.
> + */
> +void MediaDevice::fixupEntityFlags(struct media_v2_entity *entity)
> +{
> +	struct media_entity_desc desc = {};
> +	desc.id = entity->id;
> +
> +	int ret = ioctl(fd_, MEDIA_IOC_ENUM_ENTITIES, &desc);
> +	if (ret < 0) {
> +		ret = -errno;
> +		LOG(MediaDevice, Debug)
> +			<< "Failed to retrieve information for entity "
> +			<< entity->id << ": " << strerror(-ret);
> +		return;
> +	}
> +
> +	entity->flags = desc.flags;
> +}
> +
>  /**
>   * \brief Apply \a flags to a link between two pads
>   * \param link The link to apply flags to
>
Laurent Pinchart Jan. 26, 2019, 8:55 a.m. UTC | #2 
Hi Kieran,

On Sat, Jan 26, 2019 at 07:46:17AM +0000, Kieran Bingham wrote:
> On 25/01/2019 23:09, Laurent Pinchart wrote:
> > Prior to kernel v4.19, the MEDIA_IOC_G_TOPOLOGY ioctl didn't expose
> > entity flags. Fallback to calling MEDIA_IOC_ENUM_ENTITIES for each
> > entity to retrieve the flags in that case.
> > 
> 
> This does indeed fix my issue.
> 
> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Is a 'fixes' tag appropriate here? Perhaps not, because it's not so much
> a bug in the original code, as a missing feature.

I think it makes sense to add a fixes tag pointing to the patch that
broke the test. Done, addressed the issue pointed out below, and pushed.

> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Please push this as soon as you're ready.
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> > Changes since v1:
> > 
> > - Move MEDIA_V2_ENTITY_HAS_FLAGS() test to populateEntities()
> > - Rename fixupEntity() to fixupEntityFlag()
> > ---
> >  src/libcamera/include/media_device.h |  3 ++
> >  src/libcamera/media_device.cpp       | 41 ++++++++++++++++++++++++++--
> >  2 files changed, 41 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h
> > index 27a2b46a4392..9f038093b2b2 100644
> > --- a/src/libcamera/include/media_device.h
> > +++ b/src/libcamera/include/media_device.h
> > @@ -56,6 +56,8 @@ private:
> >  	std::string driver_;
> >  	std::string deviceNode_;
> >  	std::string model_;
> > +	unsigned int version_;
> > +
> >  	int fd_;
> >  	bool valid_;
> >  	bool acquired_;
> > @@ -72,6 +74,7 @@ private:
> >  	bool populateEntities(const struct media_v2_topology &topology);
> >  	bool populatePads(const struct media_v2_topology &topology);
> >  	bool populateLinks(const struct media_v2_topology &topology);
> > +	void fixupEntityFlags(struct media_v2_entity *entity);
> >  
> >  	friend int MediaLink::setEnabled(bool enable);
> >  	int setupLink(const MediaLink *link, unsigned int flags);
> > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> > index be81bd8c4c23..a4995b996601 100644
> > --- a/src/libcamera/media_device.cpp
> > +++ b/src/libcamera/media_device.cpp
> > @@ -167,6 +167,7 @@ int MediaDevice::open()
> >  
> >  	driver_ = info.driver;
> >  	model_ = info.model;
> > +	version_ = info.media_version;
> >  
> >  	return 0;
> >  }
> > @@ -553,20 +554,29 @@ bool MediaDevice::populateEntities(const struct media_v2_topology &topology)
> >  						(topology.ptr_entities);
> >  
> >  	for (unsigned int i = 0; i < topology.num_entities; ++i) {
> > +		struct media_v2_entity *ent = &mediaEntities[i];
> > +
> > +		/*
> > +		 * The media_v2_entity structure was missing the flag field before
> > +		 * v4.19.
> > +		 */
> > +		if (!MEDIA_V2_ENTITY_HAS_FLAGS(version_))
> > +			fixupEntityFlags(ent);
> > +
> >  		/*
> >  		 * Find the interface linked to this entity to get the device
> >  		 * node major and minor numbers.
> >  		 */
> >  		struct media_v2_interface *iface =
> > -			findInterface(topology, mediaEntities[i].id);
> > +			findInterface(topology, ent->id);
> >  
> >  		MediaEntity *entity;
> >  		if (iface)
> > -			entity = new MediaEntity(this, &mediaEntities[i],
> > +			entity = new MediaEntity(this, ent,
> >  						 iface->devnode.major,
> >  						 iface->devnode.minor);
> >  		else
> > -			entity = new MediaEntity(this, &mediaEntities[i]);
> > +			entity = new MediaEntity(this, ent);
> >  
> >  		if (!addObject(entity)) {
> >  			delete entity;
> > @@ -657,6 +667,31 @@ bool MediaDevice::populateLinks(const struct media_v2_topology &topology)
> >  	return true;
> >  }
> >  
> > +/**
> > + * \brief Fixup entity flags using legacy API
> 
> (Very) minor nit :
> 
> s/using legacy/using the legacy/
> 
> > + * \param[in] entity The entity
> > + *
> > + * This function is used as a fallback to query entity flags using the legacy
> > + * MEDIA_IOC_ENUM_ENTITIES ioctl when running on a kernel version that doesn't
> > + * provide them through the MEDIA_IOC_G_TOPOLOGY ioctl.
> > + */
> > +void MediaDevice::fixupEntityFlags(struct media_v2_entity *entity)
> > +{
> > +	struct media_entity_desc desc = {};
> > +	desc.id = entity->id;
> > +
> > +	int ret = ioctl(fd_, MEDIA_IOC_ENUM_ENTITIES, &desc);
> > +	if (ret < 0) {
> > +		ret = -errno;
> > +		LOG(MediaDevice, Debug)
> > +			<< "Failed to retrieve information for entity "
> > +			<< entity->id << ": " << strerror(-ret);
> > +		return;
> > +	}
> > +
> > +	entity->flags = desc.flags;
> > +}
> > +
> >  /**
> >   * \brief Apply \a flags to a link between two pads
> >   * \param link The link to apply flags to
> >

Patch
diff mbox series 

diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h
index 27a2b46a4392..9f038093b2b2 100644
--- a/src/libcamera/include/media_device.h
+++ b/src/libcamera/include/media_device.h
@@ -56,6 +56,8 @@  private:
 	std::string driver_;
 	std::string deviceNode_;
 	std::string model_;
+	unsigned int version_;
+
 	int fd_;
 	bool valid_;
 	bool acquired_;
@@ -72,6 +74,7 @@  private:
 	bool populateEntities(const struct media_v2_topology &topology);
 	bool populatePads(const struct media_v2_topology &topology);
 	bool populateLinks(const struct media_v2_topology &topology);
+	void fixupEntityFlags(struct media_v2_entity *entity);
 
 	friend int MediaLink::setEnabled(bool enable);
 	int setupLink(const MediaLink *link, unsigned int flags);
diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
index be81bd8c4c23..a4995b996601 100644
--- a/src/libcamera/media_device.cpp
+++ b/src/libcamera/media_device.cpp
@@ -167,6 +167,7 @@  int MediaDevice::open()
 
 	driver_ = info.driver;
 	model_ = info.model;
+	version_ = info.media_version;
 
 	return 0;
 }
@@ -553,20 +554,29 @@  bool MediaDevice::populateEntities(const struct media_v2_topology &topology)
 						(topology.ptr_entities);
 
 	for (unsigned int i = 0; i < topology.num_entities; ++i) {
+		struct media_v2_entity *ent = &mediaEntities[i];
+
+		/*
+		 * The media_v2_entity structure was missing the flag field before
+		 * v4.19.
+		 */
+		if (!MEDIA_V2_ENTITY_HAS_FLAGS(version_))
+			fixupEntityFlags(ent);
+
 		/*
 		 * Find the interface linked to this entity to get the device
 		 * node major and minor numbers.
 		 */
 		struct media_v2_interface *iface =
-			findInterface(topology, mediaEntities[i].id);
+			findInterface(topology, ent->id);
 
 		MediaEntity *entity;
 		if (iface)
-			entity = new MediaEntity(this, &mediaEntities[i],
+			entity = new MediaEntity(this, ent,
 						 iface->devnode.major,
 						 iface->devnode.minor);
 		else
-			entity = new MediaEntity(this, &mediaEntities[i]);
+			entity = new MediaEntity(this, ent);
 
 		if (!addObject(entity)) {
 			delete entity;
@@ -657,6 +667,31 @@  bool MediaDevice::populateLinks(const struct media_v2_topology &topology)
 	return true;
 }
 
+/**
+ * \brief Fixup entity flags using legacy API
+ * \param[in] entity The entity
+ *
+ * This function is used as a fallback to query entity flags using the legacy
+ * MEDIA_IOC_ENUM_ENTITIES ioctl when running on a kernel version that doesn't
+ * provide them through the MEDIA_IOC_G_TOPOLOGY ioctl.
+ */
+void MediaDevice::fixupEntityFlags(struct media_v2_entity *entity)
+{
+	struct media_entity_desc desc = {};
+	desc.id = entity->id;
+
+	int ret = ioctl(fd_, MEDIA_IOC_ENUM_ENTITIES, &desc);
+	if (ret < 0) {
+		ret = -errno;
+		LOG(MediaDevice, Debug)
+			<< "Failed to retrieve information for entity "
+			<< entity->id << ": " << strerror(-ret);
+		return;
+	}
+
+	entity->flags = desc.flags;
+}
+
 /**
  * \brief Apply \a flags to a link between two pads
  * \param link The link to apply flags to