[libcamera-devel] [PATCH 02/11] libcamera: media_device: Open and close media device inside populate()

[Niklas Söderlund]

Hi Kieran,

Thanks for your feedback.

On 2019-04-30 11:52:59 +0100, Kieran Bingham wrote:
> 
> 
> On 30/04/2019 10:53, Niklas Söderlund wrote:
> > Hi Kieran,
> > 
> > Thanks for your feedback.
> > 
> > On 2019-04-30 10:35:24 +0100, Kieran Bingham wrote:
> > 
> > snip
> > 
> >>> diff --git a/src/libcamera/media_device.cpp 
> >>> b/src/libcamera/media_device.cpp
> >>> index 449571fb4b78fb94..4b3b8f1fa3e6aaad 100644
> >>> --- a/src/libcamera/media_device.cpp
> >>> +++ b/src/libcamera/media_device.cpp
> >>> @@ -221,6 +221,10 @@ int MediaDevice::populate()
> >>>  
> >>>  	clear();
> >>>  
> >>> +	ret = open();
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>>  	/*
> >>>  	 * Keep calling G_TOPOLOGY until the version number stays stable.
> >>>  	 */
> >>> @@ -237,8 +241,9 @@ int MediaDevice::populate()
> >>>  			LOG(MediaDevice, Error)
> >>>  				<< "Failed to enumerate topology: "
> >>>  				<< strerror(-ret);
> >>> -			return ret;
> >>> +			goto done;
> >>>  		}
> >>> +		ret = 0;
> >>>  
> >>>  		if (version == topology.topology_version)
> >>>  			break;
> >>
> >>
> >> Perhaps secondary to this patch, but I noticed due to following the
> >> change to this error path:
> >>
> >> We iterate calling ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);
> >> allocating and freeing the associated struct arrays as we go.
> >>
> >> Would it be more efficient (or possible) to determine the topology
> >> version first by calling in a loop with null pointers for those
> >> components, and then making a final call at the end having performed a
> >> single set of allocations?
> >>
> >>
> >> I don't know if that's beneficial or not - but iterating with multiple
> >> new and deletes of arrays seems costly.
> > 
> > Unfortunate this is not possible. The reason we iterate until we have 
> > two calls in a row with the same version is to ensure that no new 
> > entities have been added or removed between the calls. If they have we 
> > need to resize the memory accordingly and refetch the data.
> >
> > Luckily it should not be common for this to happen as entities should 
> > only really be register at probe() and async complete() time. And in the 
> > case where no new entities are added to the graph between the calls 
> > there is no over head as just two calls to MEDIA_IOC_G_TOPOLOGY are 
> > preformed. The first to get the sizes and a second to get the data with 
> > only one memory allocation taking place between the two.
> 
> Do we need to check to ensure that the version has not changed between
> the two instances of open that we have?
> 
> populate <open, close>
> 
> ### Change version number?
> 
> acquire <open>
> 	# Needs to repopulate?
> release  <close>

No there is no need to check this, the idea is to make sure we capture 
all data when populating. If we want to guard against changes later in 
time we would need something much more sophisticated.

I see two possible scenarios for when this could make sens,

1. A camera is physically removed, think USB. In this case hot-plug will 
   likely save us the whole effected media device will be removed.

2. A driver is unbound from a v4l2 device or a module is unloaded. The 
   later case I think is more academic as module unloading is not really 
   supported anyhow. But the former we might to wish to support at some 
   point.

   Both unloading and unbinding could possibly be solved by extending 
   our hot-plug listener to react to not only media devices. But I think 
   this is something for the future.

In any case the graph versioning number check is IMHO only needed for 
the TOPOLOGY call to make sure we extract all available data at that 
point.

-- 
Regards,
Niklas Söderlund