[{"id":30964,"web_url":"https://patchwork.libcamera.org/comment/30964/","msgid":"<20240829213331.GG15799@pendragon.ideasonboard.com>","date":"2024-08-29T21:33:31","subject":"Re: [PATCH v2 1/3] pipeline_handler: Add acquireDevice() method to\n\tmirror existing releaseDevice()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hans,\n\nThank you for the patch.\n\nOn Tue, Aug 27, 2024 at 06:42:53PM +0200, Hans de Goede wrote:\n> libcamera always keeps all /dev/video# and /dev/v4l2_subdev# nodes\n> for a pipeline open after enumerating the camera.\n> \n> This is a problem for the uvcvideo pipeline handler. Keeping /dev/video#\n> open stops the UVC USB device from being able to enter runtime-suspend\n> causing significant unnecessary power-usage.\n> \n> Add a stub acquireDevice() method to the PipelineHandler class which\n\ns/method/function/\n\nsame in the commit message. I was surprised when I found out that the\nC++ specification uses the term \"member function\" and not \"method\".\n\n> pipeline handlers can override.\n> \n> The uvcvideo pipeline handler will use this to delay opening /dev/video#\n> until the device is acquired. This is a special case because the kernel\n> uvcvideo driver powers on the USB device as soon as /dev/video# is opened.\n> This behavior should *not* be copied by other pipeline handlers.\n> \n> Signed-off-by: Hans de Goede \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> Changes in v2:\n> - Add a note to both the doxygen documentation as well as to the commit\n> message that opening/closing /dev/video# from acquire()/release()\n> as done by the uvcvideo pipeline handler is an exception and that this\n> behavior should not be copied by other pipeline handlers\n> - Other doxygen doc fixes / improvements\n> - Only unlock media devices on acquireDevice() failure if useCount_ == 0\n> ---\n> include/libcamera/internal/pipeline_handler.h | 3 +-\n> src/libcamera/camera.cpp | 2 +-\n> src/libcamera/pipeline_handler.cpp | 48 +++++++++++++++----\n> 3 files changed, 43 insertions(+), 10 deletions(-)\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index cad5812f..597f7c3e 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -45,7 +45,7 @@ public:\n> \tMediaDevice *acquireMediaDevice(DeviceEnumerator *enumerator,\n> \t\t\t\t\tconst DeviceMatch &dm);\n> \n> -\tbool acquire();\n> +\tbool acquire(Camera *camera);\n> \tvoid release(Camera *camera);\n> \n> \tvirtual std::unique_ptr generateConfiguration(Camera *camera,\n> @@ -79,6 +79,7 @@ protected:\n> \tvirtual int queueRequestDevice(Camera *camera, Request *request) = 0;\n> \tvirtual void stopDevice(Camera *camera) = 0;\n> \n> +\tvirtual bool acquireDevice(Camera *camera);\n> \tvirtual void releaseDevice(Camera *camera);\n> \n> \tCameraManager *manager_;\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 382a68f7..4e393f89 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -995,7 +995,7 @@ int Camera::acquire()\n> \tif (ret < 0)\n> \t\treturn ret == -EACCES ? -EBUSY : ret;\n> \n> -\tif (!d->pipe_->acquire()) {\n> +\tif (!d->pipe_->acquire(this)) {\n> \t\tLOG(Camera, Info)\n> \t\t\t<< \"Pipeline handler in use by another process\";\n> \t\treturn -EBUSY;\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 1fc22d6a..861815cb 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -163,20 +163,24 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,\n> * has already acquired it\n> * \\sa release()\n> */\n> -bool PipelineHandler::acquire()\n> +bool PipelineHandler::acquire(Camera *camera)\n> {\n> \tMutexLocker locker(lock_);\n> \n> -\tif (useCount_) {\n> -\t\t++useCount_;\n> -\t\treturn true;\n> +\tif (useCount_ == 0) {\n> +\t\tfor (std::shared_ptr &media : mediaDevices_) {\n> +\t\t\tif (!media->lock()) {\n> +\t\t\t\tunlockMediaDevices();\n> +\t\t\t\treturn false;\n> +\t\t\t}\n> +\t\t}\n> \t}\n> \n> -\tfor (std::shared_ptr &media : mediaDevices_) {\n> -\t\tif (!media->lock()) {\n> +\tif (!acquireDevice(camera)) {\n> +\t\tif (useCount_ == 0)\n> \t\t\tunlockMediaDevices();\n> -\t\t\treturn false;\n> -\t\t}\n> +\n> +\t\treturn false;\n> \t}\n> \n> \t++useCount_;\n> @@ -213,12 +217,40 @@ void PipelineHandler::release(Camera *camera)\n> \t--useCount_;\n> }\n> \n> +/**\n> + * \\brief Acquire resources associated with this camera\n> + * \\param[in] camera The camera for which to acquire resources\n> + *\n> + * Pipeline handlers may override this in order to get resources such as opening\n> + * devices and allocating buffers when a camera is acquired.\n> + *\n> + * This is used by the uvcvideo pipeline handler to delay opening /dev/video#\n> + * until the camera is acquired to avoid excess power consumption. The delayed\n> + * opening of /dev/video# is a special case because the kernel uvcvideo driver\n> + * powers on the USB device as soon as /dev/video# is opened. This behavior\n> + * should *not* be copied by other pipeline handlers.\n> + *\n> + * \\return True on success, false on failure\n> + * \\sa releaseDevice()\n> + */\n> +bool PipelineHandler::acquireDevice([[maybe_unused]] Camera *camera)\n> +{\n> +\treturn true;\n> +}\n> +\n> /**\n> * \\brief Release resources associated with this camera\n> * \\param[in] camera The camera for which to release resources\n> *\n> * Pipeline handlers may override this in order to perform cleanup operations\n> * when a camera is released, such as freeing memory.\n> + *\n> + * This is called once for every camera that is released. If there are resources\n> + * shared by multiple cameras then the pipeline handler must take care to not\n> + * release them until releaseDevice() has been called for all previously\n> + * acquired cameras.\n> + *\n> + * \\sa acquireDevice()\n> */\n> void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)\n> {","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A3D5FC323E\n\tfor ;\n\tThu, 29 Aug 2024 21:34:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 978E763461;\n\tThu, 29 Aug 2024 23:34:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E626C63456\n\tfor ;\n\tThu, 29 Aug 2024 23:34:02 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0E124226;\n\tThu, 29 Aug 2024 23:32:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XbV8kBnP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724967174;\n\tbh=1HwF5Kg9MJ5Qitnw+kbDjKbpG0jm9t5x0EDL3++SYxY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XbV8kBnPbnfvpLIdritmC5gJZFASUrvxrrYZMFtIIFPWYak6czVqEz7z1Hd3z5IR2\n\t3c0Q8ub6tcXKxUEh75eAv3HHEIKbQLxILrxEAqlEHBkm62hKDz3rsHjkwyoSKbZzzj\n\tHUcmvpMcUSIriL1CabAuNSbhl136yqwLSa1/JaxA=","Date":"Fri, 30 Aug 2024 00:33:31 +0300","From":"Laurent Pinchart ","To":"Hans de Goede ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tMaxime Ripard ","Subject":"Re: [PATCH v2 1/3] pipeline_handler: Add acquireDevice() method to\n\tmirror existing releaseDevice()","Message-ID":"<20240829213331.GG15799@pendragon.ideasonboard.com>","References":"<20240827164255.314432-1-hdegoede@redhat.com>\n\t<20240827164255.314432-2-hdegoede@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240827164255.314432-2-hdegoede@redhat.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30967,"web_url":"https://patchwork.libcamera.org/comment/30967/","msgid":"","date":"2024-08-29T21:46:38","subject":"Re: [PATCH v2 1/3] pipeline_handler: Add acquireDevice() method to\n\tmirror existing releaseDevice()","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Thanks Hans for the patch.\n\nReviewed-by: Harvey Yang \n\nOn Thu, Aug 29, 2024 at 11:34 PM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Hans,\n>\n> Thank you for the patch.\n>\n> On Tue, Aug 27, 2024 at 06:42:53PM +0200, Hans de Goede wrote:\n> > libcamera always keeps all /dev/video# and /dev/v4l2_subdev# nodes\n> > for a pipeline open after enumerating the camera.\n> >\n> > This is a problem for the uvcvideo pipeline handler. Keeping /dev/video#\n> > open stops the UVC USB device from being able to enter runtime-suspend\n> > causing significant unnecessary power-usage.\n> >\n> > Add a stub acquireDevice() method to the PipelineHandler class which\n>\n> s/method/function/\n>\n> same in the commit message. I was surprised when I found out that the\n> C++ specification uses the term \"member function\" and not \"method\".\n>\n> > pipeline handlers can override.\n> >\n> > The uvcvideo pipeline handler will use this to delay opening /dev/video#\n> > until the device is acquired. This is a special case because the kernel\n> > uvcvideo driver powers on the USB device as soon as /dev/video# is\n> opened.\n> > This behavior should *not* be copied by other pipeline handlers.\n> >\n> > Signed-off-by: Hans de Goede \n>\n> Reviewed-by: Laurent Pinchart \n>\n> > ---\n> > Changes in v2:\n> > - Add a note to both the doxygen documentation as well as to the commit\n> > message that opening/closing /dev/video# from acquire()/release()\n> > as done by the uvcvideo pipeline handler is an exception and that this\n> > behavior should not be copied by other pipeline handlers\n> > - Other doxygen doc fixes / improvements\n> > - Only unlock media devices on acquireDevice() failure if useCount_ == 0\n> > ---\n> > include/libcamera/internal/pipeline_handler.h | 3 +-\n> > src/libcamera/camera.cpp | 2 +-\n> > src/libcamera/pipeline_handler.cpp | 48 +++++++++++++++----\n> > 3 files changed, 43 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/pipeline_handler.h\n> b/include/libcamera/internal/pipeline_handler.h\n> > index cad5812f..597f7c3e 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -45,7 +45,7 @@ public:\n> > MediaDevice *acquireMediaDevice(DeviceEnumerator *enumerator,\n> > const DeviceMatch &dm);\n> >\n> > - bool acquire();\n> > + bool acquire(Camera *camera);\n> > void release(Camera *camera);\n> >\n> > virtual std::unique_ptr\n> generateConfiguration(Camera *camera,\n> > @@ -79,6 +79,7 @@ protected:\n> > virtual int queueRequestDevice(Camera *camera, Request *request) =\n> 0;\n> > virtual void stopDevice(Camera *camera) = 0;\n> >\n> > + virtual bool acquireDevice(Camera *camera);\n> > virtual void releaseDevice(Camera *camera);\n> >\n> > CameraManager *manager_;\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 382a68f7..4e393f89 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -995,7 +995,7 @@ int Camera::acquire()\n> > if (ret < 0)\n> > return ret == -EACCES ? -EBUSY : ret;\n> >\n> > - if (!d->pipe_->acquire()) {\n> > + if (!d->pipe_->acquire(this)) {\n> > LOG(Camera, Info)\n> > << \"Pipeline handler in use by another process\";\n> > return -EBUSY;\n> > diff --git a/src/libcamera/pipeline_handler.cpp\n> b/src/libcamera/pipeline_handler.cpp\n> > index 1fc22d6a..861815cb 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -163,20 +163,24 @@ MediaDevice\n> *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,\n> > * has already acquired it\n> > * \\sa release()\n> > */\n> > -bool PipelineHandler::acquire()\n> > +bool PipelineHandler::acquire(Camera *camera)\n> > {\n> > MutexLocker locker(lock_);\n> >\n> > - if (useCount_) {\n> > - ++useCount_;\n> > - return true;\n> > + if (useCount_ == 0) {\n> > + for (std::shared_ptr &media : mediaDevices_) {\n> > + if (!media->lock()) {\n> > + unlockMediaDevices();\n> > + return false;\n> > + }\n> > + }\n> > }\n> >\n> > - for (std::shared_ptr &media : mediaDevices_) {\n> > - if (!media->lock()) {\n> > + if (!acquireDevice(camera)) {\n> > + if (useCount_ == 0)\n> > unlockMediaDevices();\n> > - return false;\n> > - }\n> > +\n> > + return false;\n> > }\n> >\n> > ++useCount_;\n> > @@ -213,12 +217,40 @@ void PipelineHandler::release(Camera *camera)\n> > --useCount_;\n> > }\n> >\n> > +/**\n> > + * \\brief Acquire resources associated with this camera\n> > + * \\param[in] camera The camera for which to acquire resources\n> > + *\n> > + * Pipeline handlers may override this in order to get resources such\n> as opening\n> > + * devices and allocating buffers when a camera is acquired.\n> > + *\n> > + * This is used by the uvcvideo pipeline handler to delay opening\n> /dev/video#\n> > + * until the camera is acquired to avoid excess power consumption. The\n> delayed\n> > + * opening of /dev/video# is a special case because the kernel uvcvideo\n> driver\n> > + * powers on the USB device as soon as /dev/video# is opened. This\n> behavior\n> > + * should *not* be copied by other pipeline handlers.\n> > + *\n> > + * \\return True on success, false on failure\n> > + * \\sa releaseDevice()\n> > + */\n> > +bool PipelineHandler::acquireDevice([[maybe_unused]] Camera *camera)\n> > +{\n> > + return true;\n> > +}\n> > +\n> > /**\n> > * \\brief Release resources associated with this camera\n> > * \\param[in] camera The camera for which to release resources\n> > *\n> > * Pipeline handlers may override this in order to perform cleanup\n> operations\n> > * when a camera is released, such as freeing memory.\n> > + *\n> > + * This is called once for every camera that is released. If there are\n> resources\n> > + * shared by multiple cameras then the pipeline handler must take care\n> to not\n> > + * release them until releaseDevice() has been called for all previously\n> > + * acquired cameras.\n> > + *\n> > + * \\sa acquireDevice()\n> > */\n> > void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)\n> > {\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n\nBR,\nHarvey","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A7C44C324C\n\tfor ;\n\tThu, 29 Aug 2024 21:46:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9472063466;\n\tThu, 29 Aug 2024 23:46:51 +0200 (CEST)","from mail-lj1-x229.google.com (mail-lj1-x229.google.com\n\t[IPv6:2a00:1450:4864:20::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2C02B63458\n\tfor ;\n\tThu, 29 Aug 2024 23:46:50 +0200 (CEST)","by mail-lj1-x229.google.com with SMTP id\n\t38308e7fff4ca-2f4f2868621so12736811fa.0\n\tfor ;\n\tThu, 29 Aug 2024 14:46:50 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"nD6Q7M9f\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1724968009; x=1725572809;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=7wBv55Bh+t8SuyrpVif0YTBZkgk82O857UZ1amuBqWQ=;\n\tb=nD6Q7M9fELDMDmC0n/WEpPlPUrFyUmS0D1Wk8GRulvX+3nGcf5KWbLSbh5aCca/no6\n\t4Ssiv8f2niSzxUwcJ1jXmainApsdoxBW+A3M6+8eP2ujd6xVGhAOI8sj8iZEWXOUhgdu\n\teX/WxbQzdT5umVYMN+NTt2hGU4P44tY+ahcnY=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1724968009; x=1725572809;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=7wBv55Bh+t8SuyrpVif0YTBZkgk82O857UZ1amuBqWQ=;\n\tb=g0aps6rshwun87BWw/Dq8MTG/PklhycMfdQbpLYHKI3cITmxHcJ09HTxruuH+EumfL\n\tO1E3OYb5Fh4f7PNcsanKNhjgZiYj/XS6PniNdvgsm2rNrIhNZjskxRB/WrZSWvzBWHBI\n\tk7s5eYQY4UJCowBIvnOPo9OxH4/19cmm6SmZlIUcEYgCx9smn/AI9Os87N4iuX2VDpDE\n\t46imKfIGJVTRdVnDgFsMIIpx4rbYTGSsuU627AhiHcEsjMe8esB99o4BYkm6MOtvmB9X\n\tk+pny3e2FVRVmdN7+ZREhhwcw5xaZoZHEFEjLrZDxuE4e7x+62+yzZUmxcBpAVPFlOKz\n\tCdWQ==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCW39uUyQKjIQychrubfeHuym0RNffmbo2eNkcPSGOcOx5PcYkC3ZzRVeSBspE6kPCqJQQi0LGNBkdgyPh2xdDY=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yz1hujH/RgkNuEMQ0vbRbunch56LRhrb45CdYXAYEZebj6Osoj+\n\tHNq2NgTiks6USzBOyFnx+OpRDP6vb38yePZqFWvUml30c3WGpUOCdiGg6ARNExv7apQ14PcvJq+\n\tj9WmD5T8xpS8VgKdsAIXuP6r96i2Bhs6/+rg4","X-Google-Smtp-Source":"AGHT+IHXFFGkzY6QInLwDdIuOsBxMELsLt76Y3+KSrmd6oUOxNdAsZjATKkZx8zogQIywkDY0MhgUb90VFbPVgvhlng=","X-Received":"by 2002:a05:651c:1548:b0:2ef:2f9e:dd1b with SMTP id\n\t38308e7fff4ca-2f6103a73f1mr30248451fa.14.1724968009052;\n\tThu, 29 Aug 2024 14:46:49 -0700 (PDT)","MIME-Version":"1.0","References":"<20240827164255.314432-1-hdegoede@redhat.com>\n\t<20240827164255.314432-2-hdegoede@redhat.com>\n\t<20240829213331.GG15799@pendragon.ideasonboard.com>","In-Reply-To":"<20240829213331.GG15799@pendragon.ideasonboard.com>","From":"Cheng-Hao Yang ","Date":"Thu, 29 Aug 2024 23:46:38 +0200","Message-ID":"","Subject":"Re: [PATCH v2 1/3] pipeline_handler: Add acquireDevice() method to\n\tmirror existing releaseDevice()","To":"Laurent Pinchart ","Cc":"Hans de Goede , libcamera-devel@lists.libcamera.org,\n\tMilan Zamazal , Maxime Ripard ","Content-Type":"multipart/alternative; boundary=\"0000000000006dcfbc0620d96970\"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]