<div dir="ltr"><div dir="ltr">Thanks Hans for the patch.<div><br></div><div>Reviewed-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org">chenghaoyang@chromium.org</a>></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Aug 29, 2024 at 11:34 PM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.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">Hi Hans,<br>
<br>
Thank you for the patch.<br>
<br>
On Tue, Aug 27, 2024 at 06:42:53PM +0200, Hans de Goede wrote:<br>
> 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 the uvcvideo pipeline handler. Keeping /dev/video#<br>
> open stops the UVC USB device from being able to enter runtime-suspend<br>
> causing significant unnecessary power-usage.<br>
> <br>
> Add a stub acquireDevice() method to the PipelineHandler class which<br>
<br>
s/method/function/<br>
<br>
same in the commit message. I was surprised when I found out that the<br>
C++ specification uses the term "member function" and not "method".<br>
<br>
> pipeline handlers can override.<br>
> <br>
> The uvcvideo pipeline handler will use this to delay opening /dev/video#<br>
> until the device is acquired. This is a special case because the kernel<br>
> uvcvideo driver powers on the USB device as soon as /dev/video# is opened.<br>
> This behavior should *not* be copied by other pipeline handlers.<br>
> <br>
> Signed-off-by: Hans de Goede <<a href="mailto:hdegoede@redhat.com" target="_blank">hdegoede@redhat.com</a>><br>
<br>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
> ---<br>
> Changes in v2:<br>
> - Add a note to both the doxygen documentation as well as to the commit<br>
> message that opening/closing /dev/video# from acquire()/release()<br>
> as done by the uvcvideo pipeline handler is an exception and that this<br>
> behavior should not be copied by other pipeline handlers<br>
> - Other doxygen doc fixes / improvements<br>
> - Only unlock media devices on acquireDevice() failure if useCount_ == 0<br>
> ---<br>
> include/libcamera/internal/pipeline_handler.h | 3 +-<br>
> src/libcamera/camera.cpp | 2 +-<br>
> src/libcamera/pipeline_handler.cpp | 48 +++++++++++++++----<br>
> 3 files changed, 43 insertions(+), 10 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..861815cb 100644<br>
> --- a/src/libcamera/pipeline_handler.cpp<br>
> +++ b/src/libcamera/pipeline_handler.cpp<br>
> @@ -163,20 +163,24 @@ 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>
> + if (!acquireDevice(camera)) {<br>
> + if (useCount_ == 0)<br>
> unlockMediaDevices();<br>
> - return false;<br>
> - }<br>
> +<br>
> + return false;<br>
> }<br>
> <br>
> ++useCount_;<br>
> @@ -213,12 +217,40 @@ 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 such as opening<br>
> + * devices and allocating buffers when a camera is acquired.<br>
> + *<br>
> + * This is used by the uvcvideo pipeline handler to delay opening /dev/video#<br>
> + * until the camera is acquired to avoid excess power consumption. The delayed<br>
> + * opening of /dev/video# is a special case because the kernel uvcvideo driver<br>
> + * powers on the USB device as soon as /dev/video# is opened. This behavior<br>
> + * should *not* be copied by other pipeline handlers.<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>
> + * This is called once for every camera that is released. If there are resources<br>
> + * shared by multiple cameras then the pipeline handler must take care to not<br>
> + * release them until releaseDevice() has been called for all previously<br>
> + * acquired cameras.<br>
> + *<br>
> + * \sa acquireDevice()<br>
> */<br>
> void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)<br>
> {<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br></blockquote><div><br></div><div>BR,<br>Harvey </div></div></div>