[libcamera-devel] [PATCH v4 09/11] libcamera: pipeline_handler: Keep track of MediaDevice
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri May 17 11:22:07 CEST 2019
Hi Niklas,
Thank you for the patch.
On Fri, May 17, 2019 at 02:54:45AM +0200, Niklas Söderlund wrote:
> Instead of requiring each pipeline handle implementation to keep track
> of calling release() on its media devices upon deletion, keep track of
> them in the base class. Add a helper that pipeline handlers shall use
> to acquire a media device instead of directly interacting with the
> DeviceEnumerator.
>
> This also means that pipeline handler implementations do no need to keep
> a shared_ptr<> reference to the media devices they store locally as the
> PipelineHandler base class will keep a shared_ptr<> reference to all
> media devices consumed for the entire lifetime of the pipeline handler
> implementation.
>
> Centrally keeping track of media devices will also be beneficial
> to implement exclusive access to pipelines across processes.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> src/libcamera/include/pipeline_handler.h | 4 +++
> src/libcamera/pipeline/ipu3/ipu3.cpp | 30 ++++++----------------
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++--------
> src/libcamera/pipeline/uvcvideo.cpp | 25 ++++++------------
> src/libcamera/pipeline/vimc.cpp | 20 +++------------
> src/libcamera/pipeline_handler.cpp | 32 ++++++++++++++++++++++++
> 6 files changed, 59 insertions(+), 68 deletions(-)
>
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index 5830e53108faa105..8e6a136dd4907d9f 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -22,6 +22,7 @@ class Camera;
> class CameraConfiguration;
> class CameraManager;
> class DeviceEnumerator;
> +class DeviceMatch;
> class MediaDevice;
> class PipelineHandler;
> class Request;
> @@ -53,6 +54,8 @@ public:
> virtual ~PipelineHandler();
>
> virtual bool match(DeviceEnumerator *enumerator) = 0;
> + MediaDevice *acquireMediaDevice(DeviceEnumerator *enumerator,
> + const DeviceMatch &dm);
>
> virtual CameraConfiguration
> streamConfiguration(Camera *camera, const std::vector<StreamUsage> &usages) = 0;
> @@ -84,6 +87,7 @@ private:
> void mediaDeviceDisconnected(MediaDevice *media);
> virtual void disconnect();
>
> + std::vector<std::shared_ptr<MediaDevice>> mediaDevices_;
> std::vector<std::weak_ptr<Camera>> cameras_;
> std::map<const Camera *, std::unique_ptr<CameraData>> cameraData_;
> };
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 8a6a0e2721955d15..75a70e66eacc443a 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -149,7 +149,6 @@ class PipelineHandlerIPU3 : public PipelineHandler
> {
> public:
> PipelineHandlerIPU3(CameraManager *manager);
> - ~PipelineHandlerIPU3();
>
> CameraConfiguration
> streamConfiguration(Camera *camera,
> @@ -201,8 +200,8 @@ private:
>
> ImgUDevice imgu0_;
> ImgUDevice imgu1_;
> - std::shared_ptr<MediaDevice> cio2MediaDev_;
> - std::shared_ptr<MediaDevice> imguMediaDev_;
> + MediaDevice *cio2MediaDev_;
> + MediaDevice *imguMediaDev_;
> };
>
> PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
> @@ -210,15 +209,6 @@ PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
> {
> }
>
> -PipelineHandlerIPU3::~PipelineHandlerIPU3()
> -{
> - if (cio2MediaDev_)
> - cio2MediaDev_->release();
> -
> - if (imguMediaDev_)
> - imguMediaDev_->release();
> -}
> -
> CameraConfiguration
> PipelineHandlerIPU3::streamConfiguration(Camera *camera,
> const std::vector<StreamUsage> &usages)
> @@ -614,20 +604,14 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> imgu_dm.add("ipu3-imgu 1 viewfinder");
> imgu_dm.add("ipu3-imgu 1 3a stat");
>
> - cio2MediaDev_ = enumerator->search(cio2_dm);
> + cio2MediaDev_ = acquireMediaDevice(enumerator, cio2_dm);
> if (!cio2MediaDev_)
> return false;
>
> - if (!cio2MediaDev_->acquire())
> - return false;
> -
> - imguMediaDev_ = enumerator->search(imgu_dm);
> + imguMediaDev_ = acquireMediaDevice(enumerator, imgu_dm);
> if (!imguMediaDev_)
> return false;
>
> - if (!imguMediaDev_->acquire())
> - return false;
> -
> /*
> * Disable all links that are enabled by default on CIO2, as camera
> * creation enables all valid links it finds.
> @@ -682,11 +666,11 @@ int PipelineHandlerIPU3::registerCameras()
> {
> int ret;
>
> - ret = imgu0_.init(imguMediaDev_.get(), 0);
> + ret = imgu0_.init(imguMediaDev_, 0);
> if (ret)
> return ret;
>
> - ret = imgu1_.init(imguMediaDev_.get(), 1);
> + ret = imgu1_.init(imguMediaDev_, 1);
> if (ret)
> return ret;
>
> @@ -705,7 +689,7 @@ int PipelineHandlerIPU3::registerCameras()
> };
> CIO2Device *cio2 = &data->cio2_;
>
> - ret = cio2->init(cio2MediaDev_.get(), id);
> + ret = cio2->init(cio2MediaDev_, id);
> if (ret)
> continue;
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index b94d742dd6ec33a4..b395405c9ddb7f17 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -81,7 +81,7 @@ private:
> int createCamera(MediaEntity *sensor);
> void bufferReady(Buffer *buffer);
>
> - std::shared_ptr<MediaDevice> media_;
> + MediaDevice *media_;
> V4L2Subdevice *dphy_;
> V4L2Subdevice *isp_;
> V4L2Device *video_;
> @@ -100,9 +100,6 @@ PipelineHandlerRkISP1::~PipelineHandlerRkISP1()
> delete video_;
> delete isp_;
> delete dphy_;
> -
> - if (media_)
> - media_->release();
> }
>
> /* -----------------------------------------------------------------------------
> @@ -355,24 +352,21 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
> dm.add("rkisp1-input-params");
> dm.add("rockchip-sy-mipi-dphy");
>
> - media_ = enumerator->search(dm);
> + media_ = acquireMediaDevice(enumerator, dm);
> if (!media_)
> return false;
>
> - media_->acquire();
> -
> /* Create the V4L2 subdevices we will need. */
> - dphy_ = V4L2Subdevice::fromEntityName(media_.get(),
> - "rockchip-sy-mipi-dphy");
> + dphy_ = V4L2Subdevice::fromEntityName(media_, "rockchip-sy-mipi-dphy");
> if (dphy_->open() < 0)
> return false;
>
> - isp_ = V4L2Subdevice::fromEntityName(media_.get(), "rkisp1-isp-subdev");
> + isp_ = V4L2Subdevice::fromEntityName(media_, "rkisp1-isp-subdev");
> if (isp_->open() < 0)
> return false;
>
> /* Locate and open the capture video node. */
> - video_ = V4L2Device::fromEntityName(media_.get(), "rkisp1_mainpath");
> + video_ = V4L2Device::fromEntityName(media_, "rkisp1_mainpath");
> if (video_->open() < 0)
> return false;
>
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index e40b052f4d877d5d..351712cfdc69594d 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -24,7 +24,6 @@ class PipelineHandlerUVC : public PipelineHandler
> {
> public:
> PipelineHandlerUVC(CameraManager *manager);
> - ~PipelineHandlerUVC();
>
> CameraConfiguration
> streamConfiguration(Camera *camera,
> @@ -69,21 +68,13 @@ private:
> return static_cast<UVCCameraData *>(
> PipelineHandler::cameraData(camera));
> }
> -
> - std::shared_ptr<MediaDevice> media_;
> };
>
> PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
> - : PipelineHandler(manager), media_(nullptr)
> + : PipelineHandler(manager)
> {
> }
>
> -PipelineHandlerUVC::~PipelineHandlerUVC()
> -{
> - if (media_)
> - media_->release();
> -}
> -
> CameraConfiguration
> PipelineHandlerUVC::streamConfiguration(Camera *camera,
> const std::vector<StreamUsage> &usages)
> @@ -177,19 +168,17 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
>
> bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> {
> + MediaDevice *media;
> DeviceMatch dm("uvcvideo");
>
> - media_ = enumerator->search(dm);
> - if (!media_)
> - return false;
> -
> - if (!media_->acquire())
> + media = acquireMediaDevice(enumerator, dm);
> + if (!media)
> return false;
>
> std::unique_ptr<UVCCameraData> data = utils::make_unique<UVCCameraData>(this);
>
> /* Locate and open the default video node. */
> - for (MediaEntity *entity : media_->entities()) {
> + for (MediaEntity *entity : media->entities()) {
> if (entity->flags() & MEDIA_ENT_FL_DEFAULT) {
> data->video_ = new V4L2Device(entity);
> break;
> @@ -208,11 +197,11 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>
> /* Create and register the camera. */
> std::set<Stream *> streams{ &data->stream_ };
> - std::shared_ptr<Camera> camera = Camera::create(this, media_->model(), streams);
> + std::shared_ptr<Camera> camera = Camera::create(this, media->model(), streams);
> registerCamera(std::move(camera), std::move(data));
>
> /* Enable hot-unplug notifications. */
> - hotplugMediaDevice(media_.get());
> + hotplugMediaDevice(media);
>
> return true;
> }
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 7b6ebd4cc0a27e25..737d6df67def59c7 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -24,7 +24,6 @@ class PipelineHandlerVimc : public PipelineHandler
> {
> public:
> PipelineHandlerVimc(CameraManager *manager);
> - ~PipelineHandlerVimc();
>
> CameraConfiguration
> streamConfiguration(Camera *camera,
> @@ -69,21 +68,13 @@ private:
> return static_cast<VimcCameraData *>(
> PipelineHandler::cameraData(camera));
> }
> -
> - std::shared_ptr<MediaDevice> media_;
> };
>
> PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)
> - : PipelineHandler(manager), media_(nullptr)
> + : PipelineHandler(manager)
> {
> }
>
> -PipelineHandlerVimc::~PipelineHandlerVimc()
> -{
> - if (media_)
> - media_->release();
> -}
> -
> CameraConfiguration
> PipelineHandlerVimc::streamConfiguration(Camera *camera,
> const std::vector<StreamUsage> &usages)
> @@ -189,17 +180,14 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
> dm.add("RGB/YUV Input");
> dm.add("Scaler");
>
> - media_ = enumerator->search(dm);
> - if (!media_)
> - return false;
> -
> - if (!media_->acquire())
> + MediaDevice *media = acquireMediaDevice(enumerator, dm);
> + if (!media)
> return false;
>
> std::unique_ptr<VimcCameraData> data = utils::make_unique<VimcCameraData>(this);
>
> /* Locate and open the capture video node. */
> - data->video_ = new V4L2Device(media_->getEntityByName("Raw Capture 1"));
> + data->video_ = new V4L2Device(media->getEntityByName("Raw Capture 1"));
> if (data->video_->open())
> return false;
>
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 4ecd6c49efd8ca89..c92ee782701f57bf 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -11,6 +11,7 @@
> #include <libcamera/camera.h>
> #include <libcamera/camera_manager.h>
>
> +#include "device_enumerator.h"
> #include "log.h"
> #include "media_device.h"
> #include "utils.h"
> @@ -116,6 +117,8 @@ PipelineHandler::PipelineHandler(CameraManager *manager)
>
> PipelineHandler::~PipelineHandler()
> {
> + for (std::shared_ptr<MediaDevice> media : mediaDevices_)
> + media->release();
> };
>
> /**
> @@ -149,6 +152,35 @@ PipelineHandler::~PipelineHandler()
> * created, or false otherwise
> */
>
> +/**
> + * \brief Search and acquire a MediDevice matching a device pattern
> + * \param[in] enumerator Enumerator containing all media devices in the system
> + * \param[in] dm Device match pattern
> + *
> + * Search the device \a enumerator for an available media device matching the
> + * device match pattern \a dm. Matching media device that have previously been
> + * acquired by MediaDevice::acquire() are not considered. If a match is found,
> + * the media device is acquired and returned. The caller shall not release the
> + * device explicitly, it will be automatically released when the pipeline
> + * handler is destroyed.
> + *
> + * \return A pointer to the matching MediaDevice, or nullptr if no match is found
> + */
> +MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,
> + const DeviceMatch &dm)
> +{
> + std::shared_ptr<MediaDevice> media = enumerator->search(dm);
> + if (!media)
> + return nullptr;
> +
> + if (!media->acquire())
> + return nullptr;
> +
> + mediaDevices_.push_back(media);
> +
> + return media.get();
> +}
> +
> /**
> * \fn PipelineHandler::streamConfiguration()
> * \brief Retrieve a group of stream configurations for a specified camera
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list