[libcamera-devel] [PATCH v2 2/7] libcamera: media_device: Handle ancillary links in populateLinks()

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Fri, Dec 03, 2021 at 10:42:25PM +0000, Daniel Scally wrote:
> The populateLinks() function can't currently handle ancillary links
> which causes an error to be thrown in process() when the MediaObject
> cannot be cast to a MediaPad.
> 
> Add explicit handling for the different link types, creating either
> pad-2-pad links or else storing the pointer to the ancillary device
> MediaEntity in the ancillaryEntities_ member of the primary.
> 
> Signed-off-by: Daniel Scally <djrscally at gmail.com>
> ---
> Changes in v2:
> 
> 	- Storing pointer to the sink entity in the new ancillaryEntities_
> 	vector of the source entity where in case MEDIA_LNK_FL_ANCILLARY_LINK
> 	rather than creating a MediaLink (Laurent)
> 
>  include/linux/media.h          |  1 +
>  src/libcamera/media_device.cpp | 46 +++++++++++++++++++++++-----------
>  2 files changed, 32 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/media.h b/include/linux/media.h
> index 17432318..e3123d1a 100644
> --- a/include/linux/media.h
> +++ b/include/linux/media.h
> @@ -222,6 +222,7 @@ struct media_pad_desc {
>  #define MEDIA_LNK_FL_LINK_TYPE			(0xf << 28)
>  #  define MEDIA_LNK_FL_DATA_LINK		(0 << 28)
>  #  define MEDIA_LNK_FL_INTERFACE_LINK		(1 << 28)
> +#  define MEDIA_LNK_FL_ANCILLARY_LINK		(2 << 28)

This should be split to a separate patch. You can run git log on
include/linux/media.h to see how we usually handle kernel header
updates.

>  struct media_link_desc {
>  	struct media_pad_desc source;
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index aa93da75..6eddb5a0 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -699,45 +699,61 @@ bool MediaDevice::populateLinks(const struct media_v2_topology &topology)
>  {
>  	struct media_v2_link *mediaLinks = reinterpret_cast<struct media_v2_link *>
>  					   (topology.ptr_links);
> +	unsigned int link_type;

libcamera uses camelCase, not snake_case.

> +	MediaLink *link;
>  
>  	for (unsigned int i = 0; i < topology.num_links; ++i) {
>  		/*
>  		 * Skip links between entities and interfaces: we only care
> -		 * about pad-2-pad links here.
> +		 * about pad-2-pad and entity-2-entity links here.
>  		 */
>  		if ((mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE) ==
>  		    MEDIA_LNK_FL_INTERFACE_LINK)
>  			continue;

Should this be moved to the switch-case below to avoid a special case ?

>  
> -		/* Store references to source and sink pads in the link. */
> +		/* Store references to source and sink objects in the link. */

		/* Look up the source and sink objects. */

as you don't necessarily store them in a link anymore.

>  		unsigned int source_id = mediaLinks[i].source_id;
> -		MediaPad *source = dynamic_cast<MediaPad *>
> -				   (object(source_id));
> +		MediaObject *source = object(source_id);
>  		if (!source) {
>  			LOG(MediaDevice, Error)
> -				<< "Failed to find pad with id: "
> +				<< "Failed to find MediaObject with id: "
>  				<< source_id;
>  			return false;
>  		}
>  
>  		unsigned int sink_id = mediaLinks[i].sink_id;
> -		MediaPad *sink = dynamic_cast<MediaPad *>
> -				 (object(sink_id));
> +		MediaObject *sink = object(sink_id);
>  		if (!sink) {
>  			LOG(MediaDevice, Error)
> -				<< "Failed to find pad with id: "
> +				<< "Failed to find MediaObject with id: "
>  				<< sink_id;
>  			return false;
>  		}
>  
> -		MediaLink *link = new MediaLink(&mediaLinks[i], source, sink);
> -		if (!addObject(link)) {
> -			delete link;
> -			return false;
> -		}
> +		link_type = mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE;
> +
> +		switch (link_type) {

		switch (mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE) {

> +		case MEDIA_LNK_FL_DATA_LINK:
> +			link = new MediaLink(&mediaLinks[i],
> +					     dynamic_cast<MediaPad *>(source),
> +					     dynamic_cast<MediaPad *>(sink));
> +			if (!addObject(link)) {
> +				delete link;
> +				return false;
> +			}
> +
> +			link->source()->addLink(link);
> +			link->sink()->addLink(link);
>  
> -		source->addLink(link);
> -		sink->addLink(link);
> +			break;

Missing blank line.

> +		case MEDIA_LNK_FL_ANCILLARY_LINK:
> +			MediaEntity *primary = dynamic_cast<MediaEntity *>(source);
> +			MediaEntity *ancillary = dynamic_cast<MediaEntity *>(sink);
> +
> +			primary->addAncillaryEntity(ancillary);
> +
> +			break;

Should we add a default case and log a warning ?

> +		}
>  	}
>  
>  	return true;

-- 
Regards,

Laurent Pinchart