[{"id":30895,"web_url":"https://patchwork.libcamera.org/comment/30895/","msgid":"<20240825010159.GD17238@pendragon.ideasonboard.com>","date":"2024-08-25T01:01:59","subject":"Re: [PATCH 5/5] 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 20, 2024 at 09:50:16PM +0200, Hans de Goede wrote:\n> ATM the uvcvideo pipeline handler always keeps the uvcvideo /dev/video#\n\ns/ATM/At the moment/\n\n(or just drop it)\n\n> node 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\n> validate when not open yet. To keep validate() working on unacquired\n> cameras.\n\nWon't you hit the same issue that 4/5 is meant to fix, with the\nnotifiers being created from the wrong thread ?\n\n> Signed-off-by: Hans de Goede \n> ---\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 46 ++++++++++++++++++++\n> 1 file changed, 46 insertions(+)\n> \n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 8a7409fc..d3eedfdc 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> \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> +\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> @@ -411,6 +433,24 @@ 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> +\tint ret = data->video_->open();\n> +\treturn ret == 0;\n\nYou can simplify this to\n\n\treturn data->video_->open() == 0;\n\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 +552,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> +\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 D4DB7BDE17\n\tfor ;\n\tSun, 25 Aug 2024 01:02:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DA9C6633D2;\n\tSun, 25 Aug 2024 03:02:04 +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 A7B91633CC\n\tfor ;\n\tSun, 25 Aug 2024 03:02:03 +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 42A8B22DA;\n\tSun, 25 Aug 2024 03:00:58 +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=\"cNlouEvd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724547658;\n\tbh=uBM67w3Dd5LC2viWVFlO68R4mYqsdYbGhJC8egSuvvA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cNlouEvdvUgQ/oRJSykKE3ahJTfnWY4PdUfMSO0HLrJ07rrlEC9yc9gq2d+rSss5d\n\tGwKjAl4gxCLLdxWIOZskrRdZtDTj+5WAd8lJSoyn0nDiFbzRJs6FG2PGAlJ7EJH0Xb\n\tntw+oXwf6Mf7MoWFi02sCqj4NPlzDtUQkBD7Yl10=","Date":"Sun, 25 Aug 2024 04:01:59 +0300","From":"Laurent Pinchart ","To":"Hans de Goede ","Cc":"libcamera-devel@lists.libcamera.org, Maxime Ripard , \n\tMilan Zamazal ","Subject":"Re: [PATCH 5/5] uvcvideo: Implement acquireDevice() +\n\treleaseDevice()","Message-ID":"<20240825010159.GD17238@pendragon.ideasonboard.com>","References":"<20240820195016.16028-1-hdegoede@redhat.com>\n\t<20240820195016.16028-6-hdegoede@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240820195016.16028-6-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":30903,"web_url":"https://patchwork.libcamera.org/comment/30903/","msgid":"<981494b5-5b85-478d-9240-a43fc07937b9@redhat.com>","date":"2024-08-26T08:17:17","subject":"Re: [PATCH 5/5] 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/25/24 3:01 AM, Laurent Pinchart wrote:\n> Hi Hans,\n> \n> Thank you for the patch.\n> \n> On Tue, Aug 20, 2024 at 09:50:16PM +0200, Hans de Goede wrote:\n>> ATM the uvcvideo pipeline handler always keeps the uvcvideo /dev/video#\n> \n> s/ATM/At the moment/\n> \n> (or just drop it)\n> \n>> node 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\n>> validate when not open yet. To keep validate() working on unacquired\n>> cameras.\n> \n> Won't you hit the same issue that 4/5 is meant to fix, with the\n> notifiers being created from the wrong thread ?\n\nYes, but the notifiers are also destroyed again before any other code actually\ntries to use use them.\n\nEither the camera is already acquired and no open/close of video_ is done,\nor it is not acquired and both open + close of video_ are done without\nany other code-paths being able to use video_ in the mean time.\n\nThe openLock_ mutex protects against acquireDevice() / releaseDevice()\nracing with the validate() call and all other users of video_ require\nthe camera to be acquired first.\n\nNote no open-counting is done since there with the uvcvideo pipeline\nhandler there only ever is one camera.\n\nRegards,\n\nHans\n\n\n\n\n> \n>> Signed-off-by: Hans de Goede \n>> ---\n>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 46 ++++++++++++++++++++\n>> 1 file changed, 46 insertions(+)\n>>\n>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> index 8a7409fc..d3eedfdc 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>> \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>> +\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>> @@ -411,6 +433,24 @@ 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>> +\tint ret = data->video_->open();\n>> +\treturn ret == 0;\n> \n> You can simplify this to\n> \n> \treturn data->video_->open() == 0;\n> \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 +552,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>> +\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 4B5F7BDE17\n\tfor ;\n\tMon, 26 Aug 2024 08:17:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1F94A633D0;\n\tMon, 26 Aug 2024 10:17:26 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B716A61906\n\tfor ;\n\tMon, 26 Aug 2024 10:17:23 +0200 (CEST)","from mail-ed1-f71.google.com (mail-ed1-f71.google.com\n\t[209.85.208.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-198-D4nwl68CODm9TldBRSWF6Q-1; Mon, 26 Aug 2024 04:17:21 -0400","by mail-ed1-f71.google.com with SMTP id\n\t4fb4d7f45d1cf-5bed8949b39so3216992a12.3\n\tfor ;\n\tMon, 26 Aug 2024 01:17:20 -0700 (PDT)","from [10.40.98.157] ([78.108.130.194])\n\tby smtp.gmail.com with ESMTPSA id\n\t4fb4d7f45d1cf-5c04a3ed39fsm5318454a12.57.2024.08.26.01.17.18\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tMon, 26 Aug 2024 01:17:18 -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=\"UI5uATmM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1724660242;\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=DpHegDQRftLDc1RyxLgvdmRpixbOLytH3emD13vgMnI=;\n\tb=UI5uATmM+keHi2LfiHUYHG/dbaqp088beE7iGX2OAVHXpcI+zqm88w4/dZTMrFjr0NgCfA\n\tJI6X5MnukSWEoAcE9vsj5I66+AZdVnM/z9zUHBYu4IRwOgJxdMNgEduhJxKg+aoMEjD+GK\n\tIeIOwC1Z6JQpkYWTnpaVyo2Zp6YpJPA=","X-MC-Unique":"D4nwl68CODm9TldBRSWF6Q-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1724660240; x=1725265040;\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=DpHegDQRftLDc1RyxLgvdmRpixbOLytH3emD13vgMnI=;\n\tb=QzPMJKkKtdx5QuRVlObScqSBv2v3rbH30BSc8fkDjxqOi/E2vu/xJFogm2pasdLJL6\n\tL2hywvCdel+X310ZsB33Gk0Pr3+uBZKkwQ0DX64aclXUPT9mCM8woxt6pGDYCdfjNsMd\n\tAX0OwYQjWZOerjrPB1LMjTCUMo3EbsXizpm2GsfCJTzSPHMHo7yQihBZW4LuWm8fNFVI\n\tUTskIrtt3NVaMwiqBqpV1xBk/fA9u417ZKBu6/B1xM9joNBzFNvqYt83PAtb9rMIGqza\n\txt/bPtOE7C30MaVd3wKyw/mVdHkRE2kQ5LF2j+4IgfbjFaAm0rid5i9ehcVFEfjsl9YA\n\tdtig==","X-Gm-Message-State":"AOJu0YwtvAF7LdLp9RNZIgKmOR3KhvUqDN+GPKCQBquKkGh5alyoELn4\n\tKDS43HO11cLc7N4DKel+XdCmYTk7ftYaZPNtnhJHkGWZ7W2TjEiOS7JISUqCjyZLPsBlhCu9fmc\n\tGVhjk4PuHPW2ns13sW76tRDds6KBCSfJU+1qdNW7lzETZP0urebZWpZOmBuSPaF7+6Gz7vrc=","X-Received":["by 2002:a05:6402:40c5:b0:57c:9d54:67db with SMTP id\n\t4fb4d7f45d1cf-5c08916a094mr7031460a12.9.1724660239741; \n\tMon, 26 Aug 2024 01:17:19 -0700 (PDT)","by 2002:a05:6402:40c5:b0:57c:9d54:67db with SMTP id\n\t4fb4d7f45d1cf-5c08916a094mr7031439a12.9.1724660239179; \n\tMon, 26 Aug 2024 01:17:19 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHrtFbGcCSPcogk0ABsDPhrGBo3Hwxviqa5DE7UgzHBYIN9eVbG/yV7ZsjfViPux1xu1O1OAw==","Message-ID":"<981494b5-5b85-478d-9240-a43fc07937b9@redhat.com>","Date":"Mon, 26 Aug 2024 10:17:17 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 5/5] uvcvideo: Implement acquireDevice() +\n\treleaseDevice()","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Maxime Ripard , \n\tMilan Zamazal ","References":"<20240820195016.16028-1-hdegoede@redhat.com>\n\t<20240820195016.16028-6-hdegoede@redhat.com>\n\t<20240825010159.GD17238@pendragon.ideasonboard.com>","From":"Hans de Goede ","In-Reply-To":"<20240825010159.GD17238@pendragon.ideasonboard.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US","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":30962,"web_url":"https://patchwork.libcamera.org/comment/30962/","msgid":"<20240829210343.GE15799@pendragon.ideasonboard.com>","date":"2024-08-29T21:03:43","subject":"Re: [PATCH 5/5] 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\nOn Mon, Aug 26, 2024 at 10:17:17AM +0200, Hans de Goede wrote:\n> On 8/25/24 3:01 AM, Laurent Pinchart wrote:\n> > On Tue, Aug 20, 2024 at 09:50:16PM +0200, Hans de Goede wrote:\n> >> ATM the uvcvideo pipeline handler always keeps the uvcvideo /dev/video#\n> > \n> > s/ATM/At the moment/\n> > \n> > (or just drop it)\n> > \n> >> node 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\n> >> validate when not open yet. To keep validate() working on unacquired\n> >> cameras.\n> > \n> > Won't you hit the same issue that 4/5 is meant to fix, with the\n> > notifiers being created from the wrong thread ?\n> \n> Yes, but the notifiers are also destroyed again before any other code actually\n> tries to use use them.\n\nTrue, it shouldn't cause any problem in practice, but it makes me feel a\nbit uncomfortable still.\n\n> Either the camera is already acquired and no open/close of video_ is done,\n> or it is not acquired and both open + close of video_ are done without\n> any other code-paths being able to use video_ in the mean time.\n> \n> The openLock_ mutex protects against acquireDevice() / releaseDevice()\n> racing with the validate() call and all other users of video_ require\n> the camera to be acquired first.\n> \n> Note no open-counting is done since there with the uvcvideo pipeline\n> handler there only ever is one camera.\n\nI can live with this for the uvcvideo pipeline handler for the time\nbeing. Let's make sure it doesn't spread before the API improves. To be\nhonest, I'm a bit concerned than shifting the yak shaving to the second\nuser on the grounds that we need more than a single use case to do\nthings right will be met with unhappiness on the grounds that the first\nuser got away with it so it's not fair. I suppose doing things the right\nbut hard way will always be met with this kind of problems :-)\n\n> >> Signed-off-by: Hans de Goede \n> >> ---\n> >> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 46 ++++++++++++++++++++\n> >> 1 file changed, 46 insertions(+)\n> >>\n> >> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> >> index 8a7409fc..d3eedfdc 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> >> \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> >> +\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> >> @@ -411,6 +433,24 @@ 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> >> +\tint ret = data->video_->open();\n> >> +\treturn ret == 0;\n> > \n> > You can simplify this to\n> > \n> > \treturn data->video_->open() == 0;\n> > \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 +552,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> >> +\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 57B55C324C\n\tfor ;\n\tThu, 29 Aug 2024 21:04:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 42B3663461;\n\tThu, 29 Aug 2024 23:04:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 977A363456\n\tfor ;\n\tThu, 29 Aug 2024 23:04:15 +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 AB841226;\n\tThu, 29 Aug 2024 23:03:06 +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=\"pdIYYf34\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724965386;\n\tbh=ebuZV0jOv/qX7pR5g14nVx2+rhp3AuwSe3oOwX2zGGs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pdIYYf341TNJ7kpAPeIUiKoZzpRHOvBwbiAOGrqXJJl0jpwbPSVhrsCiaJyLtx4Sg\n\t/Pe8p5AtuTAtCWg7t1wS9SnxgyZhZmyMtSNxkdqq77XRLi9odFhlcWfohR2cN94zGH\n\tUev+KIF9BUOJ1h63tSs2AMLOVTzk04pQe9c/EFAs=","Date":"Fri, 30 Aug 2024 00:03:43 +0300","From":"Laurent Pinchart ","To":"Hans de Goede ","Cc":"libcamera-devel@lists.libcamera.org, Maxime Ripard , \n\tMilan Zamazal ","Subject":"Re: [PATCH 5/5] uvcvideo: Implement acquireDevice() +\n\treleaseDevice()","Message-ID":"<20240829210343.GE15799@pendragon.ideasonboard.com>","References":"<20240820195016.16028-1-hdegoede@redhat.com>\n\t<20240820195016.16028-6-hdegoede@redhat.com>\n\t<20240825010159.GD17238@pendragon.ideasonboard.com>\n\t<981494b5-5b85-478d-9240-a43fc07937b9@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<981494b5-5b85-478d-9240-a43fc07937b9@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\" "}}]