[libcamera-devel] [PATCH v3 01/11] libcamera: Always check return value of MediaDevice::acquire()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat May 11 14:52:48 CEST 2019
Hi Niklas,
Thank you for the patch.
On Sat, May 11, 2019 at 11:18:57AM +0200, Niklas Söderlund wrote:
> In preparation for adding more responsibility to MediaDevice::acquire()
> remove unneeded calls to acquire() and release(), and make sure all
> needed calls to acquire() are checked and acted on.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> src/libcamera/pipeline/ipu3/ipu3.cpp | 10 ++++------
> src/libcamera/pipeline/uvcvideo.cpp | 3 ++-
> src/libcamera/pipeline/vimc.cpp | 3 ++-
> test/media_device/media_device_link_test.cpp | 6 +++++-
> test/v4l2_device/v4l2_device_test.cpp | 4 ----
> test/v4l2_subdevice/v4l2_subdevice_test.cpp | 7 -------
> 6 files changed, 13 insertions(+), 20 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 0041662514e1d7ca..75e878afad4e67a9 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -614,21 +614,19 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> imgu_dm.add("ipu3-imgu 1 viewfinder");
> imgu_dm.add("ipu3-imgu 1 3a stat");
>
> - /*
> - * It is safe to acquire both media devices at this point as
> - * DeviceEnumerator::search() skips the busy ones for us.
> - */
> cio2MediaDev_ = enumerator->search(cio2_dm);
> if (!cio2MediaDev_)
> return false;
>
> - cio2MediaDev_->acquire();
> + if (!cio2MediaDev_->acquire())
> + return false;
>
> imguMediaDev_ = enumerator->search(imgu_dm);
> if (!imguMediaDev_)
> return false;
>
> - imguMediaDev_->acquire();
> + if (!imguMediaDev_->acquire())
> + return false;
>
> /*
> * Disable all links that are enabled by default on CIO2, as camera
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 5d2f1c98fa36380c..e40b052f4d877d5d 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -183,7 +183,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> if (!media_)
> return false;
>
> - media_->acquire();
> + if (!media_->acquire())
> + return false;
>
> std::unique_ptr<UVCCameraData> data = utils::make_unique<UVCCameraData>(this);
>
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index cdf43770a9bfc40f..7b6ebd4cc0a27e25 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -193,7 +193,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
> if (!media_)
> return false;
>
> - media_->acquire();
> + if (!media_->acquire())
> + return false;
>
> std::unique_ptr<VimcCameraData> data = utils::make_unique<VimcCameraData>(this);
>
> diff --git a/test/media_device/media_device_link_test.cpp b/test/media_device/media_device_link_test.cpp
> index 58a55cdfee634c0b..484d3691310f78a0 100644
> --- a/test/media_device/media_device_link_test.cpp
> +++ b/test/media_device/media_device_link_test.cpp
> @@ -51,7 +51,11 @@ class MediaDeviceLinkTest : public Test
> return TestSkip;
> }
>
> - dev_->acquire();
> + if (!dev_->acquire()) {
> + cerr << "Unable to acquire media device "
> + << dev_->deviceNode() << endl;
> + return TestSkip;
> + }
>
> if (dev_->open()) {
> cerr << "Failed to open media device at "
> diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp
> index 4225291bbb6e23f0..833038d56ea4631d 100644
> --- a/test/v4l2_device/v4l2_device_test.cpp
> +++ b/test/v4l2_device/v4l2_device_test.cpp
> @@ -46,8 +46,6 @@ int V4L2DeviceTest::init()
> if (!media_)
> return TestSkip;
>
> - media_->acquire();
> -
Why is this call and the ones below unneeded ? Isn't it a good practice
to always acquire the device before operating on it ? I suppose it's not
strictly mandatory here as we only inspect the media device to find
viden and subdev nodes, but is there any drawback in keeping these calls
?
> MediaEntity *entity = media_->getEntityByName("vivid-000-vid-cap");
> if (!entity)
> return TestSkip;
> @@ -61,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 72d5f72543d29378..5810ef3c962fab8e 100644
> --- a/test/v4l2_subdevice/v4l2_subdevice_test.cpp
> +++ b/test/v4l2_subdevice/v4l2_subdevice_test.cpp
> @@ -45,20 +45,16 @@ int V4L2SubdeviceTest::init()
> return TestSkip;
> }
>
> - media_->acquire();
> -
> 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;
> }
>
> @@ -67,7 +63,6 @@ int V4L2SubdeviceTest::init()
> if (ret) {
> cerr << "Unable to open video subdevice "
> << scaler_->entity()->deviceNode() << endl;
> - media_->release();
> return TestSkip;
> }
>
> @@ -76,7 +71,5 @@ int V4L2SubdeviceTest::init()
>
> void V4L2SubdeviceTest::cleanup()
> {
> - media_->release();
> -
> delete scaler_;
> }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list