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

-- 
Regards,
Niklas Söderlund