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

[Daniel Scally]

Hi Laurent

On 04/12/2021 00:12, Laurent Pinchart wrote:
> 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.

No problem. I did wonder if this was something that would have to wait
until it was merged kernelside, but it does look like a few manual
additions have been made (and kept) when the headers were upgraded to
the newest kernel version.

> 
>>  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.

Ack
> 
>> +	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 ?

Yup, not sure why I didn't see that...

Ack, to the rest of the comments here.
> 
>>  
>> -		/* 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;
>