[libcamera-devel] [PATCH v3 01/11] libcamera: Always check return value of MediaDevice::acquire()
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sat May 11 15:22:28 CEST 2019
Hi Laurent,
Thanks for your feedback.
On 2019-05-11 15:52:48 +0300, Laurent Pinchart wrote:
> 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
> ?
It's a good idea to acquire() the device if you intend to operate on it,
at this point in time the init() do not need to operate on the media
device so there is no need to acquire() it. Later in the series I do add
a call to acquire() and release() to this base init() function to be
able to perform an operation on the device (reset the media links).
But I still think we should leave it unacquired and let each test-case
call acquire() if it needs to operate on the media device. Most tests
who inherit from this base would not need to modify the graph as it
intends to test the v4l2 device and for those who do I think it's nice
to show that clearly in the test case and not hide the acquire() in the
base.
>
> > 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
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list