[{"id":30968,"web_url":"https://patchwork.libcamera.org/comment/30968/","msgid":"<20240829215837.GI15799@pendragon.ideasonboard.com>","date":"2024-08-29T21:58:37","subject":"Re: [PATCH v2 3/3] uvcvideo: Implement acquireDevice() +\n\treleaseDevice()","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:55PM +0200, Hans de Goede wrote:\n> The uvcvideo pipeline handler always keeps the uvcvideo /dev/video# node\n> for a pipeline open after enumerating the camera.\n> \n> This is a problem for uvcvideo, as keeping the /dev/video# node open stops\n> the underlying USB device and the USB bus controller from being able to\n> enter runtime-suspend causing significant unnecessary power-usage.\n> \n> Implement acquireDevice() + releaseDevice(), openening /dev/video# on\n> acquire and closing it on release to fix this.\n> \n> And make validate do a local video_->open() + close() around validate()\n> when not open yet. To keep validate() working on unacquired cameras.\n\ns/yet. To/yet, to/\n\n> Link: https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/2669\n\nI'd replace this with\n\nBug: https://bugs.libcamera.org/show_bug.cgi?id=168\n\nwhich links to the above pipewire issue.\n\n> Signed-off-by: Hans de Goede \n> ---\n> Changes in v2:\n> - Minor commit msg + code improvements\n> ---\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 45 ++++++++++++++++++++\n> 1 file changed, 45 insertions(+)\n> \n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 8a7409fc..584f5230 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -13,6 +13,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -48,6 +49,7 @@ public:\n> \n> \tconst std::string &id() const { return id_; }\n> \n> +\tMutex openLock_;\n> \tstd::unique_ptr video_;\n> \tStream stream_;\n> \tstd::map> formats_;\n> @@ -93,6 +95,9 @@ private:\n> \t\t\t const ControlValue &value);\n> \tint processControls(UVCCameraData *data, Request *request);\n> \n> +\tbool acquireDevice(Camera *camera) override;\n> +\tvoid releaseDevice(Camera *camera) override;\n> +\n> \tUVCCameraData *cameraData(Camera *camera)\n> \t{\n> \t\treturn static_cast(camera->_d());\n> @@ -107,6 +112,7 @@ UVCCameraConfiguration::UVCCameraConfiguration(UVCCameraData *data)\n> CameraConfiguration::Status UVCCameraConfiguration::validate()\n> {\n> \tStatus status = Valid;\n> +\tbool opened = false;\n\nI would move this below.\n\n> \n> \tif (config_.empty())\n> \t\treturn Invalid;\n> @@ -158,7 +164,23 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()\n> \tformat.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);\n> \tformat.size = cfg.size;\n> \n> +\t/*\n> +\t * For power-consumption reasons video_ is closed when the camera\n> +\t * is not acquired. Open it here if necessary.\n> +\t */\n\nHere.\n\n> +\tMutexLocker locker(data_->openLock_);\n> +\n> +\tif (!data_->video_->isOpen()) {\n> +\t\tint ret = data_->video_->open();\n> +\t\tif (ret)\n> +\t\t\treturn Invalid;\n> +\n> +\t\topened = true;\n> +\t}\n> +\n> \tint ret = data_->video_->tryFormat(&format);\n> +\tif (opened)\n> +\t\tdata_->video_->close();\n> \tif (ret)\n> \t\treturn Invalid;\n\nThe code below doesn't need to be protected by the lock.\n\n\tbool opened = false;\n\tint ret;\n\n\tMutexLocker locker(data_->openLock_);\n\n\t{\n\t\tif (!data_->video_->isOpen()) {\n\t\t\tret = data_->video_->open();\n\t\t\tif (ret)\n\t\t\t\treturn Invalid;\n\n\t\t\topened = true;\n\t\t}\n\n\t\tret = data_->video_->tryFormat(&format);\n\t\tif (opened)\n\t\t\tdata_->video_->close();\n\t}\n\n\tif (ret)\n\t\treturn Invalid;\n\n> \n> @@ -411,6 +433,23 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> \treturn true;\n> }\n> \n> +bool PipelineHandlerUVC::acquireDevice(Camera *camera)\n> +{\n> +\tUVCCameraData *data = cameraData(camera);\n> +\n> +\tMutexLocker locker(data->openLock_);\n> +\n> +\treturn data->video_->open() == 0;\n> +}\n> +\n> +void PipelineHandlerUVC::releaseDevice(Camera *camera)\n> +{\n> +\tUVCCameraData *data = cameraData(camera);\n> +\n> +\tMutexLocker locker(data->openLock_);\n> +\tdata->video_->close();\n> +}\n> +\n> int UVCCameraData::init(MediaDevice *media)\n> {\n> \tint ret;\n> @@ -512,6 +551,12 @@ int UVCCameraData::init(MediaDevice *media)\n> \n> \tcontrolInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n> \n> +\t/*\n> +\t * Close to allow camera to go into runtime-suspend, video_\n> +\t * will be re-opened from acquireDevice() and validate().\n\nYou can reflow this to 80 columns.\n\nWith these small issues addressed,\n\nReviewed-by: Laurent Pinchart \n\n> +\t */\n> +\tvideo_->close();\n> +\n> \treturn 0;\n> }\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 A7FCDC323E\n\tfor ;\n\tThu, 29 Aug 2024 21:59:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B143363461;\n\tThu, 29 Aug 2024 23:59:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 91F7A63456\n\tfor ;\n\tThu, 29 Aug 2024 23:59:08 +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 B3C44226;\n\tThu, 29 Aug 2024 23:57:59 +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=\"gG7NBV18\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724968679;\n\tbh=MHzLpRd8Ltud2R88cQwNkDPhBSwJpFOYEPC7RwEYqQA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gG7NBV18ssYQet4WYUsoEfzCwUWQVYJ+NY6gdBX6vg+XjuRxaVxzquArnrlGE0ULc\n\tG2LzFC7BaLkabv6zHoA3tiPURlGeTIKY5t5kVppFg76lIvIgpwozVbdNDyEbD32LIR\n\tVXxjwwkvnpjlHsVxhFVBbB0dEzue3zilKgrn9CHo=","Date":"Fri, 30 Aug 2024 00:58:37 +0300","From":"Laurent Pinchart ","To":"Hans de Goede ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tMaxime Ripard ","Subject":"Re: [PATCH v2 3/3] uvcvideo: Implement acquireDevice() +\n\treleaseDevice()","Message-ID":"<20240829215837.GI15799@pendragon.ideasonboard.com>","References":"<20240827164255.314432-1-hdegoede@redhat.com>\n\t<20240827164255.314432-4-hdegoede@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240827164255.314432-4-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":30981,"web_url":"https://patchwork.libcamera.org/comment/30981/","msgid":"<9b6b98fd-6a0a-47d6-b0ca-07e2d6fcefb4@redhat.com>","date":"2024-08-30T08:01:01","subject":"Re: [PATCH v2 3/3] uvcvideo: Implement acquireDevice() +\n\treleaseDevice()","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi Laurent,\n\nOn 8/29/24 11:58 PM, Laurent Pinchart wrote:\n> Hi Hans,\n> \n> Thank you for the patch.\n> \n> On Tue, Aug 27, 2024 at 06:42:55PM +0200, Hans de Goede wrote:\n>> The uvcvideo pipeline handler always keeps the uvcvideo /dev/video# node\n>> for a pipeline open after enumerating the camera.\n>>\n>> This is a problem for uvcvideo, as keeping the /dev/video# node open stops\n>> the underlying USB device and the USB bus controller from being able to\n>> enter runtime-suspend causing significant unnecessary power-usage.\n>>\n>> Implement acquireDevice() + releaseDevice(), openening /dev/video# on\n>> acquire and closing it on release to fix this.\n>>\n>> And make validate do a local video_->open() + close() around validate()\n>> when not open yet. To keep validate() working on unacquired cameras.\n> \n> s/yet. To/yet, to/\n> \n>> Link: https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/2669\n> \n> I'd replace this with\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=168\n> \n> which links to the above pipewire issue.\n> \n>> Signed-off-by: Hans de Goede \n>> ---\n>> Changes in v2:\n>> - Minor commit msg + code improvements\n>> ---\n>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 45 ++++++++++++++++++++\n>> 1 file changed, 45 insertions(+)\n>>\n>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> index 8a7409fc..584f5230 100644\n>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> @@ -13,6 +13,7 @@\n>> #include \n>> \n>> #include \n>> +#include \n>> #include \n>> \n>> #include \n>> @@ -48,6 +49,7 @@ public:\n>> \n>> \tconst std::string &id() const { return id_; }\n>> \n>> +\tMutex openLock_;\n>> \tstd::unique_ptr video_;\n>> \tStream stream_;\n>> \tstd::map> formats_;\n>> @@ -93,6 +95,9 @@ private:\n>> \t\t\t const ControlValue &value);\n>> \tint processControls(UVCCameraData *data, Request *request);\n>> \n>> +\tbool acquireDevice(Camera *camera) override;\n>> +\tvoid releaseDevice(Camera *camera) override;\n>> +\n>> \tUVCCameraData *cameraData(Camera *camera)\n>> \t{\n>> \t\treturn static_cast(camera->_d());\n>> @@ -107,6 +112,7 @@ UVCCameraConfiguration::UVCCameraConfiguration(UVCCameraData *data)\n>> CameraConfiguration::Status UVCCameraConfiguration::validate()\n>> {\n>> \tStatus status = Valid;\n>> +\tbool opened = false;\n> \n> I would move this below.\n> \n>> \n>> \tif (config_.empty())\n>> \t\treturn Invalid;\n>> @@ -158,7 +164,23 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()\n>> \tformat.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);\n>> \tformat.size = cfg.size;\n>> \n>> +\t/*\n>> +\t * For power-consumption reasons video_ is closed when the camera\n>> +\t * is not acquired. Open it here if necessary.\n>> +\t */\n> \n> Here.\n> \n>> +\tMutexLocker locker(data_->openLock_);\n>> +\n>> +\tif (!data_->video_->isOpen()) {\n>> +\t\tint ret = data_->video_->open();\n>> +\t\tif (ret)\n>> +\t\t\treturn Invalid;\n>> +\n>> +\t\topened = true;\n>> +\t}\n>> +\n>> \tint ret = data_->video_->tryFormat(&format);\n>> +\tif (opened)\n>> +\t\tdata_->video_->close();\n>> \tif (ret)\n>> \t\treturn Invalid;\n> \n> The code below doesn't need to be protected by the lock.\n> \n> \tbool opened = false;\n> \tint ret;\n> \n> \tMutexLocker locker(data_->openLock_);\n> \n> \t{\n> \t\tif (!data_->video_->isOpen()) {\n> \t\t\tret = data_->video_->open();\n> \t\t\tif (ret)\n> \t\t\t\treturn Invalid;\n> \n> \t\t\topened = true;\n> \t\t}\n> \n> \t\tret = data_->video_->tryFormat(&format);\n> \t\tif (opened)\n> \t\t\tdata_->video_->close();\n> \t}\n\nI assume you meant to move the \"MutexLocker locker(data_->openLock_);\"\nto inside the { } scope here, so that its lifetime is limited to\nthat scope ?\n\n> \tif (ret)\n> \t\treturn Invalid;\n\nI would prefer to have this inside the { } scope since that is where\nit logically belongs and this check will be so fast that it does not\nmatter wrt holding the lock longer.\n\nWith those remarks I agree with your suggestion and I'll implement\nthis change for v3.\n\nRegards,\n\nHans\n\n\n\n\n> \n>> \n>> @@ -411,6 +433,23 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n>> \treturn true;\n>> }\n>> \n>> +bool PipelineHandlerUVC::acquireDevice(Camera *camera)\n>> +{\n>> +\tUVCCameraData *data = cameraData(camera);\n>> +\n>> +\tMutexLocker locker(data->openLock_);\n>> +\n>> +\treturn data->video_->open() == 0;\n>> +}\n>> +\n>> +void PipelineHandlerUVC::releaseDevice(Camera *camera)\n>> +{\n>> +\tUVCCameraData *data = cameraData(camera);\n>> +\n>> +\tMutexLocker locker(data->openLock_);\n>> +\tdata->video_->close();\n>> +}\n>> +\n>> int UVCCameraData::init(MediaDevice *media)\n>> {\n>> \tint ret;\n>> @@ -512,6 +551,12 @@ int UVCCameraData::init(MediaDevice *media)\n>> \n>> \tcontrolInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n>> \n>> +\t/*\n>> +\t * Close to allow camera to go into runtime-suspend, video_\n>> +\t * will be re-opened from acquireDevice() and validate().\n> \n> You can reflow this to 80 columns.\n> \n> With these small issues addressed,\n> \n> Reviewed-by: Laurent Pinchart \n> \n>> +\t */\n>> +\tvideo_->close();\n>> +\n>> \treturn 0;\n>> }\n>> \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 C19F1C323E\n\tfor ;\n\tFri, 30 Aug 2024 08:01:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 58C4263466;\n\tFri, 30 Aug 2024 10:01:09 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1B35A633CD\n\tfor ;\n\tFri, 30 Aug 2024 10:01:07 +0200 (CEST)","from mail-ej1-f69.google.com (mail-ej1-f69.google.com\n\t[209.85.218.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-161-nv5ttFWrNAOG4Bi4HqvcPw-1; Fri, 30 Aug 2024 04:01:04 -0400","by mail-ej1-f69.google.com with SMTP id\n\ta640c23a62f3a-a867b95db47so119273666b.1\n\tfor ;\n\tFri, 30 Aug 2024 01:01:04 -0700 (PDT)","from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec?\n\t(2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl.\n\t[2001:1c00:c32:7800:5bfa:a036:83f0:f9ec])\n\tby smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-a89891968f4sm182503766b.106.2024.08.30.01.01.01\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tFri, 30 Aug 2024 01:01:01 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"JjFrlINs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1725004865;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=qgsJoH9mSXOSy8pcMywdmndefXTPWE4uNuSvsxYtkxY=;\n\tb=JjFrlINs8m45y3vNmS77TckP9Sg3SLv63KwVOkxpdS1ZWdy1jlK6bD/CHrTY8tBbchX3E3\n\tbO8kdV3e3Ur/jV5FQt0I6bgUafTCu+eFTvwdBf5HiyG56rylzb7FioydsGThYv2lV85CFO\n\tsIW5UwcEr+KCIGZ9adeEamiJcqeNHCY=","X-MC-Unique":"nv5ttFWrNAOG4Bi4HqvcPw-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1725004863; x=1725609663;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=qgsJoH9mSXOSy8pcMywdmndefXTPWE4uNuSvsxYtkxY=;\n\tb=GR0MpFDuIJtSA3KfGvYaLUwmCoHHXAiOL3x66JvKA+BTtY0rwWhdg0cdFMAckt14R1\n\tbamn4NwTiPYuxXIZ6AV5ewz9LoiUY3x/hUUExinqMfj4pMCvkALPaLSw41BZRQKTxBsy\n\tHlicGUZa+/vcuelTw42m71AiT0q2G5etzqldi+MmQNPK8xe6tFd+PGDRzQhqy44g/HAx\n\tiA9ULxzX+8saVJtvJHESNGNUXvts6Zv84MzW8Pdy7aHwWRcL1lnp04ArRfUT4YUzIeDK\n\twL4Lo3Mbs0fFdadxPdrkSJpSFc2RhERMBXuZ0JFlQRZjJv+M+szC8ftSVvEIhaskz9PV\n\tVI3Q==","X-Gm-Message-State":"AOJu0YxSzJtynCwF5LMzh/wEZpEYQydoSe69JRASKHJrQtw3jVxjUD+k\n\tbbWFWWpwg7nOzDuU7+S3MQfmj6paA+3aelw8F3LdQBg/Osh0slFeMiwNZ18HK2GzPGilBxW0Mbd\n\tY4IHf6Vw4CU+P2Nk3Bct23Q3ofKGyDKbO33OCpeyRHsxblyNVAYXAkuZjV44OKadfKiFCoHxEUv\n\tArUJ8=","X-Received":["by 2002:a17:906:db02:b0:a86:842a:104a with SMTP id\n\ta640c23a62f3a-a897fad4d58mr432542266b.57.1725004862922; \n\tFri, 30 Aug 2024 01:01:02 -0700 (PDT)","by 2002:a17:906:db02:b0:a86:842a:104a with SMTP id\n\ta640c23a62f3a-a897fad4d58mr432539566b.57.1725004862334; \n\tFri, 30 Aug 2024 01:01:02 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFxjftprIpmhXcfSHCYYJhqw3fZymvyNpqRdSXZBmTYpdjtLsAWwZM+3qz4i7gHBakhDB0apw==","Message-ID":"<9b6b98fd-6a0a-47d6-b0ca-07e2d6fcefb4@redhat.com>","Date":"Fri, 30 Aug 2024 10:01:01 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 3/3] uvcvideo: Implement acquireDevice() +\n\treleaseDevice()","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tMaxime Ripard ","References":"<20240827164255.314432-1-hdegoede@redhat.com>\n\t<20240827164255.314432-4-hdegoede@redhat.com>\n\t<20240829215837.GI15799@pendragon.ideasonboard.com>","From":"Hans de Goede ","In-Reply-To":"<20240829215837.GI15799@pendragon.ideasonboard.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US, nl","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","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":30982,"web_url":"https://patchwork.libcamera.org/comment/30982/","msgid":"<20240830083220.GD25163@pendragon.ideasonboard.com>","date":"2024-08-30T08:32:20","subject":"Re: [PATCH v2 3/3] uvcvideo: Implement acquireDevice() +\n\treleaseDevice()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Aug 30, 2024 at 10:01:01AM +0200, Hans de Goede wrote:\n> Hi Laurent,\n> \n> On 8/29/24 11:58 PM, Laurent Pinchart wrote:\n> > Hi Hans,\n> > \n> > Thank you for the patch.\n> > \n> > On Tue, Aug 27, 2024 at 06:42:55PM +0200, Hans de Goede wrote:\n> >> The uvcvideo pipeline handler always keeps the uvcvideo /dev/video# node\n> >> for a pipeline open after enumerating the camera.\n> >>\n> >> This is a problem for uvcvideo, as keeping the /dev/video# node open stops\n> >> the underlying USB device and the USB bus controller from being able to\n> >> enter runtime-suspend causing significant unnecessary power-usage.\n> >>\n> >> Implement acquireDevice() + releaseDevice(), openening /dev/video# on\n> >> acquire and closing it on release to fix this.\n> >>\n> >> And make validate do a local video_->open() + close() around validate()\n> >> when not open yet. To keep validate() working on unacquired cameras.\n> > \n> > s/yet. To/yet, to/\n> > \n> >> Link: https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/2669\n> > \n> > I'd replace this with\n> > \n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=168\n> > \n> > which links to the above pipewire issue.\n> > \n> >> Signed-off-by: Hans de Goede \n> >> ---\n> >> Changes in v2:\n> >> - Minor commit msg + code improvements\n> >> ---\n> >> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 45 ++++++++++++++++++++\n> >> 1 file changed, 45 insertions(+)\n> >>\n> >> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> >> index 8a7409fc..584f5230 100644\n> >> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> >> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> >> @@ -13,6 +13,7 @@\n> >> #include \n> >> \n> >> #include \n> >> +#include \n> >> #include \n> >> \n> >> #include \n> >> @@ -48,6 +49,7 @@ public:\n> >> \n> >> \tconst std::string &id() const { return id_; }\n> >> \n> >> +\tMutex openLock_;\n> >> \tstd::unique_ptr video_;\n> >> \tStream stream_;\n> >> \tstd::map> formats_;\n> >> @@ -93,6 +95,9 @@ private:\n> >> \t\t\t const ControlValue &value);\n> >> \tint processControls(UVCCameraData *data, Request *request);\n> >> \n> >> +\tbool acquireDevice(Camera *camera) override;\n> >> +\tvoid releaseDevice(Camera *camera) override;\n> >> +\n> >> \tUVCCameraData *cameraData(Camera *camera)\n> >> \t{\n> >> \t\treturn static_cast(camera->_d());\n> >> @@ -107,6 +112,7 @@ UVCCameraConfiguration::UVCCameraConfiguration(UVCCameraData *data)\n> >> CameraConfiguration::Status UVCCameraConfiguration::validate()\n> >> {\n> >> \tStatus status = Valid;\n> >> +\tbool opened = false;\n> > \n> > I would move this below.\n> > \n> >> \n> >> \tif (config_.empty())\n> >> \t\treturn Invalid;\n> >> @@ -158,7 +164,23 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()\n> >> \tformat.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);\n> >> \tformat.size = cfg.size;\n> >> \n> >> +\t/*\n> >> +\t * For power-consumption reasons video_ is closed when the camera\n> >> +\t * is not acquired. Open it here if necessary.\n> >> +\t */\n> > \n> > Here.\n> > \n> >> +\tMutexLocker locker(data_->openLock_);\n> >> +\n> >> +\tif (!data_->video_->isOpen()) {\n> >> +\t\tint ret = data_->video_->open();\n> >> +\t\tif (ret)\n> >> +\t\t\treturn Invalid;\n> >> +\n> >> +\t\topened = true;\n> >> +\t}\n> >> +\n> >> \tint ret = data_->video_->tryFormat(&format);\n> >> +\tif (opened)\n> >> +\t\tdata_->video_->close();\n> >> \tif (ret)\n> >> \t\treturn Invalid;\n> > \n> > The code below doesn't need to be protected by the lock.\n> > \n> > \tbool opened = false;\n> > \tint ret;\n> > \n> > \tMutexLocker locker(data_->openLock_);\n> > \n> > \t{\n> > \t\tif (!data_->video_->isOpen()) {\n> > \t\t\tret = data_->video_->open();\n> > \t\t\tif (ret)\n> > \t\t\t\treturn Invalid;\n> > \n> > \t\t\topened = true;\n> > \t\t}\n> > \n> > \t\tret = data_->video_->tryFormat(&format);\n> > \t\tif (opened)\n> > \t\t\tdata_->video_->close();\n> > \t}\n> \n> I assume you meant to move the \"MutexLocker locker(data_->openLock_);\"\n> to inside the { } scope here, so that its lifetime is limited to\n> that scope ?\n\nYes that's what I meant sorry.\n\n> > \tif (ret)\n> > \t\treturn Invalid;\n> \n> I would prefer to have this inside the { } scope since that is where\n> it logically belongs and this check will be so fast that it does not\n> matter wrt holding the lock longer.\n\nFine with me.\n\n> With those remarks I agree with your suggestion and I'll implement\n> this change for v3.\n> \n> >> @@ -411,6 +433,23 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> >> \treturn true;\n> >> }\n> >> \n> >> +bool PipelineHandlerUVC::acquireDevice(Camera *camera)\n> >> +{\n> >> +\tUVCCameraData *data = cameraData(camera);\n> >> +\n> >> +\tMutexLocker locker(data->openLock_);\n> >> +\n> >> +\treturn data->video_->open() == 0;\n> >> +}\n> >> +\n> >> +void PipelineHandlerUVC::releaseDevice(Camera *camera)\n> >> +{\n> >> +\tUVCCameraData *data = cameraData(camera);\n> >> +\n> >> +\tMutexLocker locker(data->openLock_);\n> >> +\tdata->video_->close();\n> >> +}\n> >> +\n> >> int UVCCameraData::init(MediaDevice *media)\n> >> {\n> >> \tint ret;\n> >> @@ -512,6 +551,12 @@ int UVCCameraData::init(MediaDevice *media)\n> >> \n> >> \tcontrolInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n> >> \n> >> +\t/*\n> >> +\t * Close to allow camera to go into runtime-suspend, video_\n> >> +\t * will be re-opened from acquireDevice() and validate().\n> > \n> > You can reflow this to 80 columns.\n> > \n> > With these small issues addressed,\n> > \n> > Reviewed-by: Laurent Pinchart \n> > \n> >> +\t */\n> >> +\tvideo_->close();\n> >> +\n> >> \treturn 0;\n> >> }\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 429D1C324C\n\tfor ;\n\tFri, 30 Aug 2024 08:32:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A92F63466;\n\tFri, 30 Aug 2024 10:32:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DD74E633CD\n\tfor ;\n\tFri, 30 Aug 2024 10:32:50 +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 B3694227;\n\tFri, 30 Aug 2024 10:31:41 +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=\"Gh2ODNbJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1725006701;\n\tbh=jOd5fPnXz9bXwjt4k72AylRY0HT96ZeXs7oHlS/vmEA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Gh2ODNbJvCe6tTUqOjrOAxqvpyYtT2w8iydy1oi/162wUg8m1oaRjdEiYe6BtQo7F\n\tKLLZFNvSPrHBhMOoP0eDy3Azcp25Dzbntgo4hs+lun0iV6qrSlR7RJFQ+7g0BBFNTZ\n\tzakcQvaRIwyb12q2Wp+4bDqKkqeCOuw52uRLKoZg=","Date":"Fri, 30 Aug 2024 11:32:20 +0300","From":"Laurent Pinchart ","To":"Hans de Goede ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tMaxime Ripard ","Subject":"Re: [PATCH v2 3/3] uvcvideo: Implement acquireDevice() +\n\treleaseDevice()","Message-ID":"<20240830083220.GD25163@pendragon.ideasonboard.com>","References":"<20240827164255.314432-1-hdegoede@redhat.com>\n\t<20240827164255.314432-4-hdegoede@redhat.com>\n\t<20240829215837.GI15799@pendragon.ideasonboard.com>\n\t<9b6b98fd-6a0a-47d6-b0ca-07e2d6fcefb4@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<9b6b98fd-6a0a-47d6-b0ca-07e2d6fcefb4@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\" "}}]