[libcamera-devel] [PATCH v2 02/11] libcamera: media_device: Open and close media device inside populate()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat May 11 05:13:46 CEST 2019
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 at ragnatech.se>
> Reviewed-by: Kieran Bingham <kieran.bingham at 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_;
> }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list