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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Sun, Apr 14, 2019 at 03:34:57AM +0200, Niklas Söderlund wrote:
> Remove the need for the caller to open and close the media device when
> populating the MediaDeivce. This is done as an effort to make the usage

s/MediaDeivce/MediaDevice/

> of the MediaDevice less error prone and the interface stricter.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/device_enumerator.cpp           |  8 +-------
>  src/libcamera/media_device.cpp                | 10 ++++++++--
>  test/media_device/media_device_print_test.cpp |  6 ------
>  test/pipeline/ipu3/ipu3_pipeline_test.cpp     |  5 -----
>  4 files changed, 9 insertions(+), 20 deletions(-)
> 
> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp
> index 58b81c354a706f03..6c08806d17dbfeec 100644
> --- a/src/libcamera/device_enumerator.cpp
> +++ b/src/libcamera/device_enumerator.cpp
> @@ -212,11 +212,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
> @@ -243,8 +239,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 1e9024bf97217bcf..ecb00a1d5abffe80 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -220,6 +220,10 @@ int MediaDevice::populate()
>  
>  	clear();
>  
> +	ret = open();
> +	if (ret)
> +		return ret;
> +
>  	/*
>  	 * Keep calling G_TOPOLOGY until the version number stays stable.
>  	 */
> @@ -236,7 +240,7 @@ int MediaDevice::populate()
>  			LOG(MediaDevice, Error)
>  				<< "Failed to enumerate topology: "
>  				<< strerror(-ret);
> -			return ret;
> +			goto err_out;

This seems to fix a memory leak when the second call to
MEDIA_IOC_G_TOPOLOGY fails, that's nice. Could you mention it in the
commit message ?

>  		}
>  
>  		if (version == topology.topology_version)
> @@ -260,6 +264,8 @@ int MediaDevice::populate()
>  	    populatePads(topology) &&
>  	    populateLinks(topology))
>  		valid_ = true;

Please add a blank line.

> +err_out:

I'd name this label done as this isn't path restricted to errors.

> +	close();
>  
>  	delete[] ents;
>  	delete[] interfaces;
> @@ -268,7 +274,7 @@ int MediaDevice::populate()
>  
>  	if (!valid_) {
>  		clear();
> -		return -EINVAL;
> +		return ret ? ret : -EINVAL;

That's not very nice. How about adding instead the following before this
check ?

	if (ret)
		return ret;

>  	}
>  
>  	return 0;
> 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

-- 
Regards,

Laurent Pinchart