| Message ID | 20240827164255.314432-4-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Hans, Thank you for the patch. On Tue, Aug 27, 2024 at 06:42:55PM +0200, Hans de Goede wrote: > The uvcvideo pipeline handler always keeps the uvcvideo /dev/video# node > for a pipeline open after enumerating the camera. > > This is a problem for uvcvideo, as keeping the /dev/video# node open stops > the underlying USB device and the USB bus controller from being able to > enter runtime-suspend causing significant unnecessary power-usage. > > Implement acquireDevice() + releaseDevice(), openening /dev/video# on > acquire and closing it on release to fix this. > > And make validate do a local video_->open() + close() around validate() > when not open yet. To keep validate() working on unacquired cameras. s/yet. To/yet, to/ > Link: https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/2669 I'd replace this with Bug: https://bugs.libcamera.org/show_bug.cgi?id=168 which links to the above pipewire issue. > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v2: > - Minor commit msg + code improvements > --- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 45 ++++++++++++++++++++ > 1 file changed, 45 insertions(+) > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > index 8a7409fc..584f5230 100644 > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > @@ -13,6 +13,7 @@ > #include <tuple> > > #include <libcamera/base/log.h> > +#include <libcamera/base/mutex.h> > #include <libcamera/base/utils.h> > > #include <libcamera/camera.h> > @@ -48,6 +49,7 @@ public: > > const std::string &id() const { return id_; } > > + Mutex openLock_; > std::unique_ptr<V4L2VideoDevice> video_; > Stream stream_; > std::map<PixelFormat, std::vector<SizeRange>> formats_; > @@ -93,6 +95,9 @@ private: > const ControlValue &value); > int processControls(UVCCameraData *data, Request *request); > > + bool acquireDevice(Camera *camera) override; > + void releaseDevice(Camera *camera) override; > + > UVCCameraData *cameraData(Camera *camera) > { > return static_cast<UVCCameraData *>(camera->_d()); > @@ -107,6 +112,7 @@ UVCCameraConfiguration::UVCCameraConfiguration(UVCCameraData *data) > CameraConfiguration::Status UVCCameraConfiguration::validate() > { > Status status = Valid; > + bool opened = false; I would move this below. > > if (config_.empty()) > return Invalid; > @@ -158,7 +164,23 @@ CameraConfiguration::Status UVCCameraConfiguration::validate() > format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat); > format.size = cfg.size; > > + /* > + * For power-consumption reasons video_ is closed when the camera > + * is not acquired. Open it here if necessary. > + */ Here. > + MutexLocker locker(data_->openLock_); > + > + if (!data_->video_->isOpen()) { > + int ret = data_->video_->open(); > + if (ret) > + return Invalid; > + > + opened = true; > + } > + > int ret = data_->video_->tryFormat(&format); > + if (opened) > + data_->video_->close(); > if (ret) > return Invalid; The code below doesn't need to be protected by the lock. bool opened = false; int ret; MutexLocker locker(data_->openLock_); { if (!data_->video_->isOpen()) { ret = data_->video_->open(); if (ret) return Invalid; opened = true; } ret = data_->video_->tryFormat(&format); if (opened) data_->video_->close(); } if (ret) return Invalid; > > @@ -411,6 +433,23 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) > return true; > } > > +bool PipelineHandlerUVC::acquireDevice(Camera *camera) > +{ > + UVCCameraData *data = cameraData(camera); > + > + MutexLocker locker(data->openLock_); > + > + return data->video_->open() == 0; > +} > + > +void PipelineHandlerUVC::releaseDevice(Camera *camera) > +{ > + UVCCameraData *data = cameraData(camera); > + > + MutexLocker locker(data->openLock_); > + data->video_->close(); > +} > + > int UVCCameraData::init(MediaDevice *media) > { > int ret; > @@ -512,6 +551,12 @@ int UVCCameraData::init(MediaDevice *media) > > controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls); > > + /* > + * Close to allow camera to go into runtime-suspend, video_ > + * will be re-opened from acquireDevice() and validate(). You can reflow this to 80 columns. With these small issues addressed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + */ > + video_->close(); > + > return 0; > } >
Hi Laurent, On 8/29/24 11:58 PM, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Tue, Aug 27, 2024 at 06:42:55PM +0200, Hans de Goede wrote: >> The uvcvideo pipeline handler always keeps the uvcvideo /dev/video# node >> for a pipeline open after enumerating the camera. >> >> This is a problem for uvcvideo, as keeping the /dev/video# node open stops >> the underlying USB device and the USB bus controller from being able to >> enter runtime-suspend causing significant unnecessary power-usage. >> >> Implement acquireDevice() + releaseDevice(), openening /dev/video# on >> acquire and closing it on release to fix this. >> >> And make validate do a local video_->open() + close() around validate() >> when not open yet. To keep validate() working on unacquired cameras. > > s/yet. To/yet, to/ > >> Link: https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/2669 > > I'd replace this with > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=168 > > which links to the above pipewire issue. > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v2: >> - Minor commit msg + code improvements >> --- >> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 45 ++++++++++++++++++++ >> 1 file changed, 45 insertions(+) >> >> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp >> index 8a7409fc..584f5230 100644 >> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp >> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp >> @@ -13,6 +13,7 @@ >> #include <tuple> >> >> #include <libcamera/base/log.h> >> +#include <libcamera/base/mutex.h> >> #include <libcamera/base/utils.h> >> >> #include <libcamera/camera.h> >> @@ -48,6 +49,7 @@ public: >> >> const std::string &id() const { return id_; } >> >> + Mutex openLock_; >> std::unique_ptr<V4L2VideoDevice> video_; >> Stream stream_; >> std::map<PixelFormat, std::vector<SizeRange>> formats_; >> @@ -93,6 +95,9 @@ private: >> const ControlValue &value); >> int processControls(UVCCameraData *data, Request *request); >> >> + bool acquireDevice(Camera *camera) override; >> + void releaseDevice(Camera *camera) override; >> + >> UVCCameraData *cameraData(Camera *camera) >> { >> return static_cast<UVCCameraData *>(camera->_d()); >> @@ -107,6 +112,7 @@ UVCCameraConfiguration::UVCCameraConfiguration(UVCCameraData *data) >> CameraConfiguration::Status UVCCameraConfiguration::validate() >> { >> Status status = Valid; >> + bool opened = false; > > I would move this below. > >> >> if (config_.empty()) >> return Invalid; >> @@ -158,7 +164,23 @@ CameraConfiguration::Status UVCCameraConfiguration::validate() >> format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat); >> format.size = cfg.size; >> >> + /* >> + * For power-consumption reasons video_ is closed when the camera >> + * is not acquired. Open it here if necessary. >> + */ > > Here. > >> + MutexLocker locker(data_->openLock_); >> + >> + if (!data_->video_->isOpen()) { >> + int ret = data_->video_->open(); >> + if (ret) >> + return Invalid; >> + >> + opened = true; >> + } >> + >> int ret = data_->video_->tryFormat(&format); >> + if (opened) >> + data_->video_->close(); >> if (ret) >> return Invalid; > > The code below doesn't need to be protected by the lock. > > bool opened = false; > int ret; > > MutexLocker locker(data_->openLock_); > > { > if (!data_->video_->isOpen()) { > ret = data_->video_->open(); > if (ret) > return Invalid; > > opened = true; > } > > ret = data_->video_->tryFormat(&format); > if (opened) > data_->video_->close(); > } I assume you meant to move the "MutexLocker locker(data_->openLock_);" to inside the { } scope here, so that its lifetime is limited to that scope ? > if (ret) > return Invalid; I would prefer to have this inside the { } scope since that is where it logically belongs and this check will be so fast that it does not matter wrt holding the lock longer. With those remarks I agree with your suggestion and I'll implement this change for v3. Regards, Hans > >> >> @@ -411,6 +433,23 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) >> return true; >> } >> >> +bool PipelineHandlerUVC::acquireDevice(Camera *camera) >> +{ >> + UVCCameraData *data = cameraData(camera); >> + >> + MutexLocker locker(data->openLock_); >> + >> + return data->video_->open() == 0; >> +} >> + >> +void PipelineHandlerUVC::releaseDevice(Camera *camera) >> +{ >> + UVCCameraData *data = cameraData(camera); >> + >> + MutexLocker locker(data->openLock_); >> + data->video_->close(); >> +} >> + >> int UVCCameraData::init(MediaDevice *media) >> { >> int ret; >> @@ -512,6 +551,12 @@ int UVCCameraData::init(MediaDevice *media) >> >> controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls); >> >> + /* >> + * Close to allow camera to go into runtime-suspend, video_ >> + * will be re-opened from acquireDevice() and validate(). > > You can reflow this to 80 columns. > > With these small issues addressed, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> + */ >> + video_->close(); >> + >> return 0; >> } >> >
On Fri, Aug 30, 2024 at 10:01:01AM +0200, Hans de Goede wrote: > Hi Laurent, > > On 8/29/24 11:58 PM, Laurent Pinchart wrote: > > Hi Hans, > > > > Thank you for the patch. > > > > On Tue, Aug 27, 2024 at 06:42:55PM +0200, Hans de Goede wrote: > >> The uvcvideo pipeline handler always keeps the uvcvideo /dev/video# node > >> for a pipeline open after enumerating the camera. > >> > >> This is a problem for uvcvideo, as keeping the /dev/video# node open stops > >> the underlying USB device and the USB bus controller from being able to > >> enter runtime-suspend causing significant unnecessary power-usage. > >> > >> Implement acquireDevice() + releaseDevice(), openening /dev/video# on > >> acquire and closing it on release to fix this. > >> > >> And make validate do a local video_->open() + close() around validate() > >> when not open yet. To keep validate() working on unacquired cameras. > > > > s/yet. To/yet, to/ > > > >> Link: https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/2669 > > > > I'd replace this with > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=168 > > > > which links to the above pipewire issue. > > > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > >> Changes in v2: > >> - Minor commit msg + code improvements > >> --- > >> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 45 ++++++++++++++++++++ > >> 1 file changed, 45 insertions(+) > >> > >> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > >> index 8a7409fc..584f5230 100644 > >> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > >> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > >> @@ -13,6 +13,7 @@ > >> #include <tuple> > >> > >> #include <libcamera/base/log.h> > >> +#include <libcamera/base/mutex.h> > >> #include <libcamera/base/utils.h> > >> > >> #include <libcamera/camera.h> > >> @@ -48,6 +49,7 @@ public: > >> > >> const std::string &id() const { return id_; } > >> > >> + Mutex openLock_; > >> std::unique_ptr<V4L2VideoDevice> video_; > >> Stream stream_; > >> std::map<PixelFormat, std::vector<SizeRange>> formats_; > >> @@ -93,6 +95,9 @@ private: > >> const ControlValue &value); > >> int processControls(UVCCameraData *data, Request *request); > >> > >> + bool acquireDevice(Camera *camera) override; > >> + void releaseDevice(Camera *camera) override; > >> + > >> UVCCameraData *cameraData(Camera *camera) > >> { > >> return static_cast<UVCCameraData *>(camera->_d()); > >> @@ -107,6 +112,7 @@ UVCCameraConfiguration::UVCCameraConfiguration(UVCCameraData *data) > >> CameraConfiguration::Status UVCCameraConfiguration::validate() > >> { > >> Status status = Valid; > >> + bool opened = false; > > > > I would move this below. > > > >> > >> if (config_.empty()) > >> return Invalid; > >> @@ -158,7 +164,23 @@ CameraConfiguration::Status UVCCameraConfiguration::validate() > >> format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat); > >> format.size = cfg.size; > >> > >> + /* > >> + * For power-consumption reasons video_ is closed when the camera > >> + * is not acquired. Open it here if necessary. > >> + */ > > > > Here. > > > >> + MutexLocker locker(data_->openLock_); > >> + > >> + if (!data_->video_->isOpen()) { > >> + int ret = data_->video_->open(); > >> + if (ret) > >> + return Invalid; > >> + > >> + opened = true; > >> + } > >> + > >> int ret = data_->video_->tryFormat(&format); > >> + if (opened) > >> + data_->video_->close(); > >> if (ret) > >> return Invalid; > > > > The code below doesn't need to be protected by the lock. > > > > bool opened = false; > > int ret; > > > > MutexLocker locker(data_->openLock_); > > > > { > > if (!data_->video_->isOpen()) { > > ret = data_->video_->open(); > > if (ret) > > return Invalid; > > > > opened = true; > > } > > > > ret = data_->video_->tryFormat(&format); > > if (opened) > > data_->video_->close(); > > } > > I assume you meant to move the "MutexLocker locker(data_->openLock_);" > to inside the { } scope here, so that its lifetime is limited to > that scope ? Yes that's what I meant sorry. > > if (ret) > > return Invalid; > > I would prefer to have this inside the { } scope since that is where > it logically belongs and this check will be so fast that it does not > matter wrt holding the lock longer. Fine with me. > With those remarks I agree with your suggestion and I'll implement > this change for v3. > > >> @@ -411,6 +433,23 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) > >> return true; > >> } > >> > >> +bool PipelineHandlerUVC::acquireDevice(Camera *camera) > >> +{ > >> + UVCCameraData *data = cameraData(camera); > >> + > >> + MutexLocker locker(data->openLock_); > >> + > >> + return data->video_->open() == 0; > >> +} > >> + > >> +void PipelineHandlerUVC::releaseDevice(Camera *camera) > >> +{ > >> + UVCCameraData *data = cameraData(camera); > >> + > >> + MutexLocker locker(data->openLock_); > >> + data->video_->close(); > >> +} > >> + > >> int UVCCameraData::init(MediaDevice *media) > >> { > >> int ret; > >> @@ -512,6 +551,12 @@ int UVCCameraData::init(MediaDevice *media) > >> > >> controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls); > >> > >> + /* > >> + * Close to allow camera to go into runtime-suspend, video_ > >> + * will be re-opened from acquireDevice() and validate(). > > > > You can reflow this to 80 columns. > > > > With these small issues addressed, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >> + */ > >> + video_->close(); > >> + > >> return 0; > >> } > >>
diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp index 8a7409fc..584f5230 100644 --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp @@ -13,6 +13,7 @@ #include <tuple> #include <libcamera/base/log.h> +#include <libcamera/base/mutex.h> #include <libcamera/base/utils.h> #include <libcamera/camera.h> @@ -48,6 +49,7 @@ public: const std::string &id() const { return id_; } + Mutex openLock_; std::unique_ptr<V4L2VideoDevice> video_; Stream stream_; std::map<PixelFormat, std::vector<SizeRange>> formats_; @@ -93,6 +95,9 @@ private: const ControlValue &value); int processControls(UVCCameraData *data, Request *request); + bool acquireDevice(Camera *camera) override; + void releaseDevice(Camera *camera) override; + UVCCameraData *cameraData(Camera *camera) { return static_cast<UVCCameraData *>(camera->_d()); @@ -107,6 +112,7 @@ UVCCameraConfiguration::UVCCameraConfiguration(UVCCameraData *data) CameraConfiguration::Status UVCCameraConfiguration::validate() { Status status = Valid; + bool opened = false; if (config_.empty()) return Invalid; @@ -158,7 +164,23 @@ CameraConfiguration::Status UVCCameraConfiguration::validate() format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat); format.size = cfg.size; + /* + * For power-consumption reasons video_ is closed when the camera + * is not acquired. Open it here if necessary. + */ + MutexLocker locker(data_->openLock_); + + if (!data_->video_->isOpen()) { + int ret = data_->video_->open(); + if (ret) + return Invalid; + + opened = true; + } + int ret = data_->video_->tryFormat(&format); + if (opened) + data_->video_->close(); if (ret) return Invalid; @@ -411,6 +433,23 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) return true; } +bool PipelineHandlerUVC::acquireDevice(Camera *camera) +{ + UVCCameraData *data = cameraData(camera); + + MutexLocker locker(data->openLock_); + + return data->video_->open() == 0; +} + +void PipelineHandlerUVC::releaseDevice(Camera *camera) +{ + UVCCameraData *data = cameraData(camera); + + MutexLocker locker(data->openLock_); + data->video_->close(); +} + int UVCCameraData::init(MediaDevice *media) { int ret; @@ -512,6 +551,12 @@ int UVCCameraData::init(MediaDevice *media) controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls); + /* + * Close to allow camera to go into runtime-suspend, video_ + * will be re-opened from acquireDevice() and validate(). + */ + video_->close(); + return 0; }
The uvcvideo pipeline handler always keeps the uvcvideo /dev/video# node for a pipeline open after enumerating the camera. This is a problem for uvcvideo, as keeping the /dev/video# node open stops the underlying USB device and the USB bus controller from being able to enter runtime-suspend causing significant unnecessary power-usage. Implement acquireDevice() + releaseDevice(), openening /dev/video# on acquire and closing it on release to fix this. And make validate do a local video_->open() + close() around validate() when not open yet. To keep validate() working on unacquired cameras. Link: https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/2669 Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v2: - Minor commit msg + code improvements --- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 45 ++++++++++++++++++++ 1 file changed, 45 insertions(+)