[{"id":25770,"web_url":"https://patchwork.libcamera.org/comment/25770/","msgid":"<166817567254.50677.12585736023294951553@Monstersaurus>","date":"2022-11-11T14:07:52","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: Add a\n\tPipelineHandler::releaseDevice method","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi David,\n\nQuoting David Plowman via libcamera-devel (2022-11-11 13:30:24)\n> This notifies pipeline handlers when a camera is released, in case\n> they want to free any resources or memory buffers.\n> \n\nIndeed, that does sound like something they might need!\n\n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/internal/pipeline_handler.h | 4 +++-\n> src/libcamera/camera.cpp | 2 +-\n> src/libcamera/pipeline_handler.cpp | 16 +++++++++++++++-\n> 3 files changed, 19 insertions(+), 3 deletions(-)\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 96aab9d6..ec4f662d 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -46,7 +46,7 @@ public:\n> const DeviceMatch &dm);\n> \n> bool acquire();\n> - void release();\n> + void release(Camera *camera);\n> \n> virtual std::unique_ptr generateConfiguration(Camera *camera,\n> const StreamRoles &roles) = 0;\n> @@ -74,6 +74,8 @@ protected:\n> virtual int queueRequestDevice(Camera *camera, Request *request) = 0;\n> virtual void stopDevice(Camera *camera) = 0;\n> \n> + virtual void releaseDevice(Camera *camera);\n> +\n> CameraManager *manager_;\n> \n> private:\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index c4f65c1a..2d947a44 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -870,7 +870,7 @@ int Camera::release()\n> return ret == -EACCES ? -EBUSY : ret;\n> \n> if (d->isAcquired())\n> - d->pipe_->release();\n> + d->pipe_->release(this);\n> \n> d->setState(Private::CameraAvailable);\n> \n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 825aff5a..cfade490 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -183,6 +183,7 @@ bool PipelineHandler::acquire()\n> \n> /**\n> * \\brief Release exclusive access to the pipeline handler\n> + * \\param[in] camera The camera for which to release data\n> *\n> * This function releases access to the pipeline handler previously acquired by\n> * a call to acquire(). Every release() call shall match a previous successful\n> @@ -196,7 +197,7 @@ bool PipelineHandler::acquire()\n> *\n> * \\sa acquire()\n> */\n> -void PipelineHandler::release()\n> +void PipelineHandler::release(Camera *camera)\n> {\n> MutexLocker locker(lock_);\n> \n> @@ -204,9 +205,22 @@ void PipelineHandler::release()\n> \n> unlockMediaDevices();\n> \n> + releaseDevice(camera);\n> +\n> --useCount_;\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\nI don't know if it helps/is accurate or correct so only as a mere\npossible suggestion to clarify:\n\n'as freeing memory allocated during acquire()' ?\n\nBut equally - fine without.\n\n> + */\n> +void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)\n> +{\n> +}\n> +\n\nThis is certainly fine, and looks like it works correctly. I suspect it\ncould also have been the documentation added here, with just an\nempty implementation in the header at something like:\n\n\tvirtual void releaseDevice(Camera *camera) {};\n\nBut I think having the base/empty implementation here - does make it\nclearer that the documentation is associated with it directly, and it\nonly costs 3 easy lines so:\n\nReviewed-by: Kieran Bingham \n\n> void PipelineHandler::unlockMediaDevices()\n> {\n> for (std::shared_ptr &media : mediaDevices_)\n> -- \n> 2.30.2\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 7DA96BE08B\n\tfor ;\n\tFri, 11 Nov 2022 14:07:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E84B063083;\n\tFri, 11 Nov 2022 15:07:55 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E2CFC61F3F\n\tfor ;\n\tFri, 11 Nov 2022 15:07:54 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 691AE32A;\n\tFri, 11 Nov 2022 15:07:54 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1668175675;\n\tbh=LvKgCcWV+Ojtu/5Bv3EcU/f9uOeq+IUlbbEwpEKSNY8=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=NLXtfZu+66MdiJO/C1M5JAK+sKMox1Ux/nlDUxQ/FpoeFiwOqQfKotWNiy6LCYL+t\n\tX4p0wqtxhnqkRwzSMr8T95GSn8/dveli9h/VWrA7h76n2XRnoZaveK3FowdszOe4SW\n\tWPHtTAC8AfxAo6zYkDpfp5zDGWVH37ON37rSWXYncwj9QZ7Ie35Eel9OcrIvOJLGdR\n\tOTBlA3XwbhhE0v0E0sY4kThv7VIIeIQcUOvbu+YMqXFx+ZkNzLTTUz/b/pALO5uF7J\n\t3kWnoqYJfxS4/+vh7eMGg0Nk3SCytIEZCi79eHLeg+PdnDXzb8Kjpp2iN/3tvcCnDb\n\tc/vl2CIKpe9+g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1668175674;\n\tbh=LvKgCcWV+Ojtu/5Bv3EcU/f9uOeq+IUlbbEwpEKSNY8=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=Rc/S2lj7LWF2SSWKQ0dHjQczakTmKymicoekkg0hnkgLybNbh+O9uJj/jjIDpQ/0G\n\tSw34xq3StoLXPIcJqB3IB3KtBTxESPB3Y/5U+y3YaCtO/NyBH7fl0LY0J+UZfymJq9\n\tKY7nmwPczfoGwIuMIBQBe+TNGS+n8X9De7/RedBw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Rc/S2lj7\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20221111133025.3102-2-david.plowman@raspberrypi.com>","References":"<20221111133025.3102-1-david.plowman@raspberrypi.com>\n\t<20221111133025.3102-2-david.plowman@raspberrypi.com>","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 11 Nov 2022 14:07:52 +0000","Message-ID":"<166817567254.50677.12585736023294951553@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: Add a\n\tPipelineHandler::releaseDevice method","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25773,"web_url":"https://patchwork.libcamera.org/comment/25773/","msgid":"","date":"2022-11-11T14:20:54","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: Add a\n\tPipelineHandler::releaseDevice method","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nThank you for fixing this.\n\nOn Fri, 11 Nov 2022 at 13:30, David Plowman via libcamera-devel <\nlibcamera-devel@lists.libcamera.org> wrote:\n\n> This notifies pipeline handlers when a camera is released, in case\n> they want to free any resources or memory buffers.\n>\n> Signed-off-by: David Plowman \n>\n\nReviewed-by: Naushir Patuck \n\n\n> ---\n> include/libcamera/internal/pipeline_handler.h | 4 +++-\n> src/libcamera/camera.cpp | 2 +-\n> src/libcamera/pipeline_handler.cpp | 16 +++++++++++++++-\n> 3 files changed, 19 insertions(+), 3 deletions(-)\n>\n> diff --git a/include/libcamera/internal/pipeline_handler.h\n> b/include/libcamera/internal/pipeline_handler.h\n> index 96aab9d6..ec4f662d 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -46,7 +46,7 @@ public:\n> const DeviceMatch &dm);\n>\n> bool acquire();\n> - void release();\n> + void release(Camera *camera);\n>\n> virtual std::unique_ptr\n> generateConfiguration(Camera *camera,\n> const StreamRoles &roles) = 0;\n> @@ -74,6 +74,8 @@ protected:\n> virtual int queueRequestDevice(Camera *camera, Request *request) =\n> 0;\n> virtual void stopDevice(Camera *camera) = 0;\n>\n> + virtual void releaseDevice(Camera *camera);\n> +\n> CameraManager *manager_;\n>\n> private:\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index c4f65c1a..2d947a44 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -870,7 +870,7 @@ int Camera::release()\n> return ret == -EACCES ? -EBUSY : ret;\n>\n> if (d->isAcquired())\n> - d->pipe_->release();\n> + d->pipe_->release(this);\n>\n> d->setState(Private::CameraAvailable);\n>\n> diff --git a/src/libcamera/pipeline_handler.cpp\n> b/src/libcamera/pipeline_handler.cpp\n> index 825aff5a..cfade490 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -183,6 +183,7 @@ bool PipelineHandler::acquire()\n>\n> /**\n> * \\brief Release exclusive access to the pipeline handler\n> + * \\param[in] camera The camera for which to release data\n> *\n> * This function releases access to the pipeline handler previously\n> acquired by\n> * a call to acquire(). Every release() call shall match a previous\n> successful\n> @@ -196,7 +197,7 @@ bool PipelineHandler::acquire()\n> *\n> * \\sa acquire()\n> */\n> -void PipelineHandler::release()\n> +void PipelineHandler::release(Camera *camera)\n> {\n> MutexLocker locker(lock_);\n>\n> @@ -204,9 +205,22 @@ void PipelineHandler::release()\n>\n> unlockMediaDevices();\n>\n> + releaseDevice(camera);\n> +\n> --useCount_;\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> +void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)\n> +{\n> +}\n> +\n> void PipelineHandler::unlockMediaDevices()\n> {\n> for (std::shared_ptr &media : mediaDevices_)\n> --\n> 2.30.2\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 A24D8BE08B\n\tfor ;\n\tFri, 11 Nov 2022 14:21:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 68B016307B;\n\tFri, 11 Nov 2022 15:21:12 +0100 (CET)","from mail-qv1-xf31.google.com (mail-qv1-xf31.google.com\n\t[IPv6:2607:f8b0:4864:20::f31])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A2CF361F3F\n\tfor ;\n\tFri, 11 Nov 2022 15:21:10 +0100 (CET)","by mail-qv1-xf31.google.com with SMTP id x15so3469903qvp.1\n\tfor ;\n\tFri, 11 Nov 2022 06:21:10 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1668176472;\n\tbh=tcVRJ8FBOpcsgy/PisyQ7Wh9FRxD/hY6QklJV1vCMOc=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=3eh9h0e6lqJ3GsrO3ZT4tfOg0LIWrNcM9a3xLHEre9HOC15RF6DVAypFsbH8nWf7e\n\t00MRGdvkxxzToThO8HMTzYc9s+XhdiqUdVdQF1tAEeFOqujujkCA5Hm8eVsCD0KY9F\n\tpT0C1sN/nx2Hm4XxvBKrmCKUOz72qcs0QaCqmptbZuKgKfFiUMJHgMTq82feDcZAoQ\n\tnRXbX8+F+aYj1dsCcxIEyF1jLssQ58w9LgjNN1fRK0eQv3k5gAQJLmIgFp4hpiXmZI\n\t2J47DMC4CNnBHBS5rQjsfjiOWV748wUWbWrKOkDQDr0v0CdRBZeFqbVNERn5MNSGaY\n\tQ8Bdh2Mvg45MQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\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=DOJ1xDDX0Q6hbN9FHb+/LsHnO7IkqHm4P42WTZVGYQw=;\n\tb=mFdwVSqKAo9sLqwe+tTIvgJ532nUQ9oJxKVMdg/BSzm6dxIJma6OZmOtfJG+UG4ppt\n\thNVIAQ1RozfHvxOYmn40qdhY9vme+iTwayauroV/RPR7UOpXXPXYTq/nn6uVr4s9XaU7\n\tjh8uyWAP2acRUdg9n1fV5kY0QtoMQLGOaQNnoX2H8Q31vQzp3r62E6c9vp+d8SRqqz+e\n\tVCThIr2DOixU76zVhuJVER/7Ggt3R7QK1aQO5iR9kv0nbirCstPtrqEMSuqWGyt1MhGs\n\t8BLT3wmbAfjMrHq7b29lyrLy4IhBlmEtrt4AU5aTHUnP5btXNQANBjOS7U8fZjRLe9fw\n\tTZPA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"mFdwVSqK\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\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=DOJ1xDDX0Q6hbN9FHb+/LsHnO7IkqHm4P42WTZVGYQw=;\n\tb=LZdgHzvmbW5PcHj4xOKaPngM7WaGLfo/+LfmiRZ+DJPmib4y6djDW/hLGCEkY4uXAl\n\t8B0XG0HBxsOcro+sXNTz08gOhs4QYqUe58GQb1tTnMdqHx21LUd6wn8MBAFxOHUDRnQR\n\tvlashpBFeViY28vcgUAj0gnqt+sAHkJfFV7cNAoHLlYSqI/9hvckg9SAQWArLkDgTvMi\n\tyyhbuWpUEDjYjHd+6b5OKNblULBCa8mpd6i/EN1IWERuxn6xukLBR9wIC8efy50cSnro\n\t8kVLsfoP9DP8xx/B3/Cs9519KHgXDamzmtYkfxzSJbfRyzhyPk/NuhqcfXZBgjGcm1G7\n\teFIQ==","X-Gm-Message-State":"ANoB5pnu8DyuO/wedhrFpYOTtqHr5NprQkg7lSpvyRQvi6nDqRCVs1tb\n\tDq8lNF6QgyozdJfgWxzNhYZQVyZmN33xQdL9JjW33PNa1e4=","X-Google-Smtp-Source":"AA0mqf57vQ1hBbmUZM5zi2YWfIdI/BJsd4Jcm92q0wz5ttOOXebejaMTUbrODQPIs057dC04P7xuhq0dGouRM5J6uTY=","X-Received":"by 2002:a05:6214:4243:b0:49c:699b:d5cf with SMTP id\n\tne3-20020a056214424300b0049c699bd5cfmr1922086qvb.126.1668176469702;\n\tFri, 11 Nov 2022 06:21:09 -0800 (PST)","MIME-Version":"1.0","References":"<20221111133025.3102-1-david.plowman@raspberrypi.com>\n\t<20221111133025.3102-2-david.plowman@raspberrypi.com>","In-Reply-To":"<20221111133025.3102-2-david.plowman@raspberrypi.com>","Date":"Fri, 11 Nov 2022 14:20:54 +0000","Message-ID":"","To":"David Plowman ","Content-Type":"multipart/alternative; boundary=\"000000000000e6512e05ed32995d\"","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: Add a\n\tPipelineHandler::releaseDevice method","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25791,"web_url":"https://patchwork.libcamera.org/comment/25791/","msgid":"","date":"2022-11-14T16:56:21","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: Add a\n\tPipelineHandler::releaseDevice method","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Fri, Nov 11, 2022 at 01:30:24PM +0000, David Plowman via libcamera-devel wrote:\n> This notifies pipeline handlers when a camera is released, in case\n> they want to free any resources or memory buffers.\n> \n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/internal/pipeline_handler.h | 4 +++-\n> src/libcamera/camera.cpp | 2 +-\n> src/libcamera/pipeline_handler.cpp | 16 +++++++++++++++-\n\nThe pipeline handler writer's guide\n(Documentation/guides/pipeline-handler.rst) needs to be udpated. The\nvivid pipeline handler may also need some attention, but we can handle\nthat.\n\n> 3 files changed, 19 insertions(+), 3 deletions(-)\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 96aab9d6..ec4f662d 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -46,7 +46,7 @@ public:\n> \t\t\t\t\tconst DeviceMatch &dm);\n> \n> \tbool acquire();\n> -\tvoid release();\n> +\tvoid release(Camera *camera);\n> \n> \tvirtual std::unique_ptr generateConfiguration(Camera *camera,\n> \t\tconst StreamRoles &roles) = 0;\n> @@ -74,6 +74,8 @@ protected:\n> \tvirtual int queueRequestDevice(Camera *camera, Request *request) = 0;\n> \tvirtual void stopDevice(Camera *camera) = 0;\n> \n> +\tvirtual void releaseDevice(Camera *camera);\n> +\n> \tCameraManager *manager_;\n> \n> private:\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index c4f65c1a..2d947a44 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -870,7 +870,7 @@ int Camera::release()\n> \t\treturn ret == -EACCES ? -EBUSY : ret;\n> \n> \tif (d->isAcquired())\n> -\t\td->pipe_->release();\n> +\t\td->pipe_->release(this);\n> \n> \td->setState(Private::CameraAvailable);\n> \n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 825aff5a..cfade490 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -183,6 +183,7 @@ bool PipelineHandler::acquire()\n> \n> /**\n> * \\brief Release exclusive access to the pipeline handler\n> + * \\param[in] camera The camera for which to release data\n\nThis function doesn't release \"data\".\n\n> *\n> * This function releases access to the pipeline handler previously acquired by\n> * a call to acquire(). Every release() call shall match a previous successful\n> @@ -196,7 +197,7 @@ bool PipelineHandler::acquire()\n> *\n> * \\sa acquire()\n> */\n> -void PipelineHandler::release()\n> +void PipelineHandler::release(Camera *camera)\n> {\n> \tMutexLocker locker(lock_);\n> \n> @@ -204,9 +205,22 @@ void PipelineHandler::release()\n> \n> \tunlockMediaDevices();\n> \n> +\treleaseDevice(camera);\n> +\n> \t--useCount_;\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\nThis needs further documentation, to clearly explain what can or can't\nbe done here. Examples could help.\n\nThis being said, I'm not sure hooking up this feature in the\nCamera::release() function is the right option. In addition to the lack\nof balance between acquire() and release() (in this series the release()\nfunction does more than counterbalance acquire()), the usage pattern\nthat was envisioned is that application can hold onto cameras for a long\ntime. Having to call release() to free memory means that another\napplication could then acquire the camera.\n\n> + */\n> +void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)\n> +{\n> +}\n> +\n> void PipelineHandler::unlockMediaDevices()\n> {\n> \tfor (std::shared_ptr &media : mediaDevices_)","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 C1E98BE08B\n\tfor ;\n\tMon, 14 Nov 2022 16:56:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2CA376307F;\n\tMon, 14 Nov 2022 17:56:45 +0100 (CET)","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 D2ACC61F3B\n\tfor ;\n\tMon, 14 Nov 2022 17:56:42 +0100 (CET)","from pendragon.ideasonboard.com (unknown [46.183.103.8])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ED8DC891;\n\tMon, 14 Nov 2022 17:56:40 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1668445005;\n\tbh=zboAoBzBuq1XdtyJyerY1GkKkCzLPakGebUv71j2oJk=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=lGMAwgPUQmOQHgtUEHY+iCPmeiJB7+Ao9C3dq2XIz7c+4iDZLk9OSOO+6pP47vQFz\n\tTiUMXeVTrN8Jpx+76rTtqZLr30DOa3RvwCHTb1AtIsHhQ4W3Fukw8HtHoNtswrylFr\n\taUYtB3eq9ddS204l+PiRB3fUlzmVUr+T5xusUb1aFtfVnEVLE6YVp/dSj5blh3Enar\n\trXmoADxshP5y9tEtpcum41ucn4UKo0jqi5Zv1ztt4HpDNimLoCAnE1EF+uDvmcNC5L\n\ts+MSDyl3KobYR1CNnOYJKeXBNv/TlKbWENdtozC6GWiizy3nbw36WuQ08j7ll+G8RI\n\tqWrmxhc0CPdzA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1668445002;\n\tbh=zboAoBzBuq1XdtyJyerY1GkKkCzLPakGebUv71j2oJk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Sp10oS13ar81EFHntLhiwDVpRpgIAc5BmvBvMSsdna1XMGeJXpEwQTErPfuJniOKV\n\tFBJ4HFyN9x9URfgMtWEmpqE966zuBEjomAqMNGi8pyBMxJQtzBuWXLpsLaA7CiuEtb\n\tELr1bCV6vz+CIT9zlMfcwLjLFGjsghmB6qXUQ3L8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Sp10oS13\"; dkim-atps=neutral","Date":"Mon, 14 Nov 2022 18:56:21 +0200","To":"David Plowman ","Message-ID":"","References":"<20221111133025.3102-1-david.plowman@raspberrypi.com>\n\t<20221111133025.3102-2-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221111133025.3102-2-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: Add a\n\tPipelineHandler::releaseDevice method","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25795,"web_url":"https://patchwork.libcamera.org/comment/25795/","msgid":"<166846924642.2951678.6211101374126427327@Monstersaurus>","date":"2022-11-14T23:40:46","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: Add a\n\tPipelineHandler::releaseDevice method","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart via libcamera-devel (2022-11-14 16:56:21)\n> Hi David,\n> \n> Thank you for the patch.\n> \n> On Fri, Nov 11, 2022 at 01:30:24PM +0000, David Plowman via libcamera-devel wrote:\n> > This notifies pipeline handlers when a camera is released, in case\n> > they want to free any resources or memory buffers.\n> > \n> > Signed-off-by: David Plowman \n> > ---\n> > include/libcamera/internal/pipeline_handler.h | 4 +++-\n> > src/libcamera/camera.cpp | 2 +-\n> > src/libcamera/pipeline_handler.cpp | 16 +++++++++++++++-\n> \n> The pipeline handler writer's guide\n> (Documentation/guides/pipeline-handler.rst) needs to be udpated. The\n> vivid pipeline handler may also need some attention, but we can handle\n> that.\n> \n> > 3 files changed, 19 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > index 96aab9d6..ec4f662d 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -46,7 +46,7 @@ public:\n> > const DeviceMatch &dm);\n> > \n> > bool acquire();\n> > - void release();\n> > + void release(Camera *camera);\n> > \n> > virtual std::unique_ptr generateConfiguration(Camera *camera,\n> > const StreamRoles &roles) = 0;\n> > @@ -74,6 +74,8 @@ protected:\n> > virtual int queueRequestDevice(Camera *camera, Request *request) = 0;\n> > virtual void stopDevice(Camera *camera) = 0;\n> > \n> > + virtual void releaseDevice(Camera *camera);\n> > +\n> > CameraManager *manager_;\n> > \n> > private:\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index c4f65c1a..2d947a44 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -870,7 +870,7 @@ int Camera::release()\n> > return ret == -EACCES ? -EBUSY : ret;\n> > \n> > if (d->isAcquired())\n> > - d->pipe_->release();\n> > + d->pipe_->release(this);\n> > \n> > d->setState(Private::CameraAvailable);\n> > \n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index 825aff5a..cfade490 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -183,6 +183,7 @@ bool PipelineHandler::acquire()\n> > \n> > /**\n> > * \\brief Release exclusive access to the pipeline handler\n> > + * \\param[in] camera The camera for which to release data\n> \n> This function doesn't release \"data\".\n> \n> > *\n> > * This function releases access to the pipeline handler previously acquired by\n> > * a call to acquire(). Every release() call shall match a previous successful\n> > @@ -196,7 +197,7 @@ bool PipelineHandler::acquire()\n> > *\n> > * \\sa acquire()\n> > */\n> > -void PipelineHandler::release()\n> > +void PipelineHandler::release(Camera *camera)\n> > {\n> > MutexLocker locker(lock_);\n> > \n> > @@ -204,9 +205,22 @@ void PipelineHandler::release()\n> > \n> > unlockMediaDevices();\n> > \n> > + releaseDevice(camera);\n> > +\n> > --useCount_;\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 needs further documentation, to clearly explain what can or can't\n> be done here. Examples could help.\n> \n> This being said, I'm not sure hooking up this feature in the\n> Camera::release() function is the right option. In addition to the lack\n> of balance between acquire() and release() (in this series the release()\n> function does more than counterbalance acquire()), the usage pattern\n> that was envisioned is that application can hold onto cameras for a long\n> time. Having to call release() to free memory means that another\n> application could then acquire the camera.\n\nI was actually trying to balance this up today, and add the\ncorresponding acquires so that pipeline handlers can ensure they only\ntake resources when they are acquired. For exactly the reason that\nPipewire already obtains and holds on to 'Camera' devices, and I\nexpect it shouldn't keep device nodes open when not in use.\n\nI expected this would allow us to do things like 'only open UVC devices\nwhen the camera is actually acquired' for instance, but it seems there\nis something complex going on, meaning if I close the device after\nPipelineHandlerUVC::init() has completed, and reopen it at 'acquire()'\ntime .. I don't get any frame completion events, seemingly at the fd\nnotifier layer.\n\nPerhaps just a bug that needs clearing up...\n\n--\nKieran\n\n\n> \n> > + */\n> > +void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)\n> > +{\n> > +}\n> > +\n> > void PipelineHandler::unlockMediaDevices()\n> > {\n> > for (std::shared_ptr &media : mediaDevices_)\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 1F394BD16B\n\tfor ;\n\tMon, 14 Nov 2022 23:40:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5895C63088;\n\tTue, 15 Nov 2022 00:40:52 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 301016307E\n\tfor ;\n\tTue, 15 Nov 2022 00:40:50 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 95EE6CCE;\n\tTue, 15 Nov 2022 00:40:49 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1668469252;\n\tbh=2NOfw2JS+ct2YPfN33HkV7vp1+BhNex0o72AKD7nRes=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Ax3Kl3P5vPd8BYYwLYqj5ZRfR2swkFPE+H94NMxDnBr/KNuqUyMzWuEa3crUMncFg\n\t1u8KsPxTrAgsnJGFNs+PEr4DtFI7kZFzmsulsImhHiXf9+34yyqnUqQyTQteV+y/p3\n\tagX0fuRUy2ekKt9yL9FhrlViXLPicRKDSrfU7cyZWKRIPXW6ZAEQzzQICCkg2vmxRi\n\tplyHA6/l9mxfZvYDYsdGHb8hcXXDwRC5lseXk7YZqqEb6cZvce8+npPEWEMRf7tqqM\n\tsjKYA7XREEm7JRvzUvIo3Frw36JNgaqqcLY6aoOX9bzE3+WZXOj4WV2VKJFh+IKoXh\n\t4f14WiAoqm9Jw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1668469249;\n\tbh=2NOfw2JS+ct2YPfN33HkV7vp1+BhNex0o72AKD7nRes=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=bQ3yGmfV/LVdZ7POxZiZrSscmvLwtsrqI5+M1psU05zkpNBl8UuBk7moV5um2DH10\n\tZykY3ZFAwj7lB9M+2wsMYoQE9hTSw8RKc7LzyQNuPzr5ei5XsFZAZe0963Q02q5SU4\n\tg8KP0QK1ozfUAClwhP8kHrWLAcSMdIe5iaN5gIvU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"bQ3yGmfV\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20221111133025.3102-1-david.plowman@raspberrypi.com>\n\t<20221111133025.3102-2-david.plowman@raspberrypi.com>\n\t","To":"David Plowman ,\n\tLaurent Pinchart ,\n\tLaurent Pinchart via libcamera-devel\n\t","Date":"Mon, 14 Nov 2022 23:40:46 +0000","Message-ID":"<166846924642.2951678.6211101374126427327@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: Add a\n\tPipelineHandler::releaseDevice method","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25798,"web_url":"https://patchwork.libcamera.org/comment/25798/","msgid":"","date":"2022-11-15T10:37:46","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: Add a\n\tPipelineHandler::releaseDevice method","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nThanks for the comments. I see that this patch is already merged so\nplease let me know how you'd like to proceed.\n\nOn Mon, 14 Nov 2022 at 16:56, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> Thank you for the patch.\n>\n> On Fri, Nov 11, 2022 at 01:30:24PM +0000, David Plowman via libcamera-devel wrote:\n> > This notifies pipeline handlers when a camera is released, in case\n> > they want to free any resources or memory buffers.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > include/libcamera/internal/pipeline_handler.h | 4 +++-\n> > src/libcamera/camera.cpp | 2 +-\n> > src/libcamera/pipeline_handler.cpp | 16 +++++++++++++++-\n>\n> The pipeline handler writer's guide\n> (Documentation/guides/pipeline-handler.rst) needs to be udpated. The\n> vivid pipeline handler may also need some attention, but we can handle\n> that.\n\nThank you!\n\n>\n> > 3 files changed, 19 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > index 96aab9d6..ec4f662d 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -46,7 +46,7 @@ public:\n> > const DeviceMatch &dm);\n> >\n> > bool acquire();\n> > - void release();\n> > + void release(Camera *camera);\n> >\n> > virtual std::unique_ptr generateConfiguration(Camera *camera,\n> > const StreamRoles &roles) = 0;\n> > @@ -74,6 +74,8 @@ protected:\n> > virtual int queueRequestDevice(Camera *camera, Request *request) = 0;\n> > virtual void stopDevice(Camera *camera) = 0;\n> >\n> > + virtual void releaseDevice(Camera *camera);\n> > +\n> > CameraManager *manager_;\n> >\n> > private:\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index c4f65c1a..2d947a44 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -870,7 +870,7 @@ int Camera::release()\n> > return ret == -EACCES ? -EBUSY : ret;\n> >\n> > if (d->isAcquired())\n> > - d->pipe_->release();\n> > + d->pipe_->release(this);\n> >\n> > d->setState(Private::CameraAvailable);\n> >\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index 825aff5a..cfade490 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -183,6 +183,7 @@ bool PipelineHandler::acquire()\n> >\n> > /**\n> > * \\brief Release exclusive access to the pipeline handler\n> > + * \\param[in] camera The camera for which to release data\n>\n> This function doesn't release \"data\".\n\nSorry, I was using the word \"data\" generically. Perhaps \"resources\"?\nLet me know if you'd like me to update the patch.\n\n>\n> > *\n> > * This function releases access to the pipeline handler previously acquired by\n> > * a call to acquire(). Every release() call shall match a previous successful\n> > @@ -196,7 +197,7 @@ bool PipelineHandler::acquire()\n> > *\n> > * \\sa acquire()\n> > */\n> > -void PipelineHandler::release()\n> > +void PipelineHandler::release(Camera *camera)\n> > {\n> > MutexLocker locker(lock_);\n> >\n> > @@ -204,9 +205,22 @@ void PipelineHandler::release()\n> >\n> > unlockMediaDevices();\n> >\n> > + releaseDevice(camera);\n> > +\n> > --useCount_;\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 needs further documentation, to clearly explain what can or can't\n> be done here. Examples could help.\n>\n> This being said, I'm not sure hooking up this feature in the\n> Camera::release() function is the right option. In addition to the lack\n> of balance between acquire() and release() (in this series the release()\n> function does more than counterbalance acquire()), the usage pattern\n> that was envisioned is that application can hold onto cameras for a long\n> time. Having to call release() to free memory means that another\n> application could then acquire the camera.\n\nI did wonder about \"acquire\", but it seemed less urgent as you always\nfind out if someone really wants to use the camera! But anyway, I see\nthat Kieran is doing work in this area now.\n\nI guess I might be a bit confused about the purpose of \"release\". I\nexpected the idea would be that other applications could then use it,\nbut maybe there are 2 levels of \"releasing\"? One really means\n\"release\" (and someone else can have it). The other is perhaps closer\nto \"disable it\" (but don't let anyone else have it)? Does that sound\nright?\n\nThanks!\nDavid\n\n>\n> > + */\n> > +void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)\n> > +{\n> > +}\n> > +\n> > void PipelineHandler::unlockMediaDevices()\n> > {\n> > for (std::shared_ptr &media : mediaDevices_)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 42F6ABD16B\n\tfor ;\n\tTue, 15 Nov 2022 10:38:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AD64A6308E;\n\tTue, 15 Nov 2022 11:38:01 +0100 (CET)","from mail-pf1-x42d.google.com (mail-pf1-x42d.google.com\n\t[IPv6:2607:f8b0:4864:20::42d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 87A2661F33\n\tfor ;\n\tTue, 15 Nov 2022 11:37:59 +0100 (CET)","by mail-pf1-x42d.google.com with SMTP id d192so606077pfd.0\n\tfor ;\n\tTue, 15 Nov 2022 02:37:59 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1668508681;\n\tbh=r1MW8l7EmX7aTfnlknddNkwJknyHWjyVwP8yRapYy3w=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=jHu3rwjhhIW/Ji036M2S1zvdKNK6/zpKfV6ZTV/+54nA/HxLeDv+YME7mHOHO4wlp\n\tIfGiREKxBKAA/rBBxDpQrKZcZd8VBuSTmqmVmdP3UcCoz50cq7i92lal2I2QIo5LAh\n\tygHOXnyEnnERRlIwWyiidDpGGZdbHGUCIzjKy7RMbiYeUXSMZkVfZO71/Vscy928BQ\n\tKsA5pBFr413n7fMcnWfH+yIEKAkP/omxqnQh0zT5awscDDETiF7sRl/OaChrrvWz0j\n\tQJzU4MvySRTSrha5W2aJOLB6gYEvweZ7pfplIz6IBvOx95vIudV2Z9q/74srNBb8hP\n\t5xd5ZBMcu+CBA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\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=KTxavgMO38ajPwcecQqo5fnOOjSOa1bliIREDIs+VnY=;\n\tb=JpHJNds1gV8PEHl96c6WIVGVDIzfnt9IyDCgJki8nv66hc3uy2sU4HmtL0/1biomPT\n\tTvj2XA0RcYWLq9MmfnGUkXnNZZF+50TBxUp4fclx3/87wxrMia8OTqlU0FchVX7ltcZj\n\t8PhUNtF/7mcrGq1uiPCOecbUF4NfdigWh7uL7qBoZKUIqMtm7nMbnzjofvFiBSTfEFTI\n\tlE8QeVUrIHjx82DlW+1UxxTfYEi5ezOF2VZOzoyN+oOLvLIEA5NNHQSLz4TzYNRm2Ca/\n\tZkHbA3wq9eOsPEMzRD/+3ISCp+QLuQE2ehek/vthc0NldJMb7YfKkkz6PVQDbLKOmyKk\n\tmLMg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"JpHJNds1\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\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=KTxavgMO38ajPwcecQqo5fnOOjSOa1bliIREDIs+VnY=;\n\tb=6aQo4GGWY6hQezBKleTjlAojE8JE6R5IABuiZZlWfu2PqGgNhrmTpuKVmYVHHXV+tt\n\tqhcLrnGPzAQMTzhchW0SH5NIqDOuQEn7OzDgsnJKT/Lvj5DjlbxiVykIJd12rwiFwktW\n\t+ZUpX00/eYV3pBGWAwlb6Bg686Z2YpsGgQzDevckBvqxskv+TZqw8k+R+ulpUCQf796z\n\tgdITUWgEvtP04jMI0LFbAOr6LHoxGWyyQdaI3SgXkzZvOCCJVKZFJsIgY3+MY4v1vpOD\n\tP6JkVqOQsKhU7AwpnK9Caek1JuP32iJZwrib5Vf/+DiEX4351dVpb8jfwymKqcprdWzs\n\todqQ==","X-Gm-Message-State":"ANoB5pnRNliKWZmyECpWzHzILtT3SYKQ7atEOxqJRyxfv76TemgptIdy\n\tlMq/AX/dJIVZHNqM6OmsX7Ntc53mmxgusJuynkv3yIpjg9g=","X-Google-Smtp-Source":"AA0mqf5MsTinDRWrYAzd1Gfebq/wl2UVjliZrmMQbM8H+DsZCuZbsjeHhQrhDxi661nOO+say+Z5cYoWurvSuCqASps=","X-Received":"by 2002:a65:4606:0:b0:454:228c:742b with SMTP id\n\tv6-20020a654606000000b00454228c742bmr15225245pgq.494.1668508677892;\n\tTue, 15 Nov 2022 02:37:57 -0800 (PST)","MIME-Version":"1.0","References":"<20221111133025.3102-1-david.plowman@raspberrypi.com>\n\t<20221111133025.3102-2-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Tue, 15 Nov 2022 10:37:46 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: Add a\n\tPipelineHandler::releaseDevice method","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","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25799,"web_url":"https://patchwork.libcamera.org/comment/25799/","msgid":"<166851025607.50677.13912057220163020206@Monstersaurus>","date":"2022-11-15T11:04:16","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: Add a\n\tPipelineHandler::releaseDevice method","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting David Plowman via libcamera-devel (2022-11-15 10:37:46)\n> Hi Laurent\n> \n> Thanks for the comments. I see that this patch is already merged so\n> please let me know how you'd like to proceed.\n> \n> On Mon, 14 Nov 2022 at 16:56, Laurent Pinchart\n> wrote:\n> >\n> > Hi David,\n> >\n> > Thank you for the patch.\n> >\n> > On Fri, Nov 11, 2022 at 01:30:24PM +0000, David Plowman via libcamera-devel wrote:\n> > > This notifies pipeline handlers when a camera is released, in case\n> > > they want to free any resources or memory buffers.\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > include/libcamera/internal/pipeline_handler.h | 4 +++-\n> > > src/libcamera/camera.cpp | 2 +-\n> > > src/libcamera/pipeline_handler.cpp | 16 +++++++++++++++-\n> >\n> > The pipeline handler writer's guide\n> > (Documentation/guides/pipeline-handler.rst) needs to be udpated. The\n> > vivid pipeline handler may also need some attention, but we can handle\n> > that.\n> \n> Thank you!\n> \n> >\n> > > 3 files changed, 19 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > index 96aab9d6..ec4f662d 100644\n> > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > @@ -46,7 +46,7 @@ public:\n> > > const DeviceMatch &dm);\n> > >\n> > > bool acquire();\n> > > - void release();\n> > > + void release(Camera *camera);\n> > >\n> > > virtual std::unique_ptr generateConfiguration(Camera *camera,\n> > > const StreamRoles &roles) = 0;\n> > > @@ -74,6 +74,8 @@ protected:\n> > > virtual int queueRequestDevice(Camera *camera, Request *request) = 0;\n> > > virtual void stopDevice(Camera *camera) = 0;\n> > >\n> > > + virtual void releaseDevice(Camera *camera);\n> > > +\n> > > CameraManager *manager_;\n> > >\n> > > private:\n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index c4f65c1a..2d947a44 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -870,7 +870,7 @@ int Camera::release()\n> > > return ret == -EACCES ? -EBUSY : ret;\n> > >\n> > > if (d->isAcquired())\n> > > - d->pipe_->release();\n> > > + d->pipe_->release(this);\n> > >\n> > > d->setState(Private::CameraAvailable);\n> > >\n> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > index 825aff5a..cfade490 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -183,6 +183,7 @@ bool PipelineHandler::acquire()\n> > >\n> > > /**\n> > > * \\brief Release exclusive access to the pipeline handler\n> > > + * \\param[in] camera The camera for which to release data\n> >\n> > This function doesn't release \"data\".\n> \n> Sorry, I was using the word \"data\" generically. Perhaps \"resources\"?\n> Let me know if you'd like me to update the patch.\n> \n> >\n> > > *\n> > > * This function releases access to the pipeline handler previously acquired by\n> > > * a call to acquire(). Every release() call shall match a previous successful\n> > > @@ -196,7 +197,7 @@ bool PipelineHandler::acquire()\n> > > *\n> > > * \\sa acquire()\n> > > */\n> > > -void PipelineHandler::release()\n> > > +void PipelineHandler::release(Camera *camera)\n> > > {\n> > > MutexLocker locker(lock_);\n> > >\n> > > @@ -204,9 +205,22 @@ void PipelineHandler::release()\n> > >\n> > > unlockMediaDevices();\n> > >\n> > > + releaseDevice(camera);\n> > > +\n> > > --useCount_;\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 needs further documentation, to clearly explain what can or can't\n> > be done here. Examples could help.\n> >\n> > This being said, I'm not sure hooking up this feature in the\n> > Camera::release() function is the right option. In addition to the lack\n> > of balance between acquire() and release() (in this series the release()\n> > function does more than counterbalance acquire()), the usage pattern\n> > that was envisioned is that application can hold onto cameras for a long\n> > time. Having to call release() to free memory means that another\n> > application could then acquire the camera.\n> \n> I did wonder about \"acquire\", but it seemed less urgent as you always\n> find out if someone really wants to use the camera! But anyway, I see\n> that Kieran is doing work in this area now.\n> \n> I guess I might be a bit confused about the purpose of \"release\". I\n> expected the idea would be that other applications could then use it,\n> but maybe there are 2 levels of \"releasing\"? One really means\n> \"release\" (and someone else can have it). The other is perhaps closer\n> to \"disable it\" (but don't let anyone else have it)? Does that sound\n> right?\n\nIn my understanding, a Camera is created when an application launches\nthe CameraManager. Those Camera's exist and have been initialised, so if\nthat process is long living (like a Camera Daemon, or Pipewire), those\nobjects will persist. But I don't think they should acquire or lock\nresources until an explicit call to acquire() has occurred.\n\nIt may be unlikely that a Pipeline should need to allocate memory during\nrequire, but I'm sure that during release, they should release anything\nthat was allocated additionally during configure()!\n\nIt would be nicer to keep the base implementation symetrical I think, so\nan equivalent acquire() could be passed to the pipeline handler\n(optionally) ... but it does depend on it being needed. I tried only\nopening the video node for UVC on acquire (or rather, open it during\ninit(), obtain information required, then close it until acquire() ),\nand hit (unexpected) issues.\n\nI think I'll just post the patches anyway, with a [DNI] tag on the UVC\npart to share. Maybe it will be obvious to someone what either I've done\nwrong, or what the bug is.\n\n--\nKieran\n\n\n\n> \n> Thanks!\n> David\n> \n> >\n> > > + */\n> > > +void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)\n> > > +{\n> > > +}\n> > > +\n> > > void PipelineHandler::unlockMediaDevices()\n> > > {\n> > > for (std::shared_ptr &media : mediaDevices_)\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart","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 A1EF1BE08B\n\tfor ;\n\tTue, 15 Nov 2022 11:04:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C8EA563088;\n\tTue, 15 Nov 2022 12:04:20 +0100 (CET)","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 0A0A163079\n\tfor ;\n\tTue, 15 Nov 2022 12:04:19 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 78A2782C;\n\tTue, 15 Nov 2022 12:04:18 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1668510260;\n\tbh=AnvMISRtcGt+gUBhN9MCWijG6bW1U1ti8Qm3vBuXxnQ=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=GQANrUnL45hQdDmNiY61fPmPA6EGXM3dDItEUC2VzQaPbTQbz2xmihJAtoo89RyWA\n\tPtwQf/NCRTR4uXxLjrbqKX3toqRJfQmkISn74yd0rbxT2Kt3nP5RHU9UtyTmn0tzR6\n\tBTGWoz1SLG1DQL9NZsb2jfuY6ZM1soo3uLdjNUplTZVDF1+jhQLvETujpbiUbGnEQv\n\t3W1sUzZk/9eVYat9jz7Dz/nKqqb2Kge8a2bXTGPifonVwnn8xCBx/tBSsD3O6mFjhQ\n\tNhraNyoOKQLOweB/ft8RZh/ogAPqIVl5DDhcvk/Z9curTM6HQpK/YOEgFx8cLJTX6R\n\t3l8GyFHNJmZgQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1668510258;\n\tbh=AnvMISRtcGt+gUBhN9MCWijG6bW1U1ti8Qm3vBuXxnQ=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=XkSRkbRJLSr/WrIVVivJuhBlj2isj9pd4YJFm4v7YEUClmTzVL9RsRmUGBbbfjxxN\n\tQCtgLjyImfWTn7wE12QcoiNvl/N6zg7wjeyibb70BlBNQIa8ybMVsWTdRn1pNNDjRE\n\tD9PABnpJ5d4qtY2aX9yZTHDRfZS09ul0vq5J2jm0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"XkSRkbRJ\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20221111133025.3102-1-david.plowman@raspberrypi.com>\n\t<20221111133025.3102-2-david.plowman@raspberrypi.com>\n\t\n\t","To":"David Plowman ,\n\tDavid Plowman via libcamera-devel , \n\tLaurent Pinchart ","Date":"Tue, 15 Nov 2022 11:04:16 +0000","Message-ID":"<166851025607.50677.13912057220163020206@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: Add a\n\tPipelineHandler::releaseDevice method","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]