[libcamera-devel] [PATCH] libcamera: media_device: Fallback to legacy ioctls on older kernels

[Niklas Söderlund]

Hi Laurent,

Thanks for your patch.

On 2019-01-25 23:37:03 +0200, 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.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/libcamera/include/media_device.h |  3 ++
>  src/libcamera/media_device.cpp       | 44 ++++++++++++++++++++++++++--
>  2 files changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h
> index 27a2b46a4392..31413b9e4605 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 fixupEntity(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..715ba8263436 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,24 @@ 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];
> +
> +		fixupEntity(ent);
> +

I would move the MEDIA_V2_ENTITY_HAS_FLAGS() check here so it's clear 
why the entity needs to be fixed-up. Also bike shedding on the name, 
maybe fixupEntityFlags() ? With the check and with or without the name 
change,

Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

>  		/*
>  		 * 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 +662,39 @@ bool MediaDevice::populateLinks(const struct media_v2_topology &topology)
>  	return true;
>  }
>  
> +/**
> + * \brief Fixup entity information using legacy API
> + * \param[in] entity The entity
> + *
> + * This function is used as a fallback to query entity information using the
> + * legacy MEDIA_IOC_ENUM_ENTITIES ioctl when running on a kernel version that
> + * doesn't provide all the information needed through the MEDIA_IOC_G_TOPOLOGY
> + * ioctl.
> + */
> +void MediaDevice::fixupEntity(struct media_v2_entity *entity)
> +{
> +	/*
> +	 * The media_v2_entity structure was missing the flag field before
> +	 * v4.19.
> +	 */
> +	if (MEDIA_V2_ENTITY_HAS_FLAGS(version_))
> +		return;
> +
> +	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
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund