<div dir="ltr"><div dir="ltr">Hi Hans,<div><br></div><div>Thanks for the patch. Actually in mtkisp7, we did something very similar [1].</div><div>(And I just caught a bug by checking your changes. Thanks :)</div><div><br></div><div>[1]: <a href="https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5525900">https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5525900</a></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Aug 20, 2024 at 9:50 PM Hans de Goede <<a href="mailto:hdegoede@redhat.com">hdegoede@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">ATM libcamera always keeps all /dev/video# and /dev/v4l2_subdev# nodes<br>
for a pipeline open after enumerating the camera.<br>
<br>
This is a problem for uvcvideo and atomisp /dev/video# nodes as well as<br>
for VCM /dev/v4l2_subdev# nodes. Keeping these open stops the underlying<br>
hw-devices from being able to enter runtime-suspend causing significant<br>
unnecessary power-usage.<br>
<br>
Add a stub acquireDevice() method to the PipelineHandler class which<br>
pipeline handlers can override to delay opening /dev nodes until<br>
the camera is acquired.<br>
<br>
Note pipeline handlers typically also will need access to /dev nodes<br>
for their CameraConfig::validate() implementation for tryFormat() calls.<br>
Making sure that /dev nodes are opened as necessary from validate() is<br>
left up to the pipeline handler implementation.<br>
<br>
Signed-off-by: Hans de Goede <<a href="mailto:hdegoede@redhat.com" target="_blank">hdegoede@redhat.com</a>><br>
---<br>
include/libcamera/internal/pipeline_handler.h | 3 +-<br>
src/libcamera/camera.cpp | 2 +-<br>
src/libcamera/pipeline_handler.cpp | 43 +++++++++++++++----<br>
3 files changed, 37 insertions(+), 11 deletions(-)<br>
<br>
diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h<br>
index cad5812f..597f7c3e 100644<br>
--- a/include/libcamera/internal/pipeline_handler.h<br>
+++ b/include/libcamera/internal/pipeline_handler.h<br>
@@ -45,7 +45,7 @@ public:<br>
MediaDevice *acquireMediaDevice(DeviceEnumerator *enumerator,<br>
const DeviceMatch &dm);<br>
<br>
- bool acquire();<br>
+ bool acquire(Camera *camera);<br>
void release(Camera *camera);<br>
<br>
virtual std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,<br>
@@ -79,6 +79,7 @@ protected:<br>
virtual int queueRequestDevice(Camera *camera, Request *request) = 0;<br>
virtual void stopDevice(Camera *camera) = 0;<br>
<br>
+ virtual bool acquireDevice(Camera *camera);<br>
virtual void releaseDevice(Camera *camera);<br>
<br>
CameraManager *manager_;<br>
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp<br>
index 382a68f7..4e393f89 100644<br>
--- a/src/libcamera/camera.cpp<br>
+++ b/src/libcamera/camera.cpp<br>
@@ -995,7 +995,7 @@ int Camera::acquire()<br>
if (ret < 0)<br>
return ret == -EACCES ? -EBUSY : ret;<br>
<br>
- if (!d->pipe_->acquire()) {<br>
+ if (!d->pipe_->acquire(this)) {<br>
LOG(Camera, Info)<br>
<< "Pipeline handler in use by another process";<br>
return -EBUSY;<br>
diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp<br>
index 1fc22d6a..e1342306 100644<br>
--- a/src/libcamera/pipeline_handler.cpp<br>
+++ b/src/libcamera/pipeline_handler.cpp<br>
@@ -163,20 +163,22 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,<br>
* has already acquired it<br>
* \sa release()<br>
*/<br>
-bool PipelineHandler::acquire()<br>
+bool PipelineHandler::acquire(Camera *camera)<br>
{<br>
MutexLocker locker(lock_);<br>
<br>
- if (useCount_) {<br>
- ++useCount_;<br>
- return true;<br>
+ if (useCount_ == 0) {<br>
+ for (std::shared_ptr<MediaDevice> &media : mediaDevices_) {<br>
+ if (!media->lock()) {<br>
+ unlockMediaDevices();<br>
+ return false;<br>
+ }<br>
+ }<br>
}<br>
<br>
- for (std::shared_ptr<MediaDevice> &media : mediaDevices_) {<br>
- if (!media->lock()) {<br>
- unlockMediaDevices();<br>
- return false;<br>
- }<br>
+ if (!acquireDevice(camera)) {<br>
+ unlockMediaDevices();<br></blockquote><div> </div><div>I think we should only unlock media devices if it's the first acquire</div><div>being called, as there might be other ongoing usages on other</div><div>cameras [2]. Therefore:</div><div>```</div><div> if (useCount_ == 0)<br> unlockMediaDevices();</div><div>```</div><div><br></div><div>[2]: <a href="https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5525900/7/src/libcamera/pipeline_handler.cpp#181">https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5525900/7/src/libcamera/pipeline_handler.cpp#181</a></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ return false;<br>
}<br>
<br>
++useCount_;<br>
@@ -213,12 +215,35 @@ void PipelineHandler::release(Camera *camera)<br>
--useCount_;<br>
}<br>
<br>
+/**<br>
+ * \brief Acquire resources associated with this camera<br>
+ * \param[in] camera The camera for which to acquire resources<br>
+ *<br>
+ * Pipeline handlers may override this in order to get resources<br>
+ * such as open /dev nodes are allocate buffers when a camera is<br>
+ * acquired.<br>
+ *<br>
+ * Note this is called once for every camera which is acquired,<br>
+ * so if there are shared resources the pipeline handler must take<br>
+ * care to not release them until releaseDevice() has been called for<br>
+ * all previously acquired cameras.<br>
+ *<br>
+ * \return True on success, false on failure.<br>
+ * \sa releaseDevice()<br>
+ */<br>
+bool PipelineHandler::acquireDevice([[maybe_unused]] Camera *camera)<br>
+{<br>
+ return true;<br>
+}<br>
+<br>
/**<br>
* \brief Release resources associated with this camera<br>
* \param[in] camera The camera for which to release resources<br>
*<br>
* Pipeline handlers may override this in order to perform cleanup operations<br>
* when a camera is released, such as freeing memory.<br>
+ *<br>
+ * \sa acquireDevice()<br>
*/<br>
void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)<br>
{<br>
-- <br>
2.46.0<br>
<br></blockquote><div><br></div><div>BR,</div><div>Harvey </div></div></div>