| Message ID | 20190228162913.6508-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your work. On 2019-02-28 18:29:04 +0200, Laurent Pinchart wrote: > Media devices are acquired in the match() function of pipeline handlers, > and explicitly released if no match is found. The pipeline handler is > then deleted, which causes a second release of the media device in the > destructor. Fix this by removing the explicit release in the match() > function. Nice catch. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 28 +++++++++++----------------- > src/libcamera/pipeline/uvcvideo.cpp | 1 - > src/libcamera/pipeline/vimc.cpp | 5 +---- > 3 files changed, 12 insertions(+), 22 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 9694d0ce51ab..cf5c28577393 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -278,19 +278,20 @@ 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. > + */ > cio2_ = enumerator->search(cio2_dm); > if (!cio2_) > return false; > > + cio2_->acquire(); > + > imgu_ = enumerator->search(imgu_dm); > if (!imgu_) > return false; > > - /* > - * It is safe to acquire both media devices at this point as > - * DeviceEnumerator::search() skips the busy ones for us. > - */ > - cio2_->acquire(); > imgu_->acquire(); > > /* > @@ -301,25 +302,18 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > * not need to be changed after. > */ > if (cio2_->open()) > - goto error_release_mdev; > + return false; > > - if (cio2_->disableLinks()) > - goto error_close_cio2; > + if (cio2_->disableLinks()) { > + cio2_->close(); > + return false; > + } > > registerCameras(); > > cio2_->close(); > > return true; > - > -error_close_cio2: > - cio2_->close(); > - > -error_release_mdev: > - cio2_->release(); > - imgu_->release(); > - > - return false; > } > > /* > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > index b7b8ff109109..dd20bb085a29 100644 > --- a/src/libcamera/pipeline/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo.cpp > @@ -167,7 +167,6 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) > if (!video_) > LOG(UVC, Error) << "Could not find a default video device"; > > - media_->release(); > return false; > } > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > index 812777cffbb3..640fca5cb0e7 100644 > --- a/src/libcamera/pipeline/vimc.cpp > +++ b/src/libcamera/pipeline/vimc.cpp > @@ -165,11 +165,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator) > media_->acquire(); > > video_ = new V4L2Device(media_->getEntityByName("Raw Capture 1")); > - > - if (video_->open()) { > - media_->release(); > + if (video_->open()) > return false; > - } > > std::vector<Stream *> streams{ &stream_ }; > std::shared_ptr<Camera> camera = Camera::create(this, "VIMC Sensor B", > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, On Thu, Feb 28, 2019 at 06:29:04PM +0200, Laurent Pinchart wrote: > Media devices are acquired in the match() function of pipeline handlers, > and explicitly released if no match is found. The pipeline handler is > then deleted, which causes a second release of the media device in the > destructor. Fix this by removing the explicit release in the match() > function. > Nice! Thanks.. Acked-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 28 +++++++++++----------------- > src/libcamera/pipeline/uvcvideo.cpp | 1 - > src/libcamera/pipeline/vimc.cpp | 5 +---- > 3 files changed, 12 insertions(+), 22 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 9694d0ce51ab..cf5c28577393 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -278,19 +278,20 @@ 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. > + */ > cio2_ = enumerator->search(cio2_dm); > if (!cio2_) > return false; > > + cio2_->acquire(); > + > imgu_ = enumerator->search(imgu_dm); > if (!imgu_) > return false; > > - /* > - * It is safe to acquire both media devices at this point as > - * DeviceEnumerator::search() skips the busy ones for us. > - */ > - cio2_->acquire(); > imgu_->acquire(); > > /* > @@ -301,25 +302,18 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > * not need to be changed after. > */ > if (cio2_->open()) > - goto error_release_mdev; > + return false; > > - if (cio2_->disableLinks()) > - goto error_close_cio2; > + if (cio2_->disableLinks()) { > + cio2_->close(); > + return false; > + } > > registerCameras(); > > cio2_->close(); > > return true; > - > -error_close_cio2: > - cio2_->close(); > - > -error_release_mdev: > - cio2_->release(); > - imgu_->release(); > - > - return false; > } > > /* > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > index b7b8ff109109..dd20bb085a29 100644 > --- a/src/libcamera/pipeline/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo.cpp > @@ -167,7 +167,6 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) > if (!video_) > LOG(UVC, Error) << "Could not find a default video device"; > > - media_->release(); > return false; > } > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > index 812777cffbb3..640fca5cb0e7 100644 > --- a/src/libcamera/pipeline/vimc.cpp > +++ b/src/libcamera/pipeline/vimc.cpp > @@ -165,11 +165,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator) > media_->acquire(); > > video_ = new V4L2Device(media_->getEntityByName("Raw Capture 1")); > - > - if (video_->open()) { > - media_->release(); > + if (video_->open()) > return false; > - } > > std::vector<Stream *> streams{ &stream_ }; > std::shared_ptr<Camera> camera = Camera::create(this, "VIMC Sensor B", > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 9694d0ce51ab..cf5c28577393 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -278,19 +278,20 @@ 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. + */ cio2_ = enumerator->search(cio2_dm); if (!cio2_) return false; + cio2_->acquire(); + imgu_ = enumerator->search(imgu_dm); if (!imgu_) return false; - /* - * It is safe to acquire both media devices at this point as - * DeviceEnumerator::search() skips the busy ones for us. - */ - cio2_->acquire(); imgu_->acquire(); /* @@ -301,25 +302,18 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) * not need to be changed after. */ if (cio2_->open()) - goto error_release_mdev; + return false; - if (cio2_->disableLinks()) - goto error_close_cio2; + if (cio2_->disableLinks()) { + cio2_->close(); + return false; + } registerCameras(); cio2_->close(); return true; - -error_close_cio2: - cio2_->close(); - -error_release_mdev: - cio2_->release(); - imgu_->release(); - - return false; } /* diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp index b7b8ff109109..dd20bb085a29 100644 --- a/src/libcamera/pipeline/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo.cpp @@ -167,7 +167,6 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) if (!video_) LOG(UVC, Error) << "Could not find a default video device"; - media_->release(); return false; } diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp index 812777cffbb3..640fca5cb0e7 100644 --- a/src/libcamera/pipeline/vimc.cpp +++ b/src/libcamera/pipeline/vimc.cpp @@ -165,11 +165,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator) media_->acquire(); video_ = new V4L2Device(media_->getEntityByName("Raw Capture 1")); - - if (video_->open()) { - media_->release(); + if (video_->open()) return false; - } std::vector<Stream *> streams{ &stream_ }; std::shared_ptr<Camera> camera = Camera::create(this, "VIMC Sensor B",
Media devices are acquired in the match() function of pipeline handlers, and explicitly released if no match is found. The pipeline handler is then deleted, which causes a second release of the media device in the destructor. Fix this by removing the explicit release in the match() function. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 28 +++++++++++----------------- src/libcamera/pipeline/uvcvideo.cpp | 1 - src/libcamera/pipeline/vimc.cpp | 5 +---- 3 files changed, 12 insertions(+), 22 deletions(-)