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

[Kieran Bingham]

Hi Niklas,

On 29/04/2019 20:17, Niklas Söderlund wrote:
> Remove the need for the caller to open and close the media device when
> populating the MediaDevice. This is done as an effort to make the usage
> of the MediaDevice less error prone and the interface stricter.
> 
> The rework also revealed and fixes a potential memory leak in
> MediaDevice::populate() where if the second (or later) call to

"where resources would not be deleted if the second" ?

> MEDIA_IOC_G_TOPOLOGY would fail.
> 

Discussion topic below on MEDIA_IOC_G_TOPOLOGY related to code that is
changed, but not relevant to this patch.


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/device_enumerator.cpp           |  8 +------
>  src/libcamera/media_device.cpp                | 12 +++++++++--
>  test/media_device/media_device_print_test.cpp |  6 ------
>  test/pipeline/ipu3/ipu3_pipeline_test.cpp     |  5 -----
>  test/v4l2_device/v4l2_device_test.cpp         |  5 -----
>  test/v4l2_subdevice/v4l2_subdevice_test.cpp   | 21 +------------------
>  6 files changed, 12 insertions(+), 45 deletions(-)
> 
> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp
> index f6878b3d58b3f966..caf91dcff6efd564 100644
> --- a/src/libcamera/device_enumerator.cpp
> +++ b/src/libcamera/device_enumerator.cpp
> @@ -205,11 +205,7 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode)
>  {
>  	std::shared_ptr<MediaDevice> media = std::make_shared<MediaDevice>(deviceNode);
>  
> -	int ret = media->open();
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = media->populate();
> +	int ret = media->populate();
>  	if (ret < 0) {
>  		LOG(DeviceEnumerator, Info)
>  			<< "Unable to populate media device " << deviceNode
> @@ -236,8 +232,6 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode)
>  			return ret;
>  	}
>  
> -	media->close();
> -
>  	LOG(DeviceEnumerator, Debug)
>  		<< "Added device " << deviceNode << ": " << media->driver();
>  
> 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.



> @@ -262,6 +267,9 @@ int MediaDevice::populate()
>  	    populateLinks(topology))
>  		valid_ = true;
>  
> +done:
> +	close();
> +
>  	delete[] ents;
>  	delete[] interfaces;
>  	delete[] pads;
> @@ -272,7 +280,7 @@ int MediaDevice::populate()
>  		return -EINVAL;
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  /**
> diff --git a/test/media_device/media_device_print_test.cpp b/test/media_device/media_device_print_test.cpp
> index 3eef973939201b27..ceffd538e13fca73 100644
> --- a/test/media_device/media_device_print_test.cpp
> +++ b/test/media_device/media_device_print_test.cpp
> @@ -124,10 +124,6 @@ int MediaDevicePrintTest::testMediaDevice(const string deviceNode)
>  
>  	dev.close();
>  
> -	ret = dev.open();
> -	if (ret)
> -		return ret;
> -
>  	ret = dev.populate();
>  	if (ret)
>  		return ret;
> @@ -135,8 +131,6 @@ int MediaDevicePrintTest::testMediaDevice(const string deviceNode)
>  	/* Print out the media graph. */
>  	printMediaGraph(dev, cerr);
>  
> -	dev.close();
> -
>  	return 0;
>  }
>  
> diff --git a/test/pipeline/ipu3/ipu3_pipeline_test.cpp b/test/pipeline/ipu3/ipu3_pipeline_test.cpp
> index 953f0233cde485cb..1d4cc4d4950b8391 100644
> --- a/test/pipeline/ipu3/ipu3_pipeline_test.cpp
> +++ b/test/pipeline/ipu3/ipu3_pipeline_test.cpp
> @@ -71,11 +71,6 @@ int IPU3PipelineTest::init()
>  		return TestSkip;
>  	}
>  
> -	if (cio2->open()) {
> -		cerr << "Failed to open media device " << cio2->deviceNode() << endl;
> -		return TestFail;
> -	}
> -
>  	/*
>  	 * Camera sensor are connected to the CIO2 unit.
>  	 * Count how many sensors are connected in the system
> diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp
> index 90e5f7a3ee0c0f2f..833038d56ea4631d 100644
> --- a/test/v4l2_device/v4l2_device_test.cpp
> +++ b/test/v4l2_device/v4l2_device_test.cpp
> @@ -46,9 +46,6 @@ int V4L2DeviceTest::init()
>  	if (!media_)
>  		return TestSkip;
>  
> -	if (!media_->acquire())
> -		return TestSkip;
> -
>  	MediaEntity *entity = media_->getEntityByName("vivid-000-vid-cap");
>  	if (!entity)
>  		return TestSkip;
> @@ -62,8 +59,6 @@ int V4L2DeviceTest::init()
>  
>  void V4L2DeviceTest::cleanup()
>  {
> -	media_->release();
> -
>  	capture_->streamOff();
>  	capture_->releaseBuffers();
>  	capture_->close();
> diff --git a/test/v4l2_subdevice/v4l2_subdevice_test.cpp b/test/v4l2_subdevice/v4l2_subdevice_test.cpp
> index 4e16ab6cf3e5f874..562a638cb28ebfb2 100644
> --- a/test/v4l2_subdevice/v4l2_subdevice_test.cpp
> +++ b/test/v4l2_subdevice/v4l2_subdevice_test.cpp
> @@ -45,33 +45,16 @@ int V4L2SubdeviceTest::init()
>  		return TestSkip;
>  	}
>  
> -	if (!media_->acquire()) {
> -		cerr << "Unable to acquire media device "
> -		     << media_->deviceNode() << endl;
> -		return TestSkip;
> -	}
> -
> -	int ret = media_->open();
> -	if (ret) {
> -		cerr << "Unable to open media device: " << media_->deviceNode()
> -		     << ": " << strerror(ret) << endl;
> -		media_->release();
> -		return TestSkip;
> -	}
> -
>  	MediaEntity *videoEntity = media_->getEntityByName("Scaler");
>  	if (!videoEntity) {
>  		cerr << "Unable to find media entity 'Scaler'" << endl;
> -		media_->release();
>  		return TestFail;
>  	}
>  
>  	scaler_ = new V4L2Subdevice(videoEntity);
> -	ret = scaler_->open();
> -	if (ret) {
> +	if (scaler_->open()) {

Is this change necessary?
Don't we want to print the ret value if this fails?

Actually it looks like that's already reported by V4L2Subdevice::open()


>  		cerr << "Unable to open video subdevice "
>  		     << scaler_->entity()->deviceNode() << endl;
> -		media_->release();
>  		return TestSkip;
>  	}
>  
> @@ -80,7 +63,5 @@ int V4L2SubdeviceTest::init()
>  
>  void V4L2SubdeviceTest::cleanup()
>  {
> -	media_->release();
> -
>  	delete scaler_;
>  }
> 

-- 
Regards
--
Kieran