[{"id":26591,"web_url":"https://patchwork.libcamera.org/comment/26591/","msgid":"","date":"2023-03-07T15:37:04","subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Iterate over\n\tall Unicam instances in match()","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for the patch.\n\nOn Fri, 24 Feb 2023 at 07:30, Naushir Patuck via libcamera-devel\n wrote:\n>\n> On Raspberry Pi Compute Module platforms, it is possible to attach a\n> single camera device only to the secondary Unicam port. The current\n> logic of PipelineHandlerRPi::match() will return a failure during\n> enumeration of the first Unicam media device (due to no sensor attached,\n> or sensor failure) and thus the second Unicam media device will never be\n> enumerated.\n>\n> Fix this by looping over all Unicam instances in PipelineHandlerRPi::match()\n> until a camera is correctly registered, or return a failure otherwise.\n>\n> Reported-on: https://github.com/raspberrypi/libcamera/issues/44\n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 67 +++++++++++--------\n> 1 file changed, 40 insertions(+), 27 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 841209548350..ef01b7e166ba 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1246,41 +1246,54 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n>\n> bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> {\n> - DeviceMatch unicam(\"unicam\");\n> - MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);\n> + constexpr unsigned int numUnicamDevices = 2;\n>\n> - if (!unicamDevice) {\n> - LOG(RPI, Debug) << \"Unable to acquire a Unicam instance\";\n> - return false;\n> - }\n> + /*\n> + * Loop over all Unicam instances, but return out once a match is found.\n> + * This is to ensure we correctly enumrate the camera when an instance\n\ns/enumrate/enumerate/\n\n> + * of Unicam has registered with media controller, but has not registered\n> + * device nodes due to a sensor subdevice failure.\n> + */\n> + for (unsigned int i = 0; i < numUnicamDevices; i++) {\n> + DeviceMatch unicam(\"unicam\");\n> + MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);\n>\n> - DeviceMatch isp(\"bcm2835-isp\");\n> - MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);\n> + if (!unicamDevice) {\n> + LOG(RPI, Debug) << \"Unable to acquire a Unicam instance\";\n> + continue;\n> + }\n>\n> - if (!ispDevice) {\n> - LOG(RPI, Debug) << \"Unable to acquire ISP instance\";\n> - return false;\n> - }\n> + DeviceMatch isp(\"bcm2835-isp\");\n> + MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);\n>\n> - /*\n> - * The loop below is used to register multiple cameras behind one or more\n> - * video mux devices that are attached to a particular Unicam instance.\n> - * Obviously these cameras cannot be used simultaneously.\n> - */\n> - unsigned int numCameras = 0;\n> - for (MediaEntity *entity : unicamDevice->entities()) {\n> - if (entity->function() != MEDIA_ENT_F_CAM_SENSOR)\n> + if (!ispDevice) {\n> + LOG(RPI, Debug) << \"Unable to acquire ISP instance\";\n> continue;\n> + }\n>\n> - int ret = registerCamera(unicamDevice, ispDevice, entity);\n> - if (ret)\n> - LOG(RPI, Error) << \"Failed to register camera \"\n> - << entity->name() << \": \" << ret;\n> - else\n> - numCameras++;\n> + /*\n> + * The loop below is used to register multiple cameras behind one or more\n> + * video mux devices that are attached to a particular Unicam instance.\n> + * Obviously these cameras cannot be used simultaneously.\n> + */\n> + unsigned int numCameras = 0;\n> + for (MediaEntity *entity : unicamDevice->entities()) {\n> + if (entity->function() != MEDIA_ENT_F_CAM_SENSOR)\n> + continue;\n> +\n> + int ret = registerCamera(unicamDevice, ispDevice, entity);\n> + if (ret)\n> + LOG(RPI, Error) << \"Failed to register camera \"\n> + << entity->name() << \": \" << ret;\n> + else\n> + numCameras++;\n> + }\n> +\n> + if (numCameras)\n> + return true;\n> }\n>\n> - return !!numCameras;\n> + return false;\n> }\n\nLooks fine to me!\n\nReviewed-by: David Plowman \n\nThanks!\nDavid\n\n>\n> void PipelineHandlerRPi::releaseDevice(Camera *camera)\n> --\n> 2.25.1\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 E841DBE080\n\tfor ;\n\tTue, 7 Mar 2023 15:37:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 592F5626A7;\n\tTue, 7 Mar 2023 16:37:19 +0100 (CET)","from mail-oi1-x22a.google.com (mail-oi1-x22a.google.com\n\t[IPv6:2607:f8b0:4864:20::22a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B4E3E603AE\n\tfor ;\n\tTue, 7 Mar 2023 16:37:17 +0100 (CET)","by mail-oi1-x22a.google.com with SMTP id bm20so9888633oib.7\n\tfor ;\n\tTue, 07 Mar 2023 07:37:17 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1678203439;\n\tbh=yin28KFSYhY7AL9VJVbp2Iag6qsuTVox+JCxeGg9B8c=;\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=hUu/d0lQ4DreaRHVOUHUT+tqkoNjtdWpVSEh3BYMZMa4UAI2Cr+y88OQMrwrIyYRO\n\tTmyQeaXAb447NZY4kRh85ZqZWFkAtSS07g1G5moDDIAvk21LifbkP0ubdgNMfOrBr8\n\tYQEbfiEYE/9YEafugEymZQH0Tyae5ez4RjSoGjARqvolRTW5SZbXgWtKExhJxZMdnO\n\tRsUXIV8uAX6Qo26HOtG8qLYzTDjmXOadI35wffyE6sbS2syLdi/Ep7ljjSemRqcoD3\n\tmGT64ePU/eLRf248eM7qqKeOGNMzvr8y2VPk4lI3g/LejK6hk/OAWBN8b77vycJVhf\n\t4wxNwH4Zyoyzw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1678203436;\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=j4yzLQhEN6MEAabvihcffant9bUycDDo0HRxbrsNV7s=;\n\tb=fSs2+u1e7+wj5Ky2+9vfquz5nwg8v2wnWxCLnWMIgHhlHDfOQOYbldRd9nCqz33PCG\n\tS2QmJIKcNGXFkIG2KQbN4Zygyv8N0CyKWu5voPZ6cdXqSTwp00ktLCs8LTuUS0d3a4zr\n\t/QZT8aEozXD2gC7CSV7QrWq35gubwWj99zpoBka5Gj7uqn8fz1D+EY5ST78eho8e48py\n\t8pKM8suDkRiQe6adh8aMSDrLj+X8q6EhPCusDlu6cjVqUKvJQhByuG9QMr4cR592CFns\n\tUBLvssSmq8vtDGHhZ5MNmuCerwPljswZJwfCu9f7S+/StFK+ohLk+qB2pcHxxOHlWnBy\n\t13wQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"fSs2+u1e\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1678203436;\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=j4yzLQhEN6MEAabvihcffant9bUycDDo0HRxbrsNV7s=;\n\tb=EbuSKFXnH+iQAx8K+8L8akFwBoCg7HdxnYfmCAxbD6cxRYQJ+wSp436MNDyxczH9Ep\n\t03LaTI4w+fknTLY7h+/B6vCcXKNTBm9adRHgLye/fAzR7RGlyPcMVYY5YZCU1H6/CCNf\n\tK5l+nJZ4C/71Y1JR66Zye0yL9Vp9fF2a27W/akNdV35wEiogak28VVUizvA0KtPwxwFh\n\tk+SGaVsDIJhS42kCiDQ1AO8Jf+xNHfwhjCEbm1HZUGc4WXhm8iZ6AxM1RMMJSPt9BxlQ\n\tazdqi7WqybWwjzSSWB9GA0ehKZBkrz5YKaMJg6jUjzjfTrNFy5WMqPX9q99fnpldezWe\n\tQ9CQ==","X-Gm-Message-State":"AO0yUKVQvZkwAusVccxvA5MDraK8FRIf8XIjSmJEiMsnz2Lg9VoQMM28\n\tF5nMFYHO1TLKxai0m3z/BXzz/dZyi11homVneRswdTs2bwvUmRNRG9k=","X-Google-Smtp-Source":"AK7set8jXHCBEhTwdT2I3GivMJQ22KCdqrxsviqKUIZYpM6GTDfLRHpwlfB60UKzQ5g2yEtIMoQfcJGo/BuDhSA5d0M=","X-Received":"by 2002:a05:6808:2782:b0:378:74af:45ef with SMTP id\n\tes2-20020a056808278200b0037874af45efmr4321506oib.11.1678203434818;\n\tTue, 07 Mar 2023 07:37:14 -0800 (PST)","MIME-Version":"1.0","References":"","In-Reply-To":"","Date":"Tue, 7 Mar 2023 15:37:04 +0000","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Iterate over\n\tall Unicam instances in match()","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":26601,"web_url":"https://patchwork.libcamera.org/comment/26601/","msgid":"<20230309011431.GL31765@pendragon.ideasonboard.com>","date":"2023-03-09T01:14:31","subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Iterate over\n\tall Unicam instances in match()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nI know this has been merged, but I've noticed a few issues, which can be\nfixed in further patches.\n\nOn Fri, Feb 24, 2023 at 07:30:23AM +0000, Naushir Patuck via libcamera-devel wrote:\n> \n> On Raspberry Pi Compute Module platforms, it is possible to attach a\n> single camera device only to the secondary Unicam port. The current\n> logic of PipelineHandlerRPi::match() will return a failure during\n> enumeration of the first Unicam media device (due to no sensor attached,\n> or sensor failure) and thus the second Unicam media device will never be\n> enumerated.\n> \n> Fix this by looping over all Unicam instances in PipelineHandlerRPi::match()\n> until a camera is correctly registered, or return a failure otherwise.\n> \n> Reported-on: https://github.com/raspberrypi/libcamera/issues/44\n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 67 +++++++++++--------\n> 1 file changed, 40 insertions(+), 27 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 841209548350..ef01b7e166ba 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1246,41 +1246,54 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> \n> bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> {\n> -\tDeviceMatch unicam(\"unicam\");\n> -\tMediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);\n> +\tconstexpr unsigned int numUnicamDevices = 2;\n\nConstants should start with a k prefix, that is kNumUnicamDevices.\n\n> \n> -\tif (!unicamDevice) {\n> -\t\tLOG(RPI, Debug) << \"Unable to acquire a Unicam instance\";\n> -\t\treturn false;\n> -\t}\n> +\t/*\n> +\t * Loop over all Unicam instances, but return out once a match is found.\n> +\t * This is to ensure we correctly enumrate the camera when an instance\n> +\t * of Unicam has registered with media controller, but has not registered\n> +\t * device nodes due to a sensor subdevice failure.\n> +\t */\n> +\tfor (unsigned int i = 0; i < numUnicamDevices; i++) {\n> +\t\tDeviceMatch unicam(\"unicam\");\n> +\t\tMediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);\n> \n> -\tDeviceMatch isp(\"bcm2835-isp\");\n> -\tMediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);\n> +\t\tif (!unicamDevice) {\n> +\t\t\tLOG(RPI, Debug) << \"Unable to acquire a Unicam instance\";\n> +\t\t\tcontinue;\n\nThis looks weird, if the unicam device can't be acquired, I don't see\nhow the next iteration of the loop could successfully acquire another\ninstance. I would thus break here.\n\n> +\t\t}\n> \n> -\tif (!ispDevice) {\n> -\t\tLOG(RPI, Debug) << \"Unable to acquire ISP instance\";\n> -\t\treturn false;\n> -\t}\n> +\t\tDeviceMatch isp(\"bcm2835-isp\");\n> +\t\tMediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);\n> \n> -\t/*\n> -\t * The loop below is used to register multiple cameras behind one or more\n> -\t * video mux devices that are attached to a particular Unicam instance.\n> -\t * Obviously these cameras cannot be used simultaneously.\n> -\t */\n> -\tunsigned int numCameras = 0;\n> -\tfor (MediaEntity *entity : unicamDevice->entities()) {\n> -\t\tif (entity->function() != MEDIA_ENT_F_CAM_SENSOR)\n> +\t\tif (!ispDevice) {\n> +\t\t\tLOG(RPI, Debug) << \"Unable to acquire ISP instance\";\n> \t\t\tcontinue;\n\nShouldn't you release the unicam device in this case ? I think it would\nbe better to first loop over unicam instances, ignoring any instance\nthan has no connected camera sensor, and then, if an instance with a\nconnected sensor is found, acquire an ISP instance.\n\n> +\t\t}\n> \n> -\t\tint ret = registerCamera(unicamDevice, ispDevice, entity);\n> -\t\tif (ret)\n> -\t\t\tLOG(RPI, Error) << \"Failed to register camera \"\n> -\t\t\t\t\t<< entity->name() << \": \" << ret;\n> -\t\telse\n> -\t\t\tnumCameras++;\n> +\t\t/*\n> +\t\t * The loop below is used to register multiple cameras behind one or more\n> +\t\t * video mux devices that are attached to a particular Unicam instance.\n> +\t\t * Obviously these cameras cannot be used simultaneously.\n> +\t\t */\n> +\t\tunsigned int numCameras = 0;\n> +\t\tfor (MediaEntity *entity : unicamDevice->entities()) {\n> +\t\t\tif (entity->function() != MEDIA_ENT_F_CAM_SENSOR)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tint ret = registerCamera(unicamDevice, ispDevice, entity);\n> +\t\t\tif (ret)\n> +\t\t\t\tLOG(RPI, Error) << \"Failed to register camera \"\n> +\t\t\t\t\t\t<< entity->name() << \": \" << ret;\n> +\t\t\telse\n> +\t\t\t\tnumCameras++;\n> +\t\t}\n> +\n> +\t\tif (numCameras)\n> +\t\t\treturn true;\n> \t}\n> \n> -\treturn !!numCameras;\n> +\treturn false;\n> }\n> \n> void PipelineHandlerRPi::releaseDevice(Camera *camera)","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 6FF0ABE080\n\tfor ;\n\tThu, 9 Mar 2023 01:14:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B0C5A626CA;\n\tThu, 9 Mar 2023 02:14:32 +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 85B05603AE\n\tfor ;\n\tThu, 9 Mar 2023 02:14:30 +0100 (CET)","from pendragon.ideasonboard.com (85-76-112-28-nat.elisa-mobile.fi\n\t[85.76.112.28])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 885BB589;\n\tThu, 9 Mar 2023 02:14:29 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1678324472;\n\tbh=uCxI0au/AgZ47b3ZVlwEcuUw3EFCg8s6fWJsOiBpM50=;\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=aBqHHIZXCOnVSHshPxDQofb95jfupmEvhjqTSjn3q9s+eDRdbQ/5Nv746gnR1Vy4p\n\tBVpbGYFPKVlax/PZDY64hCj6LL5QNn+YiMa7n1aN9riPqsl00UDURPWjUDvd6R+9Af\n\tU6KqXlnOEceGNTWwFLCRyK1ChRMdtwmY/lyfDEOt7RG9k/pJpRraeEjpht0/Y5DIBR\n\thx0lqupYxbuAkpC7XJQQCaVz6F/BH5OZLtOPxhwpiPc0XcN6TIk2EZ4YZw33egJ8Nu\n\t2s5PRmbZYkqMFEvBOZT7f5cOAJCW1QBH6WGYRXSIPgaq2/ZrfBarodQE8k+Jfmbe7P\n\tnblDvYvFRUdFA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1678324470;\n\tbh=uCxI0au/AgZ47b3ZVlwEcuUw3EFCg8s6fWJsOiBpM50=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LEXPnP+DCwgq+z//bJrVhiPDvx1LESNlRl38+j3gu5I551tJQN59AU9/1UsBUbdJT\n\tHlWyMB+zdyrw5EjNazvtkd6Xuw2byAfX87VJR1v7csi6GTvPhJum6ICMRsbFF3/7ST\n\teqkurV8LDP9euV8ysFqsHfeN//fxUKqDn/s+otGw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"LEXPnP+D\"; dkim-atps=neutral","Date":"Thu, 9 Mar 2023 03:14:31 +0200","To":"Naushir Patuck ","Message-ID":"<20230309011431.GL31765@pendragon.ideasonboard.com>","References":"","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Iterate over\n\tall Unicam instances in match()","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":26602,"web_url":"https://patchwork.libcamera.org/comment/26602/","msgid":"","date":"2023-03-09T08:04:47","subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Iterate over\n\tall Unicam instances in match()","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your feedback.\n\nOn Thu, 9 Mar 2023 at 01:14, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> I know this has been merged, but I've noticed a few issues, which can be\n> fixed in further patches.\n>\n> On Fri, Feb 24, 2023 at 07:30:23AM +0000, Naushir Patuck via\n> libcamera-devel wrote:\n> >\n> > On Raspberry Pi Compute Module platforms, it is possible to attach a\n> > single camera device only to the secondary Unicam port. The current\n> > logic of PipelineHandlerRPi::match() will return a failure during\n> > enumeration of the first Unicam media device (due to no sensor attached,\n> > or sensor failure) and thus the second Unicam media device will never be\n> > enumerated.\n> >\n> > Fix this by looping over all Unicam instances in\n> PipelineHandlerRPi::match()\n> > until a camera is correctly registered, or return a failure otherwise.\n> >\n> > Reported-on: https://github.com/raspberrypi/libcamera/issues/44\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 67 +++++++++++--------\n> > 1 file changed, 40 insertions(+), 27 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 841209548350..ef01b7e166ba 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1246,41 +1246,54 @@ int\n> PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> >\n> > bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > {\n> > - DeviceMatch unicam(\"unicam\");\n> > - MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);\n> > + constexpr unsigned int numUnicamDevices = 2;\n>\n> Constants should start with a k prefix, that is kNumUnicamDevices.\n>\n> >\n> > - if (!unicamDevice) {\n> > - LOG(RPI, Debug) << \"Unable to acquire a Unicam instance\";\n> > - return false;\n> > - }\n> > + /*\n> > + * Loop over all Unicam instances, but return out once a match is\n> found.\n> > + * This is to ensure we correctly enumrate the camera when an\n> instance\n> > + * of Unicam has registered with media controller, but has not\n> registered\n> > + * device nodes due to a sensor subdevice failure.\n> > + */\n> > + for (unsigned int i = 0; i < numUnicamDevices; i++) {\n> > + DeviceMatch unicam(\"unicam\");\n> > + MediaDevice *unicamDevice = acquireMediaDevice(enumerator,\n> unicam);\n> >\n> > - DeviceMatch isp(\"bcm2835-isp\");\n> > - MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);\n> > + if (!unicamDevice) {\n> > + LOG(RPI, Debug) << \"Unable to acquire a Unicam\n> instance\";\n> > + continue;\n>\n> This looks weird, if the unicam device can't be acquired, I don't see\n> how the next iteration of the loop could successfully acquire another\n> instance. I would thus break here.\n>\n\nThis is probably me not understanding how the media device enumeration stuff\nworks, but I thought the continue would be needed for situations where we\nwant\nsimultaneous dual cameras running in separate processes. For example,\nprocess 0\nacquires \"Unicam 0\" and starts running as normal. Process 1 starts and goes\nthrough match() where \"Unicam 0\" still exists in the entity list, but fails\nto\nacquire because it is locked by process 0. So we have to move on to\n\"Unicam 1\"\nwhich is acquired correctly for process 1. Is that understanding wrong?\n\n\n>\n> > + }\n> >\n> > - if (!ispDevice) {\n> > - LOG(RPI, Debug) << \"Unable to acquire ISP instance\";\n> > - return false;\n> > - }\n> > + DeviceMatch isp(\"bcm2835-isp\");\n> > + MediaDevice *ispDevice = acquireMediaDevice(enumerator,\n> isp);\n> >\n> > - /*\n> > - * The loop below is used to register multiple cameras behind one\n> or more\n> > - * video mux devices that are attached to a particular Unicam\n> instance.\n> > - * Obviously these cameras cannot be used simultaneously.\n> > - */\n> > - unsigned int numCameras = 0;\n> > - for (MediaEntity *entity : unicamDevice->entities()) {\n> > - if (entity->function() != MEDIA_ENT_F_CAM_SENSOR)\n> > + if (!ispDevice) {\n> > + LOG(RPI, Debug) << \"Unable to acquire ISP\n> instance\";\n> > continue;\n>\n> Shouldn't you release the unicam device in this case ? I think it would\n> be better to first loop over unicam instances, ignoring any instance\n> than has no connected camera sensor, and then, if an instance with a\n> connected sensor is found, acquire an ISP instance.\n>\n\nI think we discussed this briefly in the github comments. There is no\ncompliment releaseMediaDevice() call that I can use to release the device.\n\nRegarding the second part of the comment, yes, I could move the isp acquire\nbit\ninto the for (unicamDevice->entities()) loop to optimise this a bit.\n\nRegards,\nNaush\n\n\n> > + }\n> >\n> > - int ret = registerCamera(unicamDevice, ispDevice, entity);\n> > - if (ret)\n> > - LOG(RPI, Error) << \"Failed to register camera \"\n> > - << entity->name() << \": \" << ret;\n> > - else\n> > - numCameras++;\n> > + /*\n> > + * The loop below is used to register multiple cameras\n> behind one or more\n> > + * video mux devices that are attached to a particular\n> Unicam instance.\n> > + * Obviously these cameras cannot be used simultaneously.\n> > + */\n> > + unsigned int numCameras = 0;\n> > + for (MediaEntity *entity : unicamDevice->entities()) {\n> > + if (entity->function() != MEDIA_ENT_F_CAM_SENSOR)\n> > + continue;\n> > +\n> > + int ret = registerCamera(unicamDevice, ispDevice,\n> entity);\n> > + if (ret)\n> > + LOG(RPI, Error) << \"Failed to register\n> camera \"\n> > + << entity->name() << \": \"\n> << ret;\n> > + else\n> > + numCameras++;\n> > + }\n> > +\n> > + if (numCameras)\n> > + return true;\n> > }\n> >\n> > - return !!numCameras;\n> > + return false;\n> > }\n> >\n> > void PipelineHandlerRPi::releaseDevice(Camera *camera)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 33B2EBDE17\n\tfor ;\n\tThu, 9 Mar 2023 08:05:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 65991626CF;\n\tThu, 9 Mar 2023 09:05:05 +0100 (CET)","from mail-yb1-xb29.google.com (mail-yb1-xb29.google.com\n\t[IPv6:2607:f8b0:4864:20::b29])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4B0B462664\n\tfor ;\n\tThu, 9 Mar 2023 09:05:04 +0100 (CET)","by mail-yb1-xb29.google.com with SMTP id o1so1029506ybu.13\n\tfor ;\n\tThu, 09 Mar 2023 00:05:04 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1678349105;\n\tbh=Sq9I+8l0zfHM2gc+Syqv6whgg2kQQMDhHAn8qCPRiwY=;\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=NqPjRRJpi0Hoh3oK7oKH5NLWqPpXCqHEiS+AQnycufTeUHnzq7X/hZhuIllRuAmUd\n\tCJTwqkupIZSMm060MwJ/mHntKkD/qUDZ4gvbIoxjJglM2GiXri4Wl8Ii8z81zXKtsN\n\tNxs84aZX9Ru+Zgbxb7yBwlpQVymqsRWRRFMl8XFoVTEtuLX5lBXn1QSIhCP/mO1RSv\n\tusXdwPK/7RN2Sd2c/GgokkwfQI8CXKLcx4dVS37Ecj1yAwmN9OMnwgKJANkNvJPYzv\n\teecF06VAJ8ZzX5AJWMmW68ImLwNrXs3Ejoin40DAIYsOaVIrZBHqLhV1IrtAZ7RMGv\n\tAgk9H64/K5F4Q==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1678349103;\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=byfV+q9/NxIz40oCo41f1mYWH7x0WgbX0YhAWUKQ6Ok=;\n\tb=eX8npVVi2sjlMig2MqkFMoEz0gi123Kmlz5LHbr9OXRLsC59Znk5w8a3rdkh7o/ZJT\n\t05KOEPH0LrbUz6GOnxDZPu/LU4mR7JkEES56yU5IC/c5KLUb5Ia95GIVhkVlQKOmhe8x\n\tRInLweVQ6KQFY3Xm/E8bLTopPqqgAsYs768Kc+uTqeTcyXObbBAzx1mkrnFlTA+DglY4\n\tfJ7iLYLYIGmh97HiSzN+501BWUN0Kpsa3GPvsS1ZuNPLzhffTQygVAVxmEm6QzvyA3qa\n\t7oSNTy4geuDQuMYZvQckIPQdnR8xPCJ0yaB3xdUg6mSeu1AecK29Jp4sYbJ2Aq0ASYw5\n\tJlSg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"eX8npVVi\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1678349103;\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=byfV+q9/NxIz40oCo41f1mYWH7x0WgbX0YhAWUKQ6Ok=;\n\tb=2/KQOJv7kBmAxoyxNnAcAkFZeCwOKskvGGpPosM43lVtM+nhKaACgY0nND46fTZDwS\n\t7IzNg0WEn1pkpd5Fts7vmpbe0czAluPeiwwy+tz3QM3Acc7bZV1jtHslnfDsJqRiL/qs\n\tWDhtVKZMlYpgCD66B38mk45zZhRNVf/JoPJr0KBNnXlCMYj1aVUUaZataDpKUtxXCOfK\n\tgQiuMOmndMA9q49d/DiZhopjP3NW5Dv3TwnVifbjNqYz3h0MJGuc8YX6dvV+VBz39FNb\n\tbk0g4ryMhTamHwkIHgJZgCm8ZQEz5VdiUA/XL7qRoQlryxLQzi8MaPJUfzywWZF3ATnb\n\tYd3w==","X-Gm-Message-State":"AO0yUKVxelrqaZ2ZoxdG/a8jip7AVUdSpIYlsc6X629YreD5rYowiu2S\n\toFgqb+MKh6crUR4t3kDxRuoLZjYsGIrtHVKMlwrlDg==","X-Google-Smtp-Source":"AK7set8oWUbi+yCgNvfAvRksc84BVF9glnJa+drUnIe0383DNPwZCFkkVJzlYeqqccn8d3eIY7WGxYHMzPvP+Hc2RmU=","X-Received":"by 2002:a25:8c0f:0:b0:afd:2ef2:33c2 with SMTP id\n\tk15-20020a258c0f000000b00afd2ef233c2mr3327877ybl.2.1678349102851;\n\tThu, 09 Mar 2023 00:05:02 -0800 (PST)","MIME-Version":"1.0","References":"\n\t<20230309011431.GL31765@pendragon.ideasonboard.com>","In-Reply-To":"<20230309011431.GL31765@pendragon.ideasonboard.com>","Date":"Thu, 9 Mar 2023 08:04:47 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"00000000000015c02f05f6731a64\"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Iterate over\n\tall Unicam instances in match()","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":26605,"web_url":"https://patchwork.libcamera.org/comment/26605/","msgid":"<20230309100301.GN31765@pendragon.ideasonboard.com>","date":"2023-03-09T10:03:01","subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Iterate over\n\tall Unicam instances in match()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Thu, Mar 09, 2023 at 08:04:47AM +0000, Naushir Patuck wrote:\n> On Thu, 9 Mar 2023 at 01:14, Laurent Pinchart wrote:\n> \n> > Hi Naush,\n> >\n> > Thank you for the patch.\n> >\n> > I know this has been merged, but I've noticed a few issues, which can be\n> > fixed in further patches.\n> >\n> > On Fri, Feb 24, 2023 at 07:30:23AM +0000, Naushir Patuck via libcamera-devel wrote:\n> > >\n> > > On Raspberry Pi Compute Module platforms, it is possible to attach a\n> > > single camera device only to the secondary Unicam port. The current\n> > > logic of PipelineHandlerRPi::match() will return a failure during\n> > > enumeration of the first Unicam media device (due to no sensor attached,\n> > > or sensor failure) and thus the second Unicam media device will never be\n> > > enumerated.\n> > >\n> > > Fix this by looping over all Unicam instances in PipelineHandlerRPi::match()\n> > > until a camera is correctly registered, or return a failure otherwise.\n> > >\n> > > Reported-on: https://github.com/raspberrypi/libcamera/issues/44\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 67 +++++++++++--------\n> > > 1 file changed, 40 insertions(+), 27 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 841209548350..ef01b7e166ba 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -1246,41 +1246,54 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > >\n> > > bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > {\n> > > - DeviceMatch unicam(\"unicam\");\n> > > - MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);\n> > > + constexpr unsigned int numUnicamDevices = 2;\n> >\n> > Constants should start with a k prefix, that is kNumUnicamDevices.\n> >\n> > >\n> > > - if (!unicamDevice) {\n> > > - LOG(RPI, Debug) << \"Unable to acquire a Unicam instance\";\n> > > - return false;\n> > > - }\n> > > + /*\n> > > + * Loop over all Unicam instances, but return out once a match is found.\n> > > + * This is to ensure we correctly enumrate the camera when an instance\n> > > + * of Unicam has registered with media controller, but has not registered\n> > > + * device nodes due to a sensor subdevice failure.\n> > > + */\n> > > + for (unsigned int i = 0; i < numUnicamDevices; i++) {\n> > > + DeviceMatch unicam(\"unicam\");\n> > > + MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);\n> > >\n> > > - DeviceMatch isp(\"bcm2835-isp\");\n> > > - MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);\n> > > + if (!unicamDevice) {\n> > > + LOG(RPI, Debug) << \"Unable to acquire a Unicam instance\";\n> > > + continue;\n> >\n> > This looks weird, if the unicam device can't be acquired, I don't see\n> > how the next iteration of the loop could successfully acquire another\n> > instance. I would thus break here.\n> \n> This is probably me not understanding how the media device enumeration stuff\n> works, but I thought the continue would be needed for situations where we want\n> simultaneous dual cameras running in separate processes. For example, process 0\n> acquires \"Unicam 0\" and starts running as normal. Process 1 starts and goes\n> through match() where \"Unicam 0\" still exists in the entity list, but fails to\n> acquire because it is locked by process 0. So we have to move on to \"Unicam 1\"\n> which is acquired correctly for process 1. Is that understanding wrong?\n\nThe minimal inter-process locking support in libcamera only operates\nwhen trying to acquire a *camera* with Camera::acquire(). The\nacquireMediaDevice() function is a bit confusing, its name refers to the\npipeline handler acquiring a MediaDevice from the DeviceEnumerator,\nguaranteeing that the pipeline handler gets ownership of the media\ndevice and no other pipeline handler *in the same process* will be able\nto acquire it. Two processes running libcamera will both get \"Unicam 0\"\nin the first iteration of the loop.\n\n> > > + }\n> > >\n> > > - if (!ispDevice) {\n> > > - LOG(RPI, Debug) << \"Unable to acquire ISP instance\";\n> > > - return false;\n> > > - }\n> > > + DeviceMatch isp(\"bcm2835-isp\");\n> > > + MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);\n> > >\n> > > - /*\n> > > - * The loop below is used to register multiple cameras behind one or more\n> > > - * video mux devices that are attached to a particular Unicam instance.\n> > > - * Obviously these cameras cannot be used simultaneously.\n> > > - */\n> > > - unsigned int numCameras = 0;\n> > > - for (MediaEntity *entity : unicamDevice->entities()) {\n> > > - if (entity->function() != MEDIA_ENT_F_CAM_SENSOR)\n> > > + if (!ispDevice) {\n> > > + LOG(RPI, Debug) << \"Unable to acquire ISP instance\";\n> > > continue;\n> >\n> > Shouldn't you release the unicam device in this case ? I think it would\n> > be better to first loop over unicam instances, ignoring any instance\n> > than has no connected camera sensor, and then, if an instance with a\n> > connected sensor is found, acquire an ISP instance.\n> \n> I think we discussed this briefly in the github comments. There is no\n> compliment releaseMediaDevice() call that I can use to release the device.\n\nIt's an issue indeed. The design idea was to release all acquired media\ndevices automatically when the match() function returns false, but that\ndoesn't allow releasing media device that have been acquired and turned\nout not to be needed.\n\nIn this specific case, if you acquire a unicam instance that has no\nconnected sensor, it's fine if it stays acquired as no other pipeline\nhandler instance would be able to use it for a meaningful purpose\nanyway, but in general this is something we should probably fix.\n\n> Regarding the second part of the comment, yes, I could move the isp acquire bit\n> into the for (unicamDevice->entities()) loop to optimise this a bit.\n> \n> > > + }\n> > >\n> > > - int ret = registerCamera(unicamDevice, ispDevice, entity);\n> > > - if (ret)\n> > > - LOG(RPI, Error) << \"Failed to register camera \"\n> > > - << entity->name() << \": \" << ret;\n> > > - else\n> > > - numCameras++;\n> > > + /*\n> > > + * The loop below is used to register multiple cameras behind one or more\n> > > + * video mux devices that are attached to a particular Unicam instance.\n> > > + * Obviously these cameras cannot be used simultaneously.\n> > > + */\n> > > + unsigned int numCameras = 0;\n> > > + for (MediaEntity *entity : unicamDevice->entities()) {\n> > > + if (entity->function() != MEDIA_ENT_F_CAM_SENSOR)\n> > > + continue;\n> > > +\n> > > + int ret = registerCamera(unicamDevice, ispDevice, entity);\n> > > + if (ret)\n> > > + LOG(RPI, Error) << \"Failed to register camera \"\n> > > + << entity->name() << \": \" << ret;\n> > > + else\n> > > + numCameras++;\n> > > + }\n> > > +\n> > > + if (numCameras)\n> > > + return true;\n> > > }\n> > >\n> > > - return !!numCameras;\n> > > + return false;\n> > > }\n> > >\n> > > void PipelineHandlerRPi::releaseDevice(Camera *camera)","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 6C40DBDE17\n\tfor ;\n\tThu, 9 Mar 2023 10:03:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C5137626CA;\n\tThu, 9 Mar 2023 11:03:00 +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 973E9626C8\n\tfor ;\n\tThu, 9 Mar 2023 11:02:58 +0100 (CET)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1F1CA589;\n\tThu, 9 Mar 2023 11:02:58 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1678356180;\n\tbh=Cg4seOAocmkcJvWHkg2lctTqARXUw7V3RRmua5K7jV8=;\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=Z0uXrEGrGkzw5yTBtIBpxxrQvaYhW8nE9U9oSfsSa56UPpZEG5smMhKCJ5HarHgB7\n\tcZpN6Qol/MTYaEZBHHzZyZUOr4xvIZaTX0tmJ+5dWIRbUhWcRPYspmFBJqau+/Acqm\n\thyaoCuf/4nHPcCO4NLH91epVMfoiUnRfQ/TKbLC6JMZZKKF323dbFew7Fm8C8e9I9S\n\tIWGX6K45YTY4eezPUc7akJL4tNsMi8I7mwHrmi0mVlp8qpE2vXg6W1OQqvCcYxisPI\n\tchR2Fye7OO06Uy60On8+PywvOr2W6qQwx+Kxm+aSuBJhr7E6L9vhIuEAf1+V5w+ccI\n\tHDN2mvOdBGnNw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1678356178;\n\tbh=Cg4seOAocmkcJvWHkg2lctTqARXUw7V3RRmua5K7jV8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RFhKq854+Snlw0mpl6H7lyZUdHrXUkHjICKt2QTddEP5uML7nIk2Eba1JpRK+10p/\n\twc44buQo9JiBFezf7cuR6nytpZwYkjgXXolbtdWOXBCasQFWY/bdMjd2ieOXWUnyu/\n\tASleGHGUzlv4sP6AVFvE87mYp/xCXxu7gFqF/tY0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"RFhKq854\"; dkim-atps=neutral","Date":"Thu, 9 Mar 2023 12:03:01 +0200","To":"Naushir Patuck ","Message-ID":"<20230309100301.GN31765@pendragon.ideasonboard.com>","References":"\n\t<20230309011431.GL31765@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Iterate over\n\tall Unicam instances in match()","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":26617,"web_url":"https://patchwork.libcamera.org/comment/26617/","msgid":"","date":"2023-03-10T10:01:08","subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Iterate over\n\tall Unicam instances in match()","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Thu, 9 Mar 2023 at 10:02, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Thu, Mar 09, 2023 at 08:04:47AM +0000, Naushir Patuck wrote:\n> > On Thu, 9 Mar 2023 at 01:14, Laurent Pinchart wrote:\n> >\n> > > Hi Naush,\n> > >\n> > > Thank you for the patch.\n> > >\n> > > I know this has been merged, but I've noticed a few issues, which can\n> be\n> > > fixed in further patches.\n> > >\n> > > On Fri, Feb 24, 2023 at 07:30:23AM +0000, Naushir Patuck via\n> libcamera-devel wrote:\n> > > >\n> > > > On Raspberry Pi Compute Module platforms, it is possible to attach a\n> > > > single camera device only to the secondary Unicam port. The current\n> > > > logic of PipelineHandlerRPi::match() will return a failure during\n> > > > enumeration of the first Unicam media device (due to no sensor\n> attached,\n> > > > or sensor failure) and thus the second Unicam media device will\n> never be\n> > > > enumerated.\n> > > >\n> > > > Fix this by looping over all Unicam instances in\n> PipelineHandlerRPi::match()\n> > > > until a camera is correctly registered, or return a failure\n> otherwise.\n> > > >\n> > > > Reported-on: https://github.com/raspberrypi/libcamera/issues/44\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 67\n> +++++++++++--------\n> > > > 1 file changed, 40 insertions(+), 27 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index 841209548350..ef01b7e166ba 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -1246,41 +1246,54 @@ int\n> PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > > >\n> > > > bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > > {\n> > > > - DeviceMatch unicam(\"unicam\");\n> > > > - MediaDevice *unicamDevice = acquireMediaDevice(enumerator,\n> unicam);\n> > > > + constexpr unsigned int numUnicamDevices = 2;\n> > >\n> > > Constants should start with a k prefix, that is kNumUnicamDevices.\n> > >\n> > > >\n> > > > - if (!unicamDevice) {\n> > > > - LOG(RPI, Debug) << \"Unable to acquire a Unicam\n> instance\";\n> > > > - return false;\n> > > > - }\n> > > > + /*\n> > > > + * Loop over all Unicam instances, but return out once a match\n> is found.\n> > > > + * This is to ensure we correctly enumrate the camera when an\n> instance\n> > > > + * of Unicam has registered with media controller, but has not\n> registered\n> > > > + * device nodes due to a sensor subdevice failure.\n> > > > + */\n> > > > + for (unsigned int i = 0; i < numUnicamDevices; i++) {\n> > > > + DeviceMatch unicam(\"unicam\");\n> > > > + MediaDevice *unicamDevice =\n> acquireMediaDevice(enumerator, unicam);\n> > > >\n> > > > - DeviceMatch isp(\"bcm2835-isp\");\n> > > > - MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);\n> > > > + if (!unicamDevice) {\n> > > > + LOG(RPI, Debug) << \"Unable to acquire a Unicam\n> instance\";\n> > > > + continue;\n> > >\n> > > This looks weird, if the unicam device can't be acquired, I don't see\n> > > how the next iteration of the loop could successfully acquire another\n> > > instance. I would thus break here.\n> >\n> > This is probably me not understanding how the media device enumeration\n> stuff\n> > works, but I thought the continue would be needed for situations where\n> we want\n> > simultaneous dual cameras running in separate processes. For example,\n> process 0\n> > acquires \"Unicam 0\" and starts running as normal. Process 1 starts and\n> goes\n> > through match() where \"Unicam 0\" still exists in the entity list, but\n> fails to\n> > acquire because it is locked by process 0. So we have to move on to\n> \"Unicam 1\"\n> > which is acquired correctly for process 1. Is that understanding wrong?\n>\n> The minimal inter-process locking support in libcamera only operates\n> when trying to acquire a *camera* with Camera::acquire(). The\n> acquireMediaDevice() function is a bit confusing, its name refers to the\n> pipeline handler acquiring a MediaDevice from the DeviceEnumerator,\n> guaranteeing that the pipeline handler gets ownership of the media\n> device and no other pipeline handler *in the same process* will be able\n> to acquire it. Two processes running libcamera will both get \"Unicam 0\"\n> in the first iteration of the loop.\n>\n\nFor my clarification, so process 1 will still acquire Unicam 0, but when it\ncomes to camera.start(), will fail since process 0 will have Unicam 0\nrunning in\nits process. Is that right?\n\nNaush\n\n\n> > > > + }\n> > > >\n> > > > - if (!ispDevice) {\n> > > > - LOG(RPI, Debug) << \"Unable to acquire ISP instance\";\n> > > > - return false;\n> > > > - }\n> > > > + DeviceMatch isp(\"bcm2835-isp\");\n> > > > + MediaDevice *ispDevice =\n> acquireMediaDevice(enumerator, isp);\n> > > >\n> > > > - /*\n> > > > - * The loop below is used to register multiple cameras behind\n> one or more\n> > > > - * video mux devices that are attached to a particular Unicam\n> instance.\n> > > > - * Obviously these cameras cannot be used simultaneously.\n> > > > - */\n> > > > - unsigned int numCameras = 0;\n> > > > - for (MediaEntity *entity : unicamDevice->entities()) {\n> > > > - if (entity->function() != MEDIA_ENT_F_CAM_SENSOR)\n> > > > + if (!ispDevice) {\n> > > > + LOG(RPI, Debug) << \"Unable to acquire ISP\n> instance\";\n> > > > continue;\n> > >\n> > > Shouldn't you release the unicam device in this case ? I think it would\n> > > be better to first loop over unicam instances, ignoring any instance\n> > > than has no connected camera sensor, and then, if an instance with a\n> > > connected sensor is found, acquire an ISP instance.\n> >\n> > I think we discussed this briefly in the github comments. There is no\n> > compliment releaseMediaDevice() call that I can use to release the\n> device.\n>\n> It's an issue indeed. The design idea was to release all acquired media\n> devices automatically when the match() function returns false, but that\n> doesn't allow releasing media device that have been acquired and turned\n> out not to be needed.\n>\n> In this specific case, if you acquire a unicam instance that has no\n> connected sensor, it's fine if it stays acquired as no other pipeline\n> handler instance would be able to use it for a meaningful purpose\n> anyway, but in general this is something we should probably fix.\n>\n> > Regarding the second part of the comment, yes, I could move the isp\n> acquire bit\n> > into the for (unicamDevice->entities()) loop to optimise this a bit.\n> >\n> > > > + }\n> > > >\n> > > > - int ret = registerCamera(unicamDevice, ispDevice,\n> entity);\n> > > > - if (ret)\n> > > > - LOG(RPI, Error) << \"Failed to register camera \"\n> > > > - << entity->name() << \": \" <<\n> ret;\n> > > > - else\n> > > > - numCameras++;\n> > > > + /*\n> > > > + * The loop below is used to register multiple cameras\n> behind one or more\n> > > > + * video mux devices that are attached to a particular\n> Unicam instance.\n> > > > + * Obviously these cameras cannot be used\n> simultaneously.\n> > > > + */\n> > > > + unsigned int numCameras = 0;\n> > > > + for (MediaEntity *entity : unicamDevice->entities()) {\n> > > > + if (entity->function() !=\n> MEDIA_ENT_F_CAM_SENSOR)\n> > > > + continue;\n> > > > +\n> > > > + int ret = registerCamera(unicamDevice,\n> ispDevice, entity);\n> > > > + if (ret)\n> > > > + LOG(RPI, Error) << \"Failed to register\n> camera \"\n> > > > + << entity->name() <<\n> \": \" << ret;\n> > > > + else\n> > > > + numCameras++;\n> > > > + }\n> > > > +\n> > > > + if (numCameras)\n> > > > + return true;\n> > > > }\n> > > >\n> > > > - return !!numCameras;\n> > > > + return false;\n> > > > }\n> > > >\n> > > > void PipelineHandlerRPi::releaseDevice(Camera *camera)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 BD306BD80A\n\tfor ;\n\tFri, 10 Mar 2023 10:01:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1D225626CA;\n\tFri, 10 Mar 2023 11:01:24 +0100 (CET)","from mail-yb1-xb2a.google.com (mail-yb1-xb2a.google.com\n\t[IPv6:2607:f8b0:4864:20::b2a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A2EC462661\n\tfor ;\n\tFri, 10 Mar 2023 11:01:22 +0100 (CET)","by mail-yb1-xb2a.google.com with SMTP id o199so2858253ybc.7\n\tfor ;\n\tFri, 10 Mar 2023 02:01:22 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1678442484;\n\tbh=6lo3NtjcZRuYzKtgl2rbejQ6uHvgAQN6+Hx5AQhPvaQ=;\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=2UnL/fQ+SbmHNmy69NSd8VKTduv62L+RId3z4b9BU7+8hPeFBaWGUEHNEzhB+ndx6\n\t7+tRN6MoINCwmeStK8TnqulJNMNcgDR1BF1WHkWFd1F7lU3QiW4MtLvaAvAEL0WIfn\n\tC2YNtH+DQ73TEOJKvyJELdXjt3BvaCcaq/+KLKYPjv+q9cxyZKuFI7sXO1KpXp2+qJ\n\tNrONzK6xbvqlDnb+s3Bc38kzfS/c4Bkhq3X0tSD973Zt4bBq+o+lX4squXoiKZtRoy\n\td/wllEG7alQLsEjtMJ0QOo+5xbX5hfTW2b7cQNb0Zhw395TVqkeGE5HQEE36RXTPYM\n\t5ZTTXEKlDmxzQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1678442481;\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=cKC9Pn7SMZuADeA3cpO0+IVX5Rz5mtAnLA8RM7FNr1A=;\n\tb=Wy1PYX/HEU4tghEdnXJ5ZLoYgX93PgpavXRoFnAPBOrypnU52Dx62ND2Qdi1LBL630\n\tGum1Q3J8kWXRwVH+WyZGriVq94qI+OBGfR6p01yRDqIE1QPYPAr3lRj2NotkfFFMCSSD\n\tiLphkv4fuUpoj48fMcf3iXaE8X9BI8rEPYuhyJSSmHq3LlTZQ7wV+wWrQtqxQBATvEAH\n\tN/u6eUl7UCRfn/O0qE0I9fsxxSK8XQG+zIJu3ca5PByURBAy4e+fvaBVEylMh3+UOEWU\n\t2HLzK36aUaY6WTohBFTYbKaJIZvrZ/8MVvGfaCuVGEY4KF15OawXviD5M1fqfOM5abzM\n\tfBSQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"Wy1PYX/H\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1678442481;\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=cKC9Pn7SMZuADeA3cpO0+IVX5Rz5mtAnLA8RM7FNr1A=;\n\tb=TQ6VX1WL5gzUlTIkwHqje22k3EuKJkjvZLEyEqBWwRCSww4bhYb/Ol2PlcNMjvMJbR\n\t3XaN10RZ1KSoeCG2n03+o9yKMYV80emzE9v4THq5UKosK5RyjqMnDu1rpFfPvHICCswI\n\tWFW0z3hE8E7d169k8gloDVmF7ZwllVOSW/4p5Ro4jgb0gtjcvDPeNkKY6U2R1AaT7EQc\n\tF+1WWyXbbboIuCADtaNsTlOlKGSgWeNC9xpFHc4uX7/PrYjfTnEALtT4ZsATly48jL5m\n\tFMVx2Xd65NWk3JEiUvhFboTTYTC4ORNAxOjkME6fMio+XgsZWz9j82u7kjluGMK6XpWZ\n\tTXYg==","X-Gm-Message-State":"AO0yUKWVvNpiKUnd9VGsdTVrst7Z9UN0jOhSpcNztgLrsqAM9XCL9GLX\n\t/Vtcek2e4BtKr0QO/4/fG7vVro8RR29Q+n66zat2BCf0V5I3Ue5p91p9Dg==","X-Google-Smtp-Source":"AK7set/2lvjxuwtPGcpSYW9V/yDDU4GFWmU3E0Ec7AzHLiMlNGx8yQQPDdLREd5rRZOnhEXoCm4a4Q9j+KcctEBAF7Q=","X-Received":"by 2002:a05:6902:524:b0:ab8:1ed9:cfc5 with SMTP id\n\ty4-20020a056902052400b00ab81ed9cfc5mr15442715ybs.6.1678442481431;\n\tFri, 10 Mar 2023 02:01:21 -0800 (PST)","MIME-Version":"1.0","References":"\n\t<20230309011431.GL31765@pendragon.ideasonboard.com>\n\t\n\t<20230309100301.GN31765@pendragon.ideasonboard.com>","In-Reply-To":"<20230309100301.GN31765@pendragon.ideasonboard.com>","Date":"Fri, 10 Mar 2023 10:01:08 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000e1d06f05f688d705\"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Iterate over\n\tall Unicam instances in match()","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":26618,"web_url":"https://patchwork.libcamera.org/comment/26618/","msgid":"","date":"2023-03-10T10:22:01","subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Iterate over\n\tall Unicam instances in match()","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"One more thing....\n\nOn Fri, 10 Mar 2023 at 10:01, Naushir Patuck wrote:\n\n> Hi Laurent,\n>\n> On Thu, 9 Mar 2023 at 10:02, Laurent Pinchart <\n> laurent.pinchart@ideasonboard.com> wrote:\n>\n>> Hi Naush,\n>>\n>> On Thu, Mar 09, 2023 at 08:04:47AM +0000, Naushir Patuck wrote:\n>> > On Thu, 9 Mar 2023 at 01:14, Laurent Pinchart wrote:\n>> >\n>> > > Hi Naush,\n>> > >\n>> > > Thank you for the patch.\n>> > >\n>> > > I know this has been merged, but I've noticed a few issues, which can\n>> be\n>> > > fixed in further patches.\n>> > >\n>> > > On Fri, Feb 24, 2023 at 07:30:23AM +0000, Naushir Patuck via\n>> libcamera-devel wrote:\n>> > > >\n>> > > > On Raspberry Pi Compute Module platforms, it is possible to attach a\n>> > > > single camera device only to the secondary Unicam port. The current\n>> > > > logic of PipelineHandlerRPi::match() will return a failure during\n>> > > > enumeration of the first Unicam media device (due to no sensor\n>> attached,\n>> > > > or sensor failure) and thus the second Unicam media device will\n>> never be\n>> > > > enumerated.\n>> > > >\n>> > > > Fix this by looping over all Unicam instances in\n>> PipelineHandlerRPi::match()\n>> > > > until a camera is correctly registered, or return a failure\n>> otherwise.\n>> > > >\n>> > > > Reported-on: https://github.com/raspberrypi/libcamera/issues/44\n>> > > > Signed-off-by: Naushir Patuck \n>> > > > ---\n>> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 67\n>> +++++++++++--------\n>> > > > 1 file changed, 40 insertions(+), 27 deletions(-)\n>> > > >\n>> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> > > > index 841209548350..ef01b7e166ba 100644\n>> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> > > > @@ -1246,41 +1246,54 @@ int\n>> PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n>> > > >\n>> > > > bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n>> > > > {\n>> > > > - DeviceMatch unicam(\"unicam\");\n>> > > > - MediaDevice *unicamDevice = acquireMediaDevice(enumerator,\n>> unicam);\n>> > > > + constexpr unsigned int numUnicamDevices = 2;\n>> > >\n>> > > Constants should start with a k prefix, that is kNumUnicamDevices.\n>> > >\n>> > > >\n>> > > > - if (!unicamDevice) {\n>> > > > - LOG(RPI, Debug) << \"Unable to acquire a Unicam\n>> instance\";\n>> > > > - return false;\n>> > > > - }\n>> > > > + /*\n>> > > > + * Loop over all Unicam instances, but return out once a\n>> match is found.\n>> > > > + * This is to ensure we correctly enumrate the camera when an\n>> instance\n>> > > > + * of Unicam has registered with media controller, but has\n>> not registered\n>> > > > + * device nodes due to a sensor subdevice failure.\n>> > > > + */\n>> > > > + for (unsigned int i = 0; i < numUnicamDevices; i++) {\n>> > > > + DeviceMatch unicam(\"unicam\");\n>> > > > + MediaDevice *unicamDevice =\n>> acquireMediaDevice(enumerator, unicam);\n>> > > >\n>> > > > - DeviceMatch isp(\"bcm2835-isp\");\n>> > > > - MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);\n>> > > > + if (!unicamDevice) {\n>> > > > + LOG(RPI, Debug) << \"Unable to acquire a\n>> Unicam instance\";\n>> > > > + continue;\n>> > >\n>> > > This looks weird, if the unicam device can't be acquired, I don't see\n>> > > how the next iteration of the loop could successfully acquire another\n>> > > instance. I would thus break here.\n>> >\n>> > This is probably me not understanding how the media device enumeration\n>> stuff\n>> > works, but I thought the continue would be needed for situations where\n>> we want\n>> > simultaneous dual cameras running in separate processes. For example,\n>> process 0\n>> > acquires \"Unicam 0\" and starts running as normal. Process 1 starts and\n>> goes\n>> > through match() where \"Unicam 0\" still exists in the entity list, but\n>> fails to\n>> > acquire because it is locked by process 0. So we have to move on to\n>> \"Unicam 1\"\n>> > which is acquired correctly for process 1. Is that understanding wrong?\n>>\n>> The minimal inter-process locking support in libcamera only operates\n>> when trying to acquire a *camera* with Camera::acquire(). The\n>> acquireMediaDevice() function is a bit confusing, its name refers to the\n>> pipeline handler acquiring a MediaDevice from the DeviceEnumerator,\n>> guaranteeing that the pipeline handler gets ownership of the media\n>> device and no other pipeline handler *in the same process* will be able\n>> to acquire it. Two processes running libcamera will both get \"Unicam 0\"\n>> in the first iteration of the loop.\n>>\n>\n> For my clarification, so process 1 will still acquire Unicam 0, but when it\n> comes to camera.start(), will fail since process 0 will have Unicam 0\n> running in\n> its process. Is that right?\n>\n> Naush\n>\n>\n>> > > > + }\n>> > > >\n>> > > > - if (!ispDevice) {\n>> > > > - LOG(RPI, Debug) << \"Unable to acquire ISP instance\";\n>> > > > - return false;\n>> > > > - }\n>> > > > + DeviceMatch isp(\"bcm2835-isp\");\n>> > > > + MediaDevice *ispDevice =\n>> acquireMediaDevice(enumerator, isp);\n>> > > >\n>> > > > - /*\n>> > > > - * The loop below is used to register multiple cameras behind\n>> one or more\n>> > > > - * video mux devices that are attached to a particular Unicam\n>> instance.\n>> > > > - * Obviously these cameras cannot be used simultaneously.\n>> > > > - */\n>> > > > - unsigned int numCameras = 0;\n>> > > > - for (MediaEntity *entity : unicamDevice->entities()) {\n>> > > > - if (entity->function() != MEDIA_ENT_F_CAM_SENSOR)\n>> > > > + if (!ispDevice) {\n>> > > > + LOG(RPI, Debug) << \"Unable to acquire ISP\n>> instance\";\n>> > > > continue;\n>> > >\n>> > > Shouldn't you release the unicam device in this case ? I think it\n>> would\n>> > > be better to first loop over unicam instances, ignoring any instance\n>> > > than has no connected camera sensor, and then, if an instance with a\n>> > > connected sensor is found, acquire an ISP instance.\n>> >\n>\n>\nActually, moving the ISP acquire into the inner loop would be the wrong\nthing to\ndo. The inner loop is to identify multiple cameras attached to a single\nUnicam\nport through a mux/bridge chip. As such, only a single sensor can be\nactive at\nany time, and only a single ISP can service them. So I think the ISP\nacquiring\ncode is in the right place.\n\nNaush\n\n\n>\n>> > I think we discussed this briefly in the github comments. There is no\n>> > compliment releaseMediaDevice() call that I can use to release the\n>> device.\n>>\n>> It's an issue indeed. The design idea was to release all acquired media\n>> devices automatically when the match() function returns false, but that\n>> doesn't allow releasing media device that have been acquired and turned\n>> out not to be needed.\n>>\n>> In this specific case, if you acquire a unicam instance that has no\n>> connected sensor, it's fine if it stays acquired as no other pipeline\n>> handler instance would be able to use it for a meaningful purpose\n>> anyway, but in general this is something we should probably fix.\n>>\n>> > Regarding the second part of the comment, yes, I could move the isp\n>> acquire bit\n>> > into the for (unicamDevice->entities()) loop to optimise this a bit.\n>>\n> >\n>> > > > + }\n>> > > >\n>> > > > - int ret = registerCamera(unicamDevice, ispDevice,\n>> entity);\n>> > > > - if (ret)\n>> > > > - LOG(RPI, Error) << \"Failed to register camera\n>> \"\n>> > > > - << entity->name() << \": \" <<\n>> ret;\n>> > > > - else\n>> > > > - numCameras++;\n>> > > > + /*\n>> > > > + * The loop below is used to register multiple\n>> cameras behind one or more\n>> > > > + * video mux devices that are attached to a\n>> particular Unicam instance.\n>> > > > + * Obviously these cameras cannot be used\n>> simultaneously.\n>> > > > + */\n>> > > > + unsigned int numCameras = 0;\n>> > > > + for (MediaEntity *entity : unicamDevice->entities()) {\n>> > > > + if (entity->function() !=\n>> MEDIA_ENT_F_CAM_SENSOR)\n>> > > > + continue;\n>> > > > +\n>> > > > + int ret = registerCamera(unicamDevice,\n>> ispDevice, entity);\n>> > > > + if (ret)\n>> > > > + LOG(RPI, Error) << \"Failed to\n>> register camera \"\n>> > > > + << entity->name() <<\n>> \": \" << ret;\n>> > > > + else\n>> > > > + numCameras++;\n>> > > > + }\n>> > > > +\n>> > > > + if (numCameras)\n>> > > > + return true;\n>> > > > }\n>> > > >\n>> > > > - return !!numCameras;\n>> > > > + return false;\n>> > > > }\n>> > > >\n>> > > > void PipelineHandlerRPi::releaseDevice(Camera *camera)\n>>\n>> --\n>> Regards,\n>>\n>> Laurent Pinchart\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 249F8BE080\n\tfor ;\n\tFri, 10 Mar 2023 10:22:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 82002626CA;\n\tFri, 10 Mar 2023 11:22:16 +0100 (CET)","from mail-yb1-xb2a.google.com (mail-yb1-xb2a.google.com\n\t[IPv6:2607:f8b0:4864:20::b2a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5FFDA62661\n\tfor ;\n\tFri, 10 Mar 2023 11:22:15 +0100 (CET)","by mail-yb1-xb2a.google.com with SMTP id n18so4740842ybm.10\n\tfor ;\n\tFri, 10 Mar 2023 02:22:15 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1678443736;\n\tbh=uIwSstfIQ9xBHoR6mNyds+WBRUtNq0XPvqLlDsq4Cfo=;\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=Y1N2W1snyxhw77gEDArXFnFkLaBf1RCRQzXvVdfS2sQ8nNcv9y1pzPffgWKrGlwys\n\tTHDiFLHagBr7fVF2Ka62r8BPBOQ+CtffDChFyO2dXnc0SqAd6dx6XBDlcfTFPtBVge\n\tV9/G05F9/FUJrZtr4SAf2ws6vCENWlD37lXnfI2eGO5s3HDwndkzgjxe7t/Ez3R9Tw\n\tmySBg8DpjQVuch0Bp4Anrv0JXPEyAHU/3AtN24jgzwU58SgjxZkQoTDBA/X2XVaLF1\n\tMCwqqwlEyU/VLm7fl4vygxQ1pLIpni+IDFS/NuZ149AdBEbBGUcOUhoQvtnmHxhcvP\n\tIqSEXuzVWCUgA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1678443734;\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=DkkLl4JZLKkgEdGGDH6Q12o6femzA4NhaH6NXhN1RaE=;\n\tb=quGa1S8oj43a3Z8Dz3eeErVO+LrJdQ9M22gXACx/zeHcyfq/uM+Xi5uhK0/H/vTWtf\n\tIP8p/lBw670aij4XjeP3WIsohAPiCiNDfxL+9JSvmu5EDWKCaCBBaJv2unihwNvhmciX\n\tctkeHr8p79x2IBppCzcL33+rNTk7uGlCEJ1U2X98IDRfS8DzyHezu1R4eBTRIVmniwNF\n\tP5m9LQZs6BaXvY1zZu08+pCzkZHPPXNszXiN0Xf4UWajRjwOwxN9Gj0ryPnbBWrNaCq3\n\tJ2JWIvutN7HpCbHXXeZ5OKFS2lVuHLYrCBbkPDPSGMeKxtnrs8tkdRkw2bkIAIBWJv5o\n\tPkvA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"quGa1S8o\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1678443734;\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=DkkLl4JZLKkgEdGGDH6Q12o6femzA4NhaH6NXhN1RaE=;\n\tb=QApQZsALH4u4EwIsE0wpBZFvXJ7JLWg0zKIYVYB/VQkl2dOy4DuRrFzklPmPHCRpXh\n\tW1GRaW3zuCVgBwOghGts2R1+JW6v3MYfL1n9KmOVZaNCi3vo0Fv2lcUG23/ZEbuBLHfu\n\tl642nSrRXJSEzvR3+K+3WcZ/WpRZxxSSAfJQjUi1Sug+RH4M1jmFeS1paruPN/8g3+AY\n\tQxyIzsvQ/WPn4yfvolpgAe7GgbfGCdA1zNCE7tdoVYxXd8w9xCZbWgdW74jGVMQ5GvUa\n\tMFmIiJk3N+JqeWIsch3CGzlJZDh28u/XaswTge38Xy8LtE2eOUHyt6drX+2yHY594of3\n\tDUIQ==","X-Gm-Message-State":"AO0yUKUSXXEDYRkFvC84WXpMpRHdW6nTowNyLdkFdTHp0CXgfvBBrJJm\n\tilbt+VHs1mH6IkMPAjgp+wxZv7Xqp+YVeJVM6ASY7NP3juihA7p5PTc75w==","X-Google-Smtp-Source":"AK7set+0fBK4nfudKonj/cQiAd3ScvSVVdBi+pietUAyfior83VPSPLWpx72OEY7odFwzj30t6hmZkP3GZSbVbpK+bQ=","X-Received":"by 2002:a05:6902:524:b0:ab8:1ed9:cfc5 with SMTP id\n\ty4-20020a056902052400b00ab81ed9cfc5mr15477790ybs.6.1678443734078;\n\tFri, 10 Mar 2023 02:22:14 -0800 (PST)","MIME-Version":"1.0","References":"\n\t<20230309011431.GL31765@pendragon.ideasonboard.com>\n\t\n\t<20230309100301.GN31765@pendragon.ideasonboard.com>\n\t","In-Reply-To":"","Date":"Fri, 10 Mar 2023 10:22:01 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000008ba68e05f68922db\"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Iterate over\n\tall Unicam instances in match()","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":26619,"web_url":"https://patchwork.libcamera.org/comment/26619/","msgid":"<20230310102836.GA5342@pendragon.ideasonboard.com>","date":"2023-03-10T10:28:36","subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Iterate over\n\tall Unicam instances in match()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Mar 10, 2023 at 10:01:08AM +0000, Naushir Patuck wrote:\n> On Thu, 9 Mar 2023 at 10:02, Laurent Pinchart wrote:\n> > On Thu, Mar 09, 2023 at 08:04:47AM +0000, Naushir Patuck wrote:\n> > > On Thu, 9 Mar 2023 at 01:14, Laurent Pinchart wrote:\n> > >\n> > > > Hi Naush,\n> > > >\n> > > > Thank you for the patch.\n> > > >\n> > > > I know this has been merged, but I've noticed a few issues, which can be\n> > > > fixed in further patches.\n> > > >\n> > > > On Fri, Feb 24, 2023 at 07:30:23AM +0000, Naushir Patuck via libcamera-devel wrote:\n> > > > >\n> > > > > On Raspberry Pi Compute Module platforms, it is possible to attach a\n> > > > > single camera device only to the secondary Unicam port. The current\n> > > > > logic of PipelineHandlerRPi::match() will return a failure during\n> > > > > enumeration of the first Unicam media device (due to no sensor attached,\n> > > > > or sensor failure) and thus the second Unicam media device will never be\n> > > > > enumerated.\n> > > > >\n> > > > > Fix this by looping over all Unicam instances in PipelineHandlerRPi::match()\n> > > > > until a camera is correctly registered, or return a failure otherwise.\n> > > > >\n> > > > > Reported-on: https://github.com/raspberrypi/libcamera/issues/44\n> > > > > Signed-off-by: Naushir Patuck \n> > > > > ---\n> > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 67 +++++++++++--------\n> > > > > 1 file changed, 40 insertions(+), 27 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > index 841209548350..ef01b7e166ba 100644\n> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > @@ -1246,41 +1246,54 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > > > >\n> > > > > bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > > > {\n> > > > > - DeviceMatch unicam(\"unicam\");\n> > > > > - MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);\n> > > > > + constexpr unsigned int numUnicamDevices = 2;\n> > > >\n> > > > Constants should start with a k prefix, that is kNumUnicamDevices.\n> > > >\n> > > > >\n> > > > > - if (!unicamDevice) {\n> > > > > - LOG(RPI, Debug) << \"Unable to acquire a Unicam instance\";\n> > > > > - return false;\n> > > > > - }\n> > > > > + /*\n> > > > > + * Loop over all Unicam instances, but return out once a match is found.\n> > > > > + * This is to ensure we correctly enumrate the camera when an instance\n> > > > > + * of Unicam has registered with media controller, but has not registered\n> > > > > + * device nodes due to a sensor subdevice failure.\n> > > > > + */\n> > > > > + for (unsigned int i = 0; i < numUnicamDevices; i++) {\n> > > > > + DeviceMatch unicam(\"unicam\");\n> > > > > + MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);\n> > > > >\n> > > > > - DeviceMatch isp(\"bcm2835-isp\");\n> > > > > - MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);\n> > > > > + if (!unicamDevice) {\n> > > > > + LOG(RPI, Debug) << \"Unable to acquire a Unicam instance\";\n> > > > > + continue;\n> > > >\n> > > > This looks weird, if the unicam device can't be acquired, I don't see\n> > > > how the next iteration of the loop could successfully acquire another\n> > > > instance. I would thus break here.\n> > >\n> > > This is probably me not understanding how the media device enumeration stuff\n> > > works, but I thought the continue would be needed for situations where we want\n> > > simultaneous dual cameras running in separate processes. For example, process 0\n> > > acquires \"Unicam 0\" and starts running as normal. Process 1 starts and goes\n> > > through match() where \"Unicam 0\" still exists in the entity list, but fails to\n> > > acquire because it is locked by process 0. So we have to move on to \"Unicam 1\"\n> > > which is acquired correctly for process 1. Is that understanding wrong?\n> >\n> > The minimal inter-process locking support in libcamera only operates\n> > when trying to acquire a *camera* with Camera::acquire(). The\n> > acquireMediaDevice() function is a bit confusing, its name refers to the\n> > pipeline handler acquiring a MediaDevice from the DeviceEnumerator,\n> > guaranteeing that the pipeline handler gets ownership of the media\n> > device and no other pipeline handler *in the same process* will be able\n> > to acquire it. Two processes running libcamera will both get \"Unicam 0\"\n> > in the first iteration of the loop.\n> \n> For my clarification, so process 1 will still acquire Unicam 0, but when it\n> comes to camera.start(), will fail since process 0 will have Unicam 0 running in\n> its process. Is that right?\n\nNearly. Both processes will acquire the same unicam instances in\nmatch(), being completely unaware of each other. It is only\nCamera::acquire() (not Camera::start()) that has basic inter-process\nlocking.\n\n> > > > > + }\n> > > > >\n> > > > > - if (!ispDevice) {\n> > > > > - LOG(RPI, Debug) << \"Unable to acquire ISP instance\";\n> > > > > - return false;\n> > > > > - }\n> > > > > + DeviceMatch isp(\"bcm2835-isp\");\n> > > > > + MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);\n> > > > >\n> > > > > - /*\n> > > > > - * The loop below is used to register multiple cameras behind one or more\n> > > > > - * video mux devices that are attached to a particular Unicam instance.\n> > > > > - * Obviously these cameras cannot be used simultaneously.\n> > > > > - */\n> > > > > - unsigned int numCameras = 0;\n> > > > > - for (MediaEntity *entity : unicamDevice->entities()) {\n> > > > > - if (entity->function() != MEDIA_ENT_F_CAM_SENSOR)\n> > > > > + if (!ispDevice) {\n> > > > > + LOG(RPI, Debug) << \"Unable to acquire ISP instance\";\n> > > > > continue;\n> > > >\n> > > > Shouldn't you release the unicam device in this case ? I think it would\n> > > > be better to first loop over unicam instances, ignoring any instance\n> > > > than has no connected camera sensor, and then, if an instance with a\n> > > > connected sensor is found, acquire an ISP instance.\n> > >\n> > > I think we discussed this briefly in the github comments. There is no\n> > > compliment releaseMediaDevice() call that I can use to release the device.\n> >\n> > It's an issue indeed. The design idea was to release all acquired media\n> > devices automatically when the match() function returns false, but that\n> > doesn't allow releasing media device that have been acquired and turned\n> > out not to be needed.\n> >\n> > In this specific case, if you acquire a unicam instance that has no\n> > connected sensor, it's fine if it stays acquired as no other pipeline\n> > handler instance would be able to use it for a meaningful purpose\n> > anyway, but in general this is something we should probably fix.\n> >\n> > > Regarding the second part of the comment, yes, I could move the isp acquire bit\n> > > into the for (unicamDevice->entities()) loop to optimise this a bit.\n> > >\n> > > > > + }\n> > > > >\n> > > > > - int ret = registerCamera(unicamDevice, ispDevice, entity);\n> > > > > - if (ret)\n> > > > > - LOG(RPI, Error) << \"Failed to register camera \"\n> > > > > - << entity->name() << \": \" << ret;\n> > > > > - else\n> > > > > - numCameras++;\n> > > > > + /*\n> > > > > + * The loop below is used to register multiple cameras behind one or more\n> > > > > + * video mux devices that are attached to a particular Unicam instance.\n> > > > > + * Obviously these cameras cannot be used simultaneously.\n> > > > > + */\n> > > > > + unsigned int numCameras = 0;\n> > > > > + for (MediaEntity *entity : unicamDevice->entities()) {\n> > > > > + if (entity->function() != MEDIA_ENT_F_CAM_SENSOR)\n> > > > > + continue;\n> > > > > +\n> > > > > + int ret = registerCamera(unicamDevice, ispDevice, entity);\n> > > > > + if (ret)\n> > > > > + LOG(RPI, Error) << \"Failed to register camera \"\n> > > > > + << entity->name() << \": \" << ret;\n> > > > > + else\n> > > > > + numCameras++;\n> > > > > + }\n> > > > > +\n> > > > > + if (numCameras)\n> > > > > + return true;\n> > > > > }\n> > > > >\n> > > > > - return !!numCameras;\n> > > > > + return false;\n> > > > > }\n> > > > >\n> > > > > void PipelineHandlerRPi::releaseDevice(Camera *camera)","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 227FEBD80A\n\tfor ;\n\tFri, 10 Mar 2023 10:28:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D523626CA;\n\tFri, 10 Mar 2023 11:28:35 +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 9E4F162661\n\tfor ;\n\tFri, 10 Mar 2023 11:28:33 +0100 (CET)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3A849EF;\n\tFri, 10 Mar 2023 11:28:33 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1678444115;\n\tbh=OJPcEk0RtTCUKzug5SPUxNmkxwiRDiID9a5yQV2a/qE=;\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=jWnJoqWK7GQCsiQ3vLblgujqBaE+L4cxryvjIQOA/A+2ILR6dcVY3ml3U9R1Zq2yC\n\t26C1g91MCCuFIME7e0P6o3iGY2XGa7Wr3GmpQl071fYc+cvpKHe7f0mXtgwUHxbOij\n\tblEXSUuhR89+DCisZ38N73g1mRG67xJtWq3rk9AdYm4gwid8yd5KG6OGVT9z3kFHg3\n\tKoml1qBKxTNKJPo4Cs/fWvv54WVWVG24AOVnDSd3SZ7r3H0knL4WDW4N/wC9FenLz8\n\tlogK1DWSMi6ykpkKpBqV+XXpB/zkwk48gkBuiJUAkoOh4TDEolOqarv0Hdjt1W5etk\n\tL2DOZrbOwymoQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1678444113;\n\tbh=OJPcEk0RtTCUKzug5SPUxNmkxwiRDiID9a5yQV2a/qE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eDXHLOmkeYjF6SVcBd3NtmUzukrJR5pml+ssJMJ4AP2j+ndico26XU5rKO7SSHfkY\n\thM/btAxl0OZqUwNIrJxh0mFRSteA3ybxVq3oIyRF9eJRg7t90ZUL3Y8q3buwC1mH4A\n\tVu853pnEhCigccaetjFxMZUXv7GUF+Awy1LXhELc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"eDXHLOmk\"; dkim-atps=neutral","Date":"Fri, 10 Mar 2023 12:28:36 +0200","To":"Naushir Patuck ","Message-ID":"<20230310102836.GA5342@pendragon.ideasonboard.com>","References":"\n\t<20230309011431.GL31765@pendragon.ideasonboard.com>\n\t\n\t<20230309100301.GN31765@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Iterate over\n\tall Unicam instances in match()","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":26620,"web_url":"https://patchwork.libcamera.org/comment/26620/","msgid":"<20230310103321.GB5342@pendragon.ideasonboard.com>","date":"2023-03-10T10:33:21","subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Iterate over\n\tall Unicam instances in match()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Fri, Mar 10, 2023 at 10:22:01AM +0000, Naushir Patuck wrote:\n> One more thing....\n> \n> On Fri, 10 Mar 2023 at 10:01, Naushir Patuck wrote:\n> > On Thu, 9 Mar 2023 at 10:02, Laurent Pinchart wrote:\n> >> On Thu, Mar 09, 2023 at 08:04:47AM +0000, Naushir Patuck wrote:\n> >> > On Thu, 9 Mar 2023 at 01:14, Laurent Pinchart wrote:\n> >> >\n> >> > > Hi Naush,\n> >> > >\n> >> > > Thank you for the patch.\n> >> > >\n> >> > > I know this has been merged, but I've noticed a few issues, which can be\n> >> > > fixed in further patches.\n> >> > >\n> >> > > On Fri, Feb 24, 2023 at 07:30:23AM +0000, Naushir Patuck via libcamera-devel wrote:\n> >> > > >\n> >> > > > On Raspberry Pi Compute Module platforms, it is possible to attach a\n> >> > > > single camera device only to the secondary Unicam port. The current\n> >> > > > logic of PipelineHandlerRPi::match() will return a failure during\n> >> > > > enumeration of the first Unicam media device (due to no sensor attached,\n> >> > > > or sensor failure) and thus the second Unicam media device will never be\n> >> > > > enumerated.\n> >> > > >\n> >> > > > Fix this by looping over all Unicam instances in PipelineHandlerRPi::match()\n> >> > > > until a camera is correctly registered, or return a failure otherwise.\n> >> > > >\n> >> > > > Reported-on: https://github.com/raspberrypi/libcamera/issues/44\n> >> > > > Signed-off-by: Naushir Patuck \n> >> > > > ---\n> >> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 67 +++++++++++--------\n> >> > > > 1 file changed, 40 insertions(+), 27 deletions(-)\n> >> > > >\n> >> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> > > > index 841209548350..ef01b7e166ba 100644\n> >> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> > > > @@ -1246,41 +1246,54 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> >> > > >\n> >> > > > bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> >> > > > {\n> >> > > > - DeviceMatch unicam(\"unicam\");\n> >> > > > - MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);\n> >> > > > + constexpr unsigned int numUnicamDevices = 2;\n> >> > >\n> >> > > Constants should start with a k prefix, that is kNumUnicamDevices.\n> >> > >\n> >> > > >\n> >> > > > - if (!unicamDevice) {\n> >> > > > - LOG(RPI, Debug) << \"Unable to acquire a Unicam instance\";\n> >> > > > - return false;\n> >> > > > - }\n> >> > > > + /*\n> >> > > > + * Loop over all Unicam instances, but return out once a match is found.\n> >> > > > + * This is to ensure we correctly enumrate the camera when an instance\n> >> > > > + * of Unicam has registered with media controller, but has not registered\n> >> > > > + * device nodes due to a sensor subdevice failure.\n> >> > > > + */\n> >> > > > + for (unsigned int i = 0; i < numUnicamDevices; i++) {\n> >> > > > + DeviceMatch unicam(\"unicam\");\n> >> > > > + MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);\n> >> > > >\n> >> > > > - DeviceMatch isp(\"bcm2835-isp\");\n> >> > > > - MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);\n> >> > > > + if (!unicamDevice) {\n> >> > > > + LOG(RPI, Debug) << \"Unable to acquire a Unicam instance\";\n> >> > > > + continue;\n> >> > >\n> >> > > This looks weird, if the unicam device can't be acquired, I don't see\n> >> > > how the next iteration of the loop could successfully acquire another\n> >> > > instance. I would thus break here.\n> >> >\n> >> > This is probably me not understanding how the media device enumeration stuff\n> >> > works, but I thought the continue would be needed for situations where we want\n> >> > simultaneous dual cameras running in separate processes. For example, process 0\n> >> > acquires \"Unicam 0\" and starts running as normal. Process 1 starts and goes\n> >> > through match() where \"Unicam 0\" still exists in the entity list, but fails to\n> >> > acquire because it is locked by process 0. So we have to move on to \"Unicam 1\"\n> >> > which is acquired correctly for process 1. Is that understanding wrong?\n> >>\n> >> The minimal inter-process locking support in libcamera only operates\n> >> when trying to acquire a *camera* with Camera::acquire(). The\n> >> acquireMediaDevice() function is a bit confusing, its name refers to the\n> >> pipeline handler acquiring a MediaDevice from the DeviceEnumerator,\n> >> guaranteeing that the pipeline handler gets ownership of the media\n> >> device and no other pipeline handler *in the same process* will be able\n> >> to acquire it. Two processes running libcamera will both get \"Unicam 0\"\n> >> in the first iteration of the loop.\n> >\n> > For my clarification, so process 1 will still acquire Unicam 0, but when it\n> > comes to camera.start(), will fail since process 0 will have Unicam 0 running in\n> > its process. Is that right?\n> >\n> >> > > > + }\n> >> > > >\n> >> > > > - if (!ispDevice) {\n> >> > > > - LOG(RPI, Debug) << \"Unable to acquire ISP instance\";\n> >> > > > - return false;\n> >> > > > - }\n> >> > > > + DeviceMatch isp(\"bcm2835-isp\");\n> >> > > > + MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);\n> >> > > >\n> >> > > > - /*\n> >> > > > - * The loop below is used to register multiple cameras behind one or more\n> >> > > > - * video mux devices that are attached to a particular Unicam instance.\n> >> > > > - * Obviously these cameras cannot be used simultaneously.\n> >> > > > - */\n> >> > > > - unsigned int numCameras = 0;\n> >> > > > - for (MediaEntity *entity : unicamDevice->entities()) {\n> >> > > > - if (entity->function() != MEDIA_ENT_F_CAM_SENSOR)\n> >> > > > + if (!ispDevice) {\n> >> > > > + LOG(RPI, Debug) << \"Unable to acquire ISP instance\";\n> >> > > > continue;\n> >> > >\n> >> > > Shouldn't you release the unicam device in this case ? I think it would\n> >> > > be better to first loop over unicam instances, ignoring any instance\n> >> > > than has no connected camera sensor, and then, if an instance with a\n> >> > > connected sensor is found, acquire an ISP instance.\n>\n> Actually, moving the ISP acquire into the inner loop would be the wrong thing to\n> do. The inner loop is to identify multiple cameras attached to a single Unicam\n> port through a mux/bridge chip. As such, only a single sensor can be active at\n> any time, and only a single ISP can service them. So I think the ISP acquiring\n> code is in the right place.\n\nYes, but that's not what I meant :-)\n\n\tforeach (unicam instances) {\n\t\tunicam = acquireMediaDevice(instance);\n\t\tif (!unicam)\n\t\t\t/* We've exhausted all unicam instances */\n\t\t\treturn false;\n\n\t\tif (unicam instance has connected source)\n\t\t\tbreak;\n\t}\n\n\tDeviceMatch isp(\"bcm2835-isp\");\n\tMediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);\n\n\tif (!ispDevice)\n\t\treturn false;\n\n> >> > I think we discussed this briefly in the github comments. There is no\n> >> > compliment releaseMediaDevice() call that I can use to release the device.\n> >>\n> >> It's an issue indeed. The design idea was to release all acquired media\n> >> devices automatically when the match() function returns false, but that\n> >> doesn't allow releasing media device that have been acquired and turned\n> >> out not to be needed.\n> >>\n> >> In this specific case, if you acquire a unicam instance that has no\n> >> connected sensor, it's fine if it stays acquired as no other pipeline\n> >> handler instance would be able to use it for a meaningful purpose\n> >> anyway, but in general this is something we should probably fix.\n> >>\n> >> > Regarding the second part of the comment, yes, I could move the isp acquire bit\n> >> > into the for (unicamDevice->entities()) loop to optimise this a bit.\n> >>\n> > >\n> >> > > > + }\n> >> > > >\n> >> > > > - int ret = registerCamera(unicamDevice, ispDevice, entity);\n> >> > > > - if (ret)\n> >> > > > - LOG(RPI, Error) << \"Failed to register camera \"\n> >> > > > - << entity->name() << \": \" << ret;\n> >> > > > - else\n> >> > > > - numCameras++;\n> >> > > > + /*\n> >> > > > + * The loop below is used to register multiple cameras behind one or more\n> >> > > > + * video mux devices that are attached to a particular Unicam instance.\n> >> > > > + * Obviously these cameras cannot be used simultaneously.\n> >> > > > + */\n> >> > > > + unsigned int numCameras = 0;\n> >> > > > + for (MediaEntity *entity : unicamDevice->entities()) {\n> >> > > > + if (entity->function() != MEDIA_ENT_F_CAM_SENSOR)\n> >> > > > + continue;\n> >> > > > +\n> >> > > > + int ret = registerCamera(unicamDevice, ispDevice, entity);\n> >> > > > + if (ret)\n> >> > > > + LOG(RPI, Error) << \"Failed to register camera \"\n> >> > > > + << entity->name() << \": \" << ret;\n> >> > > > + else\n> >> > > > + numCameras++;\n> >> > > > + }\n> >> > > > +\n> >> > > > + if (numCameras)\n> >> > > > + return true;\n> >> > > > }\n> >> > > >\n> >> > > > - return !!numCameras;\n> >> > > > + return false;\n> >> > > > }\n> >> > > >\n> >> > > > void PipelineHandlerRPi::releaseDevice(Camera *camera)","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 51BCDBE080\n\tfor ;\n\tFri, 10 Mar 2023 10:33:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B79EC626CF;\n\tFri, 10 Mar 2023 11:33:20 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 440F8626A2\n\tfor ;\n\tFri, 10 Mar 2023 11:33:19 +0100 (CET)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B4729EF;\n\tFri, 10 Mar 2023 11:33:18 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1678444400;\n\tbh=IyWqmUBBpx5uPwe3xs0J0MOjjQ5bKNHrKI3k9ZBruno=;\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=bil0gYhLpaeYr6JdwM07+EEFVIQmltXsZi5PI6H3JiyCf6ybGzayPXYbKbzozkAT0\n\tyJNxosFOaQhvV6N4UaDfue8lfNNNHa0FpWe7DQQCcT6FWfQPbKcM7FV6a+DP/WoBAb\n\tc/qFeoLpTIvvHv/ekWebRyzaYjATXRjEGglz8HKwkRmu1goL/ZMVE/Oo1K0DnEHsfu\n\tWGzfnxbmQgETS3hwFQj4qhUUonh5ijgjJe+jwbaoh5XzKGSciDbICiq0/Nq8GRaWUi\n\t0zU0V9/joaVUWjoUiuTaykCdhgpHEGXeIqiO8SGUS5nM5zSl37ST1HhMZeYHU2sYrT\n\tszGQvwjg2g+Ug==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1678444398;\n\tbh=IyWqmUBBpx5uPwe3xs0J0MOjjQ5bKNHrKI3k9ZBruno=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gd+YaBJvhNbz69gEd5BPySrbAXTGej/YmCHundyv8fozCSEVyxlZec4tX5JK80c8A\n\t0EFTPzaOx6f5SPMkaq78b620+thush5hxBJdvE6VsRvXxFZv7FhGQ2385kKsz/qM7f\n\t2ZHDCnwM66hLkdcrWF5chCKDNArZSbhDdO3n/WCs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"gd+YaBJv\"; dkim-atps=neutral","Date":"Fri, 10 Mar 2023 12:33:21 +0200","To":"Naushir Patuck ","Message-ID":"<20230310103321.GB5342@pendragon.ideasonboard.com>","References":"\n\t<20230309011431.GL31765@pendragon.ideasonboard.com>\n\t\n\t<20230309100301.GN31765@pendragon.ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Iterate over\n\tall Unicam instances in match()","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":26622,"web_url":"https://patchwork.libcamera.org/comment/26622/","msgid":"","date":"2023-03-10T10:49:29","subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Iterate over\n\tall Unicam instances in match()","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Fri, 10 Mar 2023 at 10:33, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Fri, Mar 10, 2023 at 10:22:01AM +0000, Naushir Patuck wrote:\n> > One more thing....\n> >\n> > On Fri, 10 Mar 2023 at 10:01, Naushir Patuck wrote:\n> > > On Thu, 9 Mar 2023 at 10:02, Laurent Pinchart wrote:\n> > >> On Thu, Mar 09, 2023 at 08:04:47AM +0000, Naushir Patuck wrote:\n> > >> > On Thu, 9 Mar 2023 at 01:14, Laurent Pinchart wrote:\n> > >> >\n> > >> > > Hi Naush,\n> > >> > >\n> > >> > > Thank you for the patch.\n> > >> > >\n> > >> > > I know this has been merged, but I've noticed a few issues, which\n> can be\n> > >> > > fixed in further patches.\n> > >> > >\n> > >> > > On Fri, Feb 24, 2023 at 07:30:23AM +0000, Naushir Patuck via\n> libcamera-devel wrote:\n> > >> > > >\n> > >> > > > On Raspberry Pi Compute Module platforms, it is possible to\n> attach a\n> > >> > > > single camera device only to the secondary Unicam port. The\n> current\n> > >> > > > logic of PipelineHandlerRPi::match() will return a failure\n> during\n> > >> > > > enumeration of the first Unicam media device (due to no sensor\n> attached,\n> > >> > > > or sensor failure) and thus the second Unicam media device will\n> never be\n> > >> > > > enumerated.\n> > >> > > >\n> > >> > > > Fix this by looping over all Unicam instances in\n> PipelineHandlerRPi::match()\n> > >> > > > until a camera is correctly registered, or return a failure\n> otherwise.\n> > >> > > >\n> > >> > > > Reported-on: https://github.com/raspberrypi/libcamera/issues/44\n> > >> > > > Signed-off-by: Naushir Patuck \n> > >> > > > ---\n> > >> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 67\n> +++++++++++--------\n> > >> > > > 1 file changed, 40 insertions(+), 27 deletions(-)\n> > >> > > >\n> > >> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> > > > index 841209548350..ef01b7e166ba 100644\n> > >> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> > > > @@ -1246,41 +1246,54 @@ int\n> PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > >> > > >\n> > >> > > > bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > >> > > > {\n> > >> > > > - DeviceMatch unicam(\"unicam\");\n> > >> > > > - MediaDevice *unicamDevice =\n> acquireMediaDevice(enumerator, unicam);\n> > >> > > > + constexpr unsigned int numUnicamDevices = 2;\n> > >> > >\n> > >> > > Constants should start with a k prefix, that is kNumUnicamDevices.\n> > >> > >\n> > >> > > >\n> > >> > > > - if (!unicamDevice) {\n> > >> > > > - LOG(RPI, Debug) << \"Unable to acquire a Unicam\n> instance\";\n> > >> > > > - return false;\n> > >> > > > - }\n> > >> > > > + /*\n> > >> > > > + * Loop over all Unicam instances, but return out once a\n> match is found.\n> > >> > > > + * This is to ensure we correctly enumrate the camera\n> when an instance\n> > >> > > > + * of Unicam has registered with media controller, but\n> has not registered\n> > >> > > > + * device nodes due to a sensor subdevice failure.\n> > >> > > > + */\n> > >> > > > + for (unsigned int i = 0; i < numUnicamDevices; i++) {\n> > >> > > > + DeviceMatch unicam(\"unicam\");\n> > >> > > > + MediaDevice *unicamDevice =\n> acquireMediaDevice(enumerator, unicam);\n> > >> > > >\n> > >> > > > - DeviceMatch isp(\"bcm2835-isp\");\n> > >> > > > - MediaDevice *ispDevice = acquireMediaDevice(enumerator,\n> isp);\n> > >> > > > + if (!unicamDevice) {\n> > >> > > > + LOG(RPI, Debug) << \"Unable to acquire a\n> Unicam instance\";\n> > >> > > > + continue;\n> > >> > >\n> > >> > > This looks weird, if the unicam device can't be acquired, I don't\n> see\n> > >> > > how the next iteration of the loop could successfully acquire\n> another\n> > >> > > instance. I would thus break here.\n> > >> >\n> > >> > This is probably me not understanding how the media device\n> enumeration stuff\n> > >> > works, but I thought the continue would be needed for situations\n> where we want\n> > >> > simultaneous dual cameras running in separate processes. For\n> example, process 0\n> > >> > acquires \"Unicam 0\" and starts running as normal. Process 1 starts\n> and goes\n> > >> > through match() where \"Unicam 0\" still exists in the entity list,\n> but fails to\n> > >> > acquire because it is locked by process 0. So we have to move on\n> to \"Unicam 1\"\n> > >> > which is acquired correctly for process 1. Is that understanding\n> wrong?\n> > >>\n> > >> The minimal inter-process locking support in libcamera only operates\n> > >> when trying to acquire a *camera* with Camera::acquire(). The\n> > >> acquireMediaDevice() function is a bit confusing, its name refers to\n> the\n> > >> pipeline handler acquiring a MediaDevice from the DeviceEnumerator,\n> > >> guaranteeing that the pipeline handler gets ownership of the media\n> > >> device and no other pipeline handler *in the same process* will be\n> able\n> > >> to acquire it. Two processes running libcamera will both get \"Unicam\n> 0\"\n> > >> in the first iteration of the loop.\n> > >\n> > > For my clarification, so process 1 will still acquire Unicam 0, but\n> when it\n> > > comes to camera.start(), will fail since process 0 will have Unicam 0\n> running in\n> > > its process. Is that right?\n> > >\n> > >> > > > + }\n> > >> > > >\n> > >> > > > - if (!ispDevice) {\n> > >> > > > - LOG(RPI, Debug) << \"Unable to acquire ISP\n> instance\";\n> > >> > > > - return false;\n> > >> > > > - }\n> > >> > > > + DeviceMatch isp(\"bcm2835-isp\");\n> > >> > > > + MediaDevice *ispDevice =\n> acquireMediaDevice(enumerator, isp);\n> > >> > > >\n> > >> > > > - /*\n> > >> > > > - * The loop below is used to register multiple cameras\n> behind one or more\n> > >> > > > - * video mux devices that are attached to a particular\n> Unicam instance.\n> > >> > > > - * Obviously these cameras cannot be used simultaneously.\n> > >> > > > - */\n> > >> > > > - unsigned int numCameras = 0;\n> > >> > > > - for (MediaEntity *entity : unicamDevice->entities()) {\n> > >> > > > - if (entity->function() != MEDIA_ENT_F_CAM_SENSOR)\n> > >> > > > + if (!ispDevice) {\n> > >> > > > + LOG(RPI, Debug) << \"Unable to acquire ISP\n> instance\";\n> > >> > > > continue;\n> > >> > >\n> > >> > > Shouldn't you release the unicam device in this case ? I think it\n> would\n> > >> > > be better to first loop over unicam instances, ignoring any\n> instance\n> > >> > > than has no connected camera sensor, and then, if an instance\n> with a\n> > >> > > connected sensor is found, acquire an ISP instance.\n> >\n> > Actually, moving the ISP acquire into the inner loop would be the wrong\n> thing to\n> > do. The inner loop is to identify multiple cameras attached to a single\n> Unicam\n> > port through a mux/bridge chip. As such, only a single sensor can be\n> active at\n> > any time, and only a single ISP can service them. So I think the ISP\n> acquiring\n> > code is in the right place.\n>\n> Yes, but that's not what I meant :-)\n>\n> foreach (unicam instances) {\n> unicam = acquireMediaDevice(instance);\n> if (!unicam)\n> /* We've exhausted all unicam instances */\n> return false;\n>\n> if (unicam instance has connected source)\n> break;\n> }\n>\n> DeviceMatch isp(\"bcm2835-isp\");\n> MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);\n>\n> if (!ispDevice)\n> return false;\n>\n>\nAh, sorry I get you now. However, this would not work again for the case\nwhere\nwe have multiple sensors attached to one Unicam port via a mux. We need a\nsecond inter loop iterating over \"connected sources\".\n\nI could refactor the entire match() function to work round this, but I don't\nreally see any benefit in making such a big change. As you said, it's ok to\nhave the ISP stay acquired as no other pipeline handler instance would be\nable\nto use it either.\n\nRegards,\nNaush\n\n\n> > >> > I think we discussed this briefly in the github comments. There is\n> no\n> > >> > compliment releaseMediaDevice() call that I can use to release the\n> device.\n> > >>\n> > >> It's an issue indeed. The design idea was to release all acquired\n> media\n> > >> devices automatically when the match() function returns false, but\n> that\n> > >> doesn't allow releasing media device that have been acquired and\n> turned\n> > >> out not to be needed.\n> > >>\n> > >> In this specific case, if you acquire a unicam instance that has no\n> > >> connected sensor, it's fine if it stays acquired as no other pipeline\n> > >> handler instance would be able to use it for a meaningful purpose\n> > >> anyway, but in general this is something we should probably fix.\n> > >>\n> > >> > Regarding the second part of the comment, yes, I could move the isp\n> acquire bit\n> > >> > into the for (unicamDevice->entities()) loop to optimise this a bit.\n> > >>\n> > > >\n> > >> > > > + }\n> > >> > > >\n> > >> > > > - int ret = registerCamera(unicamDevice, ispDevice,\n> entity);\n> > >> > > > - if (ret)\n> > >> > > > - LOG(RPI, Error) << \"Failed to register\n> camera \"\n> > >> > > > - << entity->name() << \": \"\n> << ret;\n> > >> > > > - else\n> > >> > > > - numCameras++;\n> > >> > > > + /*\n> > >> > > > + * The loop below is used to register multiple\n> cameras behind one or more\n> > >> > > > + * video mux devices that are attached to a\n> particular Unicam instance.\n> > >> > > > + * Obviously these cameras cannot be used\n> simultaneously.\n> > >> > > > + */\n> > >> > > > + unsigned int numCameras = 0;\n> > >> > > > + for (MediaEntity *entity :\n> unicamDevice->entities()) {\n> > >> > > > + if (entity->function() !=\n> MEDIA_ENT_F_CAM_SENSOR)\n> > >> > > > + continue;\n> > >> > > > +\n> > >> > > > + int ret = registerCamera(unicamDevice,\n> ispDevice, entity);\n> > >> > > > + if (ret)\n> > >> > > > + LOG(RPI, Error) << \"Failed to\n> register camera \"\n> > >> > > > + << entity->name()\n> << \": \" << ret;\n> > >> > > > + else\n> > >> > > > + numCameras++;\n> > >> > > > + }\n> > >> > > > +\n> > >> > > > + if (numCameras)\n> > >> > > > + return true;\n> > >> > > > }\n> > >> > > >\n> > >> > > > - return !!numCameras;\n> > >> > > > + return false;\n> > >> > > > }\n> > >> > > >\n> > >> > > > void PipelineHandlerRPi::releaseDevice(Camera *camera)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 BBCACBE080\n\tfor ;\n\tFri, 10 Mar 2023 10:49:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3CB68626B2;\n\tFri, 10 Mar 2023 11:49:45 +0100 (CET)","from mail-yb1-xb29.google.com (mail-yb1-xb29.google.com\n\t[IPv6:2607:f8b0:4864:20::b29])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4AF2C626A2\n\tfor ;\n\tFri, 10 Mar 2023 11:49:43 +0100 (CET)","by mail-yb1-xb29.google.com with SMTP id i40so1397832ybj.6\n\tfor ;\n\tFri, 10 Mar 2023 02:49:43 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1678445385;\n\tbh=U6B+r9FUv05/zcAVudgyKnlUw2sNlIQuYWkWixLBqpw=;\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=wZik7YN/pu1kc9OElxNka1BjBdavzjSQf+MCwuVmVt+cIq4jARPyOoj5wygudgUVm\n\tKVYIiXn8DLlZcrlQ/wy+r9+Kt/8S0qKzueFB5VT6HS1gJh1xf+8etI9hXFyB6rjycP\n\tfSx1T+HT7vyyLP95qaK4vK/43O/4gKeincOgbh65cicmlX+WIuFuR4EzOcPJaRIaQz\n\tsKMTA58T2BbcEwJfvO8icSJUiINKA+jOJ22OiG08HkZJDoB1nmH2p9Q2x8Cb1vAAlb\n\tvpGTeKS+tcXZkZbaqZav16ZfeagP86CDVfVCBR9KqvWAMuEyumtSi1tjlEdJ+JJx5p\n\tkSy7gVNCAGB2g==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1678445382;\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=JExl9t2C8IN23kL9a4Ov5mVWRCfMAvMI/VROksO1ncI=;\n\tb=EGNCxWYRm/cpfyFAK2KjlRcKj4Gh47dlcysOBAnhOc07OngCRgUco7PetC6ama8NeO\n\tLToN2w8slAPtMu84wanu8lWKMdfPj9LSk21laMuYWnctpmqBKIPguj31zsnpdZKAEER9\n\t9eI2mVqLCqWsmyX317TdiiYzvcKvqEw8H5px+AHK3LZFbutZa0tP4o2OKIVSsl3UNza2\n\tDxs3tILnU/Fw5/0U1nv8mo0QAifnsp43L1D/egmqJemRU8/DK3Dcaeex1AD/hM41Wm3U\n\tPIuTMTXwB5ZGQXLN75xk04RXg/dc8ePIzy84QnWRp880C2XVT0QjonBjo8vxpfWnpJSk\n\tipDA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"EGNCxWYR\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1678445382;\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=JExl9t2C8IN23kL9a4Ov5mVWRCfMAvMI/VROksO1ncI=;\n\tb=mEQF8Qb7L6ja8mBtB36Ma5ll2pYDfy1u54vBHczYgw4n5iRzjULiQ4orRnsx/yLXLH\n\tprzENNOvCf98i+SKpmtI51qqgKGB2tYP5MLcEnSozt6uIxGeKLzYuEFo0mV6BCp+rIBH\n\t08dBNipFaXAtT2HeLfPD3EB0Cy0qbUYOkRMScx/G2yJRfpfC+5fnhHJq1k6TsSKpd5hu\n\tH/+MVjpaOXze1YnPTtqiqXF6TsxN3l/JKEHGg3NzAlIY1uCjGyuS+QyLuV24FmS20yRu\n\tndzLFVXufTxMMEnIoPjaQiybi1acLsVCZFsTUjiy4rrLLaof01/n2XFIo2SEscVm65VC\n\tdStA==","X-Gm-Message-State":"AO0yUKXNgGz8lH7fTkxy+FaFElQrggphYu5JnyX/KDFVxAk53A4yX+fQ\n\tDuBAcbzx8HvvEPHcSAVe9L0U525VwdxZ5DzRjdeukuBGR6pIYOSRTJLI8w==","X-Google-Smtp-Source":"AK7set9SXcZrvMswZYK3xNv4ramAlkzPqvBKyXwTYQPNiJ+XIBajlJ74z9rlMk2G2Q3a91fa4ee2e4kZZ3Erg3Juzbs=","X-Received":"by 2002:a5b:88c:0:b0:ad7:b81e:69be with SMTP id\n\te12-20020a5b088c000000b00ad7b81e69bemr12388433ybq.6.1678445382041;\n\tFri, 10 Mar 2023 02:49:42 -0800 (PST)","MIME-Version":"1.0","References":"\n\t<20230309011431.GL31765@pendragon.ideasonboard.com>\n\t\n\t<20230309100301.GN31765@pendragon.ideasonboard.com>\n\t\n\t\n\t<20230310103321.GB5342@pendragon.ideasonboard.com>","In-Reply-To":"<20230310103321.GB5342@pendragon.ideasonboard.com>","Date":"Fri, 10 Mar 2023 10:49:29 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000c5969005f68984a1\"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Iterate over\n\tall Unicam instances in match()","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\" "}}]