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

1167 diff mbox series

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 resources would not be deleted if the
second MEDIA_IOC_G_TOPOLOGY would fail.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 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(-)

Hi Niklas,

Thank you for the patch.

On Wed, May 08, 2019 at 05:18:22PM +0200, 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 resources would not be deleted if the
> second MEDIA_IOC_G_TOPOLOGY would fail.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  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 259eec41776e2ec4..6fcbae76b64e3774 100644
> --- a/src/libcamera/device_enumerator.cpp
> +++ b/src/libcamera/device_enumerator.cpp
> @@ -207,11 +207,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
> @@ -238,8 +234,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;

the ioctl() call is supposed to return 0 on success, so this isn't
strictly needed. If you want to play safe I would move this assignment
just before the done: label.

>  
>  		if (version == topology.topology_version)
>  			break;
> @@ -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;
> -

Is this related to this patch ? The commit message doesn't mention the
removal of acquire() and release() calls.

>  	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()) {
>  		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_;
>  }

Hi Laurent,

Thanks for your feedback.

On 2019-05-11 06:13:46 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Wed, May 08, 2019 at 05:18:22PM +0200, 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 resources would not be deleted if the
> > second MEDIA_IOC_G_TOPOLOGY would fail.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  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 259eec41776e2ec4..6fcbae76b64e3774 100644
> > --- a/src/libcamera/device_enumerator.cpp
> > +++ b/src/libcamera/device_enumerator.cpp
> > @@ -207,11 +207,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
> > @@ -238,8 +234,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;
> 
> the ioctl() call is supposed to return 0 on success, so this isn't
> strictly needed. If you want to play safe I would move this assignment
> just before the done: label.

My documentation states that ioctl() shall return a value other then -1 
on success, the check we have are

    ret = ioctl();
    if (ret < 0) {
        ... error ...
    }

I added the ret = 0 here to illustrate that we "mark" the ioctl() OK. I 
do not feel strongly about this and will move it to just before the done 
label in the next version.

> 
> >  
> >  		if (version == topology.topology_version)
> >  			break;
> > @@ -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;
> > -
> 
> Is this related to this patch ? The commit message doesn't mention the
> removal of acquire() and release() calls.

You are correct. This was found when removing the open() and close() and 
somehow was added to this patch, will break this out to it's own patch 
or maybe even merge with 1/11.

> 
> >  	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()) {
> >  		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,
> 
> Laurent Pinchart