[libcamera-devel] [PATCH v2 04/11] libcamera: media_device: Handle media device fd in acquire() and release()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat May 11 05:41:23 CEST 2019
Hi Niklas,
Thank you for the patch.
On Wed, May 08, 2019 at 05:18:24PM +0200, Niklas Söderlund wrote:
> To gain better control of when a file descriptor is open to the media
> device and reduce the work needed in pipeline handler implementations,
> handle the file descriptor in acquire() and release().
>
> This changes the current behavior where a file descriptor is only open
> when requested by the pipeline handler to that one is always open as
> long a media device is acquired. This new behavior is desired to allow
> implementing exclusive access to a pipeline handler between processes.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> src/libcamera/include/media_device.h | 2 +-
> src/libcamera/media_device.cpp | 29 +++++++-------
> src/libcamera/pipeline/ipu3/ipu3.cpp | 39 +++---------------
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 +++++---------------
> test/media_device/media_device_link_test.cpp | 7 ----
> 5 files changed, 33 insertions(+), 86 deletions(-)
>
> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h
> index 9f038093b2b22fc7..883985055eb419dd 100644
> --- a/src/libcamera/include/media_device.h
> +++ b/src/libcamera/include/media_device.h
> @@ -27,7 +27,7 @@ public:
> ~MediaDevice();
>
> bool acquire();
> - void release() { acquired_ = false; }
> + void release();
> bool busy() const { return acquired_; }
>
> int open();
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index 416a0d0c207ea72d..ca260c9ea991dd53 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -40,10 +40,9 @@ LOG_DEFINE_CATEGORY(MediaDevice)
> * instance.
> *
> * The instance is created with an empty media graph. Before performing any
> - * other operation, it must be opened with the open() function and the media
> - * graph populated by calling populate(). Instances of MediaEntity, MediaPad and
> - * MediaLink are created to model the media graph, and stored in a map indexed
> - * by object id.
> + * other operation, it must be populate by calling populate(). Instances of
> + * MediaEntity, MediaPad and MediaLink are created to model the media graph,
> + * and stored in a map indexed by object id.
> *
> * The graph is valid once successfully populated, as reported by the valid()
> * function. It can be queried to list all entities(), or entities can be
> @@ -51,12 +50,6 @@ LOG_DEFINE_CATEGORY(MediaDevice)
> * entity to entity through pads and links as exposed by the corresponding
> * classes.
> *
> - * An open media device will keep an open file handle for the underlying media
> - * controller device node. It can be closed at any time with a call to close().
> - * This will not invalidate the media graph and all cached media objects remain
> - * valid and can be accessed normally. The device can then be later reopened if
> - * needed to perform other operations that interact with the device node.
> - *
> * Media device can be claimed for exclusive use with acquire(), released with
> * release() and tested with busy(). This mechanism is aimed at pipeline
> * managers to claim media devices they support during enumeration.
> @@ -66,8 +59,8 @@ LOG_DEFINE_CATEGORY(MediaDevice)
> * \brief Construct a MediaDevice
> * \param[in] deviceNode The media device node path
> *
> - * Once constructed the media device is invalid, and must be opened and
> - * populated with open() and populate() before the media graph can be queried.
> + * Once constructed the media device is invalid, and must be populated with
> + * populate() before the media graph can be queried.
> */
> MediaDevice::MediaDevice(const std::string &deviceNode)
> : deviceNode_(deviceNode), fd_(-1), valid_(false), acquired_(false)
> @@ -92,7 +85,8 @@ MediaDevice::~MediaDevice()
> * busy() function.
> *
> * Once claimed the device shall be released by its user when not needed anymore
> - * by calling the release() function.
> + * by calling the release() function. Acquiring the media device opens a file
> + * descriptor to the device which is kept open until release() is called.
> *
> * Exclusive access is only guaranteed if all users of the media device abide by
> * the device claiming mechanism, as it isn't enforced by the media device
> @@ -107,15 +101,22 @@ bool MediaDevice::acquire()
> if (acquired_)
> return false;
>
> + if (open())
> + return false;
> +
> acquired_ = true;
> return true;
> }
>
> /**
> - * \fn MediaDevice::release()
> * \brief Release a device previously claimed for exclusive use
> * \sa acquire(), busy()
> */
> +void MediaDevice::release()
> +{
> + close();
> + acquired_ = false;
> +}
>
> /**
> * \fn MediaDevice::busy()
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 75e878afad4e67a9..8a6a0e2721955d15 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -631,23 +631,10 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> /*
> * Disable all links that are enabled by default on CIO2, as camera
> * creation enables all valid links it finds.
> - *
> - * Close the CIO2 media device after, as links are enabled and should
> - * not need to be changed after.
> */
> - if (cio2MediaDev_->open())
> + if (cio2MediaDev_->disableLinks())
> return false;
>
> - if (cio2MediaDev_->disableLinks()) {
> - cio2MediaDev_->close();
> - return false;
> - }
> -
> - if (imguMediaDev_->open()) {
> - cio2MediaDev_->close();
> - return false;
> - }
> -
> /*
> * FIXME: enabled links in one ImgU instance interfere with capture
> * operations on the other one. This can be easily triggered by
> @@ -674,14 +661,10 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> */
> ret = imguMediaDev_->disableLinks();
> if (ret)
> - goto error;
> + return ret;
>
> ret = registerCameras();
>
> -error:
> - cio2MediaDev_->close();
> - imguMediaDev_->close();
> -
> return ret == 0;
> }
>
> @@ -1139,29 +1122,19 @@ int ImgUDevice::enableLinks(bool enable)
> std::string inputName = name_ + " input";
> int ret;
>
> - /* \todo Establish rules to handle media devices open/close. */
> - ret = media_->open();
> - if (ret)
> - return ret;
> -
> ret = linkSetup(inputName, 0, name_, PAD_INPUT, enable);
> if (ret)
> - goto done;
> + return ret;
>
> ret = linkSetup(name_, PAD_OUTPUT, outputName, 0, enable);
> if (ret)
> - goto done;
> + return ret;
>
> ret = linkSetup(name_, PAD_VF, viewfinderName, 0, enable);
> if (ret)
> - goto done;
> + return ret;
>
> - ret = linkSetup(name_, PAD_STAT, statName, 0, enable);
> -
> -done:
> - media_->close();
> -
> - return ret;
> + return linkSetup(name_, PAD_STAT, statName, 0, enable);
> }
>
> /*------------------------------------------------------------------------------
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 134d3df4e1eb2b9b..b94d742dd6ec33a4 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -148,10 +148,6 @@ int PipelineHandlerRkISP1::configureStreams(Camera *camera,
> */
> const MediaPad *pad = dphy_->entity()->getPadByIndex(0);
>
> - ret = media_->open();
> - if (ret < 0)
> - return ret;
> -
> for (MediaLink *link : pad->links()) {
> bool enable = link->source()->entity() == sensor->entity();
>
> @@ -169,8 +165,6 @@ int PipelineHandlerRkISP1::configureStreams(Camera *camera,
> break;
> }
>
> - media_->close();
> -
> if (ret < 0)
> return ret;
>
> @@ -352,7 +346,6 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
> {
> const MediaPad *pad;
> - int ret;
>
> DeviceMatch dm("rkisp1");
> dm.add("rkisp1-isp-subdev");
> @@ -368,35 +361,27 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>
> media_->acquire();
>
> - ret = media_->open();
> - if (ret < 0)
> - return ret;
> -
> /* Create the V4L2 subdevices we will need. */
> dphy_ = V4L2Subdevice::fromEntityName(media_.get(),
> "rockchip-sy-mipi-dphy");
> - ret = dphy_->open();
> - if (ret < 0)
> - goto done;
> + if (dphy_->open() < 0)
> + return false;
>
> isp_ = V4L2Subdevice::fromEntityName(media_.get(), "rkisp1-isp-subdev");
> - ret = isp_->open();
> - if (ret < 0)
> - goto done;
> + if (isp_->open() < 0)
> + return false;
>
> /* Locate and open the capture video node. */
> video_ = V4L2Device::fromEntityName(media_.get(), "rkisp1_mainpath");
> - ret = video_->open();
> - if (ret < 0)
> - goto done;
> + if (video_->open() < 0)
> + return false;
>
> video_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);
>
> /* Configure default links. */
> - ret = initLinks();
> - if (ret < 0) {
> + if (initLinks() < 0) {
> LOG(RkISP1, Error) << "Failed to setup links";
> - goto done;
> + return false;
> }
>
> /*
> @@ -404,18 +389,13 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
> * camera instance for each of them.
> */
> pad = dphy_->entity()->getPadByIndex(0);
> - if (!pad) {
> - ret = -EINVAL;
> - goto done;
> - }
> + if (!pad)
> + return false;
>
> for (MediaLink *link : pad->links())
> createCamera(link->source()->entity());
>
> -done:
> - media_->close();
> -
> - return ret == 0;
> + return true;
> }
>
> /* -----------------------------------------------------------------------------
> diff --git a/test/media_device/media_device_link_test.cpp b/test/media_device/media_device_link_test.cpp
> index 484d3691310f78a0..334bb44a6fc14371 100644
> --- a/test/media_device/media_device_link_test.cpp
> +++ b/test/media_device/media_device_link_test.cpp
> @@ -57,12 +57,6 @@ class MediaDeviceLinkTest : public Test
> return TestSkip;
> }
>
> - if (dev_->open()) {
> - cerr << "Failed to open media device at "
> - << dev_->deviceNode() << endl;
> - return TestFail;
> - }
> -
> return 0;
> }
>
> @@ -238,7 +232,6 @@ class MediaDeviceLinkTest : public Test
>
> void cleanup()
> {
> - dev_->close();
> dev_->release();
> }
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list