| Message ID | 20240820195016.16028-6-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Hans, Thank you for the patch. On Tue, Aug 20, 2024 at 09:50:16PM +0200, Hans de Goede wrote: > ATM the uvcvideo pipeline handler always keeps the uvcvideo /dev/video# s/ATM/At the moment/ (or just drop it) > 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. Won't you hit the same issue that 4/5 is meant to fix, with the notifiers being created from the wrong thread ? > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 46 ++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > index 8a7409fc..d3eedfdc 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,24 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) > return true; > } > > +bool PipelineHandlerUVC::acquireDevice(Camera *camera) > +{ > + UVCCameraData *data = cameraData(camera); > + > + MutexLocker locker(data->openLock_); > + > + int ret = data->video_->open(); > + return ret == 0; You can simplify this to 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 +552,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; > } >
Hi Laurent, On 8/25/24 3:01 AM, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Tue, Aug 20, 2024 at 09:50:16PM +0200, Hans de Goede wrote: >> ATM the uvcvideo pipeline handler always keeps the uvcvideo /dev/video# > > s/ATM/At the moment/ > > (or just drop it) > >> 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. > > Won't you hit the same issue that 4/5 is meant to fix, with the > notifiers being created from the wrong thread ? Yes, but the notifiers are also destroyed again before any other code actually tries to use use them. Either the camera is already acquired and no open/close of video_ is done, or it is not acquired and both open + close of video_ are done without any other code-paths being able to use video_ in the mean time. The openLock_ mutex protects against acquireDevice() / releaseDevice() racing with the validate() call and all other users of video_ require the camera to be acquired first. Note no open-counting is done since there with the uvcvideo pipeline handler there only ever is one camera. Regards, Hans > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 46 ++++++++++++++++++++ >> 1 file changed, 46 insertions(+) >> >> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp >> index 8a7409fc..d3eedfdc 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,24 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) >> return true; >> } >> >> +bool PipelineHandlerUVC::acquireDevice(Camera *camera) >> +{ >> + UVCCameraData *data = cameraData(camera); >> + >> + MutexLocker locker(data->openLock_); >> + >> + int ret = data->video_->open(); >> + return ret == 0; > > You can simplify this to > > 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 +552,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; >> } >> >
Hi Hans, On Mon, Aug 26, 2024 at 10:17:17AM +0200, Hans de Goede wrote: > On 8/25/24 3:01 AM, Laurent Pinchart wrote: > > On Tue, Aug 20, 2024 at 09:50:16PM +0200, Hans de Goede wrote: > >> ATM the uvcvideo pipeline handler always keeps the uvcvideo /dev/video# > > > > s/ATM/At the moment/ > > > > (or just drop it) > > > >> 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. > > > > Won't you hit the same issue that 4/5 is meant to fix, with the > > notifiers being created from the wrong thread ? > > Yes, but the notifiers are also destroyed again before any other code actually > tries to use use them. True, it shouldn't cause any problem in practice, but it makes me feel a bit uncomfortable still. > Either the camera is already acquired and no open/close of video_ is done, > or it is not acquired and both open + close of video_ are done without > any other code-paths being able to use video_ in the mean time. > > The openLock_ mutex protects against acquireDevice() / releaseDevice() > racing with the validate() call and all other users of video_ require > the camera to be acquired first. > > Note no open-counting is done since there with the uvcvideo pipeline > handler there only ever is one camera. I can live with this for the uvcvideo pipeline handler for the time being. Let's make sure it doesn't spread before the API improves. To be honest, I'm a bit concerned than shifting the yak shaving to the second user on the grounds that we need more than a single use case to do things right will be met with unhappiness on the grounds that the first user got away with it so it's not fair. I suppose doing things the right but hard way will always be met with this kind of problems :-) > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > >> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 46 ++++++++++++++++++++ > >> 1 file changed, 46 insertions(+) > >> > >> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > >> index 8a7409fc..d3eedfdc 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,24 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) > >> return true; > >> } > >> > >> +bool PipelineHandlerUVC::acquireDevice(Camera *camera) > >> +{ > >> + UVCCameraData *data = cameraData(camera); > >> + > >> + MutexLocker locker(data->openLock_); > >> + > >> + int ret = data->video_->open(); > >> + return ret == 0; > > > > You can simplify this to > > > > 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 +552,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; > >> } > >>
diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp index 8a7409fc..d3eedfdc 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,24 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) return true; } +bool PipelineHandlerUVC::acquireDevice(Camera *camera) +{ + UVCCameraData *data = cameraData(camera); + + MutexLocker locker(data->openLock_); + + int ret = data->video_->open(); + return ret == 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 +552,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; }
ATM 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. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 46 ++++++++++++++++++++ 1 file changed, 46 insertions(+)